Merge branch 'reorder-designs-fixes' into 'master'

Fixes for design reordering

See merge request gitlab-org/gitlab!39555

app/assets/javascripts/design_management/utils/cache_update.js

 /* eslint-disable @gitlab/require-i18n-strings */
 
+import { groupBy } from 'lodash';
 import createFlash from '~/flash';
 import { extractCurrentDiscussion, extractDesign } from './design_management_utils';
 import {
@@ -159,13 +160,11 @@ const updateImageDiffNoteInStore = (store, updateImageDiffNote, query, variables
 const addNewDesignToStore = (store, designManagementUpload, query) => {
   const data = store.readQuery(query);
 
-  const newDesigns = data.project.issue.designCollection.designs.nodes.reduce((acc, design) => {
-    if (!acc.find(d => d.filename === design.filename)) {
-      acc.push(design);
-    }
-
-    return acc;
-  }, designManagementUpload.designs);
+  const currentDesigns = data.project.issue.designCollection.designs.nodes;
+  const existingDesigns = groupBy(currentDesigns, 'filename');
+  const newDesigns = currentDesigns.concat(
+    designManagementUpload.designs.filter(d => !existingDesigns[d.filename]),
+  );
 
   let newVersionNode;
   const findNewVersions = designManagementUpload.designs.find(design => design.versions);

app/graphql/mutations/design_management/move.rb

@@ -21,7 +21,7 @@ module Mutations
             description: "The current state of the collection"
 
       def ready(*)
-        raise ::Gitlab::Graphql::Errors::ResourceNotAvailable unless ::Feature.enabled?(:reorder_designs)
+        raise ::Gitlab::Graphql::Errors::ResourceNotAvailable unless ::Feature.enabled?(:reorder_designs, default_enabled: true)
       end
 
       def resolve(**args)

app/models/design_management/design.rb

@@ -82,7 +82,7 @@ module DesignManagement
     scope :ordered, -> (project) do
       # TODO: Always order by relative position after the feature flag is removed
       # https://gitlab.com/gitlab-org/gitlab/-/issues/34382
-      if Feature.enabled?(:reorder_designs, project)
+      if Feature.enabled?(:reorder_designs, project, default_enabled: true)
         # We need to additionally sort by `id` to support keyset pagination.
         # See https://gitlab.com/gitlab-org/gitlab/-/merge_requests/17788/diffs#note_230875678
         order(:relative_position, :id)

app/models/design_management/design_collection.rb

@@ -16,9 +16,7 @@ module DesignManagement
 
     def find_or_create_design!(filename:)
       designs.find { |design| design.filename == filename } ||
-        designs.safe_find_or_create_by!(project: project, filename: filename) do |design|
-          design.move_to_end
-        end
+        designs.safe_find_or_create_by!(project: project, filename: filename)
     end
 
     def versions

app/policies/project_policy.rb

@@ -103,7 +103,7 @@ class ProjectPolicy < BasePolicy
 
   with_scope :subject
   condition(:moving_designs_disabled) do
-    !::Feature.enabled?(:reorder_designs, @subject)
+    !::Feature.enabled?(:reorder_designs, @subject, default_enabled: true)
   end
 
   with_scope :subject

app/services/design_management/move_designs_service.rb

@@ -13,7 +13,7 @@ module DesignManagement
 
     def execute
       return error(:no_focus) unless current_design.present?
-      return error(:cannot_move) unless ::Feature.enabled?(:reorder_designs, project)
+      return error(:cannot_move) unless ::Feature.enabled?(:reorder_designs, project, default_enabled: true)
       return error(:cannot_move) unless current_user.can?(:move_design, current_design)
       return error(:no_neighbors) unless neighbors.present?
       return error(:not_distinct) unless all_distinct?

changelogs/unreleased/reorder-designs-fixes.yml

+---
+title: Enable reorder_designs feature by default
+merge_request: 39555
+author:
+type: changed

config/feature_flags/development/reorder_designs.yml

@@ -4,4 +4,4 @@ introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/37835
 rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/232992
 group: group::knowledge
 type: development
-default_enabled: false
+default_enabled: true

db/post_migrate/20200724130639_backfill_designs_relative_position.rb

 # 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

doc/development/deleting_migrations.md

@@ -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.

lib/gitlab/background_migration/backfill_designs_relative_position.rb

@@ -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

spec/features/projects/issues/design_management/user_uploads_designs_spec.rb

@@ -5,9 +5,9 @@ require 'spec_helper'
 RSpec.describe 'User uploads new design', :js do
   include DesignManagementTestHelpers
 
-  let_it_be(:project) { create(:project_empty_repo, :public) }
-  let_it_be(:user) { project.owner }
-  let_it_be(:issue) { create(:issue, project: project) }
+  let(:project) { create(:project_empty_repo, :public) }
+  let(:user) { project.owner }
+  let(:issue) { create(:issue, project: project) }
 
   before do
     sign_in(user)
@@ -28,7 +28,7 @@ RSpec.describe 'User uploads new design', :js do
       let(:feature_enabled) { true }
 
       it 'uploads designs' do
-        attach_file(:design_file, logo_fixture, make_visible: true)
+        upload_design(logo_fixture, count: 1)
 
         expect(page).to have_selector('.js-design-list-item', count: 1)
 
@@ -36,9 +36,12 @@ RSpec.describe 'User uploads new design', :js do
           expect(page).to have_content('dk.png')
         end
 
-        attach_file(:design_file, gif_fixture, make_visible: true)
+        upload_design(gif_fixture, count: 2)
 
+        # Known bug in the legacy implementation: new designs are inserted
+        # in the beginning on the frontend.
         expect(page).to have_selector('.js-design-list-item', count: 2)
+        expect(page.all('.js-design-list-item').map(&:text)).to eq(['banana_sample.gif', 'dk.png'])
       end
     end
 
@@ -61,8 +64,8 @@ RSpec.describe 'User uploads new design', :js do
     context "when the feature is available" do
       let(:feature_enabled) { true }
 
-      it 'uploads designs', quarantine: 'https://gitlab.com/gitlab-org/gitlab/-/issues/225616' do
-        attach_file(:design_file, logo_fixture, make_visible: true)
+      it 'uploads designs' do
+        upload_design(logo_fixture, count: 1)
 
         expect(page).to have_selector('.js-design-list-item', count: 1)
 
@@ -70,9 +73,10 @@ RSpec.describe 'User uploads new design', :js do
           expect(page).to have_content('dk.png')
         end
 
-        attach_file(:design_file, gif_fixture, make_visible: true)
+        upload_design(gif_fixture, count: 2)
 
         expect(page).to have_selector('.js-design-list-item', count: 2)
+        expect(page.all('.js-design-list-item').map(&:text)).to eq(['dk.png', 'banana_sample.gif'])
       end
     end
 
@@ -92,4 +96,12 @@ RSpec.describe 'User uploads new design', :js do
   def gif_fixture
     Rails.root.join('spec', 'fixtures', 'banana_sample.gif')
   end
+
+  def upload_design(fixture, count:)
+    attach_file(:design_file, fixture, match: :first, make_visible: true)
+
+    wait_for('designs uploaded') do
+      issue.reload.designs.count == count
+    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/models/design_management/design_collection_spec.rb

@@ -37,9 +37,11 @@ RSpec.describe DesignManagement::DesignCollection do
 
     it 'inserts the design after any existing designs' do
       design1 = collection.find_or_create_design!(filename: 'design1.jpg')
+      design1.update!(relative_position: 100)
+
       design2 = collection.find_or_create_design!(filename: 'design2.jpg')
 
-      expect(design1.relative_position).to be < design2.relative_position
+      expect(collection.designs.ordered(issue.project)).to eq([design1, design2])
     end
   end