diff options
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 |