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

feat: generic sidebar notification plugin #1379

Merged
merged 3 commits into from
May 7, 2024
Merged

feat: generic sidebar notification plugin #1379

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a86fb36
feat: make notification plugin api generic
zacharis278 May 2, 2024
041d22e
feat: include new sidebar and tests
zacharis278 May 6, 2024
eba6583
feat: tweak sidebar toggle
zacharis278 May 7, 2024
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
33 changes: 15 additions & 18 deletions src/course-home/outline-tab/OutlineTab.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,20 +124,6 @@ const OutlineTab = ({ intl }) => {
}
}, [location.search]);

const upgradeNotificationProps = {
offer,
verifiedMode,
accessExpiration,
contentTypeGatingEnabled: datesBannerInfo.contentTypeGatingEnabled,
marketingUrl,
upsellPageName: 'course_home',
userTimezone,
timeOffsetMillis,
courseId,
org,
shouldDisplayBorder: true,
};

return (
<>
<div data-learner-type={learnerType} className="row w-100 mx-0 my-3 justify-content-between">
Expand Down Expand Up @@ -210,11 +196,22 @@ const OutlineTab = ({ intl }) => {
)}
<CourseTools />
<PluginSlot
id="outline_tab"
pluginProps={upgradeNotificationProps}
testId="outline-tab-slot"
id="outline_tab_notifications_plugin"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed the name on these to align convention with existing plugins xxxxx_plugin

pluginProps={{ courseId }}
>
<UpgradeNotification {...upgradeNotificationProps} />
<UpgradeNotification
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

revert back to what's in main today, no change in behavior if the plugin is off.

offer={offer}
verifiedMode={verifiedMode}
accessExpiration={accessExpiration}
contentTypeGatingEnabled={datesBannerInfo.contentTypeGatingEnabled}
marketingUrl={marketingUrl}
upsellPageName="course_home"
userTimezone={userTimezone}
shouldDisplayBorder
timeOffsetMillis={timeOffsetMillis}
courseId={courseId}
org={org}
/>
</PluginSlot>
<CourseDates />
<CourseHandouts />
Expand Down
9 changes: 2 additions & 7 deletions src/course-home/outline-tab/OutlineTab.test.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,19 +132,14 @@ describe('Outline Tab', () => {
expect(expandedSectionNode).toHaveAttribute('aria-expanded', 'true');
});

it('renders the Notification wrapper', async () => {
it('includes outline_tab_notifications_plugin slot', async () => {
const { courseBlocks } = await buildMinimalCourseBlocks(courseId, 'Title', { resumeBlock: true });
setTabData({
course_blocks: { blocks: courseBlocks.blocks },
});
await fetchAndRender();

const pluginSlot = screen.getByTestId('outline-tab-slot');
expect(pluginSlot).toBeInTheDocument();

// The Upgrade Notification should be inside the PluginSlot.
const UpgradeNotification = pluginSlot.querySelector('.upgrade-notification');
expect(UpgradeNotification).toBeInTheDocument();
expect(screen.getByTestId('outline_tab_notifications_plugin')).toBeInTheDocument();
rijuma marked this conversation as resolved.
Show resolved Hide resolved
});

it('handles expand/collapse all button click', async () => {
Expand Down
48 changes: 27 additions & 21 deletions ...urse/new-sidebar/sidebars/discussions-notifications/notifications/NotificationsWidget.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,10 @@ const NotificationsWidget = () => {
verification_status: verificationStatus,
};

const onToggleSidebar = () => {
toggleSidebar(currentSidebar, WIDGETS.NOTIFICATIONS);
};

// After three seconds, update notificationSeen (to hide red dot)
useEffect(() => {
setTimeout(onNotificationSeen, 3000);
Expand All @@ -67,31 +71,33 @@ const NotificationsWidget = () => {

if (hideNotificationbar || !isNotificationbarAvailable) { return null; }

const upgradeNotificationProps = {
offer,
verifiedMode,
accessExpiration,
contentTypeGatingEnabled,
marketingUrl,
upsellPageName: 'in_course',
userTimezone,
timeOffsetMillis,
courseId,
org,
upgradeNotificationCurrentState,
setupgradeNotificationCurrentState: setUpgradeNotificationCurrentState, // TODO: Check typo in component?
shouldDisplayBorder: false,
toggleSidebar: () => toggleSidebar(currentSidebar, WIDGETS.NOTIFICATIONS),
};

return (
<div className="border border-light-400 rounded-sm" data-testid="notification-widget">
<PluginSlot
id="notification_widget"
pluginProps={upgradeNotificationProps}
testId="notification-widget-slot"
id="notification_widget_plugin"
pluginProps={{
courseId,
notificationCurrentState: upgradeNotificationCurrentState,
setNotificationCurrentState: setUpgradeNotificationCurrentState,
toggleSidebar: onToggleSidebar,
}}
>
<UpgradeNotification {...upgradeNotificationProps} />
<UpgradeNotification
offer={offer}
verifiedMode={verifiedMode}
accessExpiration={accessExpiration}
contentTypeGatingEnabled={contentTypeGatingEnabled}
marketingUrl={marketingUrl}
upsellPageName="in_course"
userTimezone={userTimezone}
shouldDisplayBorder={false}
timeOffsetMillis={timeOffsetMillis}
courseId={courseId}
org={org}
upgradeNotificationCurrentState={upgradeNotificationCurrentState}
setupgradeNotificationCurrentState={setUpgradeNotificationCurrentState}
toggleSidebar={onToggleSidebar}
/>
</PluginSlot>
</div>
);
Expand Down
20 changes: 16 additions & 4 deletions ...new-sidebar/sidebars/discussions-notifications/notifications/NotificationsWidget.test.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ describe('NotificationsWidget', () => {
});
});

it('renders upgrade card', async () => {
it('includes notification_widget_plugin slot', async () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for this and other components I split testing the plugin slot and behavior of the default content w/o a plugin since we expect the latter to be removed.

await fetchAndRender(
<SidebarContext.Provider value={{
currentSidebar: ID,
Expand All @@ -92,12 +92,24 @@ describe('NotificationsWidget', () => {
<NotificationsWidget />
</SidebarContext.Provider>,
);
expect(screen.getByTestId('notification_widget_plugin')).toBeInTheDocument();
});

const pluginSlot = screen.getByTestId('notification-widget-slot');
expect(pluginSlot).toBeInTheDocument();
it('renders upgrade card', async () => {
await fetchAndRender(
<SidebarContext.Provider value={{
currentSidebar: ID,
courseId,
hideNotificationbar: false,
isNotificationbarAvailable: true,
}}
>
<NotificationsWidget />
</SidebarContext.Provider>,
);

// The Upgrade Notification should be inside the PluginSlot.
const UpgradeNotification = pluginSlot.querySelector('.upgrade-notification');
const UpgradeNotification = document.querySelector('.upgrade-notification');
expect(UpgradeNotification).toBeInTheDocument();

expect(screen.getByRole('link', { name: 'Upgrade for $149' })).toBeInTheDocument();
Expand Down
41 changes: 21 additions & 20 deletions src/courseware/course/sidebar/sidebars/notifications/NotificationTray.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,22 +66,6 @@ const NotificationTray = ({ intl }) => {
sendTrackEvent('edx.ui.course.upgrade.old_sidebar.notifications', notificationTrayEventProperties);
}, []);

const upgradeNotificationProps = {
offer,
verifiedMode,
accessExpiration,
contentTypeGatingEnabled,
marketingUrl,
upsellPageName: 'in_course',
userTimezone,
shouldDisplayBorder: false,
timeOffsetMillis,
courseId,
org,
upgradeNotificationCurrentState,
setupgradeNotificationCurrentState: setUpgradeNotificationCurrentState, // TODO: Check typo in component?
};

return (
<SidebarBase
title={intl.formatMessage(messages.notificationTitle)}
Expand All @@ -93,11 +77,28 @@ const NotificationTray = ({ intl }) => {
<div>{verifiedMode
? (
<PluginSlot
id="notification_tray"
pluginProps={upgradeNotificationProps}
testId="notification-tray-slot"
id="notification_tray_plugin"
pluginProps={{
courseId,
notificationCurrentState: upgradeNotificationCurrentState,
Copy link
Contributor Author

@zacharis278 zacharis278 May 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This drives the 'red dot' notification icon. I've removed the 'upgrade' from how this is passed to the plugin to keep it general. On the learning MFE side all the variables relating to this are still called 'upgrade...' we should rename all of that eventually. Probably at the same time we remove all the upgrade code from the learning MFE.

I think these would belong on the API for this plugin slot since any plugin here may want to drive that icon. That said, the code for the red dot is tightly coupled to upgrade right now but you could use it creatively for other purposes. It would probably take further consideration on how we handle notifications from multiple sources (if you put more than 1 plugin here) when the need arises. My thinking is the existence of a getter/setter wouldn't need to change so what I have here should be a stable interface.

setNotificationCurrentState: setUpgradeNotificationCurrentState,
}}
>
<UpgradeNotification {...upgradeNotificationProps} />
<UpgradeNotification
offer={offer}
verifiedMode={verifiedMode}
accessExpiration={accessExpiration}
contentTypeGatingEnabled={contentTypeGatingEnabled}
marketingUrl={marketingUrl}
upsellPageName="in_course"
userTimezone={userTimezone}
shouldDisplayBorder={false}
timeOffsetMillis={timeOffsetMillis}
courseId={courseId}
org={org}
upgradeNotificationCurrentState={upgradeNotificationCurrentState}
setupgradeNotificationCurrentState={setUpgradeNotificationCurrentState}
/>
</PluginSlot>
) : (
<p className="p-3 small">{intl.formatMessage(messages.noNotificationsMessage)}</p>
Expand Down
20 changes: 14 additions & 6 deletions src/courseware/course/sidebar/sidebars/notifications/NotificationTray.test.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ describe('NotificationTray', () => {
.toBeInTheDocument();
});

it('renders upgrade card', async () => {
it('includes notification_tray_plugin slot', async () => {
await fetchAndRender(
<SidebarContext.Provider value={{
currentSidebar: ID,
Expand All @@ -91,13 +91,21 @@ describe('NotificationTray', () => {
<NotificationTray />
</SidebarContext.Provider>,
);
expect(screen.getByTestId('notification_tray_plugin')).toBeInTheDocument();
});

const pluginSlot = screen.getByTestId('notification-tray-slot');
expect(pluginSlot).toBeInTheDocument();
it('renders upgrade card', async () => {
await fetchAndRender(
<SidebarContext.Provider value={{
currentSidebar: ID,
courseId,
}}
>
<NotificationTray />
</SidebarContext.Provider>,
);

// The Upgrade Notification should be inside the PluginSlot.
const UpgradeNotification = pluginSlot.querySelector('.upgrade-notification');
expect(UpgradeNotification).toBeInTheDocument();
expect(document.querySelector('.upgrade-notification')).toBeInTheDocument();

expect(screen.getByRole('link', { name: 'Upgrade for $149' }))
.toBeInTheDocument();
Expand Down
15 changes: 8 additions & 7 deletions src/tests/MockedPluginSlot.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
import React from 'react';
import PropTypes from 'prop-types';

const MockedPluginSlot = ({ children, testId }) => {
if (!testId) { return children ?? 'PluginSlot'; } // Return its content if PluginSlot slot is wrapping any.

return <div data-testid={testId}>{children}</div>;
};
const MockedPluginSlot = ({ children, id }) => (
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to modify how this works a bit because we get react errors if testId is passed to the PluginSlot component. We can only pass id, pluginProps, and children. I opted to use id instead here since that would be required to have a functional plugin slot.

<div data-testid={id}>
PluginSlot_{id}
{ children && <div>{children}</div> }
</div>
);

MockedPluginSlot.displayName = 'PluginSlot';

Expand All @@ -14,12 +15,12 @@ MockedPluginSlot.propTypes = {
PropTypes.arrayOf(PropTypes.node),
PropTypes.node,
]),
testId: PropTypes.string,
id: PropTypes.string,
};

MockedPluginSlot.defaultProps = {
children: undefined,
testId: undefined,
id: undefined,
};

export default MockedPluginSlot;
12 changes: 6 additions & 6 deletions src/tests/MockedPluginSlot.test.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,14 @@ import { render, screen } from '@testing-library/react';
import MockedPluginSlot from './MockedPluginSlot';

describe('MockedPluginSlot', () => {
it('renders as plain "PluginSlot" text node if no clildren nor testId is', () => {
render(<MockedPluginSlot />);
it('renders mock plugin with "PluginSlot" text', () => {
render(<MockedPluginSlot id="test_plugin" />);

const component = screen.getByText('PluginSlot');
const component = screen.getByText('PluginSlot_test_plugin');
expect(component).toBeInTheDocument();
});

it('renders as the slot children directly if there is content within and no testId', () => {
it('renders as the slot children directly if there is content within', () => {
render(
<div role="article">
<MockedPluginSlot>
Expand All @@ -27,9 +27,9 @@ describe('MockedPluginSlot', () => {
expect(quote.getAttribute('role')).toBe('note');
});

it('renders a div when a testId is provided ', () => {
it('renders mock plugin with a data-testid ', () => {
render(
<MockedPluginSlot testId="guybrush">
<MockedPluginSlot id="guybrush">
<q role="note">I am selling these fine leather jackets.</q>
</MockedPluginSlot>,
);
Expand Down
Loading