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

[Debt] Invert stored notification settings #10430

Merged
merged 8 commits into from
May 23, 2024

Conversation

esizer
Copy link
Member

@esizer esizer commented May 21, 2024

🤖 Resolves #10227

👋 Introduction

Updates the existing notifications values to invert it form ignored to enabled.

🧪 Testing

Assist reviewers with steps they can take to test that the PR does what it says it does.

  1. Take note of any users current settings in the DB
  2. Run migration make refresh-api
  3. Confirm the users notifications from step one are reversed properly
  4. Build pnpm run dev
  5. Login as any user
  6. Navigate to the notifications section of the account settings page
  7. Confirm the settings are accurate and updates are reflected properly

🛑 Blockers

@petertgiles
Copy link
Contributor

Thanks for jumping on this so quickly! Unfortunately, I'm working to merge two more notifications that will need a similar change as the one you made to api/app/Notifications/ApplicationDeadlineApproaching.php. Could we hold the merge on this one until #10372 is in?

@esizer esizer added the blocked: dependencies Blocked by other issues. label May 21, 2024
@esizer
Copy link
Member Author

esizer commented May 21, 2024

Thanks for jumping on this so quickly! Unfortunately, I'm working to merge two more notifications that will need a similar change as the one you made to api/app/Notifications/ApplicationDeadlineApproaching.php. Could we hold the merge on this one until #10372 is in?

Of course, added the blocked: dependencies label and put the ticket number in the description to make it visible 👍

@petertgiles
Copy link
Contributor

Of course, added the blocked: dependencies label

Thanks for waiting. 🙏 It's been merged now.

@esizer esizer removed the blocked: dependencies Blocked by other issues. label May 22, 2024
@codecov-commenter
Copy link

codecov-commenter commented May 22, 2024

Codecov Report

Attention: Patch coverage is 41.17647% with 20 lines in your changes are missing coverage. Please review.

Project coverage is 38.61%. Comparing base (0c04b6e) to head (50c20d1).
Report is 24 commits behind head on main.

Files Patch % Lines
...pps/web/src/pages/Profile/AccountSettings/utils.ts 0.00% 10 Missing ⚠️
...s/Profile/AccountSettings/NotificationSettings.tsx 0.00% 5 Missing ⚠️
...es/Profile/AccountSettings/AccountSettingsPage.tsx 0.00% 4 Missing ⚠️
...p/GraphQL/Mutations/UpdateEnabledNotifications.php 75.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main   #10430      +/-   ##
============================================
- Coverage     38.62%   38.61%   -0.01%     
- Complexity     1404     1408       +4     
============================================
  Files           994      995       +1     
  Lines         30555    30570      +15     
  Branches       6562     6557       -5     
============================================
+ Hits          11801    11805       +4     
- Misses        18591    18732     +141     
+ Partials        163       33     -130     
Flag Coverage Δ
integrationtests 71.08% <93.33%> (-0.04%) ⬇️
unittests 31.57% <0.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@petertgiles petertgiles self-requested a review May 22, 2024 15:02
Copy link
Contributor

@petertgiles petertgiles left a comment

Choose a reason for hiding this comment

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

Looking great! I just did code rethrough and flagged a few things to check.

@esizer esizer requested a review from petertgiles May 23, 2024 13:55
@esizer esizer added this pull request to the merge queue May 23, 2024
Merged via the queue into main with commit f611b0c May 23, 2024
9 of 10 checks passed
@esizer esizer deleted the 10227-reverse-notification-stored-values branch May 23, 2024 16:28
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.

♻️ Store enabled notification preferences, not ignored preferences
3 participants