chore: update sidebar logic (#1236)

* fix: make side bar notification behave correctly --------- Co-authored-by: Awais Ansari <[email protected]>
openedx · Nov 22, 2023 · bce25c4 · bce25c4
1 parent fe773d1
commit bce25c4
Show file tree

Hide file tree

Showing 7 changed files with 19 additions and 82 deletions.
diff --git a/src/courseware/course/Course.jsx b/src/courseware/course/Course.jsx
@@ -17,7 +17,6 @@ import SidebarProvider from './sidebar/SidebarContextProvider';
 import SidebarTriggers from './sidebar/SidebarTriggers';
 
 import { useModel } from '../../generic/model-store';
-import { getSessionStorage, setSessionStorage } from '../../data/sessionStorage';
 
 const Course = ({
   courseId,
@@ -32,7 +31,6 @@ const Course = ({
   const {
     celebrations,
     isStaff,
-    verifiedMode,
   } = useModel('courseHomeMeta', courseId);
   const sequence = useModel('sequences', sequenceId);
   const section = useModel('sections', sequence ? sequence.sectionId : null);
@@ -55,16 +53,6 @@ const Course = ({
   const shouldDisplayTriggers = windowWidth >= breakpoints.small.minWidth;
   const daysPerWeek = course?.courseGoals?.selectedGoal?.daysPerWeek;
 
-  // 1. open the notification tray if the user has not purchased the course and is on a desktop
-  // 2. default to close on the first time access
-  // 3. use the last state if the user has access the course before
-  const shouldDisplayNotificationTrayOpenOnLoad = windowWidth > breakpoints.medium.minWidth && !!verifiedMode;
-  if (shouldDisplayNotificationTrayOpenOnLoad) {
-    setSessionStorage(`notificationTrayStatus.${courseId}`, 'open');
-  } else if (!getSessionStorage(`notificationTrayStatus.${courseId}`)) {
-    setSessionStorage(`notificationTrayStatus.${courseId}`, 'closed');
-  }
-
   useEffect(() => {
     const celebrateFirstSection = celebrations && celebrations.firstSection;
     setFirstSectionCelebrationOpen(shouldCelebrateOnSectionLoad(

diff --git a/src/courseware/course/Course.test.jsx b/src/courseware/course/Course.test.jsx
@@ -43,8 +43,6 @@ describe('Course', () => {
       sequenceId,
       unitId: Object.values(models.units)[0].id,
     });
-    getItemSpy = jest.spyOn(Object.getPrototypeOf(window.sessionStorage), 'getItem');
-    setItemSpy = jest.spyOn(Object.getPrototypeOf(window.sessionStorage), 'setItem');
     global.innerWidth = breakpoints.extraLarge.minWidth;
   });
 
@@ -54,7 +52,8 @@ describe('Course', () => {
   });
 
   const setupDiscussionSidebar = async () => {
-    const testStore = await initializeTestStore({ provider: 'openedx' });
+    const courseHomeMetadata = Factory.build('courseHomeMetadata', { verified_mode: null });
+    const testStore = await initializeTestStore({ provider: 'openedx', courseHomeMetadata });
     const state = testStore.getState();
     const { courseware: { courseId } } = state;
     const axiosMock = new MockAdapter(getAuthenticatedHttpClient());
@@ -136,9 +135,9 @@ describe('Course', () => {
 
     const notificationTrigger = screen.getByRole('button', { name: /Show notification tray/i });
     expect(notificationTrigger).toBeInTheDocument();
-    expect(notificationTrigger.parentNode).toHaveClass('mt-3');
-    fireEvent.click(notificationTrigger);
     expect(notificationTrigger.parentNode).not.toHaveClass('mt-3', { exact: true });
+    fireEvent.click(notificationTrigger);
+    expect(notificationTrigger.parentNode).toHaveClass('mt-3');
   });
 
   it('handles click to open/close discussions sidebar', async () => {
@@ -184,54 +183,13 @@ describe('Course', () => {
   });
 
   it('handles click to open/close notification tray', async () => {
-    sessionStorage.clear();
     render(<Course {...mockData} />, { wrapWithRouter: true });
-    expect(sessionStorage.getItem(`notificationTrayStatus.${mockData.courseId}`)).toBe('"open"');
     const notificationShowButton = await screen.findByRole('button', { name: /Show notification tray/i });
     expect(screen.queryByRole('region', { name: /notification tray/i })).toHaveClass('d-none');
     fireEvent.click(notificationShowButton);
-    expect(sessionStorage.getItem(`notificationTrayStatus.${mockData.courseId}`)).toBe('"closed"');
     expect(screen.queryByRole('region', { name: /notification tray/i })).not.toHaveClass('d-none');
   });
 
-  it('handles reload persisting notification tray status', async () => {
-    sessionStorage.clear();
-    render(<Course {...mockData} />, { wrapWithRouter: true });
-    const notificationShowButton = await screen.findByRole('button', { name: /Show notification tray/i });
-    fireEvent.click(notificationShowButton);
-    expect(sessionStorage.getItem(`notificationTrayStatus.${mockData.courseId}`)).toBe('"closed"');
-
-    // Mock reload window, this doesn't happen in the Course component,
-    // calling the reload to check if the tray remains closed
-    const { location } = window;
-    delete window.location;
-    window.location = { reload: jest.fn() };
-    window.location.reload();
-    expect(window.location.reload).toHaveBeenCalled();
-    window.location = location;
-    expect(sessionStorage.getItem(`notificationTrayStatus.${mockData.courseId}`)).toBe('"closed"');
-    expect(screen.queryByTestId('NotificationTray')).not.toBeInTheDocument();
-  });
-
-  it('handles sessionStorage from a different course for the notification tray', async () => {
-    sessionStorage.clear();
-    const courseMetadataSecondCourse = Factory.build('courseMetadata', { id: 'second_course' });
-
-    // set sessionStorage for a different course before rendering Course
-    sessionStorage.setItem(`notificationTrayStatus.${courseMetadataSecondCourse.id}`, '"open"');
-
-    render(<Course {...mockData} />, { wrapWithRouter: true });
-    expect(sessionStorage.getItem(`notificationTrayStatus.${mockData.courseId}`)).toBe('"open"');
-    const notificationShowButton = await screen.findByRole('button', { name: /Show notification tray/i });
-    fireEvent.click(notificationShowButton);
-
-    // Verify sessionStorage was updated for the original course
-    expect(sessionStorage.getItem(`notificationTrayStatus.${mockData.courseId}`)).toBe('"closed"');
-
-    // Verify the second course sessionStorage was not changed
-    expect(sessionStorage.getItem(`notificationTrayStatus.${courseMetadataSecondCourse.id}`)).toBe('"open"');
-  });
-
   it('renders course breadcrumbs as expected', async () => {
     const courseMetadata = Factory.build('courseMetadata');
     const unitBlocks = Array.from({ length: 3 }).map(() => Factory.build(

diff --git a/src/courseware/course/sidebar/SidebarContextProvider.jsx b/src/courseware/course/sidebar/SidebarContextProvider.jsx
@@ -4,7 +4,9 @@ import React, {
   useEffect, useState, useMemo, useCallback,
 } from 'react';
 
+import { useModel } from '../../../generic/model-store';
 import { getLocalStorage, setLocalStorage } from '../../../data/localStorage';
+
 import SidebarContext from './SidebarContext';
 import { SIDEBARS } from './sidebars';
 
@@ -13,6 +15,7 @@ const SidebarProvider = ({
   unitId,
   children,
 }) => {
+  const { verifiedMode } = useModel('courseHomeMeta', courseId);
   const shouldDisplayFullScreen = useWindowSize().width < breakpoints.large.minWidth;
   const shouldDisplaySidebarOpen = useWindowSize().width > breakpoints.medium.minWidth;
   const query = new URLSearchParams(window.location.search);
@@ -22,8 +25,8 @@ const SidebarProvider = ({
   const [upgradeNotificationCurrentState, setUpgradeNotificationCurrentState] = useState(getLocalStorage(`upgradeNotificationCurrentState.${courseId}`));
 
   useEffect(() => {
-    setCurrentSidebar(SIDEBARS.DISCUSSIONS.ID);
-  // eslint-disable-next-line react-hooks/exhaustive-deps
+    // if the user hasn't purchased the course, show the notifications sidebar
+    setCurrentSidebar(verifiedMode ? SIDEBARS.NOTIFICATIONS.ID : SIDEBARS.DISCUSSIONS.ID);
   }, [unitId]);
 
   const onNotificationSeen = useCallback(() => {

diff --git a/src/courseware/course/sidebar/sidebars/index.js b/src/courseware/course/sidebar/sidebars/index.js
@@ -1,20 +1,20 @@
 import * as notifications from './notifications';
-import * as discusssions from './discussions';
+import * as discussions from './discussions';
 
 export const SIDEBARS = {
   [notifications.ID]: {
     ID: notifications.ID,
     Sidebar: notifications.Sidebar,
     Trigger: notifications.Trigger,
   },
-  [discusssions.ID]: {
-    ID: discusssions.ID,
-    Sidebar: discusssions.Sidebar,
-    Trigger: discusssions.Trigger,
+  [discussions.ID]: {
+    ID: discussions.ID,
+    Sidebar: discussions.Sidebar,
+    Trigger: discussions.Trigger,
   },
 };
 
 export const SIDEBAR_ORDER = [
-  discusssions.ID,
+  discussions.ID,
   notifications.ID,
 ];
diff --git a/src/courseware/course/sidebar/sidebars/notifications/NotificationTray.jsx b/src/courseware/course/sidebar/sidebars/notifications/NotificationTray.jsx
@@ -42,6 +42,7 @@ const NotificationTray = ({ intl }) => {
       title={intl.formatMessage(messages.notificationTitle)}
       ariaLabel={intl.formatMessage(messages.notificationTray)}
       sidebarId={ID}
+      width="50rem"
       className={classNames({ 'h-100': !verifiedMode && !shouldDisplayFullScreen })}
     >
       <div>{verifiedMode

diff --git a/src/courseware/course/sidebar/sidebars/notifications/NotificationTrigger.jsx b/src/courseware/course/sidebar/sidebars/notifications/NotificationTrigger.jsx
@@ -2,7 +2,6 @@ import { injectIntl, intlShape } from '@edx/frontend-platform/i18n';
 import PropTypes from 'prop-types';
 import React, { useContext, useEffect } from 'react';
 import { getLocalStorage, setLocalStorage } from '../../../../../data/localStorage';
-import { getSessionStorage, setSessionStorage } from '../../../../../data/sessionStorage';
 import messages from '../../../messages';
 import SidebarTriggerBase from '../../common/TriggerBase';
 import SidebarContext from '../../SidebarContext';
@@ -47,17 +46,8 @@ const NotificationTrigger = ({
     UpdateUpgradeNotificationLastSeen();
   });
 
-  const handleClick = () => {
-    if (getSessionStorage(`notificationTrayStatus.${courseId}`) === 'open') {
-      setSessionStorage(`notificationTrayStatus.${courseId}`, 'closed');
-    } else {
-      setSessionStorage(`notificationTrayStatus.${courseId}`, 'open');
-    }
-    onClick();
-  };
-
   return (
-    <SidebarTriggerBase onClick={handleClick} ariaLabel={intl.formatMessage(messages.openNotificationTrigger)}>
+    <SidebarTriggerBase onClick={onClick} ariaLabel={intl.formatMessage(messages.openNotificationTrigger)}>
       <NotificationIcon status={notificationStatus} notificationColor="bg-danger-500" />
     </SidebarTriggerBase>
   );

diff --git a/src/data/sessionStorage.js b/src/data/sessionStorage.js
@@ -7,10 +7,7 @@
 function getSessionStorage(key) {
   try {
     if (global.sessionStorage) {
-      const rawItem = global.sessionStorage.getItem(key);
-      if (rawItem) {
-        return JSON.parse(rawItem);
-      }
+      return global.sessionStorage.getItem(key);
     }
   } catch (e) {
     // If this fails for some reason, just return null.
@@ -21,7 +18,7 @@ function getSessionStorage(key) {
 function setSessionStorage(key, value) {
   try {
     if (global.sessionStorage) {
-      global.sessionStorage.setItem(key, JSON.stringify(value));
+      global.sessionStorage.setItem(key, value);
     }
   } catch (e) {
     // If this fails, just bail.