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.

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

feat: persist discussion alert dismissal #59

Merged

Conversation

CefBoud
Copy link

@CefBoud CefBoud commented Aug 23, 2024

Description

The alert used to inform about the usage of an upgraded version of discussion forum still shows on refresh after being dismissed. The goal of this PR is correct that and save the dismissal per course.

Testing instruction

  • Clone this repository and run npm insall
  • Ensure your are running Tutor redwood locally (>= 18)
  • Mount the clone repository to Tutor using tutor mounts add your/path/to/frontend-app-course-authoring
  • Run tutor dev launch
  • To simplify testing and show the Alert, comment out the following or set the condition to be always false.
if (providerType !== 'openedx') {
     return null;
   }
  • Dismiss the Alert then refresh. The alert will show again
  • Checkout this PR's branch then run npm install
  • Now after dismissal and refresh, the alert shouldn't show.

ref

BB-9079

@CefBoud CefBoud force-pushed the cef/BB-9079/save-discussion-alert-dismissal branch from 723a176 to b9113a5 Compare August 23, 2024 21:38
@CefBoud CefBoud requested a review from DanielVZ96 August 23, 2024 21:38
@CefBoud CefBoud force-pushed the cef/BB-9079/save-discussion-alert-dismissal branch 3 times, most recently from 0aebc09 to b036f93 Compare August 23, 2024 22:40
@DanielVZ96
Copy link
Member

@CefBoud hey, I can't find the line if (providerType !== 'openedx') {. Do you have a link to where I can find it?

@CefBoud
Copy link
Author

CefBoud commented Aug 23, 2024

@DanielVZ96 apologies. I forgot to add the link. Here it is.

@CefBoud CefBoud force-pushed the cef/BB-9079/save-discussion-alert-dismissal branch from b036f93 to 04bdc64 Compare August 23, 2024 22:50
Copy link

codecov bot commented Aug 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.44%. Comparing base (f336daa) to head (4584d6a).
Report is 4 commits behind head on asu-moe/redwood-css.

Additional details and impacted files
@@                 Coverage Diff                  @@
##           asu-moe/redwood-css      #59   +/-   ##
====================================================
  Coverage                92.43%   92.44%           
====================================================
  Files                      708      708           
  Lines                    12539    12541    +2     
  Branches                  2690     2726   +36     
====================================================
+ Hits                     11591    11594    +3     
+ Misses                     913      912    -1     
  Partials                    35       35           

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

Copy link
Member

@DanielVZ96 DanielVZ96 left a comment

Choose a reason for hiding this comment

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

👍

  • I tested instructions above
  • I read through the code
  • I checked for accessibility issues
  • Includes documentation
  • Added to the Code Drift project board (for backports)

Coverage went down, so I'd prefer for you to add a test for this... but I'll leave it up to you

@CefBoud CefBoud force-pushed the cef/BB-9079/save-discussion-alert-dismissal branch 2 times, most recently from fdb9e17 to d900386 Compare August 27, 2024 14:29
@CefBoud CefBoud force-pushed the cef/BB-9079/save-discussion-alert-dismissal branch from d900386 to 4584d6a Compare August 27, 2024 14:30
@CefBoud CefBoud merged commit ed3b54b into asu-moe/redwood-css Aug 27, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants