Note subscriptions db table #5284
Conversation
AntonKhorev
added
api
| @@ -401,6 +401,8 @@ def add_comment(note, text, event, notify: true) | |||
| note.comments.map(&:author).uniq.each do |user| | |||
| UserMailer.note_comment_notification(comment, user).deliver_later if notify && user && user != current_user && user.visible? | |||
| end | |||
|
|
|||
| NoteSubscription.find_or_create_by(:note => note, :user => current_user) if current_user | |||
There was a problem hiding this comment.
I think this could be simplified using the association?
| NoteSubscription.find_or_create_by(:note => note, :user => current_user) if current_user | |
| current_user&.note__subscriptions.find_or_create_by(:note => note) |
There was a problem hiding this comment.
That would require one more &. after note__subscriptions, so it depends on whether you think that chained &.s are simplifications.
There was a problem hiding this comment.
I don't believe it does - if current_user is defined then the note_subscriptions association will always exist. It might evaluate to an empty list but the association object will exist and can have methods invoked on it.
There was a problem hiding this comment.
Try running Rubocop.
There was a problem hiding this comment.
Hmm you're quite right, and rubocop is quite right, which I find quite odd but apparently ruby does continue evaluating the line after the first &. fails.
Personally I still think I prefer it but it's not a huge issue.
| assert_equal user.display_name, js["properties"]["comments"].last["user"] | ||
|
|
||
| note = Note.last | ||
| subscription = NoteSubscription.last |
There was a problem hiding this comment.
This (and all the other similar versions in other tests) is relying on nothing else having created notes or users in a way that overlaps this test and I'm not sure how safe that is when tests are running in parallel?
Maybe it would be better to get the node ID from the JSON response and then do assert Note.exists?(id) and a similar exists assertion on the subscription record?
There was a problem hiding this comment.
I'm not sure how safe that is when tests are running in parallel?
Parallel tests are run in separate databases (e.g. openstreetmap_test-0 etc)
There was a problem hiding this comment.
This ... is relying on nothing else having created notes
There is a check assert_difference "Note.count", 1
(and all the other similar versions in other tests)
Other tests in this file or other tests elsewhere? You can find similar uses of .last from years ago.
AntonKhorev
force-pushed
the
note-subscriptions-db-table
branch
from
954e7b3 to
2d7e0a3
Compare
|
It looks like there are no significant issues here, so I'll merge this. Thanks for the work. |
Part one of #5283. Creates the note subscription table and starts adding subscriptions for anyone who comments a note. These subscriptions are not yet used for anything, email notifications still go to all note commenters.