Merge branch '198325-migrate-design-mentions-to-db-table' into 'master'

Migrate mentions from design notes to DB table

Closes #198325

See merge request gitlab-org/gitlab!23704

changelogs/unreleased/198325-migrate-design-mentions-to-db-table.yml

+---
+title: Migrate mentions for design notes to design_user_mentions DB table
+merge_request: 23704
+author:
+type: changed

db/post_migrate/20200124110831_migrate_design_notes_mentions_to_db.rb

+# frozen_string_literal: true
+
+class MigrateDesignNotesMentionsToDb < ActiveRecord::Migration[5.2]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  DELAY = 2.minutes.to_i
+  BATCH_SIZE = 10000
+  MIGRATION = 'UserMentions::CreateResourceUserMention'
+
+  INDEX_NAME = 'design_mentions_temp_index'
+  INDEX_CONDITION = "note LIKE '%@%'::text AND notes.noteable_type = 'DesignManagement::Design'"
+  QUERY_CONDITIONS = "#{INDEX_CONDITION} AND design_user_mentions.design_id IS NULL"
+  JOIN = 'INNER JOIN design_management_designs ON design_management_designs.id = notes.noteable_id LEFT JOIN design_user_mentions ON notes.id = design_user_mentions.note_id'
+
+  class DesignUserMention < ActiveRecord::Base
+    include EachBatch
+
+    self.table_name = 'design_user_mentions'
+  end
+
+  class Note < ActiveRecord::Base
+    include EachBatch
+
+    self.table_name = 'notes'
+  end
+
+  def up
+    return unless Gitlab.ee?
+
+    # cleanup design user mentions with no actual mentions,
+    # re https://gitlab.com/gitlab-org/gitlab/-/merge_requests/24586#note_285982468
+    DesignUserMention
+      .where(mentioned_users_ids: nil)
+      .where(mentioned_groups_ids: nil)
+      .where(mentioned_projects_ids: nil)
+      .each_batch(of: BATCH_SIZE) do |batch|
+      batch.delete_all
+    end
+
+    # create temporary index for notes with mentions, may take well over 1h
+    add_concurrent_index(:notes, :id, where: INDEX_CONDITION, name: INDEX_NAME)
+
+    Note
+      .joins(JOIN)
+      .where(QUERY_CONDITIONS)
+      .each_batch(of: BATCH_SIZE) do |batch, index|
+      range = batch.pluck(Arel.sql('MIN(notes.id)'), Arel.sql('MAX(notes.id)')).first
+      BackgroundMigrationWorker.perform_in(index * DELAY, MIGRATION, ['DesignManagement::Design', JOIN, QUERY_CONDITIONS, true, *range])
+    end
+  end
+
+  def down
+    # no-op
+    # temporary index is to be dropped in a different migration in an upcoming release:
+    # https://gitlab.com/gitlab-org/gitlab/issues/196842
+  end
+end

db/schema.rb

@@ -2808,6 +2808,7 @@ ActiveRecord::Schema.define(version: 2020_02_13_220211) do
     t.index ["commit_id"], name: "index_notes_on_commit_id"
     t.index ["created_at"], name: "index_notes_on_created_at"
     t.index ["discussion_id"], name: "index_notes_on_discussion_id"
+    t.index ["id"], name: "design_mentions_temp_index", where: "((note ~~ '%@%'::text) AND ((noteable_type)::text = 'DesignManagement::Design'::text))"
     t.index ["id"], name: "epic_mentions_temp_index", where: "((note ~~ '%@%'::text) AND ((noteable_type)::text = 'Epic'::text))"
     t.index ["line_code"], name: "index_notes_on_line_code"
     t.index ["note"], name: "index_notes_on_note_trigram", opclass: :gin_trgm_ops, using: :gin

ee/spec/lib/gitlab/background_migration/user_mentions/create_resource_user_mention_spec.rb

@@ -3,6 +3,7 @@
 require 'spec_helper'
 require './db/post_migrate/20191115115043_migrate_epic_mentions_to_db'
 require './db/post_migrate/20191115115522_migrate_epic_notes_mentions_to_db'
+require './db/post_migrate/20200124110831_migrate_design_notes_mentions_to_db'
 
 describe Gitlab::BackgroundMigration::UserMentions::CreateResourceUserMention do
   include MigrationsHelpers
@@ -50,10 +51,14 @@ describe Gitlab::BackgroundMigration::UserMentions::CreateResourceUserMention do
         epics.create!(iid: 1, group_id: group.id, author_id: author.id, title: "epic title @#{author.username}",
                       title_html: "epic title  @#{author.username}", description: description_mentions)
       end
