refactor: fixed all review points

src/Notifications/NotificationRowItem.jsx

@@ -7,7 +7,7 @@ import { Link } from 'react-router-dom';
 import * as timeago from 'timeago.js';
 import { getIconByType } from './utils';
 import { markNotificationsAsRead } from './data/thunks';
-import { messages } from './messages';
+import messages from './messages';
 import timeLocale from '../common/time-locale';
 
 const NotificationRowItem = ({
@@ -21,15 +21,16 @@ const NotificationRowItem = ({
     dispatch(markNotificationsAsRead(id));
   }, [dispatch, id]);
 
-  const iconComponent = getIconByType(type);
+  const { icon: iconComponent, class: iconClass } = getIconByType(type);
 
   return (
-    <Link className="d-flex mb-2 align-items-center text-decoration-none" to={contentUrl} onClick={handleMarkAsRead}>
-      <Icon
-        src={iconComponent && iconComponent.icon}
-        style={{ height: '23.33px', width: '23.33px' }}
-        className={iconComponent && `${iconComponent.class} mr-4`}
-      />
+    <Link
+      target="_blank"
+      className="d-flex mb-2 align-items-center text-decoration-none"
+      to={contentUrl}
+      onClick={handleMarkAsRead}
+    >
+      <Icon src={iconComponent} className={`${iconClass} mr-4 notification-icon`} />
       <div className="d-flex w-100">
         <div className="d-flex align-items-center w-100">
           <div className="py-10px w-100 px-0 cursor-pointer">

src/Notifications/NotificationSections.jsx

@@ -3,7 +3,7 @@ import { Button } from '@edx/paragon';
 import { useDispatch, useSelector } from 'react-redux';
 import { useIntl } from '@edx/frontend-platform/i18n';
 import isEmpty from 'lodash/isEmpty';
-import { messages } from './messages';
+import messages from './messages';
 import NotificationRowItem from './NotificationRowItem';
 import { markAllNotificationsAsRead } from './data/thunks';
 import { selectNotificationsByIds, selectPaginationData, selectSelectedAppName } from './data/selectors';
@@ -14,8 +14,8 @@ const NotificationSections = () => {
   const intl = useIntl();
   const dispatch = useDispatch();
   const selectedAppName = useSelector(selectSelectedAppName());
-  const notifications = useSelector(selectNotificationsByIds);
-  const paginationData = useSelector(selectPaginationData());
+  const notifications = useSelector(selectNotificationsByIds(selectedAppName));
+  const { currentPage, numPages } = useSelector(selectPaginationData());
   const { today = [], earlier = [] } = useMemo(
     () => splitNotificationsByTime(notifications),
     [notifications],
@@ -69,7 +69,7 @@ const NotificationSections = () => {
     <div className="mt-4 px-4">
       {renderNotificationSection('today', today)}
       {renderNotificationSection('earlier', earlier)}
-      {paginationData.currentPage < paginationData.numPages && (
+      {currentPage < numPages && (
         <Button variant="primary" className="w-100 bg-primary-500" onClick={updatePagination}>
           {intl.formatMessage(messages.loadMoreNotifications)}
         </Button>

src/Notifications/NotificationTabs.jsx

@@ -1,3 +1,4 @@
+/* eslint-disable react-hooks/exhaustive-deps */
 import React, { useCallback, useEffect, useMemo } from 'react';
 import { useDispatch, useSelector } from 'react-redux';
 import { Tab, Tabs } from '@edx/paragon';
@@ -13,16 +14,16 @@ const NotificationTabs = () => {
   const selectedAppName = useSelector(selectSelectedAppName());
   const notificationUnseenCounts = useSelector(selectNotificationTabsCount());
   const notificationTabs = useSelector(selectNotificationTabs());
-  const paginationData = useSelector(selectPaginationData());
+  const { currentPage } = useSelector(selectPaginationData());
 
   useEffect(() => {
-    dispatch(fetchNotificationList({ appName: selectedAppName, page: paginationData.currentPage, pageSize: 10 }));
+    dispatch(fetchNotificationList({ appName: selectedAppName, page: currentPage, pageSize: 10 }));
     if (selectedAppName) { dispatch(markNotificationsAsSeen(selectedAppName)); }
-  }, [dispatch, paginationData.currentPage, selectedAppName]);
+  }, [currentPage, selectedAppName]);
 
   const handleActiveTab = useCallback((appName) => {
     dispatch(updateAppNameRequest({ appName }));
-  }, [dispatch]);
+  }, []);
 
   const tabArray = useMemo(() => notificationTabs?.map((appName) => (
     <Tab

src/Notifications/data/selectors.js

@@ -10,12 +10,12 @@ export const selectSelectedAppNotificationIds = (appName) => state => state.noti
 
 export const selectShowNotificationTray = () => state => state.notifications.showNotificationTray;
 
-export const selectNotifications = () => state => state.notifications.notification;
+export const selectNotifications = () => state => state.notifications.notifications;
 
-export const selectNotificationsByIds = createSelector(
-  state => state.notifications.notifications,
-  state => state.notifications.apps[state.notifications.appName] || [],
-  (notifications, notificationIds) => notificationIds.map(notificationId => notifications[notificationId]),
+export const selectNotificationsByIds = (appName) => createSelector(
+  selectNotifications(),
+  selectSelectedAppNotificationIds(appName),
+  (notifications, notificationIds) => notificationIds.map((notificationId) => notifications[notificationId]) || [],
 );
 
 export const selectSelectedAppName = () => state => state.notifications.appName;

src/Notifications/data/slice.js

@@ -1,15 +1,17 @@
 /* eslint-disable no-param-reassign */
 import { createSlice } from '@reduxjs/toolkit';
 
-export const IDLE = 'idle';
-export const LOADING = 'loading';
-export const LOADED = 'loaded';
-export const FAILED = 'failed';
-export const DENIED = 'denied';
+export const RequestStatus = {
+  IDLE: 'idle',
+  LOADING: 'in-progress',
+  LOADED: 'successful',
+  FAILED: 'failed',
+  DENIED: 'denied',
+};
 
 const initialState = {
   notificationStatus: 'idle',
-  appName: 'reminders',
+  appName: 'discussions',
   appsId: [],
   apps: {},
   notifications: {},
@@ -26,65 +28,62 @@ const slice = createSlice({
   name: 'notifications',
   initialState,
   reducers: {
-    fetchNotificationDenied: (state, { payload }) => {
-      state.appName = payload.appName;
-      state.notificationStatus = DENIED;
+    fetchNotificationDenied: (state) => {
+      state.notificationStatus = RequestStatus.DENIED;
     },
-    fetchNotificationFailure: (state, { payload }) => {
-      state.appName = payload.appName;
-      state.notificationStatus = FAILED;
+    fetchNotificationFailure: (state) => {
+      state.notificationStatus = RequestStatus.FAILED;
     },
-    fetchNotificationRequest: (state, { payload }) => {
-      if (state.appName !== payload.appName) { state.apps[payload.appName] = []; }
-      state.appName = payload.appName;
-      state.notificationStatus = LOADING;
+    fetchNotificationRequest: (state) => {
+      state.notificationStatus = RequestStatus.LOADING;
     },
     fetchNotificationSuccess: (state, { payload }) => {
-      const { notifications, numPages, currentPage } = payload;
-      const newNotificationIds = notifications.map(notification => notification.id.toString());
+      const {
+        newNotificationIds, notificationsKeyValuePair, numPages, currentPage,
+      } = payload;
       const existingNotificationIds = state.apps[state.appName];
-      const notificationsKeyValuePair = notifications.reduce((acc, obj) => { acc[obj.id] = obj; return acc; }, {});
-      const currentAppCount = state.tabsCount[state.appName];
 
       state.apps[state.appName] = Array.from(new Set([...existingNotificationIds, ...newNotificationIds]));
       state.notifications = { ...state.notifications, ...notificationsKeyValuePair };
-      state.tabsCount.count -= currentAppCount;
+      state.tabsCount.count -= state.tabsCount[state.appName];
       state.tabsCount[state.appName] = 0;
-      state.notificationStatus = LOADED;
+      state.notificationStatus = RequestStatus.LOADED;
       state.pagination.numPages = numPages;
       state.pagination.currentPage = currentPage;
     },
     fetchNotificationsCountDenied: (state) => {
-      state.notificationStatus = DENIED;
+      state.notificationStatus = RequestStatus.DENIED;
     },
     fetchNotificationsCountFailure: (state) => {
-      state.notificationStatus = FAILED;
+      state.notificationStatus = RequestStatus.FAILED;
     },
     fetchNotificationsCountRequest: (state) => {
-      state.notificationStatus = LOADING;
+      state.notificationStatus = RequestStatus.LOADING;
     },
     fetchNotificationsCountSuccess: (state, { payload }) => {
-      const { countByAppName, count, showNotificationTray } = payload;
+      const {
+        countByAppName, appIds, apps, count, showNotificationTray,
+      } = payload;
       state.tabsCount = { count, ...countByAppName };
-      state.appsId = Object.keys(countByAppName);
-      state.apps = Object.fromEntries(Object.keys(countByAppName).map(key => [key, []]));
+      state.appsId = appIds;
+      state.apps = apps;
       state.showNotificationTray = showNotificationTray;
-      state.notificationStatus = LOADED;
+      state.notificationStatus = RequestStatus.LOADED;
     },
     markNotificationsAsSeenRequest: (state) => {
-      state.notificationStatus = LOADING;
+      state.notificationStatus = RequestStatus.LOADING;
     },
     markNotificationsAsSeenSuccess: (state) => {
-      state.notificationStatus = LOADED;
+      state.notificationStatus = RequestStatus.LOADED;
     },
     markNotificationsAsSeenDenied: (state) => {
-      state.notificationStatus = DENIED;
+      state.notificationStatus = RequestStatus.DENIED;
     },
     markNotificationsAsSeenFailure: (state) => {
-      state.notificationStatus = FAILED;
+      state.notificationStatus = RequestStatus.FAILED;
     },
     markAllNotificationsAsReadRequest: (state) => {
-      state.notificationStatus = LOADING;
+      state.notificationStatus = RequestStatus.LOADING;
     },
     markAllNotificationsAsReadSuccess: (state) => {
       const updatedNotifications = Object.fromEntries(
@@ -93,27 +92,27 @@ const slice = createSlice({
         ]),
       );
       state.notifications = updatedNotifications;
-      state.notificationStatus = LOADED;
+      state.notificationStatus = RequestStatus.LOADED;
     },
     markAllNotificationsAsReadDenied: (state) => {
-      state.notificationStatus = DENIED;
+      state.notificationStatus = RequestStatus.DENIED;
     },
     markAllNotificationsAsReadFailure: (state) => {
-      state.notificationStatus = FAILED;
+      state.notificationStatus = RequestStatus.FAILED;
     },
     markNotificationsAsReadRequest: (state) => {
-      state.notificationStatus = LOADING;
+      state.notificationStatus = RequestStatus.LOADING;
     },
     markNotificationsAsReadSuccess: (state, { payload }) => {
       const date = new Date().toISOString();
       state.notifications[payload.id] = { ...state.notifications[payload.id], lastRead: date };
-      state.notificationStatus = LOADED;
+      state.notificationStatus = RequestStatus.LOADED;
     },
     markNotificationsAsReadDenied: (state) => {
-      state.notificationStatus = DENIED;
+      state.notificationStatus = RequestStatus.DENIED;
     },
     markNotificationsAsReadFailure: (state) => {
-      state.notificationStatus = FAILED;
+      state.notificationStatus = RequestStatus.FAILED;
     },
     resetNotificationStateRequest: () => initialState,
     updateAppNameRequest: (state, { payload }) => {

src/Notifications/data/thunks.js

@@ -1,4 +1,3 @@
-import { camelCaseObject } from '@edx/frontend-platform';
 import {
   fetchNotificationSuccess,
   fetchNotificationRequest,
@@ -27,14 +26,29 @@ import {
 } from './api';
 import { getHttpErrorStatus } from '../utils';
 
-export const fetchNotificationList = ({
-  appName, page, pageSize,
-}) => (
+const normalizeNotificationCounts = ({ countByAppName, count, showNotificationTray }) => {
+  const appIds = Object.keys(countByAppName);
+  const apps = appIds.reduce((acc, appId) => { acc[appId] = []; return acc; }, {});
+  return {
+    countByAppName, appIds, apps, count, showNotificationTray,
+  };
+};
+
+const normalizeNotifications = ({ notifications }) => {
+  const newNotificationIds = notifications.map(notification => notification.id.toString());
+  const notificationsKeyValuePair = notifications.reduce((acc, obj) => { acc[obj.id] = obj; return acc; }, {});
+  return {
+    newNotificationIds, notificationsKeyValuePair,
+  };
+};
+
+export const fetchNotificationList = ({ appName, page, pageSize }) => (
   async (dispatch) => {
     try {
       dispatch(fetchNotificationRequest({ appName }));
       const data = await getNotifications(appName, page, pageSize);
-      dispatch(fetchNotificationSuccess(data));
+      const normalisedData = normalizeNotifications((data));
+      dispatch(fetchNotificationSuccess({ ...normalisedData, numPages: data.numPages, currentPage: data.currentPage }));
     } catch (error) {
       if (getHttpErrorStatus(error) === 403) {
         dispatch(fetchNotificationDenied(appName));
@@ -50,7 +64,13 @@ export const fetchAppsNotificationCount = () => (
     try {
       dispatch(fetchNotificationsCountRequest());
       const data = await getNotificationCounts();
-      dispatch(fetchNotificationsCountSuccess(camelCaseObject(data)));
+      const normalisedData = normalizeNotificationCounts((data));
+      dispatch(fetchNotificationsCountSuccess({
+        ...normalisedData,
+        countByAppName: data.countByAppName,
+        count: data.count,
+        showNotificationTray: data.showNotificationTray,
+      }));
     } catch (error) {
       if (getHttpErrorStatus(error) === 403) {
         dispatch(fetchNotificationsCountDenied());

src/Notifications/index.jsx

@@ -13,33 +13,33 @@ import { selectNotificationTabsCount } from './data/selectors';
 import { resetNotificationState } from './data/thunks';
 import { useIsOnLargeScreen, useIsOnMediumScreen } from './data/hook';
 import NotificationTabs from './NotificationTabs';
-import { messages } from './messages';
+import messages from './messages';
 
 const Notifications = () => {
   const intl = useIntl();
   const dispatch = useDispatch();
   const popoverRef = useRef(null);
   const buttonRef = useRef(null);
-  const [showNotificationTray, setShowNotificationTray] = useState(false);
+  const [enableNotificationTray, setEnableNotificationTray] = useState(false);
   const notificationCounts = useSelector(selectNotificationTabsCount());
   const isOnMediumScreen = useIsOnMediumScreen();
   const isOnLargeScreen = useIsOnLargeScreen();
 
   const hideNotificationTray = useCallback(() => {
-    setShowNotificationTray(prevState => !prevState);
+    setEnableNotificationTray(prevState => !prevState);
   }, []);
 
-  const handleClickOutside = useCallback((event) => {
+  const handleClickOutsideNotificationTray = useCallback((event) => {
     if (!popoverRef.current?.contains(event.target) && !buttonRef.current?.contains(event.target)) {
-      setShowNotificationTray(false);
+      setEnableNotificationTray(false);
     }
   }, []);
 
   useEffect(() => {
-    document.addEventListener('mousedown', handleClickOutside);
+    document.addEventListener('mousedown', handleClickOutsideNotificationTray);
 
     return () => {
-      document.removeEventListener('mousedown', handleClickOutside);
+      document.removeEventListener('mousedown', handleClickOutsideNotificationTray);
       dispatch(resetNotificationState());
     };
   }, []);
@@ -50,7 +50,7 @@ const Notifications = () => {
       key="bottom"
       placement="bottom"
       id="notificationTray"
-      show={showNotificationTray}
+      show={enableNotificationTray}
       overlay={(
         <Popover
           id="notificationTray"
@@ -75,7 +75,7 @@ const Notifications = () => {
     >
       <div ref={buttonRef}>
         <IconButton
-          isActive={showNotificationTray}
+          isActive={enableNotificationTray}
           alt="notification bell icon"
           onClick={hideNotificationTray}
           src={NotificationsNone}

src/Notifications/messages.js

@@ -1,7 +1,6 @@
 import { defineMessages } from '@edx/frontend-platform/i18n';
 
-// eslint-disable-next-line import/prefer-default-export
-export const messages = defineMessages({
+const messages = defineMessages({
   notificationTitle: {
     id: 'notification.title',
     defaultMessage: 'Notifications',
@@ -33,3 +32,5 @@ export const messages = defineMessages({
     description: 'Load more button to load more notifications',
   },
 });
+
+export default messages;

src/Notifications/utils.js

@@ -48,5 +48,5 @@ export const getIconByType = (type) => {
     commentLiked: { icon: ThumbUpOutline, class: 'text-primary-500' },
     edited: { icon: EditOutline, class: 'text-primary-500' },
   };
-  return iconMap[type] || null;
+  return iconMap[type] || { icon: PostOutline, class: 'text-primary-500' };
 };