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

[Gregory Igelmund]

@grekko commented on this pull request.



> +
+    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")

Thanks for taking a close look here. I agree with your concerns regarding the data integrity.

> […] rework it to only be concerned about the first comment.

Yup. This will simplify things and keep the behaviour identical. I will update the PR to reflect this.

> @@ -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?

Will change to simply `@note_comments = @note_comments.drop(1)` since we'll only look at the first comment and ignore the `event` when migrating the data.

> @@ -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?

I think its needed for the behaviour of the API NotesController.

> […] and then note.comments which would return either N (if body migrated) or N-1 (if body not migrated) comments

I understand you are suggesting to implement a custom `Note#comments`-method instead of having the dedicated `Note#comments_with_extra_open_comment`-method?

I tried this out but realised quickly some issues with this:
a) We have calls to `Note#comments` from the (Web) NotesController context, which expects no prepended `open`-comment.

b) `Note#comments` returns an Active Record Collection Proxy-object. Overriding the `Note#comments`-behaviour so that it occasionally returns an Array instead, may lead to weird bugs. E.g. calls like `note.comments.create` will fail with a `NoMethodError`.

> Does it leak abstractions too much?

I am not sure what you mean with that. The idea of `Note#comments_with_extra_open_comment` is to hide the logic inside the model and make it obvious to developers that this method returns something special.

I've now added a `Note#api_comments`-method which should always return the full list of note comments including the `open`-comment.

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

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