Merge branch 'ajk-reorder-designs-reset-collection' into 'master'

Apply changes when moving designs

See merge request gitlab-org/gitlab!39084

app/finders/design_management/designs_finder.rb

@@ -37,7 +37,8 @@ module DesignManagement
       issue.designs
     end
 
-    # Returns all designs that existed at a particular design version
+    # Returns all designs that existed at a particular design version,
+    # where `nil` means `at-current-version`.
     def by_visible_at_version(items)
       items.visible_at_version(params[:visible_at_version])
     end

app/graphql/resolvers/design_management/designs_resolver.rb

@@ -27,19 +27,20 @@ module Resolvers
           current_user,
           ids: design_ids(ids),
           filenames: filenames,
-          visible_at_version: version(at_version),
-          order: :id
+          visible_at_version: version(at_version)
         ).execute
       end
 
       private
 
       def version(at_version)
-        GitlabSchema.object_from_id(at_version)&.sync if at_version
+        return unless at_version
+
+        GitlabSchema.object_from_id(at_version, expected_type: ::DesignManagement::Version)&.sync
       end
 
       def design_ids(ids)
-        ids&.map { |id| GlobalID.parse(id).model_id }
+        ids&.map { |id| GlobalID.parse(id, expected_type: ::DesignManagement::Design).model_id }
       end
 
       def issue

app/services/design_management/move_designs_service.rb

@@ -20,7 +20,9 @@ module DesignManagement
       return error(:not_adjacent) if any_in_gap?
       return error(:not_same_issue) unless all_same_issue?
 
+      move_nulls_to_end
       current_design.move_between(previous_design, next_design)
+      current_design.save!
       success
     end
 
@@ -36,6 +38,12 @@ module DesignManagement
 
     delegate :issue, :project, to: :current_design
 
+    def move_nulls_to_end
+      current_design.class.move_nulls_to_end(issue.designs)
+      next_design.reset if next_design && next_design.relative_position.nil?
+      previous_design.reset if previous_design && previous_design.relative_position.nil?
+    end
+
     def neighbors
       [previous_design, next_design].compact
     end
@@ -45,7 +53,7 @@ module DesignManagement
     end
 
     def any_in_gap?
-      return false unless previous_design && next_design
+      return false unless previous_design&.relative_position && next_design&.relative_position
 
       !previous_design.immediately_before?(next_design)
     end

spec/requests/api/graphql/mutations/design_management/move_spec.rb

+# frozen_string_literal: true
+require "spec_helper"
+
+RSpec.describe "moving designs" do
+  include GraphqlHelpers
+  include DesignManagementTestHelpers
+
+  let_it_be(:issue) { create(:issue) }
+  let_it_be(:designs) { create_list(:design, 3, :with_versions, :with_relative_position, issue: issue) }
+  let_it_be(:developer) { create(:user, developer_projects: [issue.project]) }
+
+  let(:user) { developer }
+
+  let(:current_design) { designs.first }
+  let(:previous_design) { designs.second }
+  let(:next_design) { designs.third }
+  let(:mutation_name) { :design_management_move }
+
+  let(:mutation) do
+    input = {
+      id: current_design.to_global_id.to_s,
+      previous: previous_design&.to_global_id&.to_s,
+      next: next_design&.to_global_id&.to_s
+    }.compact
+
+    graphql_mutation(mutation_name, input, <<~FIELDS)
+    errors
+    designCollection {
+      designs {
+        nodes {
+          filename
+        }
+      }
+    }
+    FIELDS
+  end
+
+  let(:move_designs) { post_graphql_mutation(mutation, current_user: user) }
+  let(:mutation_response) { graphql_mutation_response(mutation_name) }
+
+  before do
+    enable_design_management
+    designs.each(&:reset)
+    issue.reset
+  end
+
+  shared_examples 'a successful move' do
+    it 'does not error, and reports the current order' do
+      move_designs
+
+      expect(graphql_errors).not_to be_present
+
+      expect(mutation_response).to eq(
+        'errors' => [],
+        'designCollection' => {
+          'designs' => {
+            'nodes' => new_order.map { |d| { 'filename' => d.filename } }
+          }
+        }
+      )
+    end
+  end
+
+  context 'the user is not allowed to move designs' do
+    let(:user) { create(:user) }
+
+    it 'returns an error' do
+      move_designs
+
+      expect(graphql_errors).to be_present
+    end
+  end
+
+  context 'the neighbors do not have positions' do
+    let!(:previous_design) { create(:design, :with_versions, issue: issue) }
+    let!(:next_design) { create(:design, :with_versions, issue: issue) }
+
+    let(:new_order) do
+      [
+        designs.second,
+        designs.third,
+        previous_design, current_design, next_design
+      ]
+    end
+
+    it_behaves_like 'a successful move'
+
+    it 'maintains the correct order in the presence of other unpositioned designs' do
+      other_design = create(:design, :with_versions, issue: issue)
+
+      move_designs
+      moved_designs = mutation_response.dig('designCollection', 'designs', 'nodes')
+
+      expect(moved_designs.map { |d| d['filename'] })
+        .to eq([*new_order.map(&:filename), other_design.filename])
+    end
+  end
+
+  context 'moving a design between two others' do
+    let(:new_order) { [designs.second, designs.first, designs.third] }
+
+    it_behaves_like 'a successful move'
+  end
+
+  context 'moving a design to the start' do
+    let(:current_design) { designs.last }
+    let(:next_design) { designs.first }
+    let(:previous_design) { nil }
+    let(:new_order) { [designs.last, designs.first, designs.second] }
+
+    it_behaves_like 'a successful move'
+  end
+
+  context 'moving a design to the end' do
+    let(:current_design) { designs.first }
+    let(:next_design) { nil }
+    let(:previous_design) { designs.last }
+    let(:new_order) { [designs.second, designs.third, designs.first] }
+
+    it_behaves_like 'a successful move'
+  end
+end