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

Remove notifications upon user deletion #1558

Merged
merged 3 commits into from
Sep 29, 2016

Conversation

nickvergessen
Copy link
Member

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?

@mention-bot
Copy link

@nickvergessen, thanks for your PR! By analyzing the annotation information on this pull request, we identified @icewind1991, @blizzz and @LukasReschke to be potential reviewers

@MorrisJobke
Copy link
Member

Tested and works 👍

@blizzz
Copy link
Member

blizzz commented Sep 29, 2016

Any form of tests possible?

@nickvergessen
Copy link
Member Author

nickvergessen commented Sep 29, 2016

Create a "local" federated share and then delete the recipient.
Afterwards check the notifications table

@MorrisJobke
Copy link
Member

Any form of tests possible?

  • create a remote share with this user
  • log in as user and check if there is the bell with the red dot in the top right (don't click it)
  • delete user
  • create user with same name
  • login as new user
  • no share notifiaction should be there

@blizzz
Copy link
Member

blizzz commented Sep 29, 2016

I mean, automated tests. We would not notice if it breaks.

@nickvergessen
Copy link
Member Author

Hmm let me try to hack into the IUser test when deleting a user, should be possible.

@nickvergessen
Copy link
Member Author

Added tests for deleting preferences, comments and notifications.
Also made sure the "post" hooks are only triggered, when the deletion was successful...

@MorrisJobke
Copy link
Member

@nickvergessen signed-off comment is missing ;)

@MorrisJobke
Copy link
Member

@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>
@nickvergessen nickvergessen force-pushed the remove-notifications-upon-user-deletion branch from ef6e2fc to 4d1acfd Compare September 29, 2016 13:41
@nickvergessen
Copy link
Member Author

Rebased

Copy link
Member

@blizzz blizzz left a comment

Choose a reason for hiding this comment

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

LGTM

@MorrisJobke
Copy link
Member

👍

@MorrisJobke MorrisJobke merged commit 85522ff into master Sep 29, 2016
@MorrisJobke MorrisJobke deleted the remove-notifications-upon-user-deletion branch September 29, 2016 22:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants