feat: removes sidebar upgrade notification component #1410

zacharis278 · 2024-06-20T21:45:20Z

Followup to #1368.

These components are specific to edx.org and will be moved to a 2U owned plugin. There are some open questions and I'd like to go back and manually test everything once the 2U plugin is updated to no longer import these. Leaving this in draft state for now.

edx.org has migrated these components to a 2U owned plugin. This code is no longer being used and can be deprecated. While the LockPaywall is also unused we have opted not to remove it as originally intended. See note below

Removed by this PR

~~LockPaywall: Upgrade ad shown in place of graded unit if content gating is enabled~~ No longer included in this PR
UpgradeNotification: Sidebar upgrade advertisement
~~UpsellBullets: Only used by Upgrade/Paywall~~ Cannot be removed since we're keeping the LockPaywall

Not Removed by this PR

Other instances of the Upgrade Button

Open Questions

Unsure on what to do with the LockPaywall. Should this actually be removed? It puts us in a weird state where 'nothing' would be displayed if content gating is enabled and that plugin slot isn't configured. I'm thinking this should either be reworked so the shouldDisplayContentGating logic moves into the plugin itself or we don't touch LockPaywall at all here.

Decision: We will not be removing the LockPaywall as part of this effort

codecov · 2024-06-20T21:52:30Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.02%. Comparing base (7efe8f5) to head (397f6a7).
Report is 137 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1410      +/-   ##
==========================================
+ Coverage   88.87%   89.02%   +0.15%     
==========================================
  Files         307      306       -1     
  Lines        5285     5168     -117     
  Branches     1344     1280      -64     
==========================================
- Hits         4697     4601      -96     
+ Misses        572      551      -21     
  Partials       16       16

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

zacharis278 · 2024-06-21T13:02:44Z

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

