Commit 38e321c6 authored by Markus Koller's avatar Markus Koller

Disable migration to backfill design positions

We don't actually need this anymore, since we're now also backfilling
immediately before moving a design.
parent 5f9b6c98
# frozen_string_literal: true
# This migration is not needed anymore and was disabled, because we're now
# also backfilling design positions immediately before moving a design.
#
# See https://gitlab.com/gitlab-org/gitlab/-/merge_requests/39555
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
# no-op
end
def down
# no-op
end
end
......@@ -23,7 +23,7 @@ Migrations can be disabled if:
In order to disable a migration, the following steps apply to all types of migrations:
1. Turn the migration into a no-op by removing the code inside `#up`, `#down`
or `#perform` methods, and adding `#no-op` comment instead.
or `#perform` methods, and adding `# no-op` comment instead.
1. Add a comment explaining why the code is gone.
Disabling migrations requires explicit approval of Database Maintainer.
......
......@@ -2,52 +2,13 @@
module Gitlab
module BackgroundMigration
# Backfill `relative_position` column in `design_management_designs` table
# This migration is not needed anymore and was disabled, because we're now
# also backfilling design positions immediately before moving a design.
#
# See https://gitlab.com/gitlab-org/gitlab/-/merge_requests/39555
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)
# no-op
end
end
end
......
# 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
# 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
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment