summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorAlexandru Croitor <acroitor@gitlab.com>2019-09-11 12:17:51 +0000
committerNick Thomas <nick@gitlab.com>2019-09-11 12:17:51 +0000
commitb012174d6fb5dcef10db1dc67754bb5c59aa2a07 (patch)
tree48fd9320ab6c018d7aac6828a4547244d0682b2e
parente4b28b42c3b125a7836eaabf77c2fe6df4639429 (diff)
downloadgitlab-ce-b012174d6fb5dcef10db1dc67754bb5c59aa2a07.tar.gz
Change discussion_ids on promoted epics notes
Notes on epics promoted from an issue used to get same discussion_id as the notes from the issue the epic was promoted from, which would cause problems when trying to reply to the epic discussion.
-rw-r--r--db/post_migrate/20190715193142_migrate_discussion_id_on_promoted_epics.rb62
-rw-r--r--lib/gitlab/background_migration/fix_promoted_epics_discussion_ids.rb22
-rw-r--r--spec/lib/gitlab/background_migration/fix_promoted_epics_discussion_ids_spec.rb49
-rw-r--r--spec/migrations/migrate_discussion_id_on_promoted_epics_spec.rb81
4 files changed, 214 insertions, 0 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
new file mode 100644
index 00000000000..13f8477f3ea
--- /dev/null
+++ b/db/post_migrate/20190715193142_migrate_discussion_id_on_promoted_epics.rb
@@ -0,0 +1,62 @@
+# frozen_string_literal: true
+
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class MigrateDiscussionIdOnPromotedEpics < ActiveRecord::Migration[5.2]
+ include Gitlab::Database::MigrationHelpers
+
+ # We have ~5000 unique discussion_ids -> this migration will take about 102 minutes
+ # (5000/100 * 2 minutes + 2 minutes initial delay) on gitlab.com.
+ DOWNTIME = false
+ BATCH_SIZE = 100
+ DELAY_INTERVAL = 2.minutes
+ MIGRATION = 'FixPromotedEpicsDiscussionIds'
+
+ disable_ddl_transaction!
+
+ class SystemNoteMetadata < ActiveRecord::Base
+ self.table_name = 'system_note_metadata'
+ self.inheritance_column = :_type_disabled
+ end
+
+ class Note < ActiveRecord::Base
+ include EachBatch
+
+ has_one :system_note_metadata, class_name: 'MigrateDiscussionIdOnPromotedEpics::SystemNoteMetadata'
+
+ self.table_name = 'notes'
+ self.inheritance_column = :_type_disabled
+
+ def self.fetch_discussion_ids_query
+ promoted_epics_query = Note
+ .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)
+ .distinct.pluck(:discussion_id)
+ end
+ end
+
+ def up
+ add_concurrent_index(:system_note_metadata, :note_id, where: "action='moved'", name: 'temp_index_system_note_metadata_on_moved_note_id')
+ add_concurrent_index(:notes, [:id, :noteable_id], where: "noteable_type='Epic' AND system", name: 'temp_index_notes_on_id_and_noteable_id' )
+
+ 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
+
+ remove_concurrent_index(:system_note_metadata, :note_id, where: "action='moved'", name: 'temp_index_system_note_metadata_on_moved_note_id')
+ remove_concurrent_index(:notes, [:id, :noteable_id], where: "noteable_type='Epic' AND system", name: 'temp_index_notes_on_id_and_noteable_id')
+ end
+
+ def down
+ # no-op
+ end
+end
diff --git a/lib/gitlab/background_migration/fix_promoted_epics_discussion_ids.rb b/lib/gitlab/background_migration/fix_promoted_epics_discussion_ids.rb
new file mode 100644
index 00000000000..1a80ccdee92
--- /dev/null
+++ b/lib/gitlab/background_migration/fix_promoted_epics_discussion_ids.rb
@@ -0,0 +1,22 @@
+# frozen_string_literal: true
+
+module Gitlab
+ module BackgroundMigration
+ # This migration updates discussion ids for epics that were promoted from issue so that the discussion id on epics
+ # is different from discussion id on issue, which was causing problems when replying to epic discussions as it would
+ # identify the discussion as related to an issue and complaint about missing project_id
+ class FixPromotedEpicsDiscussionIds
+ # notes model to iterate through the notes to be updated
+ class Note < ActiveRecord::Base
+ self.table_name = 'notes'
+ self.inheritance_column = :_type_disabled
+ end
+
+ def perform(discussion_ids)
+ 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
+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..73c855ac184
--- /dev/null
+++ b/spec/lib/gitlab/background_migration/fix_promoted_epics_discussion_ids_spec.rb
@@ -0,0 +1,49 @@
+# frozen_string_literal: true
+
+require 'spec_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(/\A\h{40}\z/)
+ 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 discussion 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/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..5e25d1aed82
--- /dev/null
+++ b/spec/migrations/migrate_discussion_id_on_promoted_epics_spec.rb
@@ -0,0 +1,81 @@
+# 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 = create_note(model, id, { system: true,
+ note: 'promoted from issue XXX' })
+ 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, extra_params = {})
+ params = {
+ note: 'note',
+ noteable_id: model.id,
+ noteable_type: model.class.name,
+ discussion_id: id
+ }.merge(extra_params)
+
+ notes.create!(params)
+ 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