fix: redundant API calls in notification module (#411)

edx · Jul 24, 2023 · 3584919 · 3584919
1 parent 686fde8
commit 3584919
Show file tree

Hide file tree

Showing 11 changed files with 112 additions and 324 deletions.
diff --git a/src/Notifications/NotificationRowItem.jsx b/src/Notifications/NotificationRowItem.jsx
@@ -17,8 +17,8 @@ const NotificationRowItem = ({
   const dispatch = useDispatch();
 
   const handleMarkAsRead = useCallback(() => {
-    dispatch(markNotificationsAsRead(id));
-  }, [dispatch, id]);
+    if (!lastRead) { dispatch(markNotificationsAsRead(id)); }
+  }, [dispatch, id, lastRead]);
 
   const { icon: iconComponent, class: iconClass } = getIconByType(type);
 

diff --git a/src/Notifications/NotificationSections.jsx b/src/Notifications/NotificationSections.jsx
@@ -5,20 +5,20 @@ import { useIntl } from '@edx/frontend-platform/i18n';
 import isEmpty from 'lodash/isEmpty';
 import messages from './messages';
 import NotificationRowItem from './NotificationRowItem';
-import { markAllNotificationsAsRead } from './data/thunks';
+import { markAllNotificationsAsRead, fetchNotificationList } from './data/thunks';
 import {
   selectNotificationsByIds, selectPaginationData, selectSelectedAppName, selectNotificationStatus,
 } from './data/selectors';
 import { splitNotificationsByTime } from './utils';
-import { updatePaginationRequest, RequestStatus } from './data/slice';
+import { RequestStatus } from './data/slice';
 
 const NotificationSections = () => {
   const intl = useIntl();
   const dispatch = useDispatch();
-  const selectedAppName = useSelector(selectSelectedAppName());
-  const notificationRequestStatus = useSelector(selectNotificationStatus());
+  const selectedAppName = useSelector(selectSelectedAppName);
+  const notificationRequestStatus = useSelector(selectNotificationStatus);
   const notifications = useSelector(selectNotificationsByIds(selectedAppName));
-  const { hasMorePages } = useSelector(selectPaginationData());
+  const { hasMorePages, currentPage } = useSelector(selectPaginationData);
   const { today = [], earlier = [] } = useMemo(
     () => splitNotificationsByTime(notifications),
     [notifications],
@@ -28,9 +28,9 @@ const NotificationSections = () => {
     dispatch(markAllNotificationsAsRead(selectedAppName));
   }, [dispatch, selectedAppName]);
 
-  const updatePagination = useCallback(() => {
-    dispatch(updatePaginationRequest());
-  }, [dispatch]);
+  const loadMoreNotifications = useCallback(() => {
+    dispatch(fetchNotificationList({ appName: selectedAppName, page: currentPage + 1 }));
+  }, [currentPage, dispatch, selectedAppName]);
 
   const renderNotificationSection = (section, items) => {
     if (isEmpty(items)) { return null; }
@@ -77,17 +77,16 @@ const NotificationSections = () => {
         <div className="d-flex justify-content-center p-4">
           <Spinner animation="border" variant="primary" size="lg" />
         </div>
-      ) : (hasMorePages && notificationRequestStatus === RequestStatus.SUCCESSFUL
-        && (
-          <Button
-            variant="primary"
-            className="w-100 bg-primary-500"
-            onClick={updatePagination}
-            data-testid="load-more-notifications"
-          >
-            {intl.formatMessage(messages.loadMoreNotifications)}
-          </Button>
-        )
+      ) : (hasMorePages && notificationRequestStatus === RequestStatus.SUCCESSFUL && (
+        <Button
+          variant="primary"
+          className="w-100 bg-primary-500"
+          onClick={loadMoreNotifications}
+          data-testid="load-more-notifications"
+        >
+          {intl.formatMessage(messages.loadMoreNotifications)}
+        </Button>
+      )
       )}
     </div>
   );

diff --git a/src/Notifications/NotificationTabs.jsx b/src/Notifications/NotificationTabs.jsx
@@ -1,42 +1,30 @@
-/* eslint-disable react-hooks/exhaustive-deps */
-import React, { useCallback, useEffect, useMemo } from 'react';
+import React, { useCallback, useEffect } from 'react';
 import { useDispatch, useSelector } from 'react-redux';
 import { Tab, Tabs } from '@edx/paragon';
 import NotificationSections from './NotificationSections';
 import { fetchNotificationList, markNotificationsAsSeen } from './data/thunks';
 import {
-  selectNotificationTabs, selectNotificationTabsCount, selectPaginationData, selectSelectedAppName,
+  selectNotificationTabs, selectNotificationTabsCount, selectSelectedAppName,
 } from './data/selectors';
 import { updateAppNameRequest } from './data/slice';
 
 const NotificationTabs = () => {
   const dispatch = useDispatch();
-  const selectedAppName = useSelector(selectSelectedAppName());
-  const notificationUnseenCounts = useSelector(selectNotificationTabsCount());
-  const notificationTabs = useSelector(selectNotificationTabs());
-  const { currentPage } = useSelector(selectPaginationData());
+  const selectedAppName = useSelector(selectSelectedAppName);
+  const notificationUnseenCounts = useSelector(selectNotificationTabsCount);
+  const notificationTabs = useSelector(selectNotificationTabs);
 
   useEffect(() => {
-    dispatch(fetchNotificationList({ appName: selectedAppName, page: currentPage }));
-    if (selectedAppName) { dispatch(markNotificationsAsSeen(selectedAppName)); }
-  }, [currentPage, selectedAppName]);
+    dispatch(fetchNotificationList({ appName: selectedAppName }));
+    if (notificationUnseenCounts[selectedAppName]) {
+      dispatch(markNotificationsAsSeen(selectedAppName));
+    }
+  // eslint-disable-next-line react-hooks/exhaustive-deps
+  }, [selectedAppName]);
 
   const handleActiveTab = useCallback((appName) => {
     dispatch(updateAppNameRequest({ appName }));
-  }, []);
-
-  const tabArray = useMemo(() => notificationTabs?.map((appName) => (
-    <Tab
-      key={appName}
-      eventKey={appName}
-      title={appName}
-      notification={notificationUnseenCounts[appName]}
-      tabClassName="pt-0 pb-10px px-2.5 d-flex border-top-0 mb-0 align-items-center line-height-24 text-capitalize"
-      data-testid={`notification-tab-${appName}`}
-    >
-      {appName === selectedAppName && (<NotificationSections />)}
-    </Tab>
-  )), [notificationUnseenCounts, selectedAppName, notificationTabs]);
+  }, [dispatch]);
 
   return (
     <Tabs
@@ -45,7 +33,18 @@ const NotificationTabs = () => {
       onSelect={handleActiveTab}
       className="px-2.5 text-primary-500"
     >
-      {tabArray}
+      {notificationTabs?.map((appName) => (
+        <Tab
+          key={appName}
+          eventKey={appName}
+          title={appName}
+          notification={notificationUnseenCounts[appName]}
+          tabClassName="pt-0 pb-10px px-2.5 d-flex border-top-0 mb-0 align-items-center line-height-24 text-capitalize"
+          data-testid={`notification-tab-${appName}`}
+        >
+          {selectedAppName === appName && <NotificationSections />}
+        </Tab>
+      ))}
     </Tabs>
   );
 };

