[openstreetmap/openstreetmap-website] Note: Add body & author (PR #4481)

[Andy Allan]

@gravitystorm requested changes on this pull request.

So apologies from me. I created this review 3 weeks ago on the 19th June (and you can see one side-effect of me doing the review in #4913 ) but it was only today I was asked about it again and found that all my review comments were just "pending"! Somewhere I must have got distracted before submitting.

Anyway, sorry for the unintentional delay.

> +
+    def call
+      return false if skip?
+
+      attributes = opened_comment_note.attributes.slice(*%w[body author_id author_ip]).compact_blank
+      @note.update_columns(attributes) # rubocop:disable Rails/SkipsModelValidations
+    end
+
+    def skip?
+      opened_comment_note.blank?
+    end
+
+    private
+
+    def opened_comment_note
+      @note.comments.unscope(:where => :visible).find_by(:event => "opened")

I think this is a key part to consider. This migration (and various other bits of new code, like in the model) assumes that the `:event => "opened"` comment is a) present and b) the first comment c) only one per note. Which it should be, in theory, but that's not enforced by any database constraints. My concern is that there might be notes from the distant past where these assumptions don't hold true. Perhaps through manual moderation, or through timing problems, or through bugs in the original code, etc.

I don't have access to the production db to check or reassure myself.

I'm reasonably confident that in the current codebase, there is no special handling of any `:event => "opened"` comments, but rather that it is strictly (and only) the **first** comment that is considered special. And that works for both the views and the API. So I think we should do the same throughout this PR, i.e. remove all mention of "opened", and rework it to only be concerned about the first comment.

Thoughts?

> @@ -37,6 +37,9 @@ def show
       @note = Note.visible.find(params[:id])
       @note_comments = @note.comments
     end
+
+    # FIXME: notes_refactoring remove this once the backfilling is completed
+    @note_comments = @note_comments.reject { |comment| comment.event == "opened" } unless @note.body_migrated?

(assumes only one "opened" note comment here, that might not be true)

> @@ -39,6 +49,10 @@ class Note < ApplicationRecord
 
   DEFAULT_FRESHLY_CLOSED_LIMIT = 7.days
 
+  def comments_with_extra_open_comment

Is this required in the model? Does it leak abstractions too much?

I would have thought that 

a) In views, we need `note.body` (as implemented, with the `super` handling etc) and then `note.comments` which would return either N (if body migrated) or N-1 (if body not migrated) comments
b) in the api, we would synthesise the output from `note.body` making the fake first comment, then `note.comments`

So the Note model would entirely hide the complexity during the transition, while the long-running API complexity (until API 0.7) would just be in the API views.

(In fact, now that I check, this is what I was originally suggesting in #3831 - the first step would be to refactor the code, and hide all the complexity into the model, and do this first before any database migrations / backfilling etc.)

-- 
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/4481#pullrequestreview-2128837491
You are receiving this because you are subscribed to this thread.

Message ID: <openstreetmap/openstreetmap-website/pull/4481/review/2128837491 at github.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/rails-dev/attachments/20240712/b33e902f/attachment.htm>