Merge branch '221167-model-changes' into 'master'

Add relative positioning on designs

See merge request gitlab-org/gitlab!37835

app/finders/design_management/designs_finder.rb

@@ -22,7 +22,9 @@ module DesignManagement
       items = by_filename(items)
       items = by_id(items)
 
-      items
+      # TODO: We don't need to pass the project anymore after the feature flag is removed
+      # https://gitlab.com/gitlab-org/gitlab/-/issues/34382
+      items.ordered(issue.project)
     end
 
     private

app/models/application_record.rb

@@ -42,15 +42,15 @@ class ApplicationRecord < ActiveRecord::Base
     limit(count)
   end
 
-  def self.safe_find_or_create_by!(*args)
-    safe_find_or_create_by(*args).tap do |record|
+  def self.safe_find_or_create_by!(*args, &block)
+    safe_find_or_create_by(*args, &block).tap do |record|
       record.validate! unless record.persisted?
     end
   end
 
-  def self.safe_find_or_create_by(*args)
+  def self.safe_find_or_create_by(*args, &block)
     safe_ensure_unique(retries: 1) do
-      find_or_create_by(*args)
+      find_or_create_by(*args, &block)
     end
   end
 

app/models/design_management/design.rb

@@ -9,6 +9,7 @@ module DesignManagement
     include Referable
     include Mentionable
     include WhereComposite
+    include RelativePositioning
 
     belongs_to :project, inverse_of: :designs
     belongs_to :issue
@@ -75,7 +76,19 @@ module DesignManagement
       join = designs.join(actions)
         .on(actions[:design_id].eq(designs[:id]))
 
-      joins(join.join_sources).where(actions[:event].not_eq(deletion)).order(:id)
+      joins(join.join_sources).where(actions[:event].not_eq(deletion))
+    end
+
+    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)
+        # 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)
+      else
+        order(:id)
+      end
     end
 
     scope :with_filename, -> (filenames) { where(filename: filenames) }
@@ -87,6 +100,14 @@ module DesignManagement
     # A design is current if the most recent event is not a deletion
     scope :current, -> { visible_at_version(nil) }
 
+    def self.relative_positioning_query_base(design)
+      on_issue(design.issue_id)
+    end
+
+    def self.relative_positioning_parent_column
+      :issue_id
+    end
+
     def status
       if new_design?
         :new

app/models/design_management/design_collection.rb

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

changelogs/unreleased/221167-model-changes.yml

+---
+title: Add relative positioning on designs
+merge_request: 37835
+author:
+type: changed

config/feature_flags/development/reorder_designs.yml

+---
+name: reorder_designs
+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

db/migrate/20200722091435_add_relative_position_to_design_management_designs.rb

+# frozen_string_literal: true
+
+class AddRelativePositionToDesignManagementDesigns < ActiveRecord::Migration[6.0]
+  DOWNTIME = false
+
+  def change
+    add_column :design_management_designs, :relative_position, :integer
+  end
+end

db/migrate/20200728174137_add_index_on_design_management_designs_issue_id_and_relative_position_and_id.rb

+# frozen_string_literal: true
+
+class AddIndexOnDesignManagementDesignsIssueIdAndRelativePositionAndId < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+  INDEX_NAME = 'index_design_management_designs_issue_id_relative_position_id'
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_index :design_management_designs, [:issue_id, :relative_position, :id], name: INDEX_NAME
+  end
+
+  def down
+    remove_concurrent_index :design_management_designs, [:issue_id, :relative_position, :id], name: INDEX_NAME
+  end
+end

db/schema_migrations/20200722091435

+d4e389b1469b968b703432de9ece6c362e45ec4ba3ad20d1f6c6418253969379
\ No newline at end of file

db/schema_migrations/20200728174137

+a16e7fdcc62f39af3038317cb39ffb4c35f41ae45f5de429f18837309739110b
\ No newline at end of file

db/structure.sql

@@ -11203,7 +11203,8 @@ CREATE TABLE public.design_management_designs (
     id bigint NOT NULL,
     project_id integer NOT NULL,
     issue_id integer,
-    filename character varying NOT NULL
+    filename character varying NOT NULL,
+    relative_position integer
 );
 
 CREATE SEQUENCE public.design_management_designs_id_seq
@@ -19394,6 +19395,8 @@ CREATE INDEX index_description_versions_on_issue_id ON public.description_versio
 
 CREATE INDEX index_description_versions_on_merge_request_id ON public.description_versions USING btree (merge_request_id) WHERE (merge_request_id IS NOT NULL);
 
+CREATE INDEX index_design_management_designs_issue_id_relative_position_id ON public.design_management_designs USING btree (issue_id, relative_position, id);
+
 CREATE UNIQUE INDEX index_design_management_designs_on_issue_id_and_filename ON public.design_management_designs USING btree (issue_id, filename);
 
 CREATE INDEX index_design_management_designs_on_project_id ON public.design_management_designs USING btree (project_id);

spec/finders/design_management/designs_finder_spec.rb

@@ -8,9 +8,9 @@ RSpec.describe DesignManagement::DesignsFinder do
   let_it_be(:user) { create(:user) }
   let_it_be(:project) { create(:project, :private) }
   let_it_be(:issue) { create(:issue, project: project) }
