Skip to content

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.

5 participants