@tomhughes commented on this pull request.
> @@ -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
I think this could be simplified using the association?
```suggestion
current_user&.note__subscriptions.find_or_create_by(:note => note)
```
> + end
+ end
+ assert_response :success
+ js = ActiveSupport::JSON.decode(@response.body)
+ assert_not_nil js
+ assert_equal "Feature", js["type"]
+ assert_equal "Point", js["geometry"]["type"]
+ assert_equal [-1.0, -1.0], js["geometry"]["coordinates"]
+ assert_equal "open", js["properties"]["status"]
+ assert_equal 1, js["properties"]["comments"].count
+ assert_equal "opened", js["properties"]["comments"].last["action"]
+ assert_equal "This is a comment",
js["properties"]["comments"].last["text"]
+ assert_equal user.display_name, js["properties"]["comments"].last["user"]
+
+ note = Note.last
+ subscription = NoteSubscription.last
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?
--
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/5284#pullrequestreview-2397146273
You are receiving this because you are subscribed to this thread.
Message ID:
<openstreetmap/openstreetmap-website/pull/5284/review/2397146...@github.com>_______________________________________________
rails-dev mailing list
rails-dev@openstreetmap.org
https://lists.openstreetmap.org/listinfo/rails-dev
