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

Delete notifications when deleting comments #499

Merged
merged 6 commits into from Mar 26, 2019

Conversation

vbrandl
Copy link
Contributor

@vbrandl vbrandl commented Mar 23, 2019

This PR implements a new function find_for_comment on Notification to list all notifications for a specific comment. The delete function on Comment is changed to list and delete all connected notifications.

I wasn't able to test the changes yet since I don't have the test environment set up, so the PR is WIP for now.

This should close #463 and close #500.

@trinity-1686a trinity-1686a added C: Enhancement New feature or request S: Incomplete This PR is not complete yet A: Backend Code running on the server labels Mar 23, 2019
@codecov
Copy link

codecov bot commented Mar 23, 2019

Codecov Report

Merging #499 into master will decrease coverage by 0.05%.
The diff coverage is 19.04%.

@@            Coverage Diff             @@
##           master     #499      +/-   ##
==========================================
- Coverage   26.89%   26.83%   -0.06%     
==========================================
  Files          65       65              
  Lines        8966     9000      +34     
==========================================
+ Hits         2411     2415       +4     
- Misses       6555     6585      +30

@vbrandl
Copy link
Contributor Author

vbrandl commented Mar 25, 2019

The following changes have been made:

  • Mentions inside comments are now properly inserted into the database (comment_id is set, instead of post_id)
  • Notifications for mentions are deleted, when the comment gets deleted
  • Notifications for comments enabled

The following use cases were tested:

  • Comment on a post, check for notification and delete comment again -> notification gets deleted too
  • Mention one or multiple user(s) in a comment, check notifications for all users and delete comment again -> notifications for all users are deleted

@vbrandl vbrandl changed the title [WIP] Delete notifications when deleting comments Delete notifications when deleting comments Mar 25, 2019
@trinity-1686a trinity-1686a added S: Ready for review This PR is ready to be reviewed and removed S: Incomplete This PR is not complete yet labels Mar 25, 2019
@trinity-1686a
Copy link
Contributor

Mentions inside comments are now properly inserted into the database (comment_id is set, instead of post_id)

Does this mean it fixes #500 too?

@vbrandl
Copy link
Contributor Author

vbrandl commented Mar 25, 2019

No. That's another issue. In that issue, mentioning and mentioned user are confused somewhere, I guess.

@vbrandl
Copy link
Contributor Author

vbrandl commented Mar 25, 2019

It actually looks like this also closes #500.

Copy link
Member

@elegaanz elegaanz left a comment

Choose a reason for hiding this comment

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

It is a bit weird to receive two notifications for the same comment when you are mentioned in a comment under your own article, but it is not really related to this PR, and otherwise it works great. Thank you. 😊

@elegaanz elegaanz merged commit c7ee779 into Plume-org:master Mar 26, 2019
@vbrandl vbrandl deleted the fix/delete-notifications branch April 15, 2019 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: Backend Code running on the server C: Enhancement New feature or request S: Ready for review This PR is ready to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mention notification shows wrong username Delete notification when deleting comment
3 participants