-  let_it_be(:design1) { create(:design, :with_file, issue: issue, versions_count: 1) }
-  let_it_be(:design2) { create(:design, :with_file, issue: issue, versions_count: 1) }
-  let_it_be(:design3) { create(:design, :with_file, issue: issue, versions_count: 1) }
+  let_it_be(:design1) { create(:design, :with_file, issue: issue, versions_count: 1, relative_position: 3) }
+  let_it_be(:design2) { create(:design, :with_file, issue: issue, versions_count: 1, relative_position: 2) }
+  let_it_be(:design3) { create(:design, :with_file, issue: issue, versions_count: 1, relative_position: 1) }
   let(:params) { {} }
 
   subject(:designs) { described_class.new(issue, user, params).execute }
@@ -38,8 +38,28 @@ RSpec.describe DesignManagement::DesignsFinder do
           enable_design_management
         end
 
-        it 'returns the designs' do
-          is_expected.to contain_exactly(design1, design2, design3)
+        it 'returns the designs sorted by their relative position' do
+          is_expected.to eq([design3, design2, design1])
+        end
+
+        context 'when the :reorder_designs feature is enabled for the project' do
+          before do
+            stub_feature_flags(reorder_designs: project)
+          end
+
+          it 'returns the designs sorted by their relative position' do
+            is_expected.to eq([design3, design2, design1])
+          end
+        end
+
+        context 'when the :reorder_designs feature is disabled' do
+          before do
+            stub_feature_flags(reorder_designs: false)
+          end
+
+          it 'returns the designs sorted by ID' do
+            is_expected.to eq([design1, design2, design3])
+          end
         end
 
         context 'when argument is the ids of designs' do

spec/lib/gitlab/import_export/safe_model_attributes.yml

@@ -768,6 +768,7 @@ DesignManagement::Design:
 - id
 - project_id
 - filename
+- relative_position
 DesignManagement::Action:
 - id
 - event

spec/models/application_record_spec.rb

@@ -38,6 +38,14 @@ RSpec.describe ApplicationRecord do
       expect { Suggestion.safe_find_or_create_by(build(:suggestion).attributes) }
         .to change { Suggestion.count }.by(1)
     end
+
+    it 'passes a block to find_or_create_by' do
+      attributes = build(:suggestion).attributes
+
+      expect do |block|
+        Suggestion.safe_find_or_create_by(attributes, &block)
+      end.to yield_with_args(an_object_having_attributes(attributes))
+    end
   end
 
   describe '.safe_find_or_create_by!' do
@@ -51,6 +59,14 @@ RSpec.describe ApplicationRecord do
     it 'raises a validation error if the record was not persisted' do
       expect { Suggestion.find_or_create_by!(note: nil) }.to raise_error(ActiveRecord::RecordInvalid)
     end
+
+    it 'passes a block to find_or_create_by' do
+      attributes = build(:suggestion).attributes
+
+      expect do |block|
+        Suggestion.safe_find_or_create_by!(attributes, &block)
+      end.to yield_with_args(an_object_having_attributes(attributes))
+    end
   end
 
   describe '.underscore' do

spec/models/design_management/design_collection_spec.rb

@@ -34,6 +34,13 @@ RSpec.describe DesignManagement::DesignCollection do
         collection.find_or_create_design!(filename: 'world.jpg')
       end.not_to exceed_query_limit(1)
     end
+
+    it 'inserts the design after any existing designs' do
+      design1 = collection.find_or_create_design!(filename: 'design1.jpg')
+      design2 = collection.find_or_create_design!(filename: 'design2.jpg')
+
+      expect(design1.relative_position).to be < design2.relative_position
+    end
   end
 
   describe "#versions" do

spec/models/design_management/design_spec.rb

@@ -11,6 +11,11 @@ RSpec.describe DesignManagement::Design do
   let_it_be(:design3) { create(:design, :with_versions, issue: issue, versions_count: 1) }
   let_it_be(:deleted_design) { create(:design, :with_versions, deleted: true) }
 
+  it_behaves_like 'a class that supports relative positioning' do
+    let(:factory) { :design }
+    let(:default_params) { { issue: issue } }
+  end
+
   describe 'relations' do
     it { is_expected.to belong_to(:project) }
     it { is_expected.to belong_to(:issue) }
@@ -147,6 +152,39 @@ RSpec.describe DesignManagement::Design do
       end
     end
 
+    describe '.ordered' do
+      before do
+        design1.update!(relative_position: 2)
+        design2.update!(relative_position: 1)
+        design3.update!(relative_position: nil)
+        deleted_design.update!(relative_position: nil)
+      end
+
+      it 'sorts by relative position and ID in ascending order' do
+        expect(described_class.ordered(issue.project)).to eq([design2, design1, design3, deleted_design])
+      end
+
+      context 'when the :reorder_designs feature is enabled for the project' do
+        before do
+          stub_feature_flags(reorder_designs: issue.project)
+        end
+
+        it 'sorts by relative position and ID in ascending order' do
+          expect(described_class.ordered(issue.project)).to eq([design2, design1, design3, deleted_design])
+        end
+      end
+
+      context 'when the :reorder_designs feature is disabled' do
+        before do
+          stub_feature_flags(reorder_designs: false)
+        end
+
+        it 'sorts by ID in ascending order' do
+          expect(described_class.ordered(issue.project)).to eq([design1, design2, design3, deleted_design])
+        end
+      end
+    end
+
     describe '.with_filename' do
       it 'returns correct design when passed a single filename' do
         expect(described_class.with_filename(design1.filename)).to eq([design1])