<p></p>
<p><b>@gravitystorm</b> requested changes on this pull request.</p>
<p dir="auto">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 <a class="issue-link js-issue-link" data-error-text="Failed to load title" data-id="2362804073" data-permission-text="Title is private" data-url="https://github.com/openstreetmap/openstreetmap-website/issues/4913" data-hovercard-type="pull_request" data-hovercard-url="/openstreetmap/openstreetmap-website/pull/4913/hovercard" href="https://github.com/openstreetmap/openstreetmap-website/pull/4913">#4913</a> ) 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.</p>
<p dir="auto">Anyway, sorry for the unintentional delay.</p><hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/4481#discussion_r1646561004">app/models/note/migrate_opened_comment.rb</a>:</p>
<pre style='color:#555'>> +
+ 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")
</pre>
<p dir="auto">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 <code class="notranslate">:event => "opened"</code> 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.</p>
<p dir="auto">I don't have access to the production db to check or reassure myself.</p>
<p dir="auto">I'm reasonably confident that in the current codebase, there is no special handling of any <code class="notranslate">:event => "opened"</code> comments, but rather that it is strictly (and only) the <strong>first</strong> 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.</p>
<p dir="auto">Thoughts?</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/4481#discussion_r1646570277">app/controllers/notes_controller.rb</a>:</p>
<pre style='color:#555'>> @@ -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?
</pre>
<p dir="auto">(assumes only one "opened" note comment here, that might not be true)</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/4481#discussion_r1646577461">app/models/note.rb</a>:</p>
<pre style='color:#555'>> @@ -39,6 +49,10 @@ class Note < ApplicationRecord
DEFAULT_FRESHLY_CLOSED_LIMIT = 7.days
+ def comments_with_extra_open_comment
</pre>
<p dir="auto">Is this required in the model? Does it leak abstractions too much?</p>
<p dir="auto">I would have thought that</p>
<p dir="auto">a) In views, we need <code class="notranslate">note.body</code> (as implemented, with the <code class="notranslate">super</code> handling etc) and then <code class="notranslate">note.comments</code> which would return either N (if body migrated) or N-1 (if body not migrated) comments<br>
b) in the api, we would synthesise the output from <code class="notranslate">note.body</code> making the fake first comment, then <code class="notranslate">note.comments</code></p>
<p dir="auto">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.</p>
<p dir="auto">(In fact, now that I check, this is what I was originally suggesting in <a class="issue-link js-issue-link" data-error-text="Failed to load title" data-id="1482565414" data-permission-text="Title is private" data-url="https://github.com/openstreetmap/openstreetmap-website/issues/3831" data-hovercard-type="issue" data-hovercard-url="/openstreetmap/openstreetmap-website/issues/3831/hovercard" href="https://github.com/openstreetmap/openstreetmap-website/issues/3831">#3831</a> - 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.)</p>
<p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">—<br />Reply to this email directly, <a href="https://github.com/openstreetmap/openstreetmap-website/pull/4481#pullrequestreview-2128837491">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/AAK2OLNSWGRH34UFZT2F5YLZL7IQJAVCNFSM6AAAAABB4FLGOWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCMRYHAZTONBZGE">unsubscribe</a>.<br />You are receiving this because you are subscribed to this thread.<img src="https://github.com/notifications/beacon/AAK2OLPVCITZV5NOBRPYR2DZL7IQJA5CNFSM6AAAAABB4FLGOWWGG33NNVSW45C7OR4XAZNRKB2WY3CSMVYXKZLTORJGK5TJMV32UY3PNVWWK3TUL5UWJTT64N5XG.gif" height="1" width="1" alt="" /><span style="color: transparent; font-size: 0; display: none; visibility: hidden; overflow: hidden; opacity: 0; width: 0; height: 0; max-width: 0; max-height: 0; mso-hide: all">Message ID: <span><openstreetmap/openstreetmap-website/pull/4481/review/2128837491</span><span>@</span><span>github</span><span>.</span><span>com></span></span></p>
<script type="application/ld+json">[
{
"@context": "http://schema.org",
"@type": "EmailMessage",
"potentialAction": {
"@type": "ViewAction",
"target": "https://github.com/openstreetmap/openstreetmap-website/pull/4481#pullrequestreview-2128837491",
"url": "https://github.com/openstreetmap/openstreetmap-website/pull/4481#pullrequestreview-2128837491",
"name": "View Pull Request"
},
"description": "View this Pull Request on GitHub",
"publisher": {
"@type": "Organization",
"name": "GitHub",
"url": "https://github.com"
}
}
]</script>