-
-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Remove notifications upon user deletion #1558
Conversation
@nickvergessen, thanks for your PR! By analyzing the annotation information on this pull request, we identified @icewind1991, @blizzz and @LukasReschke to be potential reviewers |
Tested and works 👍 |
Any form of tests possible? |
Create a "local" federated share and then delete the recipient. |
|
I mean, automated tests. We would not notice if it breaks. |
Hmm let me try to hack into the IUser test when deleting a user, should be possible. |
Added tests for deleting preferences, comments and notifications. |
@nickvergessen signed-off comment is missing ;) |
Rebase fixes the detection script. |
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
ef6e2fc
to
4d1acfd
Compare
Rebased |
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.
LGTM
👍 |
Ref #1551
Not sure if this is correct, or if it should be done by the app. When it's done by the app, each of the apps would need to listen to the hook. What's different from comments to notifications, is that comments stores the data inside "core/server" instead of an app.
@LukasReschke @MorrisJobke thoughts?