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

Jump to bottom

feat: Notification Plugin Slots #1368

Merged
merged 8 commits into from
May 13, 2024
Merged

feat: Notification Plugin Slots #1368

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
377da57
feat: add plugin slot for fbe lock paywall (#1347)
alangsto Apr 9, 2024
d3ef306
feat: Added PluginSlot wrapping UpgradeNotification components (#1366)
rijuma Apr 19, 2024
1b5337a
feat: Added Plugin Slot wrapping UpgradeNotification in NotificationT…
rijuma Apr 22, 2024
1e29715
fix: Removed PluginSlot prop scoping for UpgradeNotification (#1369)
rijuma Apr 23, 2024
0989559
feat: generic sidebar notification plugin (#1379)
Zacharis278 May 7, 2024
144a80a
Merge branch 'master' into feat/upgrade-plugin-slots
Zacharis278 May 7, 2024
af3eaf6
style: fix extra space from merge
Zacharis278 May 7, 2024
71a23d2
feat: rename plugin slots
Zacharis278 May 13, 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
32 changes: 19 additions & 13 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 @@ -5,6 +5,7 @@ import { sendTrackEvent } from '@edx/frontend-platform/analytics';
import { getAuthenticatedUser } from '@edx/frontend-platform/auth';
import { injectIntl, intlShape } from '@edx/frontend-platform/i18n';
import { Button } from '@openedx/paragon';
import { PluginSlot } from '@openedx/frontend-plugin-framework';
import { AlertList } from '../../generic/user-messages';

import CourseDates from './widgets/CourseDates';
Expand Down Expand Up @@ -194,19 +195,24 @@ const OutlineTab = ({ intl }) => {
/>
)}
<CourseTools />
<UpgradeNotification
offer={offer}
verifiedMode={verifiedMode}
accessExpiration={accessExpiration}
contentTypeGatingEnabled={datesBannerInfo.contentTypeGatingEnabled}
marketingUrl={marketingUrl}
upsellPageName="course_home"
userTimezone={userTimezone}
shouldDisplayBorder
timeOffsetMillis={timeOffsetMillis}
courseId={courseId}
org={org}
/>
<PluginSlot
id="outline_tab_notifications_plugin"
pluginProps={{ courseId }}
>
<UpgradeNotification
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 />
</div>
Expand Down
10 changes: 10 additions & 0 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,6 +132,16 @@ describe('Outline Tab', () => {
expect(expandedSectionNode).toHaveAttribute('aria-expanded', 'true');
});

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

expect(screen.getByTestId('outline_tab_notifications_plugin')).toBeInTheDocument();
});

it('handles expand/collapse all button click', async () => {
await fetchAndRender();
// Button renders as "Expand All"
Expand Down
47 changes: 31 additions & 16 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
@@ -1,6 +1,7 @@
import React, { useContext, useEffect, useMemo } from 'react';

import { sendTrackEvent } from '@edx/frontend-platform/analytics';
import { PluginSlot } from '@openedx/frontend-plugin-framework';
import { useModel } from '../../../../../../generic/model-store';
import UpgradeNotification from '../../../../../../generic/upgrade-notification/UpgradeNotification';
import { WIDGETS } from '../../../../../../constants';
Expand Down Expand Up @@ -58,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 @@ -68,22 +73,32 @@ const NotificationsWidget = () => {

return (
<div className="border border-light-400 rounded-sm" data-testid="notification-widget">
<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={() => toggleSidebar(currentSidebar, WIDGETS.NOTIFICATIONS)}
/>
<PluginSlot
id="notification_widget_slot"
pluginProps={{
courseId,
notificationCurrentState: upgradeNotificationCurrentState,
setNotificationCurrentState: setUpgradeNotificationCurrentState,
toggleSidebar: onToggleSidebar,
}}
>
<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
19 changes: 18 additions & 1 deletion ...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 @@ -78,6 +78,21 @@ describe('NotificationsWidget', () => {
});
});

it('includes notification_widget_slot', async () => {
await fetchAndRender(
<SidebarContext.Provider value={{
currentSidebar: ID,
courseId,
hideNotificationbar: false,
isNotificationbarAvailable: true,
}}
>
<NotificationsWidget />
</SidebarContext.Provider>,
);
expect(screen.getByTestId('notification_widget_slot')).toBeInTheDocument();
});

it('renders upgrade card', async () => {
await fetchAndRender(
<SidebarContext.Provider value={{
Expand All @@ -90,9 +105,11 @@ describe('NotificationsWidget', () => {
<NotificationsWidget />
</SidebarContext.Provider>,
);
const UpgradeNotification = document.querySelector('.upgrade-notification');

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

expect(screen.getByRole('link', { name: 'Upgrade for $149' })).toBeInTheDocument();
expect(screen.queryByText('You have no new notifications at this time.')).not.toBeInTheDocument();
});
Expand Down
22 changes: 14 additions & 8 deletions src/courseware/course/sequence/Unit/UnitSuspense.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import React, { Suspense } from 'react';
import PropTypes from 'prop-types';

import { useIntl } from '@edx/frontend-platform/i18n';
import { PluginSlot } from '@openedx/frontend-plugin-framework';

import { useModel } from '@src/generic/model-store';
import PageLoading from '@src/generic/PageLoading';
Expand All @@ -24,19 +25,24 @@ const UnitSuspense = ({
meta.contentTypeGatingEnabled && unit.containsContentTypeGatedContent
);

const suspenseComponent = (message, Component) => (
<Suspense fallback={<PageLoading srMessage={formatMessage(message)} />}>
<Component courseId={courseId} />
</Suspense>
);

return (
<>
{shouldDisplayContentGating && (
suspenseComponent(messages.loadingLockedContent, LockPaywall)
<Suspense fallback={<PageLoading srMessage={formatMessage(messages.loadingLockedContent)} />}>
<PluginSlot
id="gated_unit_content_message_slot"
pluginProps={{
courseId,
}}
>
<LockPaywall courseId={courseId} />
Copy link

Choose a reason for hiding this comment

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

courseId seems a harmless enough prop to pass down, but like above, we're going to want to make this as generic a slot as possible. Otherwise, the same concept applies as to any other slot: does it make sense for LockPaywall to remain on by default? I suspect not, so it should be removed (and/or moved into a plugin elsewhere).

Copy link
Contributor

Choose a reason for hiding this comment

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

re: "does it make sense for LockPaywall to remain on by default?" This is kinda the same as my comments above on 'DEPR'. I think my answer is initially yes we keep the default unchanged to limit risk. If this new plugin is deployed and we're confident there's no issues with the new experience we can remove the old default. This is a very sensitive component to alter. I'd like to keep 'turn off the plugin' as a switch we can pull rather than a large revert if there are problems.

Copy link

Choose a reason for hiding this comment

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

Do you have an idea of what the timeframe would be to gain the confidence necessary to do the removal? We need not remove the component itself from the codebase, but it would be best if we could at least remove it as default content inside the PluginSlot by Redwood.

Copy link
Contributor

@Zacharis278 Zacharis278 May 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

If we aren't actually removing the existing component we can import it inside the plugin as a second step. I'd say we can follow up with that part very quickly, maybe In a week or so.

edit: a week after we turn this on, not a week after this is merged.

</PluginSlot>
</Suspense>
)}
{shouldDisplayHonorCode && (
suspenseComponent(messages.loadingHonorCode, HonorCode)
<Suspense fallback={<PageLoading srMessage={formatMessage(messages.loadingHonorCode)} />}>
<HonorCode courseId={courseId} />
</Suspense>
)}
</>
);
Expand Down
7 changes: 4 additions & 3 deletions src/courseware/course/sequence/Unit/UnitSuspense.test.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ describe('UnitSuspense component', () => {
describe('output', () => {
describe('LockPaywall', () => {
const testNoPaywall = () => {
it('does not display LockPaywal', () => {
it('does not display LockPaywall', () => {
el = shallow(<UnitSuspense {...props} />);
expect(el.instance.findByType(LockPaywall).length).toEqual(0);
});
Expand All @@ -79,8 +79,9 @@ describe('UnitSuspense component', () => {
it('displays LockPaywall in Suspense wrapper with PageLoading fallback', () => {
el = shallow(<UnitSuspense {...props} />);
const [component] = el.instance.findByType(LockPaywall);
expect(component.parent.type).toEqual('Suspense');
expect(component.parent.props.fallback)
expect(component.parent.type).toEqual('PluginSlot');
expect(component.parent.parent.type).toEqual('Suspense');
expect(component.parent.parent.props.fallback)
.toEqual(<PageLoading srMessage={formatMessage(messages.loadingLockedContent)} />);
expect(component.props.courseId).toEqual(props.courseId);
});
Expand Down
44 changes: 27 additions & 17 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 @@ -2,8 +2,9 @@ import { injectIntl, intlShape } from '@edx/frontend-platform/i18n';
import classNames from 'classnames';
import { useContext, useEffect, useMemo } from 'react';
import { sendTrackEvent } from '@edx/frontend-platform/analytics';
import { useModel } from '@src/generic/model-store';
import UpgradeNotification from '@src/generic/upgrade-notification/UpgradeNotification';
import { PluginSlot } from '@openedx/frontend-plugin-framework';
import { useModel } from '../../../../../generic/model-store';
import UpgradeNotification from '../../../../../generic/upgrade-notification/UpgradeNotification';

import messages from '../../../messages';
import SidebarBase from '../../common/SidebarBase';
Expand Down Expand Up @@ -77,21 +78,30 @@ const NotificationTray = ({ intl }) => {
>
<div>{verifiedMode
? (
<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
id="notification_tray_plugin"
pluginProps={{
courseId,
notificationCurrentState: upgradeNotificationCurrentState,
setNotificationCurrentState: setUpgradeNotificationCurrentState,
}}
>
<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
18 changes: 15 additions & 3 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,6 +81,19 @@ describe('NotificationTray', () => {
.toBeInTheDocument();
});

it('includes notification_tray_plugin slot', async () => {
await fetchAndRender(
<SidebarContext.Provider value={{
currentSidebar: ID,
courseId,
}}
>
<NotificationTray />
</SidebarContext.Provider>,
);
expect(screen.getByTestId('notification_tray_plugin')).toBeInTheDocument();
});

it('renders upgrade card', async () => {
await fetchAndRender(
<SidebarContext.Provider value={{
Expand All @@ -91,10 +104,9 @@ describe('NotificationTray', () => {
<NotificationTray />
</SidebarContext.Provider>,
);
const UpgradeNotification = document.querySelector('.upgrade-notification');

expect(UpgradeNotification)
.toBeInTheDocument();
expect(document.querySelector('.upgrade-notification')).toBeInTheDocument();

expect(screen.getByRole('link', { name: 'Upgrade for $149' }))
.toBeInTheDocument();
expect(screen.queryByText('You have no new notifications at this time.'))
Expand Down
3 changes: 2 additions & 1 deletion src/setupTest.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,12 @@ import { getCourseOutlineStructure } from './courseware/data/thunks';
import { appendBrowserTimezoneToUrl, executeThunk } from './utils';
import buildSimpleCourseAndSequenceMetadata from './courseware/data/__factories__/sequenceMetadata.factory';
import { buildOutlineFromBlocks } from './courseware/data/__factories__/learningSequencesOutline.factory';
import MockedPluginSlot from './tests/MockedPluginSlot';

jest.mock('@openedx/frontend-plugin-framework', () => ({
...jest.requireActual('@openedx/frontend-plugin-framework'),
Plugin: () => 'Plugin',
PluginSlot: () => 'PluginSlot',
PluginSlot: MockedPluginSlot,
}));

jest.mock('@src/generic/plugin-store', () => ({
Expand Down
26 changes: 26 additions & 0 deletions src/tests/MockedPluginSlot.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
import React from 'react';
import PropTypes from 'prop-types';

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

MockedPluginSlot.displayName = 'PluginSlot';

MockedPluginSlot.propTypes = {
children: PropTypes.oneOfType([
PropTypes.arrayOf(PropTypes.node),
PropTypes.node,
]),
id: PropTypes.string,
};

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

export default MockedPluginSlot;
Loading
Loading