#561 Improve notification management

Merged
Plume_migration_agent merged 4 commits from notifications into master 1 year ago

fix #472
fix #505

fix #472 fix #505
elegaanz (Migrated from github.com) reviewed 1 year ago
Plume_migration_agent commented 1 year 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 1 year ago
Plume_migration_agent commented 1 year 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 1 year ago
trinity-1686a commented 1 year ago

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 1 year ago (Migrated from github.com)
Owner

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 1 year ago
Plume_migration_agent commented 1 year 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 1 year ago
Plume_migration_agent commented 1 year 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 1 year ago
elegaanz (Migrated from github.com) left a comment

Seems to work! Thank you! 😊

Reviewers

Plume_migration_agent approved these changes 1 year ago
The pull request has been merged as c9070930d2.
Sign in to join this conversation.
No Milestone
No Assignees
2 Participants
Notifications
Due Date

No due date set.

Dependencies

This pull request currently doesn't have any dependencies.

Loading…
There is no content yet.