[openstreetmap/openstreetmap-website] Note subscriptions db table (PR #5284)

[Tom Hughes]

@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/2397146273 at github.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/rails-dev/attachments/20241026/6a76caff/attachment-0001.htm>