Merge branch '221167-background-migration' into 'master'

Backfill relative positions on designs

See merge request gitlab-org/gitlab!37837

app/models/concerns/relative_positioning.rb

@@ -32,17 +32,32 @@ module RelativePositioning
   class_methods do
     def move_nulls_to_end(objects)
       objects = objects.reject(&:relative_position)
-
       return if objects.empty?
 
+      self.transaction do
         max_relative_position = objects.first.max_relative_position
 
-      self.transaction do
         objects.each do |object|
           relative_position = position_between(max_relative_position || START_POSITION, MAX_POSITION)
-          object.relative_position = relative_position
+          object.update_column(:relative_position, relative_position)
+
           max_relative_position = relative_position
-          object.save(touch: false)
+        end
+      end
+    end
+
+    def move_nulls_to_start(objects)
+      objects = objects.reject(&:relative_position)
+      return if objects.empty?
+
+      self.transaction do
+        min_relative_position = objects.first.min_relative_position
+
+        objects.reverse_each do |object|
+          relative_position = position_between(MIN_POSITION, min_relative_position || START_POSITION)
+          object.update_column(:relative_position, relative_position)
+
+          min_relative_position = relative_position
         end
       end
     end

changelogs/unreleased/221167-background-migration.yml

+---
+title: Backfill relative positions on designs
+merge_request: 37837
+author:
+type: changed

db/post_migrate/20200724130639_backfill_designs_relative_position.rb

+# frozen_string_literal: true
+
+class BackfillDesignsRelativePosition < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+  INTERVAL = 2.minutes
+  BATCH_SIZE = 1000
+  MIGRATION = 'BackfillDesignsRelativePosition'
+
+  disable_ddl_transaction!
+
+  class Issue < ActiveRecord::Base
+    include EachBatch
+
+    self.table_name = 'issues'
+
+    has_many :designs
+  end
+
+  class Design < ActiveRecord::Base
+    self.table_name = 'design_management_designs'
+  end
+
+  def up
+    issues_with_designs = Issue.where(id: Design.select(:issue_id))
+
+    issues_with_designs.each_batch(of: BATCH_SIZE) do |relation, index|
+      issue_ids = relation.pluck(:id)
+      delay = INTERVAL * index
+
+      migrate_in(delay, MIGRATION, [issue_ids])
+    end
+  end
+
+  def down
+  end
+end

db/schema_migrations/20200724130639

+9d711a0c4f785660c0a2317e598e427d5e2f91b177e4c0b96cef2958f787aa6e
\ No newline at end of file

lib/gitlab/background_migration/backfill_designs_relative_position.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module BackgroundMigration
+    # Backfill `relative_position` column in `design_management_designs` table
+    class BackfillDesignsRelativePosition
+      # Define the issue model
+      class Issue < ActiveRecord::Base
+        self.table_name = 'issues'
+      end
+
+      # Define the design model
+      class Design < ActiveRecord::Base
+        include RelativePositioning if defined?(RelativePositioning)
+
+        self.table_name = 'design_management_designs'
+
+        def self.relative_positioning_query_base(design)
+          where(issue_id: design.issue_id)
+        end
+
+        def self.relative_positioning_parent_column
+          :issue_id
+        end
+
+        def self.move_nulls_to_start(designs)
+          if defined?(super)
+            super(designs)
+          else
+            logger.error "BackfillDesignsRelativePosition failed because move_nulls_to_start is no longer included in the RelativePositioning concern"
+          end
+        end
+      end
+
+      def perform(issue_ids)
+        issue_ids.each do |issue_id|
+          migrate_issue(issue_id)
+        end
+      end
+
+      private
+
+      def migrate_issue(issue_id)
+        issue = Issue.find_by(id: issue_id)
+        return unless issue
+
+        designs = Design.where(issue_id: issue.id).order(:id)
+        return unless designs.any?
+
+        Design.move_nulls_to_start(designs)
+      end
+    end
+  end
+end