-      let!(:epic_without_mentions) do
+      let!(:epic2) do
         epics.create!(iid: 2, group_id: group.id, author_id: author.id, title: "epic title}",
                       title_html: "epic title", description: 'simple description')
       end
+      let!(:epic3) do
+        epics.create!(iid: 2, group_id: group.id, author_id: author.id, title: "epic title}",
+                      title_html: "epic title", description: 'description with an email@example.com and some other @ char here.')
+      end
 
       let(:user_mentions) { epic_user_mentions }
       let(:resource) { epic }
@@ -65,13 +70,38 @@ describe Gitlab::BackgroundMigration::UserMentions::CreateResourceUserMention do
       end
 
       context 'mentions in epic notes' do
-        let(:note1) { notes.create!(noteable_id: epic.id, noteable_type: 'Epic', author_id: author.id, note: description_mentions) }
-        let(:note2) { notes.create!(noteable_id: epic.id, noteable_type: 'Epic', author_id: author.id, note: 'sample note') }
-        let(:note3) { notes.create!(noteable_id: epic.id, noteable_type: 'Epic', author_id: author.id, note: description_mentions, system: true) }
-        let!(:note4) { notes.create!(noteable_id: epics.maximum(:id) + 10, noteable_type: 'Epic', author_id: author.id, note: description_mentions, project_id: project.id) }
+        let!(:note1) { notes.create!(noteable_id: epic.id, noteable_type: 'Epic', author_id: author.id, note: description_mentions) }
+        let!(:note2) { notes.create!(noteable_id: epic.id, noteable_type: 'Epic', author_id: author.id, note: 'sample note') }
+        let!(:note3) { notes.create!(noteable_id: epic.id, noteable_type: 'Epic', author_id: author.id, note: description_mentions, system: true) }
+        # this not does not have actual mentions
+        let!(:note4) { notes.create!(noteable_id: epic.id, noteable_type: 'Epic', author_id: author.id, note: 'note3 for an email@somesite.com and some other rando @ ref' ) }
+        # this not points to an innexistent noteable record in desigs table
+        let!(:note5) { notes.create!(noteable_id: epics.maximum(:id) + 10, noteable_type: 'Epic', author_id: author.id, note: description_mentions, project_id: project.id) }
 
         it_behaves_like 'resource notes mentions migration', MigrateEpicNotesMentionsToDb, Epic
       end
     end
   end
+
+  describe 'design mentions' do
+    let(:projects) { table(:projects) }
+    let(:designs) { table(:design_management_designs) }
+    let(:design_user_mentions) { table(:design_user_mentions) }
+
+    let(:project) { projects.create!(id: 1, name: 'gitlab1', path: 'gitlab1', namespace_id: group.id, visibility_level: 0) }
+    let!(:design) { designs.create!(filename: 'test.png', project_id: project.id) }
+
+    let!(:note1) { notes.create!(noteable_id: design.id, noteable_type: 'DesignManagement::Design', project_id: project.id, author_id: author.id, note: description_mentions) }
+    let!(:note2) { notes.create!(noteable_id: design.id, noteable_type: 'DesignManagement::Design', project_id: project.id, author_id: author.id, note: 'sample note') }
+    let!(:note3) { notes.create!(noteable_id: design.id, noteable_type: 'DesignManagement::Design', project_id: project.id, author_id: author.id, note: description_mentions, system: true) }
+    # this not does not have actual mentions
+    let!(:note4) { notes.create!(noteable_id: design.id, noteable_type: 'DesignManagement::Design', project_id: project.id, author_id: author.id, note: 'note3 for an email@somesite.com and some other rando @ ref' ) }
+    # this not points to an innexistent noteable record in desigs table
+    let!(:note5) { notes.create!(noteable_id: designs.maximum(:id) + 10, noteable_type: 'DesignManagement::Design', project_id: project.id, author_id: author.id, note: description_mentions) }
+
+    let(:user_mentions) { design_user_mentions }
+    let(:resource) { design }
+
+    it_behaves_like 'resource notes mentions migration', MigrateDesignNotesMentionsToDb, DesignManagement::Design
+  end
 end