diff --git a/src/Notifications/data/api.js b/src/Notifications/data/api.js
@@ -6,34 +6,30 @@ export const getNotificationsListApiUrl = () => `${getConfig().LMS_BASE_URL}/api
 export const markNotificationsSeenApiUrl = (appName) => `${getConfig().LMS_BASE_URL}/api/notifications/mark-seen/${appName}/`;
 export const markNotificationAsReadApiUrl = () => `${getConfig().LMS_BASE_URL}/api/notifications/read/`;
 
-export async function getNotificationsList(appName, page) {
-  const params = snakeCaseObject({ appName, page });
+export async function getNotificationsList(appName, page, pageSize) {
+  const params = snakeCaseObject({ appName, page, pageSize });
   const { data } = await getAuthenticatedHttpClient().get(getNotificationsListApiUrl(), { params });
   return data;
 }
 
 export async function getNotificationCounts() {
   const { data } = await getAuthenticatedHttpClient().get(getNotificationsCountApiUrl());
-
   return data;
 }
 
 export async function markNotificationSeen(appName) {
   const { data } = await getAuthenticatedHttpClient().put(`${markNotificationsSeenApiUrl(appName)}`);
-
   return data;
 }
 
 export async function markAllNotificationRead(appName) {
   const params = snakeCaseObject({ appName });
   const { data } = await getAuthenticatedHttpClient().patch(markNotificationAsReadApiUrl(), params);
-
   return data;
 }
 
 export async function markNotificationRead(notificationId) {
   const params = snakeCaseObject({ notificationId });
   const { data } = await getAuthenticatedHttpClient().patch(markNotificationAsReadApiUrl(), params);
-
   return { data, id: notificationId };
 }
diff --git a/src/Notifications/data/notifications.json b/src/Notifications/data/notifications.json
diff --git a/src/Notifications/data/selector.test.jsx b/src/Notifications/data/selector.test.jsx
@@ -46,14 +46,14 @@ describe('Notification Selectors', () => {
 
   it('Should return notification status.', async () => {
     const state = store.getState();
-    const status = selectNotificationStatus()(state);
+    const status = selectNotificationStatus(state);
 
     expect(status).toEqual('successful');
   });
 
   it('Should return notification tabs count.', async () => {
     const state = store.getState();
-    const tabsCount = selectNotificationTabsCount()(state);
+    const tabsCount = selectNotificationTabsCount(state);
 
     expect(tabsCount.count).toEqual(25);
     expect(tabsCount.reminders).toEqual(10);
@@ -64,7 +64,7 @@ describe('Notification Selectors', () => {
 
   it('Should return notification tabs.', async () => {
     const state = store.getState();
-    const tabs = selectNotificationTabs()(state);
+    const tabs = selectNotificationTabs(state);
 
     expect(tabs).toHaveLength(4);
   });
@@ -78,14 +78,14 @@ describe('Notification Selectors', () => {
 
   it('Should return show notification tray status.', async () => {
     const state = store.getState();
-    const showNotificationTrayStatus = selectShowNotificationTray()(state);
+    const showNotificationTrayStatus = selectShowNotificationTray(state);
 
     expect(showNotificationTrayStatus).toEqual(true);
   });
 
   it('Should return notifications.', async () => {
     const state = store.getState();
-    const notifications = selectNotifications()(state);
+    const notifications = selectNotifications(state);
 
     expect(Object.keys(notifications)).toHaveLength(10);
   });
@@ -99,14 +99,14 @@ describe('Notification Selectors', () => {
 
   it('Should return selected app name.', async () => {
     const state = store.getState();
-    const appName = selectSelectedAppName()(state);
+    const appName = selectSelectedAppName(state);
 
     expect(appName).toEqual('discussion');
   });
 
   it('Should return pagination data.', async () => {
     const state = store.getState();
-    const paginationData = selectPaginationData()(state);
+    const paginationData = selectPaginationData(state);
 
     expect(paginationData.currentPage).toEqual(1);
     expect(paginationData.numPages).toEqual(2);