feat: Notification Plugin Slots #1368

rijuma · 2024-04-22T19:44:15Z

Description

This feature is part of the roadmap for the openedx/platform-roadmap#332 issue.

It adds three Plugin Slots wrapping the UpgradeNotifications component and one for the LockPaywall.

It also refactors the mocked PluginSlot to allow proper testing on rendering and wrapped content.

* chore: Updated PluginSlot mock to support children and test ids * chore: Updated mocked PluginSlot * chore: Added unit test for MockedPluginSlot * fix: Updated slot name ids

…ray (#1367)

codecov · 2024-04-22T19:51:50Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.73%. Comparing base (1c3610e) to head (71a23d2).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1368      +/-   ##
==========================================
+ Coverage   88.72%   88.73%   +0.01%     
==========================================
  Files         302      303       +1     
  Lines        5217     5222       +5     
  Branches     1295     1296       +1     
==========================================
+ Hits         4629     4634       +5     
  Misses        572      572              
  Partials       16       16

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

arbrandes

Thanks for this! However, as noted in individual comments, we should remove content from plugin slots that shouldn't be on by default. Furthermore, I'd like to try and make the slots more generic, if at all possible.

arbrandes · 2024-04-26T14:02:51Z

src/course-home/outline-tab/OutlineTab.jsx

+              pluginProps={upgradeNotificationProps}
+              testId="outline-tab-slot"
+            >
+              <UpgradeNotification {...upgradeNotificationProps} />


A few things:

The UpgradeNotification component should not be present by default

If there are no other desirable uses of UpgradeNotification, the component itself should be removed from the codebase (and made into a plugin that lives elsewhere)

The props are too feature-specific for a generic plugin slot; we should find a way to generalize the API. Off the top of my head: we need a way to have a backend Django plugin insert data into the React context, and then either pass that down explicitly into the plugin as a prop, or have the plugin itself reach into the global context. Is the backend code for this feature already a plugin?

I'd agree the UpgradeNotification is likely not used by other sites but hard to know for sure. For points 1 and 2 would it make sense to separate the actual DEPR of this component from the additive change here? It seems to me we can separate these two initiatives.

There's probably an opportunity to narrow down or fully remove props passed into this. I'll have us take a look into that. Removing anything that isn't direct context of the parent location (OutlineTab) would help us keep this more generalized for the future. But, I'd hesitate to over architect how this slot is works, such as requiring a backend package for props, if we don't have any other use cases yet. For this case, our plugin is just small UX changes that don't require a backend.

On the DEPR front, there's some nuance here that we need to explore. From Open edX's point of view, the deprecation and removal process would ideally happen before the Redwood cutoff on May 9th. This is because 1) this code doesn't exist in Quince, and 2) if it becomes part of Redwood we'll have to support it for much longer, and I believe the consensus on the Axim engineering side is that we don't want the community to have to do that.

Also, to make sure we're on the same page, since this has been found not to be a part of the core Open edX product (as per the roadmap proposal: openedx/platform-roadmap#332 (comment)), it should leave no room for doubt whether it should stay in or not. It's just a matter of when.

For this case, our plugin is just small UX changes that don't require a backend.

My concern here is that by creating a PluginSlot, we're actually creating an API entrypoint. We want to strive to make it both generic and stable. If the plugin slot only works with a very specific component, then it's not much better than just having the component be there behind a configuration variable. And if we don´t make it stable, it doesn't make it very useful for people that don´t want to fork the codebase to introduce something here.

Hence, the effort to generalize this as much as we can. It might not be entirely possible given the short time-frame, but I believe we should at least give it our best shot.

I think there might be an example of a callback doing something about generalization in the AI Translation plugin implementation. Let me see if I can find it.

Thanks, I'm actually exploring the option that maybe we simply shouldn't be passing props at all to these slots. I suspect everything we're using can be gotten from redux on the plugin side. That seems like a much simpler and generic pattern to follow than trying to maintain an API that's likely to change despite our best efforts.

If that works is that a pattern we'd be okay with?

Found it:

https://github.com/openedx/frontend-app-learning/blob/master/src/generic/plugin-store/hooks.js#L4

@leangseu-edx came up with a way for plugins to register callback functions that can act on a plugin-store redux slice.

After you've checked which props are actually necessary, and if any feature-specific ones remain, I'd be interested in investigating if we could generalize them with methods such as the above.

This does not need a DEPR, it can just be removed. It's 2U specific and has not been released in an Open edX release so you can simply make the PR to remove it.

Good to know we can remove this without DEPR. I'd still propose we make the additive change here that keeps the current learning MFE code as-is but pluggable and follow up with a separate PR that removes the upgrade notification and paywall components from this repo entirely. That is assuming the properties we're passing to the plugin are solved to be generic or removed entirely.

I don't think we could hit that Redwood cutoff if we must include the removal of the current components and any supporting code in this MFE. That work opens up some additional unsolved issues around where the translations go I'd rather not get into if we can help it.

@arbrandes I've put together a quick POC of removing 99% of these parameters here: #1379.

That is just the sidebar since that's the most complex bit. If this approach generally looks good we can do the same for everything and update this PR.

I think going with useModel() is a good direction to take, for now. It doesn't mean we're guaranteeing that the data structures will remain the same, but it will be a demonstration that you can technically do it if you're fine with bearing the risk as a plugin author.

In other words, it works for us if it works for you.

arbrandes · 2024-04-26T14:03:41Z

.../course/new-sidebar/sidebars/discussions-notifications/notifications/NotificationsWidget.jsx

+        pluginProps={upgradeNotificationProps}
+        testId="notification-widget-slot"
+      >
+        <UpgradeNotification {...upgradeNotificationProps} />


The same applies here as in the previous comment.

arbrandes · 2024-04-26T14:05:50Z

src/courseware/course/sequence/Unit/UnitSuspense.jsx

+              courseId,
+            }}
+          >
+            <LockPaywall courseId={courseId} />


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).

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.

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.

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.

arbrandes · 2024-04-26T14:08:07Z

src/courseware/course/sidebar/sidebars/notifications/NotificationTray.jsx

+            pluginProps={upgradeNotificationProps}
+            testId="notification-tray-slot"
+          >
+            <UpgradeNotification {...upgradeNotificationProps} />


Same here. Plus, we should check with Product what to do if verifiedMode is never used. Does it mean we'll always just show a "no notifications" message?

* feat: make notification plugin api generic * feat: include new sidebar and tests * feat: tweak sidebar toggle

Zacharis278 · 2024-05-07T18:47:42Z

@arbrandes this is now updated with all of the upgrade specific props removed and is ready for review. If everything looks good, we can probably make the Redwood cutoff.

Zacharis278 · 2024-05-10T13:17:15Z

@arbrandes apologies for multiple pings on this but we're running up against some internal timelines to get this plugin testable on edx.org stage. Do you know when this can get reviewed merged?

.

arbrandes

I'm fine with us moving forward here and improving things later, with one exception: can you please change the slot IDs as per openedx/wg-frontend#178 so that we don't have to introduce a breaking change later?

arbrandes · 2024-05-13T19:53:38Z

.../course/new-sidebar/sidebars/discussions-notifications/notifications/NotificationsWidget.jsx

-        toggleSidebar={() => toggleSidebar(currentSidebar, WIDGETS.NOTIFICATIONS)}
-      />
+      <PluginSlot
+        id="notification_widget_plugin"


We're trying to standardize how slots are exposed (details here: openedx/wg-frontend#178). I won't ask you to tackle all that here (we'll probably do it ourselves in this other PR), but in order to avoid a breaking change to the slot ID, do you mind changing it as suggested below?

Suggested change

id="notification_widget_plugin"

id="notification_widget_slot"

ahh sounds good I can rename both of these

arbrandes · 2024-05-13T19:56:55Z

src/courseware/course/sequence/Unit/UnitSuspense.jsx

-        suspenseComponent(messages.loadingLockedContent, LockPaywall)
+        <Suspense fallback={<PageLoading srMessage={formatMessage(messages.loadingLockedContent)} />}>
+          <PluginSlot
+            id="fbe_message_plugin"


Following the new naming convention as outlined above:

Suggested change

id="fbe_message_plugin"

id="fbe_message_slot"

Also, what does "fbe" stand for? Might be worth making the slot ID a little longer if it means it's easier to deduce what it's for just from the name.

It's short for feature based enrollment, I went back and named this 'gated content' since that's a bit more general purpose anyway.

arbrandes

Thanks for bearing with me. Approved!

alangsto and others added 3 commits April 19, 2024 14:11

feat: add plugin slot for fbe lock paywall (#1347)

377da57

feat: Added PluginSlot wrapping UpgradeNotification components (#1366)

d3ef306

* chore: Updated PluginSlot mock to support children and test ids * chore: Updated mocked PluginSlot * chore: Added unit test for MockedPluginSlot * fix: Updated slot name ids

feat: Added Plugin Slot wrapping UpgradeNotification in NotificationT…

1b5337a

…ray (#1367)

rijuma marked this pull request as ready for review April 22, 2024 19:59

Zacharis278 mentioned this pull request Apr 22, 2024

Pluggable Upgrade Messaging openedx/platform-roadmap#332

Open

fix: Removed PluginSlot prop scoping for UpgradeNotification (#1369)

1e29715

arbrandes self-requested a review April 26, 2024 13:26

Zacharis278 mentioned this pull request Apr 26, 2024

feat: Added Plugin Slot wrapping UpgradeNotification in NotificationTray #1367

Merged

arbrandes requested changes Apr 26, 2024

View reviewed changes

Zacharis278 changed the title ~~feat: Upgrade Plugin Slots~~ feat: Notification Plugin Slots May 2, 2024

Zacharis278 added 3 commits May 7, 2024 14:22

feat: generic sidebar notification plugin (#1379)

0989559

* feat: make notification plugin api generic * feat: include new sidebar and tests * feat: tweak sidebar toggle

Merge branch 'master' into feat/upgrade-plugin-slots

144a80a

style: fix extra space from merge

af3eaf6

Zacharis278 previously approved these changes May 10, 2024

View reviewed changes

Zacharis278 self-requested a review May 10, 2024 20:31

Zacharis278 removed their request for review May 13, 2024 16:30

arbrandes requested changes May 13, 2024

View reviewed changes

feat: rename plugin slots

71a23d2

arbrandes approved these changes May 13, 2024

View reviewed changes

arbrandes mentioned this pull request May 13, 2024

feat!: organize plugin slots as components, add footer slot #1381

Merged

schenedx merged commit 1857b86 into master May 13, 2024
7 checks passed

schenedx deleted the feat/upgrade-plugin-slots branch May 13, 2024 20:59

Zacharis278 mentioned this pull request May 16, 2024

fix: incorrectly named plugin slots #1388

Merged

Zacharis278 mentioned this pull request Jun 20, 2024

feat: removes sidebar upgrade notification component #1410

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Notification Plugin Slots #1368

feat: Notification Plugin Slots #1368

rijuma commented Apr 22, 2024 •

edited

Loading

codecov bot commented Apr 22, 2024 •

edited

Loading

arbrandes left a comment

arbrandes Apr 26, 2024

Zacharis278 Apr 26, 2024

arbrandes Apr 26, 2024

arbrandes Apr 26, 2024 •

edited

Loading

Zacharis278 Apr 26, 2024

arbrandes Apr 26, 2024

feanil Apr 26, 2024

Zacharis278 Apr 29, 2024 •

edited

Loading

Zacharis278 May 2, 2024

arbrandes May 2, 2024

arbrandes Apr 26, 2024

arbrandes Apr 26, 2024

Zacharis278 May 2, 2024

arbrandes May 2, 2024

Zacharis278 May 3, 2024 •

edited

Loading

arbrandes Apr 26, 2024

Zacharis278 commented May 7, 2024

Zacharis278 commented May 10, 2024

arbrandes left a comment •

edited

Loading

arbrandes May 13, 2024

Zacharis278 May 13, 2024

arbrandes May 13, 2024

Zacharis278 May 13, 2024

arbrandes left a comment

	id="notification_widget_plugin"
	id="notification_widget_slot"

feat: Notification Plugin Slots #1368

feat: Notification Plugin Slots #1368

Conversation

rijuma commented Apr 22, 2024 • edited Loading

Description

codecov bot commented Apr 22, 2024 • edited Loading

Codecov Report

arbrandes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arbrandes Apr 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zacharis278 Apr 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zacharis278 May 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zacharis278 commented May 7, 2024

Zacharis278 commented May 10, 2024

arbrandes left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arbrandes left a comment

Choose a reason for hiding this comment

rijuma commented Apr 22, 2024 •

edited

Loading

codecov bot commented Apr 22, 2024 •

edited

Loading

arbrandes Apr 26, 2024 •

edited

Loading

Zacharis278 Apr 29, 2024 •

edited

Loading

Zacharis278 May 3, 2024 •

edited

Loading

arbrandes left a comment •

edited

Loading