spec/lib/gitlab/background_migration/backfill_designs_relative_position_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::BackgroundMigration::BackfillDesignsRelativePosition do
+  let(:namespace) { table(:namespaces).create!(name: 'gitlab', path: 'gitlab') }
+  let(:project) { table(:projects).create!(namespace_id: namespace.id) }
+  let(:issues) { table(:issues) }
+  let(:designs) { table(:design_management_designs) }
+
+  before do
+    issues.create!(id: 1, project_id: project.id)
+    issues.create!(id: 2, project_id: project.id)
+    issues.create!(id: 3, project_id: project.id)
+    issues.create!(id: 4, project_id: project.id)
+
+    designs.create!(id: 1, issue_id: 1, project_id: project.id, filename: 'design1.jpg')
+    designs.create!(id: 2, issue_id: 1, project_id: project.id, filename: 'design2.jpg')
+    designs.create!(id: 3, issue_id: 2, project_id: project.id, filename: 'design3.jpg')
+    designs.create!(id: 4, issue_id: 2, project_id: project.id, filename: 'design4.jpg')
+    designs.create!(id: 5, issue_id: 3, project_id: project.id, filename: 'design5.jpg')
+  end
+
+  describe '#perform' do
+    it 'backfills the position for the designs in each issue' do
+      expect(described_class::Design).to receive(:move_nulls_to_start).with(
+        a_collection_containing_exactly(
+          an_object_having_attributes(id: 1, issue_id: 1),
+          an_object_having_attributes(id: 2, issue_id: 1)
+        )
+      ).ordered.and_call_original
+
+      expect(described_class::Design).to receive(:move_nulls_to_start).with(
+        a_collection_containing_exactly(
+          an_object_having_attributes(id: 3, issue_id: 2),
+          an_object_having_attributes(id: 4, issue_id: 2)
+        )
+      ).ordered.and_call_original
+
+      # We only expect calls to `move_nulls_to_start` with issues 1 and 2:
+      # - Issue 3 should be skipped because we're not passing its ID
+      # - Issue 4 should be skipped because it doesn't have any designs
+      # - Issue 0 should be skipped because it doesn't exist
+      subject.perform([1, 2, 4, 0])
+
+      expect(designs.find(1).relative_position).to be < designs.find(2).relative_position
+      expect(designs.find(3).relative_position).to be < designs.find(4).relative_position
+      expect(designs.find(5).relative_position).to be_nil
+    end
+  end
+end

spec/migrations/backfill_designs_relative_position_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('db', 'post_migrate', '20200724130639_backfill_designs_relative_position.rb')
+
+RSpec.describe BackfillDesignsRelativePosition do
+  let(:namespace) { table(:namespaces).create!(name: 'gitlab', path: 'gitlab') }
+  let(:project) { table(:projects).create!(namespace_id: namespace.id) }
+  let(:issues) { table(:issues) }
+  let(:designs) { table(:design_management_designs) }
+
+  before do
+    issues.create!(id: 1, project_id: project.id)
+    issues.create!(id: 2, project_id: project.id)
+    issues.create!(id: 3, project_id: project.id)
+    issues.create!(id: 4, project_id: project.id)
+
+    designs.create!(issue_id: 1, project_id: project.id, filename: 'design1.jpg')
+    designs.create!(issue_id: 2, project_id: project.id, filename: 'design2.jpg')
+    designs.create!(issue_id: 4, project_id: project.id, filename: 'design3.jpg')
+
+    stub_const("#{described_class.name}::BATCH_SIZE", 2)
+  end
+
+  it 'correctly schedules background migrations' do
+    Sidekiq::Testing.fake! do
+      Timecop.freeze do
+        migrate!
+
+        expect(described_class::MIGRATION)
+          .to be_scheduled_delayed_migration(2.minutes, [1, 2])
+
+        expect(described_class::MIGRATION)
+          .to be_scheduled_delayed_migration(4.minutes, [4])
+
+        # Issue 3 should be skipped because it doesn't have any designs
+        expect(BackgroundMigrationWorker.jobs.size).to eq(2)
+      end
+    end
+  end
+end

