Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

revert: notification made forum click rate drop #1237

Merged
merged 1 commit into from
Nov 17, 2023
Merged

revert: notification made forum click rate drop #1237

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions src/courseware/course/Course.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import SidebarTriggers from './sidebar/SidebarTriggers';

import { useModel } from '../../generic/model-store';
import { getSessionStorage, setSessionStorage } from '../../data/sessionStorage';

const Course = ({
courseId,
Expand All @@ -31,6 +32,7 @@
const {
celebrations,
isStaff,
verifiedMode,
} = useModel('courseHomeMeta', courseId);
const sequence = useModel('sequences', sequenceId);
const section = useModel('sections', sequence ? sequence.sectionId : null);
Expand All @@ -53,6 +55,16 @@
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');

Check warning on line 65 in src/courseware/course/Course.jsx

View check run for this annotation

Codecov / codecov/patch

src/courseware/course/Course.jsx#L65 

Added line #L65 was not covered by tests
}

useEffect(() => {
const celebrateFirstSection = celebrations && celebrations.firstSection;
setFirstSectionCelebrationOpen(shouldCelebrateOnSectionLoad(
Expand Down
34 changes: 15 additions & 19 deletions src/courseware/course/Course.test.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,27 +136,27 @@ describe('Course', () => {

const notificationTrigger = screen.getByRole('button', { name: /Show notification tray/i });
expect(notificationTrigger).toBeInTheDocument();
expect(notificationTrigger.parentNode).not.toHaveClass('mt-3', { exact: true });
fireEvent.click(notificationTrigger);
expect(notificationTrigger.parentNode).toHaveClass('mt-3');
fireEvent.click(notificationTrigger);
expect(notificationTrigger.parentNode).not.toHaveClass('mt-3', { exact: true });
});

it('handles click to open/close discussions sidebar', async () => {
await setupDiscussionSidebar();
const discussionsTrigger = await screen.getByRole('button', { name: /Show discussions tray/i });
const discussionsSideBar = await waitFor(() => screen.findByTestId('sidebar-DISCUSSIONS'));

expect(discussionsSideBar).toHaveClass('d-none');
expect(discussionsSideBar).not.toHaveClass('d-none');

await act(async () => {
fireEvent.click(discussionsTrigger);
});
await expect(discussionsSideBar).not.toHaveClass('d-none');
await expect(discussionsSideBar).toHaveClass('d-none');

await act(async () => {
fireEvent.click(discussionsTrigger);
});
await expect(discussionsSideBar).toHaveClass('d-none');
await expect(discussionsSideBar).not.toHaveClass('d-none');
});

it('displays discussions sidebar when unit changes', async () => {
Expand All @@ -177,9 +177,6 @@ describe('Course', () => {

await waitFor(() => {
expect(screen.getByTestId('sidebar-DISCUSSIONS')).toBeInTheDocument();
expect(screen.getByTestId('sidebar-DISCUSSIONS')).toHaveClass('d-none');
const discussionsTrigger = screen.getByRole('button', { name: /Show discussions tray/i });
fireEvent.click(discussionsTrigger);
expect(screen.getByTestId('sidebar-DISCUSSIONS')).not.toHaveClass('d-none');
});

Expand All @@ -189,21 +186,20 @@ 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(null);
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 })).not.toHaveClass('d-none');
fireEvent.click(notificationShowButton);
expect(sessionStorage.getItem(`notificationTrayStatus.${mockData.courseId}`)).toBe('open');
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 });
expect(sessionStorage.getItem(`notificationTrayStatus.${mockData.courseId}`)).toBe(null);
fireEvent.click(notificationShowButton);
expect(sessionStorage.getItem(`notificationTrayStatus.${mockData.courseId}`)).toBe('open');
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
Expand All @@ -213,7 +209,7 @@ describe('Course', () => {
window.location.reload();
expect(window.location.reload).toHaveBeenCalled();
window.location = location;
expect(sessionStorage.getItem(`notificationTrayStatus.${mockData.courseId}`)).toBe('open');
expect(sessionStorage.getItem(`notificationTrayStatus.${mockData.courseId}`)).toBe('"closed"');
expect(screen.queryByTestId('NotificationTray')).not.toBeInTheDocument();
});

Expand All @@ -222,18 +218,18 @@ describe('Course', () => {
const courseMetadataSecondCourse = Factory.build('courseMetadata', { id: 'second_course' });

// set sessionStorage for a different course before rendering Course
sessionStorage.setItem(`notificationTrayStatus.${courseMetadataSecondCourse.id}`, 'open');
sessionStorage.setItem(`notificationTrayStatus.${courseMetadataSecondCourse.id}`, '"open"');

render(<Course {...mockData} />, { wrapWithRouter: true });
expect(sessionStorage.getItem(`notificationTrayStatus.${mockData.courseId}`)).toBe(null);
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('open');
expect(sessionStorage.getItem(`notificationTrayStatus.${mockData.courseId}`)).toBe('"closed"');

// Verify the second course sessionStorage was not changed
expect(sessionStorage.getItem(`notificationTrayStatus.${courseMetadataSecondCourse.id}`)).toBe('open');
expect(sessionStorage.getItem(`notificationTrayStatus.${courseMetadataSecondCourse.id}`)).toBe('"open"');
});

it('renders course breadcrumbs as expected', async () => {
Expand Down
11 changes: 1 addition & 10 deletions src/courseware/course/sidebar/SidebarContextProvider.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,7 @@ import React, {
useEffect, useState, useMemo, useCallback,
} from 'react';

import { useModel } from '../../../generic/model-store';
import { getLocalStorage, setLocalStorage } from '../../../data/localStorage';
import { getSessionStorage } from '../../../data/sessionStorage';

import SidebarContext from './SidebarContext';
import { SIDEBARS } from './sidebars';

Expand All @@ -16,8 +13,6 @@ 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);
Expand All @@ -27,11 +22,7 @@ const SidebarProvider = ({
const [upgradeNotificationCurrentState, setUpgradeNotificationCurrentState] = useState(getLocalStorage(`upgradeNotificationCurrentState.${courseId}`));

useEffect(() => {
// if the user has not purchased the course or previously opened the notification tray, show the notification tray
const showNotificationsOnLoad = !!verifiedMode || getSessionStorage(`notificationTrayStatus.${courseId}`) !== 'closed';
if (showNotificationsOnLoad) {
setCurrentSidebar(SIDEBARS.NOTIFICATIONS.ID);
}
setCurrentSidebar(SIDEBARS.DISCUSSIONS.ID);
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [unitId]);

Expand Down
2 changes: 1 addition & 1 deletion src/data/sessionStorage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ function getSessionStorage(key) {
function setSessionStorage(key, value) {
try {
if (global.sessionStorage) {
global.sessionStorage.setItem(key, value);
global.sessionStorage.setItem(key, JSON.stringify(value));
}
} catch (e) {
// If this fails, just bail.
Expand Down