Update epic discussion migration

* added temporary index to make scheduling SQL query faster
* update is done on DB side using MD5 function
* refactoring of specs

5 files changed, 148 insertions, 95 deletions

diff --git a/db/post_migrate/20190715193142_migrate_discussion_id_on_promoted_epics.rb b/db/post_migrate/20190715193142_migrate_discussion_id_on_promoted_epics.rb
index 3fd68198f42..efafcee4723 100644
--- a/db/post_migrate/20190715193142_migrate_discussion_id_on_promoted_epics.rb
+++ b/db/post_migrate/20190715193142_migrate_discussion_id_on_promoted_epics.rb
@@ -13,34 +13,42 @@ class MigrateDiscussionIdOnPromotedEpics < ActiveRecord::Migration[5.2]
 
   disable_ddl_transaction!
 
+  class SystemNoteMtadata < ActiveRecord::Base
+    self.table_name = 'system_note_metadata'
+  end
+
   class Note < ActiveRecord::Base
     include EachBatch
+
+    has_one :system_note_metadata
+
     self.table_name = 'notes'
 
     def self.fetch_discussion_ids_query
       promoted_epics_query = Note
-                               .where(system: true)
-                               .where(noteable_type: 'Epic')
-                               .where("note LIKE 'promoted from%'")
-                               .select("DISTINCT noteable_id")
+        .joins(:system_note_metadata)
+        .where(system: true)
+        .where(noteable_type: 'Epic')
+        .where("system_note_metadata.action='moved'")
+        .select("DISTINCT noteable_id")
+
       Note.where(noteable_type: 'Epic')
         .where(noteable_id: promoted_epics_query)
-        .select("DISTINCT discussion_id").order(:discussion_id)
+        .order(:discussion_id)
+        .distinct.pluck(:discussion_id)
     end
   end
 
   def up
-    # add_concurrent_index(:system_note_metadata, :action)
-
-    all_discussion_ids = Note.fetch_discussion_ids_query.collect(&:discussion_id)
-    index = 0
-    all_discussion_ids.in_groups_of(BATCH_SIZE) do |ids|
-      index += 1
-      delay = DELAY_INTERVAL * index
-      BackgroundMigrationWorker.perform_in(delay, MIGRATION, [ids.compact])
+    add_concurrent_index(:system_note_metadata, :action, where: "action='moved'")
+
+    all_discussion_ids = Note.fetch_discussion_ids_query
+    all_discussion_ids.in_groups_of(BATCH_SIZE, false).each_with_index do |ids, index|
+      delay = DELAY_INTERVAL * (index + 1)
+      BackgroundMigrationWorker.perform_in(delay, MIGRATION, [ids])
     end
 
-    # add_concurrent_index(:system_note_metadata, :action)
+    remove_concurrent_index(:system_note_metadata, :action)
   end
 
   def down
diff --git a/lib/gitlab/background_migration/fix_promoted_epics_discussion_ids.rb b/lib/gitlab/background_migration/fix_promoted_epics_discussion_ids.rb
index 8c910f66fc8..77c317741e9 100644
--- a/lib/gitlab/background_migration/fix_promoted_epics_discussion_ids.rb
+++ b/lib/gitlab/background_migration/fix_promoted_epics_discussion_ids.rb
@@ -12,35 +12,9 @@ module Gitlab
       end
 
       def perform(discussion_ids)
-        discussion_values = build_discussion_values(discussion_ids)
-
-        update_notes_discussion_ids(discussion_values) if discussion_values
-      end
-
-      def build_discussion_values(discussion_ids)
-        discussion_ids.map do |discussion_id|
-          new_id = generate_id(discussion_id)
-
-          ActiveRecord::Base.sanitize_sql(["(?, ?)", discussion_id, new_id])
-        end.join(', ')
-      end
-
-      def update_notes_discussion_ids(values)
-        sql = <<-SQL.squish
-          UPDATE notes SET discussion_id = v.new_discussion_id
-          FROM (
-            VALUES
-            #{values}
-          ) AS v(old_discussion_id, new_discussion_id)
-          WHERE notes.discussion_id = v.old_discussion_id
-          AND notes.noteable_type = 'Epic'
-        SQL
-
-        Note.connection.execute(sql)
-      end
-
-      def generate_id(id)
-        Digest::SHA1.hexdigest([:discussion, 'epic', id, SecureRandom.hex].join("-"))
+        Note.where(noteable_type: 'Epic')
+          .where(discussion_id: discussion_ids)
+          .update_all("discussion_id=MD5(discussion_id)||substring(discussion_id from 1 for 8)")
       end
     end
   end
