Improve notification management #561

Merged
Plume_migration_agent merged 4 commits from notifications into master 5 years ago
Owner

fix #472
fix #505

fix #472 fix #505
elegaanz (Migrated from github.com) reviewed 5 years ago
elegaanz (Migrated from github.com) commented 5 years ago

Shouln't this be .any(...)? Because I'm still getting duplicated notifications.

Shouln't this be `.any(...)`? Because I'm still getting duplicated notifications.
elegaanz (Migrated from github.com) reviewed 5 years ago
elegaanz (Migrated from github.com) commented 5 years ago

Also, AsActor::is_local is technically broken, since it only does instance_id == 1 which is not always correct (even if it is most of the time (like, unless you ran plm instance new twice, it will probably be correct)).

Maybe we should have a global storing the local instance info somewhere BTW (loaded only once when the app starts)? It would probably avoid a lot of queries to the database, and would allow to fix this issue without changing AsActor definition (but probably in another PR if we ever do it).

Also, `AsActor::is_local` is technically broken, since it only does `instance_id == 1` which is not always correct (even if it is most of the time (like, unless you ran `plm instance new` twice, it will probably be correct)). Maybe we should have a global storing the local instance info somewhere BTW (loaded only once when the app starts)? It would probably avoid a lot of queries to the database, and would allow to fix this issue without changing `AsActor` definition (but probably in another PR if we ever do it).
trinity-1686a reviewed 5 years ago
Poster
Owner

if all mentioned users are different from this author, send notification. I think it's correct, and when I tested it, it seemed to work.
Edit : My bad, forgot to add some file

if *all* mentioned users are different from this author, send notification. I think it's correct, and when I tested it, it seemed to work. Edit : My bad, forgot to add some file
codecov[bot] commented 5 years ago (Migrated from github.com)

Codecov Report

Merging #561 into master will decrease coverage by <.01%.
The diff coverage is 54.34%.

@@            Coverage Diff             @@
##           master     #561      +/-   ##
==========================================
- Coverage   35.05%   35.05%   -0.01%     
==========================================
  Files          67       67              
  Lines        7834     7843       +9     
  Branches     1877     1885       +8     
==========================================
+ Hits         2746     2749       +3     
- Misses       4331     4332       +1     
- Partials      757      762       +5
# [Codecov](https://codecov.io/gh/Plume-org/Plume/pull/561?src=pr&el=h1) Report > Merging [#561](https://codecov.io/gh/Plume-org/Plume/pull/561?src=pr&el=desc) into [master](https://codecov.io/gh/Plume-org/Plume/commit/918bda14ececc7f72b5741896c2c7ca8fba114f9?src=pr&el=desc) will **decrease** coverage by `<.01%`. > The diff coverage is `54.34%`. ```diff @@ Coverage Diff @@ ## master #561 +/- ## ========================================== - Coverage 35.05% 35.05% -0.01% ========================================== Files 67 67 Lines 7834 7843 +9 Branches 1877 1885 +8 ========================================== + Hits 2746 2749 +3 - Misses 4331 4332 +1 - Partials 757 762 +5 ```
elegaanz (Migrated from github.com) reviewed 5 years ago
elegaanz (Migrated from github.com) commented 5 years ago

Oh, I found what is wrong I think: the mention was inserted after the comment in the database.

Oh, I found what is wrong I think: the mention was inserted after the comment in the database.
elegaanz (Migrated from github.com) reviewed 5 years ago
elegaanz (Migrated from github.com) commented 5 years ago

OK, I'm too late, you already found it.

OK, I'm too late, you already found it.
elegaanz (Migrated from github.com) approved these changes 5 years ago
elegaanz (Migrated from github.com) left a comment

Seems to work! Thank you! 😊

Seems to work! Thank you! :blush:

Reviewers

The pull request has been merged as c9070930d2.
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 notifications master
git pull origin notifications

Step 2:

Merge the changes and update on Forgejo.
git checkout master
git merge --no-ff notifications
git push origin master
Sign in to join this conversation.
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.

Reference: Plume/Plume#561
Loading…
There is no content yet.