Delete notifications when deleting comments #499

Merged

vbrandl merged 6 commits from fix/delete-notifications into master

vbrandl commented (Migrated from github.com)

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.

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.

👍 1 ❤️ 1

codecov[bot] commented (Migrated from github.com)

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

# [Codecov](https://codecov.io/gh/Plume-org/Plume/pull/499?src=pr&el=h1) Report > Merging [#499](https://codecov.io/gh/Plume-org/Plume/pull/499?src=pr&el=desc) into [master](https://codecov.io/gh/Plume-org/Plume/commit/bdfad844d7939e757929d1643132a9346f82dd66?src=pr&el=desc) will **decrease** coverage by `0.05%`. > The diff coverage is `19.04%`. ```diff @@ 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 commented (Migrated from github.com)

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

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

👍 1 ❤️ 1

trinity-1686a commented

Owner

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?

> 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 commented (Migrated from github.com)

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

vbrandl commented (Migrated from github.com)

It actually looks like this also closes #500.

elegaanz (Migrated from github.com) approved these changes

elegaanz (Migrated from github.com) left a comment

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. 😊

Reviewers

The pull request has been merged as c7ee779f51.

You can also view command line instructions.

Step 1:

From your project repository, check out a new branch and test the changes.

git checkout -b fix/delete-notifications master

git pull origin fix/delete-notifications

Step 2:

Merge the changes and update on Forgejo.

git checkout master

git merge --no-ff fix/delete-notifications

git push origin master

No reviewers

No Milestone

No Assignees

2 Participants

Notifications

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Loading…

There is no content yet.

Labels Milestones

Delete notifications when deleting comments #499

Codecov Report

Reviewers

Step 1:

Step 2: