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

[Andy Allan]

@gravitystorm commented on this pull request.



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

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:

|Situation | Note id | Note body | first comment body | second comment body | 
|---|---|---|---|---|
| A | 123 | - | shop here | I fixed it |
| B | 123 | shop here | shop here | I fixed it | 
| C | 123 | shop here | I fixed it | - |

Situation A is before we do anything - no body, two comments. At the end of the project, we're aiming for situation C.

However, if we migrate the contents of the first comment, without destroying that comment, don't we end up in situation B? i.e. `@note.body_migrated?` is `true`, `build_comments_with_extra_open_comment` will then have three comments (two real ones plus the unshifted synthetic one)?

Here's a test to show my point:

```ruby
  def test_comments_after_migration
    note = create(:note, :body => nil, :author => nil, :author_ip => nil)
    create(:note_comment, :note => note, :event => "opened", :body => "Hey hey!", :author => create(:user), :author_ip => "10.0.0.1")
    create(:note_comment, :note => note, :event => "commented", :body => "done", :author => create(:user), :author_ip => "10.0.0.2")

    assert_equal 2, note.comments_with_extra_open_comment.length

    assert Note::MigrateOpenedComment.new(note).call

    n = Note.find(note.id) # avoids all caching, `||=` caches etc
    assert_equal 2, n.comments_with_extra_open_comment.length
  end
```

```
Failure:
NoteMigrateOpenedCommentTest#test_comments_after_migration [test/models/note/migrate_opened_comment_test.rb:24]:
Expected: 2
  Actual: 3
```

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

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