diff --git a/spec/lib/gitlab/background_migration/fix_promoted_epics_discussion_ids_spec.rb b/spec/lib/gitlab/background_migration/fix_promoted_epics_discussion_ids_spec.rb
new file mode 100644
index 00000000000..de25ca192af
--- /dev/null
+++ b/spec/lib/gitlab/background_migration/fix_promoted_epics_discussion_ids_spec.rb
@@ -0,0 +1,46 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+describe Gitlab::BackgroundMigration::FixPromotedEpicsDiscussionIds, :migration, schema: 20190715193142 do
+  let(:namespaces) { table(:namespaces) }
+  let(:users) { table(:users) }
+  let(:epics) { table(:epics) }
+  let(:notes) { table(:notes) }
+
+  let(:user) { users.create!(email: 'test@example.com', projects_limit: 100, username: 'test') }
+  let(:namespace) { namespaces.create(name: 'gitlab', path: 'gitlab-org') }
+  let(:epic1) { epics.create(id: 1, author_id: user.id, iid: 1, group_id: namespace.id, title: 'Epic with discussion', title_html: 'Epic with discussion') }
+
+  def create_note(discussion_id)
+    notes.create(note: 'note comment', noteable_id: epic1.id, noteable_type: 'Epic', discussion_id: discussion_id)
+  end
+
+  def expect_valid_discussion_id(id)
+    expect(id).to match(/[0-9a-f]{40}/)
+  end
+
+  describe '#perform with batch of discussion ids' do
+    it 'updates discussion ids' do
+      note1 = create_note('00000000')
+      note2 = create_note('00000000')
+      note3 = create_note('10000000')
+
+      subject.perform(%w(00000000 10000000))
+
+      expect_valid_discussion_id(note1.reload.discussion_id)
+      expect_valid_discussion_id(note2.reload.discussion_id)
+      expect_valid_discussion_id(note3.reload.discussion_id)
+      expect(note1.discussion_id).to eq(note2.discussion_id)
+      expect(note1.discussion_id).not_to eq(note3.discussion_id)
+    end
+
+    it 'skips notes with discsussion id not in range' do
+      note4 = create_note('20000000')
+
+      subject.perform(%w(00000000 10000000))
+
+      expect(note4.reload.discussion_id).to eq('20000000')
+    end
+  end
+end
diff --git a/spec/lib/gitlab/background_migration/migrate_promoted_epics_discussion_ids_spec.rb b/spec/lib/gitlab/background_migration/migrate_promoted_epics_discussion_ids_spec.rb
deleted file mode 100644
index 16ca0e96ac6..00000000000
--- a/spec/lib/gitlab/background_migration/migrate_promoted_epics_discussion_ids_spec.rb
+++ /dev/null
@@ -1,52 +0,0 @@
-# frozen_string_literal: true
-
-require 'rails_helper'
-
-describe Gitlab::BackgroundMigration::FixPromotedEpicsDiscussionIds, :migration, schema: 20190715193142 do
-  let(:namespaces) { table(:namespaces) }
-  let(:projects) { table(:projects) }
-  let(:users) { table(:users) }
-  let(:issues) { table(:issues) }
-  let(:epics) { table(:epics) }
-  let(:notes) { table(:notes) }
-
-  let(:user) { users.create!(email: 'test@example.com', projects_limit: 100, username: 'test') }
-  let(:namespace) { namespaces.create(name: 'gitlab', path: 'gitlab-org') }
-  let(:project) { projects.create(namespace_id: namespace.id, name: 'foo') }
-  let(:issue) { issues.create(project_id: project.id, title: 'Issue with discussion') }
-  let!(:issue_note) { notes.create(project_id: project.id, note: 'note comment', noteable_id: issue.id, noteable_type: 'Issue', discussion_id: 'd1') }
-
-  describe '#perform with batch of discussion ids' do
-    let(:epic1) { epics.create(id: 1, author_id: user.id, iid: 1, group_id: namespace.id, title: 'Epic with discussion', title_html: 'Epic with discussion') }
-    let!(:epic1_note1) { notes.create(note: 'note comment', noteable_id: epic1.id, noteable_type: 'Epic', discussion_id: 'd1') }
-    let!(:epic1_note2) { notes.create(system: true, note: 'promoted from issue XXX', noteable_id: epic1.id, noteable_type: 'Epic', discussion_id: 'system1') }
-
-    let(:epic2) { epics.create(id: 2, author_id: user.id, iid: 2, group_id: namespace.id, title: 'Epic with discussion', title_html: 'Epic with discussion') }
-    let!(:epic2_note1) { notes.create(note: 'note comment', noteable_id: epic2.id, noteable_type: 'Epic', discussion_id: 'd2') }
-    let!(:epic2_note2) { notes.create(system: true, note: 'promoted from issue YYY', noteable_id: epic2.id, noteable_type: 'Epic', discussion_id: 'system2') }
-
-    it 'executes perform and changes discussion ids on all promoted epic discussions' do
-      expect(notes.where(discussion_id: 'd1').count).to eq(2)
-
-      subject.perform(%w(d1 system1 d2 system2))
-
-      old_discussion_notes = notes.where(discussion_id: 'd1')
-      expect(old_discussion_notes.count).to eq(1)
-      expect(old_discussion_notes.first.noteable_type).to eq('Issue')
-      expect(epic1_note1.reload.discussion_id).not_to eq('d1')
-      expect(epic1_note2.reload.discussion_id).not_to eq('system1')
-      expect(epic2_note1.reload.discussion_id).not_to eq('d2')
-      expect(epic2_note2.reload.discussion_id).not_to eq('system2')
-    end
-
-    it 'sets same new discussion id for all notes with the same old discussion id' do
-      note = notes.create(note: 'note comment', noteable_id: epic2.id, noteable_type: 'Epic', discussion_id: 'd2')
-      expect(notes.where(discussion_id: note.discussion_id).count).to eq(2)
-
-      subject.perform(['d2'])
-
-      expect(note.reload.discussion_id).not_to eq('d2')
-      expect(notes.where(discussion_id: note.discussion_id).count).to eq(2)
-    end
-  end
-end
diff --git a/spec/migrations/migrate_discussion_id_on_promoted_epics_spec.rb b/spec/migrations/migrate_discussion_id_on_promoted_epics_spec.rb
new file mode 100644
index 00000000000..57c4bd16f2e
--- /dev/null
+++ b/spec/migrations/migrate_discussion_id_on_promoted_epics_spec.rb
@@ -0,0 +1,77 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('db', 'post_migrate', '20190715193142_migrate_discussion_id_on_promoted_epics.rb')
+
+describe MigrateDiscussionIdOnPromotedEpics, :migration, :sidekiq do
+  let(:migration_class) { described_class::MIGRATION }
+  let(:migration_name)  { migration_class.to_s.demodulize }
+
+  let(:namespaces) { table(:namespaces) }
+  let(:projects) { table(:projects) }
+  let(:users) { table(:users) }
+  let(:issues) { table(:issues) }
+  let(:epics) { table(:epics) }
+  let(:notes) { table(:notes) }
+  let(:system_note_metadata) { table(:system_note_metadata) }
+
+  let(:user) { users.create!(email: 'test@example.com', projects_limit: 100, username: 'test') }
+  let(:namespace) { namespaces.create(name: 'gitlab', path: 'gitlab-org') }
+
+  def create_promotion_note(model, id)
+    note = notes.create!(system: true, note: 'promoted from issue XXX',
+                         noteable_id: model.id,
+                         noteable_type: model.class.name,
+                         discussion_id: id)
+    system_note_metadata.create!(note_id: note.id, action: 'moved')
+  end
+
+  def create_epic
+    epics.create!(author_id: user.id, iid: 1,
+                  group_id: namespace.id,
+                  title: 'Epic with discussion',
+                  title_html: 'Epic with discussion')
+  end
+
+  def create_note(model, id)
+    notes.create!(note: 'note', noteable_id: model.id,
+                  noteable_type: model.class.name, discussion_id: id)
+  end
+
+  context 'with promoted epic' do
+    let(:epic1) { create_epic }
+    let!(:note1) { create_promotion_note(epic1, 'id1') }
+
+    it 'correctly schedules background migrations in batches' do
+      create_note(epic1, 'id2')
+      create_note(epic1, 'id3')
+
+      stub_const("#{described_class.name}::BATCH_SIZE", 2)
+
+      Sidekiq::Testing.fake! do
+        Timecop.freeze do
+          migrate!
+
+          expect(migration_name).to be_scheduled_delayed_migration(2.minutes, %w(id1 id2))
+          expect(migration_name).to be_scheduled_delayed_migration(4.minutes, %w(id3))
+          expect(BackgroundMigrationWorker.jobs.size).to eq(2)
+        end
+      end
+    end
+
+    it 'schedules only promoted epics' do
+      issue = issues.create(description: 'first', state: 'opened')
+      create_promotion_note(issue, 'id2')
+      create_note(create_epic, 'id3')
+
+      Sidekiq::Testing.fake! do
+        Timecop.freeze do
+          migrate!
+
+          expect(migration_name).to be_scheduled_delayed_migration(2.minutes, %w(id1))
+          expect(BackgroundMigrationWorker.jobs.size).to eq(1)
+        end
+      end
+    end
+  end
+end