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.

Sign up for GitHub

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

Jump to bottom

[#12048] Migrate Notification Banner E2E #12840

Merged
merged 7 commits into from
Feb 26, 2024
Merged

[#12048] Migrate Notification Banner E2E #12840

merged 7 commits into from
Feb 26, 2024

Conversation

yuanxi1
Copy link
Contributor

@yuanxi1 yuanxi1 commented Feb 26, 2024 •
edited
Loading

Part of #12048

Outline of Solution

  1. Migrate to use sql entities for all non-course entities
  2. Fix failing tests due to multiple notifications in DB causing the notif being displayed being non deterministic
  • Modified the FE html component to include a data-testid attribute that stores the notification id, so that Selenium can retrieve the notification id and verify if the correct notification is marked as read.
  • Removed the assertion that verifies if the correct banner is displayed through banner content since it fails and is already covered by unit test.

@yuanxi1 yuanxi1 requested review from weiquu and cedricongjh and removed request for weiquu February 26, 2024 07:24
cedricongjh
cedricongjh approved these changes Feb 26, 2024
View reviewed changes
Copy link
Contributor

@cedricongjh cedricongjh left a comment

Choose a reason for hiding this comment

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

LGTM! thanks for working on this

@cedricongjh cedricongjh added the s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging label Feb 26, 2024
@cedricongjh cedricongjh merged commit 7b2a69c into TEAMMATES:master Feb 26, 2024
6 of 9 checks passed
@cedricongjh cedricongjh added this to the V9.0.0-beta.0 milestone Mar 10, 2024
@weiquu weiquu added the c.Task Other non-user-facing works, e.g. refactoring, adding tests label Mar 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cedricongjh cedricongjh cedricongjh approved these changes

Assignees

@yuanxi1 yuanxi1

Labels
c.Task Other non-user-facing works, e.g. refactoring, adding tests s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging
Projects
None yet
Milestone
V9.0.0-beta.0
Development

Successfully merging this pull request may close these issues.
3 participants
@yuanxi1 @cedricongjh @weiquu