<p></p>
<p><b>@gravitystorm</b> commented on this pull request.</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/4481#discussion_r1481692089">lib/tasks/migrate_notes.rake</a>:</p>
<pre style='color:#555'>> + desc "Backfills notes-columns `body`, `author_ip` and `author_id`"
+ task :migrate_notes => :environment do
+ scope = Note.where(:body => nil, :author => nil, :author_ip => nil)
+ total_count = scope.count
+ remaining_count = total_count
+ puts "A total of #{total_count} Note-records have to be migrated."
+
+ # NB: default batch size is 1000
+ scope.find_in_batches do |batch|
+ puts "Processing batch of #{batch.size} records."
+ batch.each do |record|
+ opened_comment = record.comments.unscope(:where => :visible).find_by(:event => "opened")
+ (putc "x" && next) unless opened_comment
+
+ attributes = opened_comment.attributes.slice(*%w[body author_id author_ip]).compact_blank
+ record.update_columns(attributes) # rubocop:disable Rails/SkipsModelValidations
</pre>
<blockquote>
<p dir="auto">I am hesitant to perform any destructive operations until the very last step, but I am open to change the approach here since it would simplify the code.</p>
</blockquote>
<p dir="auto">I'm not sure I understand how this migration works, please let me know if I've missed something obvious! Let's say we start with a note with two comments:</p>
<table role="table">
<thead>
<tr>
<th>Situation</th>
<th>Note id</th>
<th>Note body</th>
<th>first comment body</th>
<th>second comment body</th>
</tr>
</thead>
<tbody>
<tr>
<td>A</td>
<td>123</td>
<td>-</td>
<td>shop here</td>
<td>I fixed it</td>
</tr>
<tr>
<td>B</td>
<td>123</td>
<td>shop here</td>
<td>shop here</td>
<td>I fixed it</td>
</tr>
<tr>
<td>C</td>
<td>123</td>
<td>shop here</td>
<td>I fixed it</td>
<td>-</td>
</tr>
</tbody>
</table>
<p dir="auto">Situation A is before we do anything - no body, two comments. At the end of the project, we're aiming for situation C.</p>
<p dir="auto">However, if we migrate the contents of the first comment, without destroying that comment, don't we end up in situation B? i.e. <code class="notranslate">@note.body_migrated?</code> is <code class="notranslate">true</code>, <code class="notranslate">build_comments_with_extra_open_comment</code> will then have three comments (two real ones plus the unshifted synthetic one)?</p>
<p dir="auto">Here's a test to show my point:</p>
<div class="highlight highlight-source-ruby" dir="auto"><pre class="notranslate"> <span class="pl-k">def</span> <span class="pl-en">test_comments_after_migration</span>
<span class="pl-s1">note</span> <span class="pl-c1">=</span> <span class="pl-en">create</span><span class="pl-kos">(</span><span class="pl-pds">:note</span><span class="pl-kos">,</span> <span class="pl-pds">:body</span> <span class="pl-c1">=></span> <span class="pl-c1">nil</span><span class="pl-kos">,</span> <span class="pl-pds">:author</span> <span class="pl-c1">=></span> <span class="pl-c1">nil</span><span class="pl-kos">,</span> <span class="pl-pds">:author_ip</span> <span class="pl-c1">=></span> <span class="pl-c1">nil</span><span class="pl-kos">)</span>
<span class="pl-en">create</span><span class="pl-kos">(</span><span class="pl-pds">:note_comment</span><span class="pl-kos">,</span> <span class="pl-pds">:note</span> <span class="pl-c1">=></span> <span class="pl-s1">note</span><span class="pl-kos">,</span> <span class="pl-pds">:event</span> <span class="pl-c1">=></span> <span class="pl-s">"opened"</span><span class="pl-kos">,</span> <span class="pl-pds">:body</span> <span class="pl-c1">=></span> <span class="pl-s">"Hey hey!"</span><span class="pl-kos">,</span> <span class="pl-pds">:author</span> <span class="pl-c1">=></span> <span class="pl-en">create</span><span class="pl-kos">(</span><span class="pl-pds">:user</span><span class="pl-kos">)</span><span class="pl-kos">,</span> <span class="pl-pds">:author_ip</span> <span class="pl-c1">=></span> <span class="pl-s">"10.0.0.1"</span><span class="pl-kos">)</span>
<span class="pl-en">create</span><span class="pl-kos">(</span><span class="pl-pds">:note_comment</span><span class="pl-kos">,</span> <span class="pl-pds">:note</span> <span class="pl-c1">=></span> <span class="pl-s1">note</span><span class="pl-kos">,</span> <span class="pl-pds">:event</span> <span class="pl-c1">=></span> <span class="pl-s">"commented"</span><span class="pl-kos">,</span> <span class="pl-pds">:body</span> <span class="pl-c1">=></span> <span class="pl-s">"done"</span><span class="pl-kos">,</span> <span class="pl-pds">:author</span> <span class="pl-c1">=></span> <span class="pl-en">create</span><span class="pl-kos">(</span><span class="pl-pds">:user</span><span class="pl-kos">)</span><span class="pl-kos">,</span> <span class="pl-pds">:author_ip</span> <span class="pl-c1">=></span> <span class="pl-s">"10.0.0.2"</span><span class="pl-kos">)</span>
<span class="pl-en">assert_equal</span> <span class="pl-c1">2</span><span class="pl-kos">,</span> <span class="pl-s1">note</span><span class="pl-kos">.</span><span class="pl-en">comments_with_extra_open_comment</span><span class="pl-kos">.</span><span class="pl-en">length</span>
<span class="pl-en">assert</span> <span class="pl-v">Note</span>::<span class="pl-v">MigrateOpenedComment</span><span class="pl-kos">.</span><span class="pl-en">new</span><span class="pl-kos">(</span><span class="pl-s1">note</span><span class="pl-kos">)</span><span class="pl-kos">.</span><span class="pl-en">call</span>
<span class="pl-s1">n</span> <span class="pl-c1">=</span> <span class="pl-v">Note</span><span class="pl-kos">.</span><span class="pl-en">find</span><span class="pl-kos">(</span><span class="pl-s1">note</span><span class="pl-kos">.</span><span class="pl-en">id</span><span class="pl-kos">)</span> <span class="pl-c"># avoids all caching, `||=` caches etc</span>
<span class="pl-en">assert_equal</span> <span class="pl-c1">2</span><span class="pl-kos">,</span> <span class="pl-s1">n</span><span class="pl-kos">.</span><span class="pl-en">comments_with_extra_open_comment</span><span class="pl-kos">.</span><span class="pl-en">length</span>
<span class="pl-k">end</span></pre></div>
<pre class="notranslate"><code class="notranslate">Failure:
NoteMigrateOpenedCommentTest#test_comments_after_migration [test/models/note/migrate_opened_comment_test.rb:24]:
Expected: 2
Actual: 3
</code></pre>
<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#discussion_r1481692089">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/AAK2OLJB2ZACWPZWTUOY3HTYSOOX3AVCNFSM6AAAAABB4FLGOWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTQNRYGE2TQNBSGA">unsubscribe</a>.<br />You are receiving this because you are subscribed to this thread.<img src="https://github.com/notifications/beacon/AAK2OLIPN4REM45JGFUGN2TYSOOX3A5CNFSM6AAAAABB4FLGOWWGG33NNVSW45C7OR4XAZNRKB2WY3CSMVYXKZLTORJGK5TJMV32UY3PNVWWK3TUL5UWJTTPLHK5I.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/1868158420</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#discussion_r1481692089",
"url": "https://github.com/openstreetmap/openstreetmap-website/pull/4481#discussion_r1481692089",
"name": "View Pull Request"
},
"description": "View this Pull Request on GitHub",
"publisher": {
"@type": "Organization",
"name": "GitHub",
"url": "https://github.com"
}
}
]</script>