ee/spec/migrations/migrate_design_notes_mentions_to_db_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('db', 'post_migrate', '20200124110831_migrate_design_notes_mentions_to_db')
+
+describe MigrateDesignNotesMentionsToDb, :migration, :sidekiq do
+  let(:users) { table(:users) }
+  let(:projects) { table(:projects) }
+  let(:namespaces) { table(:namespaces) }
+  let(:designs) { table(:design_management_designs) }
+  let(:design_user_mentions) { table(:design_user_mentions) }
+  let(:notes) { table(:notes) }
+
+  let(:user) { users.create!(name: 'root', email: 'root@example.com', username: 'root', projects_limit: 0) }
+  let(:group) { namespaces.create!(name: 'group1', path: 'group1', owner_id: user.id) }
+  let(:project) { projects.create!(name: 'gitlab1', path: 'gitlab1', namespace_id: group.id, visibility_level: 0) }
+  let(:design) { designs.create!(filename: 'test.png', project_id: project.id) }
+
+  let!(:resource1) { notes.create!(note: 'note1 for @root to check', noteable_id: design.id, noteable_type: 'DesignManagement::Design') }
+  let!(:resource2) { notes.create!(note: 'note2 for @root to check', noteable_id: design.id, noteable_type: 'DesignManagement::Design', system: true) }
+  let!(:resource3) { notes.create!(note: 'note3 for @root to check', noteable_id: design.id, noteable_type: 'DesignManagement::Design') }
+
+  # non-migrateable resources
+  # this note is already migrated, as it has a record in the design_user_mentions table
+  let!(:resource4) { notes.create!(note: 'note3 for @root to check', noteable_id: design.id, noteable_type: 'DesignManagement::Design') }
+  let!(:user_mention) { design_user_mentions.create!(design_id: design.id, note_id: resource4.id, mentioned_users_ids: [1]) }
+  # this note points to an innexistent noteable record
+  let!(:resource5) { notes.create!(note: 'note3 for @root to check', noteable_id: designs.maximum(:id) + 10, noteable_type: 'DesignManagement::Design') }
+
+  before do
+    stub_const("#{described_class.name}::BATCH_SIZE", 1)
+  end
+
+  it_behaves_like 'schedules resource mentions migration', DesignManagement::Design, true
+end

ee/spec/migrations/migrate_epic_mentions_to_db_spec.rb

@@ -7,13 +7,21 @@ describe MigrateEpicMentionsToDb, :migration do
   let(:users) { table(:users) }
   let(:namespaces) { table(:namespaces) }
   let(:epics) { table(:epics) }
+  let(:epic_user_mentions) { table(:epic_user_mentions) }
 
   let(:user) { users.create!(name: 'root', email: 'root@example.com', username: 'root', projects_limit: 0) }
   let(:group) { namespaces.create!(name: 'group1', path: 'group1', owner_id: user.id, type: 'Group') }
+
+  # migrateable resources
   let!(:resource1) { epics.create!(iid: 1, title: "title1", title_html: 'title1', description: 'epic description with @root mention', group_id: group.id, author_id: user.id) }
   let!(:resource2) { epics.create!(iid: 2, title: "title2", title_html: 'title2', description: 'epic description with @root mention', group_id: group.id, author_id: user.id) }
   let!(:resource3) { epics.create!(iid: 3, title: "title3", title_html: 'title3', description: 'epic description with @root mention', group_id: group.id, author_id: user.id) }
 
+  # non-migrateable resources
+  # this epic is already migrated, as it has a record in the epic_user_mentions table
+  let!(:resource4) { epics.create!(iid: 4, title: "title3", title_html: 'title3', description: 'epic description with @root mention', group_id: group.id, author_id: user.id) }
+  let!(:user_mention) { epic_user_mentions.create!(epic_id: resource4.id, mentioned_users_ids: [1]) }
+
   before do
     stub_const("#{described_class.name}::BATCH_SIZE", 1)
   end

ee/spec/migrations/migrate_epic_notes_mentions_to_db_spec.rb

@@ -8,14 +8,24 @@ describe MigrateEpicNotesMentionsToDb, :migration do
   let(:namespaces) { table(:namespaces) }
   let(:epics) { table(:epics) }
   let(:notes) { table(:notes) }
+  let(:epic_user_mentions) { table(:epic_user_mentions) }
 
   let(:user) { users.create!(name: 'root', email: 'root@example.com', username: 'root', projects_limit: 0) }
   let(:group) { namespaces.create!(name: 'group1', path: 'group1', owner_id: user.id, type: 'Group') }
   let(:epic) { epics.create!(iid: 1, title: "title", title_html: 'title', description: 'epic description', group_id: group.id, author_id: user.id) }
+
+  # migrateable resources
   let!(:resource1) { notes.create!(note: 'note1 for @root to check', noteable_id: epic.id, noteable_type: 'Epic') }
   let!(:resource2) { notes.create!(note: 'note2 for @root to check', noteable_id: epic.id, noteable_type: 'Epic', system: true) }
   let!(:resource3) { notes.create!(note: 'note3 for @root to check', noteable_id: epic.id, noteable_type: 'Epic') }
 
