@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/2317292...@github.com>_______________________________________________
rails-dev mailing list
rails-dev@openstreetmap.org
https://lists.openstreetmap.org/listinfo/rails-dev