spec/support/shared_examples/models/relative_positioning_shared_examples.rb

 # frozen_string_literal: true
 
 RSpec.shared_examples 'a class that supports relative positioning' do
-  let(:item1) { create(factory, default_params) }
-  let(:item2) { create(factory, default_params) }
-  let(:new_item) { create(factory, default_params) }
+  let(:item1) { create_item }
+  let(:item2) { create_item }
+  let(:new_item) { create_item }
 
-  def create_item(params)
+  def create_item(params = {})
     create(factory, params.merge(default_params))
   end
 
@@ -16,21 +16,30 @@ RSpec.shared_examples 'a class that supports relative positioning' do
   end
 
   describe '.move_nulls_to_end' do
+    let(:item3) { create_item }
+
     it 'moves items with null relative_position to the end' do
-      item1.update!(relative_position: nil)
+      item1.update!(relative_position: 1000)
       item2.update!(relative_position: nil)
+      item3.update!(relative_position: nil)
 
-      described_class.move_nulls_to_end([item1, item2])
+      items = [item1, item2, item3]
+      described_class.move_nulls_to_end(items)
+      items.map(&:reload)
 
-      expect(item2.prev_relative_position).to eq item1.relative_position
-      expect(item1.prev_relative_position).to eq nil
-      expect(item2.next_relative_position).to eq nil
+      expect(items.sort_by(&:relative_position)).to eq(items)
+      expect(item1.relative_position).to be(1000)
+      expect(item1.prev_relative_position).to be_nil
+      expect(item1.next_relative_position).to eq(item2.relative_position)
+      expect(item2.next_relative_position).to eq(item3.relative_position)
+      expect(item3.next_relative_position).to be_nil
     end
 
     it 'moves the item near the start position when there are no existing positions' do
       item1.update!(relative_position: nil)
 
       described_class.move_nulls_to_end([item1])
+      item1.reload
 
       expect(item1.relative_position).to eq(described_class::START_POSITION + described_class::IDEAL_DISTANCE)
     end
@@ -38,9 +47,49 @@ RSpec.shared_examples 'a class that supports relative positioning' do
     it 'does not perform any moves if all items have their relative_position set' do
       item1.update!(relative_position: 1)
 
-      expect(item1).not_to receive(:save)
-
       described_class.move_nulls_to_end([item1])
+      item1.reload
+
+      expect(item1.relative_position).to be(1)
+    end
+  end
+
+  describe '.move_nulls_to_start' do
+    let(:item3) { create_item }
+
+    it 'moves items with null relative_position to the start' do
+      item1.update!(relative_position: nil)
+      item2.update!(relative_position: nil)
+      item3.update!(relative_position: 1000)
+
+      items = [item1, item2, item3]
+      described_class.move_nulls_to_start(items)
+      items.map(&:reload)
+
+      expect(items.sort_by(&:relative_position)).to eq(items)
+      expect(item1.prev_relative_position).to eq nil
+      expect(item1.next_relative_position).to eq item2.relative_position
+      expect(item2.next_relative_position).to eq item3.relative_position
+      expect(item3.next_relative_position).to eq nil
+      expect(item3.relative_position).to be(1000)
+    end
+
+    it 'moves the item near the start position when there are no existing positions' do
+      item1.update!(relative_position: nil)
+
+      described_class.move_nulls_to_start([item1])
+      item1.reload
+
+      expect(item1.relative_position).to eq(described_class::START_POSITION - described_class::IDEAL_DISTANCE)
+    end
+
+    it 'does not perform any moves if all items have their relative_position set' do
+      item1.update!(relative_position: 1)
+
+      described_class.move_nulls_to_start([item1])
+      item1.reload
+
+      expect(item1.relative_position).to be(1)
     end
   end