feat: removes sidebar upgrade notification component by zacharis278 · Pull Request #1410 · openedx/frontend-app-learning

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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

              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

 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

  } = 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

            <PluginSlot
              id="outline_tab_notifications_slot"
              pluginProps={{ courseId }}
-            >


Nice, so clean!

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

@@ -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

              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.

Details

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.

itsjeyd · 2025-04-10T08:06:46Z

Hey @zacharis278, the CLA check for this PR is failing so it looks like you'll need to go back and follow the instructions from the bot about submitting a signed CLA.

jristau1984 · 2025-04-10T11:13:16Z

@nsprenkle and @jansenk are 2U learning MFE folks.

zacharis278 · 2025-04-10T12:40:26Z

@itsjeyd thanks I'm no longer with 2U or contributing to the project. This kinda just got left over, happy to close the PR if the branch isn't useful anymore otherwise someone else will need to pick this up.

arbrandes · 2025-04-14T19:51:38Z

Ported over to #1675.

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

openedx-webhooks added this to Contributions Feb 28, 2025

github-project-automation Bot moved this to Needs Triage in Contributions Feb 28, 2025

itsjeyd moved this from Needs Triage to In Eng Review in Contributions Mar 14, 2025

itsjeyd added the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Mar 14, 2025

itsjeyd added waiting for eng review PR is ready for review. Review and merge it, or suggest changes. and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Apr 1, 2025

itsjeyd added the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Apr 10, 2025

arbrandes mentioned this pull request Apr 14, 2025

feat: removes Upgrade Notification as default content #1675

Merged

arbrandes closed this Apr 14, 2025

github-project-automation Bot moved this from In Eng Review to Done in Contributions Apr 14, 2025

openedx-webhooks removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. waiting for eng review PR is ready for review. Review and merge it, or suggest changes. labels Apr 14, 2025

itsjeyd added the duplicate This issue or pull request already exists elsewhere label Apr 24, 2025

Conversation

zacharis278 commented Jun 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Removed by this PR

Not Removed by this PR

Open Questions

Uh oh!

codecov Bot commented Jun 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

zacharis278 Jun 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

varshamenon4 Jun 21, 2024

Choose a reason for hiding this comment

Uh oh!

zacharis278 Jun 21, 2024

Choose a reason for hiding this comment

Uh oh!

zacharis278 commented Jun 21, 2024

Uh oh!

varshamenon4 left a comment

Choose a reason for hiding this comment

Uh oh!

varshamenon4 Jun 21, 2024

Choose a reason for hiding this comment

Uh oh!

varshamenon4 Jun 21, 2024

Choose a reason for hiding this comment

Uh oh!

varshamenon4 Jun 21, 2024

Choose a reason for hiding this comment

Uh oh!

zacharis278 Jul 29, 2024

Choose a reason for hiding this comment

Uh oh!

varshamenon4 Jun 21, 2024

Choose a reason for hiding this comment

Uh oh!

zacharis278 commented Jul 29, 2024

Uh oh!

zacharis278 commented Aug 26, 2024

Uh oh!

bradenmacdonald commented Sep 12, 2024

Uh oh!

bradenmacdonald commented Feb 28, 2025

Uh oh!

openedx-webhooks commented Feb 28, 2025

Uh oh!

itsjeyd commented Apr 10, 2025

Uh oh!

jristau1984 commented Apr 10, 2025

Uh oh!

zacharis278 commented Apr 10, 2025

Uh oh!

arbrandes commented Apr 14, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

zacharis278 commented Jun 20, 2024 •

edited

Loading

codecov Bot commented Jun 20, 2024 •

edited

Loading

zacharis278 Jun 21, 2024 •

edited

Loading