Merge pull request #3307 from NoelDeMartin/MOBILE-4081

MOBILE-4081: Improve notifications source and fix behat steps

local-moodleappbehat/tests/behat/behat_app.php

@@ -46,6 +46,7 @@ class behat_app extends behat_app_helper {
      *
      * @When I enter the app
      * @Given I entered the app as :username
+     * @param string $username Username
      * @throws DriverException Issue with configuration or feature file
      * @throws dml_exception Problem with Moodle setup
      * @throws ExpectationException Problem with resizing window
@@ -77,6 +78,7 @@ class behat_app extends behat_app_helper {
      *
      * @When I launch the app :runtime
      * @When I launch the app
+     * @param string $runtime Runtime
      * @throws DriverException Issue with configuration or feature file
      * @throws dml_exception Problem with Moodle setup
      * @throws ExpectationException Problem with resizing window
@@ -111,6 +113,9 @@ class behat_app extends behat_app_helper {
      * Finds elements in the app.
      *
      * @Then /^I should( not)? find (".+")( inside the .+)? in the app$/
+     * @param bool $not Whether assert that the element was not found
+     * @param string $locator Element locator
+     * @param string $containerName Container name
      */
     public function i_find_in_the_app(bool $not, string $locator, string $containerName = '') {
         $locator = $this->parse_element_locator($locator);
@@ -141,7 +146,7 @@ class behat_app extends behat_app_helper {
      * Scroll to an element in the app.
      *
      * @When /^I scroll to (".+") in the app$/
-     * @param string $locator
+     * @param string $locator Element locator
      */
     public function i_scroll_to_in_the_app(string $locator) {
         $locator = $this->parse_element_locator($locator);
@@ -166,7 +171,7 @@ class behat_app extends behat_app_helper {
      * Load more items in a list with an infinite loader.
      *
      * @When /^I (should not be able to )?load more items in the app$/
-     * @param bool $not
+     * @param bool $not Whether assert that it is not possible to load more items
      */
     public function i_load_more_items_in_the_app(bool $not = false) {
         $this->spin(function() use ($not) {
@@ -190,7 +195,7 @@ class behat_app extends behat_app_helper {
      * Trigger swipe gesture.
      *
      * @When /^I swipe to the (left|right) in the app$/
-     * @param string $direction
+     * @param string $direction Swipe direction
      */
     public function i_swipe_in_the_app(string $direction) {
         $method = 'swipe' . ucwords($direction);
@@ -207,8 +212,8 @@ class behat_app extends behat_app_helper {
      * Check if elements are selected in the app.
      *
      * @Then /^(".+") should( not)? be selected in the app$/
-     * @param string $locator
+     * @param string $locator Element locator
-     * @param bool $not
+     * @param bool $not Whether to assert that the element is not selected
      */
     public function be_selected_in_the_app(string $locator, bool $not = false) {
         $locator = $this->parse_element_locator($locator);
@@ -288,6 +293,7 @@ class behat_app extends behat_app_helper {
      * @Given I entered the course :coursename as :username in the app
      * @Given I entered the course :coursename in the app
      * @param string $coursename Course name
+     * @param string $username Username
      * @throws DriverException If the button push doesn't work
      */
     public function i_entered_the_course_in_the_app(string $coursename, ?string $username = null) {
@@ -308,8 +314,10 @@ class behat_app extends behat_app_helper {
     /**
      * User enters a course in the app.
      *
+     * @Given I enter the course :coursename as :username in the app
      * @Given I enter the course :coursename in the app
      * @param string $coursename Course name
+     * @param string $username Username
      * @throws DriverException If the button push doesn't work
      */
     public function i_enter_the_course_in_the_app(string $coursename, ?string $username = null) {
@@ -342,6 +350,10 @@ class behat_app extends behat_app_helper {
      *
      * @Given I entered the :activity activity :activityname on course :course as :username in the app
      * @Given I entered the :activity activity :activityname on course :course in the app
+     * @param string $activity Activity
+     * @param string $activityname Activity name
+     * @param string $coursename Course name
+     * @param string $username Username
      * @throws DriverException If the button push doesn't work
      */
     public function i_enter_the_activity_in_the_app(string $activity, string $activityname, string $coursename, ?string $username = null) {
@@ -386,7 +398,7 @@ class behat_app extends behat_app_helper {
      * Receives push notifications.
      *
      * @When /^I receive a push notification in the app for:$/
-     * @param TableNode $data
+     * @param TableNode $data Table data
      */
     public function i_receive_a_push_notification(TableNode $data) {
         global $DB, $CFG;
@@ -416,6 +428,8 @@ class behat_app extends behat_app_helper {
      * Replace arguments from the content in the given activity field.
      *
      * @Given /^I replace the arguments in "([^"]+)" "([^"]+)"$/
+     * @param string $idnumber Id number
+     * @param string $field Field
      */
     public function i_replace_arguments_in_the_activity(string $idnumber, string $field) {
         global $DB;
@@ -434,7 +448,7 @@ class behat_app extends behat_app_helper {
      * Opens a custom link.
      *
      * @Given /^I open a custom link in the app for:$/
-     * @param TableNode $data
+     * @param TableNode $data Table data
      */
     public function i_open_a_custom_link(TableNode $data) {
         global $DB;
@@ -510,7 +524,7 @@ class behat_app extends behat_app_helper {
      * Override app config.
      *
      * @Given /^the app has the following config:$/
-     * @param TableNode $data
+     * @param TableNode $data Table data
      */
     public function the_app_has_the_following_config(TableNode $data) {
         foreach ($data->getRows() as $configrow) {
@@ -552,8 +566,8 @@ class behat_app extends behat_app_helper {
      * to race conditions.
      *
      * @Then /^I (unselect|select) (".+") in the app$/
-     * @param string $selectedtext
+     * @param string $selectedtext Text inidicating if the element should be selected or unselected
-     * @param string $locator
+     * @param string $locator Element locator
      * @throws DriverException If the press doesn't work
      */
     public function i_select_in_the_app(string $selectedtext, string $locator) {
@@ -673,8 +687,8 @@ class behat_app extends behat_app_helper {
      * Check that the app opened a new browser tab.
      *
      * @Then /^the app should( not)? have opened a browser tab(?: with url "(?P<pattern>[^"]+)")?$/
-     * @param bool $not
+     * @param bool $not Whether to check if the app did not open a new browser tab
-     * @param string $urlpattern
+     * @param string $urlpattern Url pattern
      */
     public function the_app_should_have_opened_a_browser_tab(bool $not = false, ?string $urlpattern = null) {
         $this->spin(function() use ($not, $urlpattern) {

src/addons/notifications/classes/legacy-notifications-source.ts

@@ -0,0 +1,62 @@
+// (C) Copyright 2015 Moodle Pty Ltd.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+import { AddonNotificationsNotificationsSource } from '@addons/notifications/classes/notifications-source';
+import { AddonNotificationsGetReadType } from '@addons/notifications/services/notifications';
+import { AddonNotificationsNotificationToRender } from '@addons/notifications/services/notifications-helper';
+
+/**
+ * Provides a list of notifications using legacy web services.
+ */
+export class AddonLegacyNotificationsNotificationsSource extends AddonNotificationsNotificationsSource {
+
+    /**
+     * @inheritdoc
+     */
+    protected async loadPageItems(page: number): Promise<{
+        items: AddonNotificationsNotificationToRender[];
+        hasMoreItems: boolean;
+    }> {
+        let items: AddonNotificationsNotificationToRender[] = [];
+        let hasMoreItems = false;
+        let pageUnreadCount = 0;
+        const pageLength = this.getPageLength();
+        const totalUnread = () => this.totals[AddonNotificationsGetReadType.UNREAD] ?? Number.MAX_VALUE;
+
+        // Load unread notifications first.
+        if (totalUnread() > page * pageLength) {
+            const pageResults = await this.loadNotifications(AddonNotificationsGetReadType.UNREAD, page * pageLength);
+
+            items = items.concat(pageResults.notifications);
+            hasMoreItems = pageResults.hasMoreNotifications;
+            pageUnreadCount = pageResults.notifications.length;
+        }
+
+        // If all unread notifications have been fetched, load read notifications first.
+        if (totalUnread() < (page + 1) * pageLength) {
+            const offset = Math.max(0, page * pageLength - totalUnread());
+            const pageResults = await this.loadNotifications(
+                AddonNotificationsGetReadType.READ,
+                offset,
+                pageLength - pageUnreadCount,
+            );
+
+            items = items.concat(pageResults.notifications);
+            hasMoreItems = pageResults.hasMoreNotifications;
+        }
+
+        return { items, hasMoreItems };
+    }
+
+}

src/addons/notifications/classes/notifications-source.ts

@@ -12,30 +12,20 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+import {
+    AddonNotifications,
+    AddonNotificationsGetReadType,
+    AddonNotificationsProvider,
+} from '@addons/notifications/services/notifications';
+import { AddonNotificationsNotificationToRender } from '@addons/notifications/services/notifications-helper';
 import { CoreRoutedItemsManagerSource } from '@classes/items-management/routed-items-manager-source';
-import { AddonNotifications } from '../services/notifications';
-import { AddonNotificationsHelper, AddonNotificationsNotificationToRender } from '../services/notifications-helper';
 
 /**
- * Provides a list of notifications
+ * Provides a list of notifications.
  */
-export class AddonsNotificationsNotificationsSource extends CoreRoutedItemsManagerSource<AddonNotificationsNotificationToRender> {
+export class AddonNotificationsNotificationsSource extends CoreRoutedItemsManagerSource<AddonNotificationsNotificationToRender> {
 
-    /**
+    protected totals: Record<string, number> = {};
-     * @inheritdoc
-     */
-    protected async loadPageItems(page: number): Promise<{
-        items: AddonNotificationsNotificationToRender[];
-        hasMoreItems: boolean;
-    }> {
-        // TODO this should be refactored to avoid using the existing items.
-        const { notifications, canLoadMore } = await AddonNotifications.getNotifications(page === 0 ? [] : this.getItems() ?? []);
-
-        return {
-            items: notifications.map(notification => AddonNotificationsHelper.formatNotificationText(notification)),
-            hasMoreItems: canLoadMore,
-        };
-    }
 
     /**
      * @inheritdoc
@@ -44,4 +34,79 @@ export class AddonsNotificationsNotificationsSource extends CoreRoutedItemsManag
         return notification.id.toString();
     }
 
+    /**
+     * @inheritdoc
+     */
+    reset(): void {
+        this.totals = {};
+
+        super.reset();
+    }
+
+    /**
+     * @inheritdoc
+     */
+    protected async loadPageItems(page: number): Promise<{
+        items: AddonNotificationsNotificationToRender[];
+        hasMoreItems: boolean;
+    }> {
+        const results = await this.loadNotifications(AddonNotificationsGetReadType.BOTH, page * this.getPageLength());
+
+        return {
+            items: results.notifications,
+            hasMoreItems: results.hasMoreNotifications,
+        };
+    }
+
+    /**
+     * Load notifications of the given type.
+     *
+     * @param type Type.
+     * @param offset Offset.
+     * @param limit Limit.
+     * @returns Notifications and whether there are any more.
+     */
+    protected async loadNotifications(type: AddonNotificationsGetReadType, offset: number, limit?: number): Promise<{
+        notifications: AddonNotificationsNotificationToRender[];
+        hasMoreNotifications: boolean;
+    }> {
+        limit = limit ?? this.getPageLength();
+
+        if (type in this.totals && this.totals[type] <= offset) {
+            return {
+                notifications: [],
+                hasMoreNotifications: false,
+            };
+        }
+
+        const notifications = await AddonNotifications.getNotificationsWithStatus(type, { offset, limit });
+
+        if (notifications.length < limit) {
+            this.totals[type] = offset + notifications.length;
+        }
+
+        return {
+            notifications,
+            hasMoreNotifications: (this.totals[type] ?? Number.MAX_VALUE) > offset + limit,
+        };
+    }
+
+    /**
+     * @inheritdoc
+     */
+    protected setItems(notifications: AddonNotificationsNotificationToRender[], hasMoreItems: boolean): void {
+        const sortedNotifications = notifications.slice(0);
+
+        sortedNotifications.sort((a, b) => a.timecreated < b.timecreated ? 1 : -1);
+
+        super.setItems(sortedNotifications, hasMoreItems);
+    }
+
+    /**
+     * @inheritdoc
+     */
+    protected getPageLength(): number {
+        return AddonNotificationsProvider.LIST_LIMIT;
+    }
+
 }

src/addons/notifications/pages/list/list.ts

@@ -29,9 +29,10 @@ import { CorePushNotificationsDelegate } from '@features/pushnotifications/servi
 import { CoreSites } from '@services/sites';
 import { CoreMainMenuDeepLinkManager } from '@features/mainmenu/classes/deep-link-manager';
 import { CoreTimeUtils } from '@services/utils/time';
-import { AddonsNotificationsNotificationsSource } from '@addons/notifications/classes/notifications-source';
+import { AddonNotificationsNotificationsSource } from '@addons/notifications/classes/notifications-source';
 import { CoreListItemsManager } from '@classes/items-management/list-items-manager';
 import { AddonNotificationsNotificationToRender } from '@addons/notifications/services/notifications-helper';
+import { AddonLegacyNotificationsNotificationsSource } from '@addons/notifications/classes/legacy-notifications-source';
 
 /**
  * Page that displays the list of notifications.
@@ -44,7 +45,7 @@ import { AddonNotificationsNotificationToRender } from '@addons/notifications/se
 export class AddonNotificationsListPage implements AfterViewInit, OnDestroy {
 
     @ViewChild(CoreSplitViewComponent) splitView!: CoreSplitViewComponent;
-    notifications!: CoreListItemsManager<AddonNotificationsNotificationToRender, AddonsNotificationsNotificationsSource>;
+    notifications!: CoreListItemsManager<AddonNotificationsNotificationToRender, AddonNotificationsNotificationsSource>;
     fetchMoreNotificationsFailed = false;
     canMarkAllNotificationsAsRead = false;
     loadingMarkAllNotificationsAsRead = false;
@@ -58,7 +59,9 @@ export class AddonNotificationsListPage implements AfterViewInit, OnDestroy {
     constructor() {
         try {
             const source = CoreRoutedItemsManagerSourcesTracker.getOrCreateSource(
-                AddonsNotificationsNotificationsSource,
+                CoreSites.getRequiredCurrentSite().isVersionGreaterEqualThan('4.0')
+                    ? AddonNotificationsNotificationsSource
+                    : AddonLegacyNotificationsNotificationsSource,
                 [],
             );
 

src/addons/notifications/pages/notification/notification.ts

@@ -12,7 +12,8 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-import { AddonsNotificationsNotificationsSource } from '@addons/notifications/classes/notifications-source';
+import { AddonLegacyNotificationsNotificationsSource } from '@addons/notifications/classes/legacy-notifications-source';
+import { AddonNotificationsNotificationsSource } from '@addons/notifications/classes/notifications-source';
 import { AddonNotificationsNotificationData } from '@addons/notifications/services/handlers/push-click';
 import {
     AddonNotificationsHelper,
@@ -118,7 +119,9 @@ export class AddonNotificationsNotificationPage implements OnInit, OnDestroy {
      */
     getNotificationById(notificationId: number): AddonNotificationsNotification | undefined {
         const source = CoreRoutedItemsManagerSourcesTracker.getOrCreateSource(
-            AddonsNotificationsNotificationsSource,
+            CoreSites.getRequiredCurrentSite().isVersionGreaterEqualThan('4.0')
+                ? AddonNotificationsNotificationsSource
+                : AddonLegacyNotificationsNotificationsSource,
             [],
         );
         const notification = source.getItems()?.find(({ id }) => id === notificationId);
@@ -137,7 +140,7 @@ export class AddonNotificationsNotificationPage implements OnInit, OnDestroy {
      *
      * @param source Notifications source
      */
-    async loadNotifications(source: AddonsNotificationsNotificationsSource): Promise<void> {
+    async loadNotifications(source: AddonNotificationsNotificationsSource): Promise<void> {
         this.notifications = new AddonNotificationSwipeItemsManager(source);
 
         await this.notifications.start();

src/addons/notifications/services/notifications.ts

@@ -165,6 +165,7 @@ export class AddonNotificationsProvider {
      * @param notifications Current list of loaded notifications. It's used to calculate the offset.
      * @param options Other options.
      * @return Promise resolved with notifications and if can load more.
+     * @deprecated since 4.1. Use getNotificationsWithStatus instead.
      */
     async getNotifications(
         notifications: AddonNotificationsNotificationMessageFormatted[],
@@ -231,7 +232,7 @@ export class AddonNotificationsProvider {
      * @param options Other options.
      * @return Promise resolved with notifications.
      */
-    protected async getNotificationsWithStatus(
+    async getNotificationsWithStatus(
         read: AddonNotificationsGetReadType,
         options: AddonNotificationsGetNotificationsOptions = {},
     ): Promise<AddonNotificationsNotificationMessageFormatted[]> {