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

[Andy Allan]

@gravitystorm requested changes on this pull request.

This is just an intermediate review, I've skimmed the rest but I haven't gone into detail yet.

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

I think this needs to remote the comment too, right? Otherwise there will be a body, and then the first comment will duplicate the body.

I also think this needs to have tests, and so I would move all the logic into the app (e.g. Note.migrate_bodies, or a standalone class in lib, or somesuch) to make it easier to test. The rake task can then just be a one-liner to call that method and therefore doesn't need tests itself.

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

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