-
Notifications
You must be signed in to change notification settings - Fork 31
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
fix(notifications_push_repository): Delete push subscription when account is deleted #2667
base: main
Are you sure you want to change the base?
fix(notifications_push_repository): Delete push subscription when account is deleted #2667
Conversation
…efore an account is logged out Signed-off-by: provokateurin <[email protected]>
…ount is deleted Signed-off-by: provokateurin <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2667 +/- ##
==========================================
+ Coverage 28.90% 28.91% +0.01%
==========================================
Files 372 372
Lines 136624 136644 +20
==========================================
+ Hits 39492 39512 +20
Misses 97132 97132
*This pull request uses carry forward flags. Click here to find out more.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still don't like interlocking the repositories.
I think that the account_bloc should handle this. Once a LogoutEvent
is emitted, the bloc should unregister the notifications and afterward remove the account from the repository.
for (final callback in _beforeLogOutCallbacks) { | ||
await callback(account!); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that running these callbacks in parallel should be ok.
The account_repo can't guarantee execution order anyway.
Please use a Future.wait
Part of #2114