+  # non-migrateable resources
+  # this note is already migrated, as it has a record in the epic_user_mentions table
+  let!(:resource4) { notes.create!(note: 'note3 for @root to check', noteable_id: epic.id, noteable_type: 'Epic') }
+  let!(:user_mention) { epic_user_mentions.create!(epic_id: epic.id, note_id: resource4.id, mentioned_users_ids: [1]) }
+  # this note points to an innexistent noteable record
+  let!(:resource5) { notes.create!(note: 'note3 for @root to check', noteable_id: epics.maximum(:id) + 10, noteable_type: 'Epic') }
+
   before do
     stub_const("#{described_class.name}::BATCH_SIZE", 1)
   end

lib/gitlab/background_migration/user_mentions/create_resource_user_mention.rb

@@ -21,7 +21,8 @@ module Gitlab
           records.in_groups_of(BULK_INSERT_SIZE, false).each do |records|
             mentions = []
             records.each do |record|
-              mentions << record.build_mention_values(resource_user_mention_model.resource_foreign_key)
+              mention_record = record.build_mention_values(resource_user_mention_model.resource_foreign_key)
+              mentions << mention_record unless mention_record.blank?
             end
 
             Gitlab::Database.bulk_insert(

lib/gitlab/background_migration/user_mentions/models/concerns/isolated_mentionable.rb

@@ -68,12 +68,18 @@ module Gitlab
           def build_mention_values(resource_foreign_key)
             refs = all_references(author)
 
+            mentioned_users_ids = array_to_sql(refs.mentioned_users.pluck(:id))
+            mentioned_projects_ids = array_to_sql(refs.mentioned_projects.pluck(:id))
+            mentioned_groups_ids = array_to_sql(refs.mentioned_groups.pluck(:id))
+
+            return if mentioned_users_ids.blank? && mentioned_projects_ids.blank? && mentioned_groups_ids.blank?
+
             {
               "#{resource_foreign_key}": user_mention_resource_id,
               note_id: user_mention_note_id,
-              mentioned_users_ids: array_to_sql(refs.mentioned_users.pluck(:id)),
-              mentioned_projects_ids: array_to_sql(refs.mentioned_projects.pluck(:id)),
-              mentioned_groups_ids: array_to_sql(refs.mentioned_groups.pluck(:id))
+              mentioned_users_ids: mentioned_users_ids,
+              mentioned_projects_ids: mentioned_projects_ids,
+              mentioned_groups_ids: mentioned_groups_ids
             }
           end
 

lib/gitlab/background_migration/user_mentions/models/design.rb

+# frozen_string_literal: true
+# rubocop:disable Style/Documentation
+
+module Gitlab
+  module BackgroundMigration
+    module UserMentions
+      module Models
+        module DesignManagement
+          class Design < ActiveRecord::Base
+            include MentionableMigrationMethods
+
+            def self.user_mention_model
+              Gitlab::BackgroundMigration::UserMentions::Models::DesignUserMention
+            end
+
+            def user_mention_model
+              self.class.user_mention_model
+            end
+
+            def user_mention_resource_id
+              id
+            end
+
+            def user_mention_note_id
+              'NULL'
+            end
+          end
+        end
+      end
+    end
+  end
+end

lib/gitlab/background_migration/user_mentions/models/design_user_mention.rb

+# frozen_string_literal: true
+# rubocop:disable Style/Documentation
+
+module Gitlab
+  module BackgroundMigration
+    module UserMentions
+      module Models
+        class DesignUserMention < ActiveRecord::Base
+          self.table_name = 'design_user_mentions'
+
+          def self.resource_foreign_key
+            :design_id
+          end
+        end
+      end
+    end
+  end
+end

spec/support/shared_examples/lib/gitlab/background_migration/mentions_migration_shared_examples.rb

@@ -26,16 +26,18 @@ shared_examples 'resource notes mentions migration' do |migration_class, resourc
     note1.becomes(Note).save!
     note2.becomes(Note).save!
     note3.becomes(Note).save!
-    # note4.becomes(Note).save(validate: false)
+    note4.becomes(Note).save!
+    note5.becomes(Note).save(validate: false)
   end
 
   it 'migrates mentions from note' do
     join = migration_class::JOIN
     conditions = migration_class::QUERY_CONDITIONS
 
-    # there are 4 notes for each noteable_type, but one does not have mentions and
+    # there are 5 notes for each noteable_type, but two do not have mentions and
     # another one's noteable_id points to an inexistent resource
-    expect(notes.where(noteable_type: resource_class.to_s).count).to eq 4
+    expect(notes.where(noteable_type: resource_class.to_s).count).to eq 5
+    expect(user_mentions.count).to eq 0
 
     expect do
       subject.perform(resource_class.name, join, conditions, true, Note.minimum(:id), Note.maximum(:id))