feat: Remove upsell banner on course home - #499
Conversation
REV-2233: The upsell banner now duplicates the sidebar banner. Removing it in favor of the new implementation on course home, but keeping the masquerade message that shows instructors when a learner lost access to the course.
| userTimezone, | ||
| } = payload; | ||
|
|
||
| if (!accessExpiration) { |
There was a problem hiding this comment.
Removing this check and the masqueradingExpiredCourse check below were what caused the production issue.
Codecov Report
@@ Coverage Diff @@
## master #499 +/- ##
==========================================
+ Coverage 84.59% 84.61% +0.01%
==========================================
Files 213 214 +1
Lines 3610 3627 +17
Branches 869 872 +3
==========================================
+ Hits 3054 3069 +15
- Misses 539 541 +2
Partials 17 17
Continue to review full report at Codecov.
|
| }); | ||
| await fetchAndRender(); | ||
| await screen.findByText('This learner does not have access to this course.', { exact: false }); | ||
| const check = await screen.queryByText('This learner does not have access to this course.', { exact: false }); |
There was a problem hiding this comment.
I changed this test to mirror the one below to make sure they are actually both working.
| }); | ||
|
|
||
| it('shows expiration', async () => { | ||
| it('does not have special masquerade text', async () => { |
There was a problem hiding this comment.
This is the new test to make sure the masquerade banner doesn't show up if we're not masquerading.
| if (!masqueradingExpiredCourse) { | ||
| return null; | ||
| } | ||
|
|
There was a problem hiding this comment.
Not a comment on the code, but on the cause of the prod issue: I'm confused why not including that last bit (return null if not masqueradingExpiredCourse) made the bug. I think we only include this component if (masqueradingExpiredCourse). How can we have if masqueradingExpiredCourse and also !masqueradingExpiredCourse?
Answer from Brian's slack (all clear now): There are two paths to display the masquerade “user lost access” banner:
1- Via the AccessExpirationAlert on non-course home pages
2- Via the hook on the course home page
This bit above worked for #1 since AccessExpirationAlert already performs that check. The hook on #2 didn’t perform that check (but does perform a different check for accessExpiration). So the course home case showed the banner.
| } | ||
|
|
||
| export function useAccessExpirationAlertMasquerade(accessExpiration, userTimezone, topic) { | ||
| const isVisible = !!accessExpiration; // If it exists, show it. |
There was a problem hiding this comment.
Thank you for the comment here- I had to look up "!!"
2 participants
REV-2233: The upsell banner now duplicates the sidebar banner. Removing it in favor of the new implementation on course home, but keeping the masquerade message that shows instructors when a learner lost access to the course.