@@ -34,9 +33,7 @@ const UnitSuspense = ({
            pluginProps={{
              courseId,
            }}
-          >
-            <LockPaywall courseId={courseId} />


I had this removed originally but after thinking it through my instinct is we actually don't want to touch this. I'll leave it here for now to discuss.

Is content gating edx.org specific or a core feature of Open edX that should have an associated paywall/view in this MFE?

My instinct is that this should remain as it feels like a core feature of Open edX, but I'm guessing you're leaving here to confirm with Axim?

zacharis278 · 2024-06-21T13:10:08Z

...se/new-sidebar/sidebars/discussions-notifications/notifications/NotificationsWidget.test.jsx

 jest.mock('@edx/frontend-platform/analytics');

+/* eslint-disable react/prop-types */
+jest.mock('@openedx/frontend-plugin-framework', () => ({


this is the only addition here since there are tests that rely on a close button within the notification that toggle the sidebar. We need a 'mock' notification now since there is no default

zacharis278 · 2024-06-21T13:21:24Z

FYI @arbrandes

varshamenon4

Looks good! Just added some comments and questions.

varshamenon4 · 2024-06-21T13:51:41Z

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

  } = useModel('courseHomeMeta', courseId);

  const {
-    accessExpiration,


The props removed here are just the ones that are no longer being used by the UpgradeNotification, correct?

varshamenon4 · 2024-06-21T13:51:57Z

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

@@ -198,21 +187,7 @@ const OutlineTab = ({ intl }) => {
            <PluginSlot
              id="outline_tab_notifications_slot"
              pluginProps={{ courseId }}
-            >


Nice, so clean!

varshamenon4 · 2024-06-21T13:55:01Z

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

@@ -81,24 +74,7 @@ const NotificationsWidget = () => {
          setNotificationCurrentState: setUpgradeNotificationCurrentState,
          toggleSidebar: onToggleSidebar,
        }}


Suggestion: the notificationCurrentState and setNotificationCurrentState in the PluginSlot pluginProps above can also be removed, correct?

these are still needed since pluggable notification components use this as a callback to open or close the sidebar

varshamenon4 · 2024-06-21T13:58:23Z

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

@@ -34,9 +33,7 @@ const UnitSuspense = ({
            pluginProps={{
              courseId,
            }}
-          >
-            <LockPaywall courseId={courseId} />


My instinct is that this should remain as it feels like a core feature of Open edX, but I'm guessing you're leaving here to confirm with Axim?

zacharis278 · 2024-07-29T15:46:07Z

@arbrandes as requested this is a follow up to remove unused code as part of openedx/platform-roadmap#332.

I'll leave timing of merging this up to Axim since this has already been removed from edx.org

zacharis278 · 2024-08-26T13:14:10Z

@arbrandes is there still desire to have this removed as part of the 2U work? I'm clearing out this project on our end and this is the only open task.

bradenmacdonald · 2024-09-12T18:41:00Z

@zacharis278 @arbrandes I would like to get this merged. Is there anything blocking merging it? An open question about LockPaywall ?

bradenmacdonald · 2025-02-28T00:39:24Z

@jristau1984 Is there someone from TNL who can advise on getting this PR finished? I'd like to get this unused code removed, and I think it just needs a rebase. I'm happy to help but don't have as much context on what's still needed or not.

CC @arbrandes

openedx-webhooks · 2025-02-28T00:39:28Z

Thanks for the pull request, @zacharis278!

This repository is currently maintained by @openedx/committers-frontend-app-learning.

Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review.

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
- This process (including the steps you'll need to take) is documented here.
If it doesn't, simply proceed with the next step.

🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

Dependencies

This PR must be merged before / after / at the same time as ...
Blockers

This PR is waiting for OEP-1234 to be accepted.
Timeline information

This PR must be merged by XX date because ...
Partner information

This is for a course on edx.org.
Supporting documentation
Relevant Open edX discussion forum threads

🔘 Submit a signed contributor agreement (CLA)

⚠️ We ask all contributors to the Open edX project to submit a signed contributor agreement or indicate their institutional affiliation.
Please see the CONTRIBUTING file for more information.

If you've signed an agreement in the past, you may need to re-sign.
See The New Home of the Open edX Codebase for details.

Once you've signed the CLA, please allow 1 business day for it to be processed.
After this time, you can re-run the CLA check by adding a comment below that you have signed it.
If the CLA check continues to fail, you can tag the @openedx/cla-problems team in a comment for further assistance.

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

The size and impact of the changes that it introduces
The need for product review
Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

zacharis278 commented Jun 21, 2024

View reviewed changes

varshamenon4 reviewed Jun 21, 2024

View reviewed changes

zacharis278 changed the title ~~feat: removes sidebar upgrade and fbe paywall~~ feat: removes sidebar upgrade notification component Jun 28, 2024

zacharis278 added 4 commits July 29, 2024 10:48

feat: removes sidebar upgrade and fbe paywall

73de57d

feat: undo remove lockpaywall

58cbed2

feat: add upsell bullets back in, required by lock paywall

30c6b23

feat: adding more code needed by lockpaywall

397f6a7

zacharis278 force-pushed the zhancock/remove-upgrade-ads branch from 48309c7 to 397f6a7 Compare July 29, 2024 15:36

zacharis278 marked this pull request as ready for review July 29, 2024 15:43

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Feb 28, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: removes sidebar upgrade notification component #1410

feat: removes sidebar upgrade notification component #1410

zacharis278 commented Jun 20, 2024 •

edited

Loading

codecov bot commented Jun 20, 2024 •

edited

Loading

zacharis278 Jun 21, 2024 •

edited

Loading

varshamenon4 Jun 21, 2024

zacharis278 Jun 21, 2024

zacharis278 commented Jun 21, 2024

varshamenon4 left a comment

varshamenon4 Jun 21, 2024

varshamenon4 Jun 21, 2024

varshamenon4 Jun 21, 2024

zacharis278 Jul 29, 2024

varshamenon4 Jun 21, 2024

zacharis278 commented Jul 29, 2024

zacharis278 commented Aug 26, 2024

bradenmacdonald commented Sep 12, 2024

bradenmacdonald commented Feb 28, 2025

openedx-webhooks commented Feb 28, 2025

feat: removes sidebar upgrade notification component #1410

Are you sure you want to change the base?

feat: removes sidebar upgrade notification component #1410

Conversation

zacharis278 commented Jun 20, 2024 • edited Loading

Removed by this PR

Not Removed by this PR

Open Questions

codecov bot commented Jun 20, 2024 • edited Loading

Codecov Report

zacharis278 Jun 21, 2024 • edited Loading

Choose a reason for hiding this comment

varshamenon4 Jun 21, 2024

Choose a reason for hiding this comment

zacharis278 Jun 21, 2024

Choose a reason for hiding this comment

zacharis278 commented Jun 21, 2024

varshamenon4 left a comment

Choose a reason for hiding this comment

varshamenon4 Jun 21, 2024

Choose a reason for hiding this comment

varshamenon4 Jun 21, 2024

Choose a reason for hiding this comment

varshamenon4 Jun 21, 2024

Choose a reason for hiding this comment

zacharis278 Jul 29, 2024

Choose a reason for hiding this comment

varshamenon4 Jun 21, 2024

Choose a reason for hiding this comment

zacharis278 commented Jul 29, 2024

zacharis278 commented Aug 26, 2024

bradenmacdonald commented Sep 12, 2024

bradenmacdonald commented Feb 28, 2025

openedx-webhooks commented Feb 28, 2025

zacharis278 commented Jun 20, 2024 •

edited

Loading

codecov bot commented Jun 20, 2024 •

edited

Loading

zacharis278 Jun 21, 2024 •

edited

Loading