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

Add move designs service

See merge request gitlab-org/gitlab!37603

app/graphql/gitlab_schema.rb

@@ -77,6 +77,8 @@ class GitlabSchema < GraphQL::Schema
     #   will be called.
     # * All other classes will use `GlobalID#find`
     def find_by_gid(gid)
+      return unless gid
+
       if gid.model_class < ApplicationRecord
         Gitlab::Graphql::Loaders::BatchModelLoader.new(gid.model_class, gid.model_id).find
       elsif gid.model_class.respond_to?(:lazy_find)

app/graphql/mutations/design_management/move.rb

+# frozen_string_literal: true
+
+module Mutations
+  module DesignManagement
+    class Move < ::Mutations::BaseMutation
+      graphql_name "DesignManagementMove"
+
+      DesignID = ::Types::GlobalIDType[::DesignManagement::Design]
+
+      argument :id, DesignID, required: true, as: :current_design,
+               description: "ID of the design to move"
+
+      argument :previous, DesignID, required: false, as: :previous_design,
+               description: "ID of the immediately preceding design"
+
+      argument :next, DesignID, required: false, as: :next_design,
+               description: "ID of the immediately following design"
+
+      field :design_collection, Types::DesignManagement::DesignCollectionType,
+            null: true,
+            description: "The current state of the collection"
+
+      def ready(*)
+        raise ::Gitlab::Graphql::Errors::ResourceNotAvailable unless ::Feature.enabled?(:reorder_designs)
+      end
+
+      def resolve(**args)
+        service = ::DesignManagement::MoveDesignsService.new(current_user, parameters(args))
+
+        { design_collection: service.collection, errors: service.execute.errors }
+      end
+
+      private
+
+      def parameters(**args)
+        args.transform_values { |id| GitlabSchema.find_by_gid(id) }.transform_values(&:sync).tap do |hash|
+          hash.each { |k, design| not_found(args[k]) unless current_user.can?(:read_design, design) }
+        end
+      end
+
+      def not_found(gid)
+        raise Gitlab::Graphql::Errors::ResourceNotAvailable, "Resource not available: #{gid}"
+      end
+    end
+  end
+end

app/graphql/types/mutation_type.rb

@@ -57,6 +57,7 @@ module Types
     mount_mutation Mutations::JiraImport::ImportUsers
     mount_mutation Mutations::DesignManagement::Upload, calls_gitaly: true
     mount_mutation Mutations::DesignManagement::Delete, calls_gitaly: true
+    mount_mutation Mutations::DesignManagement::Move
     mount_mutation Mutations::ContainerExpirationPolicies::Update
   end
 end

app/models/design_management/design.rb

@@ -217,6 +217,17 @@ module DesignManagement
       project
     end
 
+    def immediately_before?(next_design)
+      return false if next_design.relative_position <= relative_position
+
+      interloper = self.class.on_issue(issue).where(
+        "relative_position <@ int4range(?, ?, '()')",
+        *[self, next_design].map(&:relative_position)
+      )
+
+      !interloper.exists?
+    end
+
     private
 
     def head_version

app/models/design_management/design_collection.rb

@@ -10,6 +10,10 @@ module DesignManagement
       @issue = issue
     end
 
+    def ==(other)
+      other.is_a?(self.class) && issue == other.issue
+    end
+
     def find_or_create_design!(filename:)
       designs.find { |design| design.filename == filename } ||
         designs.safe_find_or_create_by!(project: project, filename: filename) do |design|

app/services/design_management/move_designs_service.rb

+# frozen_string_literal: true
+
+module DesignManagement
+  class MoveDesignsService < DesignService
+    # @param user [User] The current user
+    # @param [Hash] params
+    # @option params [DesignManagement::Design] :current_design
+    # @option params [DesignManagement::Design] :previous_design (nil)
+    # @option params [DesignManagement::Design] :next_design (nil)
+    def initialize(user, params)
+      super(nil, user, params.merge(issue: nil))
+    end
+
+    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 current_user.can?(:move_design, current_design)
+      return error(:no_neighbors) unless neighbors.present?
+      return error(:not_distinct) unless all_distinct?
+      return error(:not_adjacent) if any_in_gap?
+      return error(:not_same_issue) unless all_same_issue?
+
+      current_design.move_between(previous_design, next_design)
+      success
+    end
+
+    def error(message)
+      ServiceResponse.error(message: message)
+    end
+
+    def success
+      ServiceResponse.success
+    end
+
+    private
+
+    delegate :issue, :project, to: :current_design
+
+    def neighbors
+      [previous_design, next_design].compact
+    end
+
+    def all_distinct?
+      ids.uniq.size == ids.size
+    end
+
+    def any_in_gap?
+      return false unless previous_design && next_design
+
+      !previous_design.immediately_before?(next_design)
+    end
+
+    def all_same_issue?
+      issue.designs.id_in(ids).count == ids.size
+    end
+
+    def ids
+      @ids ||= [current_design, *neighbors].map(&:id)
+    end
+
+    def current_design
+      params[:current_design]
+    end
+
+    def previous_design
+      params[:previous_design]
+    end
+
+    def next_design
+      params[:next_design]
+    end
+  end
+end

changelogs/unreleased/ajk-reorder-designs.yml

+---
+title: Add GraphQL mutation to re-order designs
+merge_request: 37603
+author:
+type: added

doc/api/graphql/reference/gitlab_schema.graphql

@@ -3201,6 +3201,56 @@ type DesignManagementDeletePayload {
   version: DesignVersion
 }
 
+"""
+Identifier of DesignManagement::Design
+"""
+scalar DesignManagementDesignID
+
+"""
+Autogenerated input type of DesignManagementMove
+"""
+input DesignManagementMoveInput {
+  """
+  A unique identifier for the client performing the mutation.
+  """
+  clientMutationId: String
+
+  """
+  ID of the design to move
+  """
+  id: DesignManagementDesignID!
+
+  """
+  ID of the immediately following design
+  """
+  next: DesignManagementDesignID
+
+  """
+  ID of the immediately preceding design
+  """
+  previous: DesignManagementDesignID
+}
+
+"""
+Autogenerated return type of DesignManagementMove
+"""
+type DesignManagementMovePayload {
+  """
+  A unique identifier for the client performing the mutation.
+  """
+  clientMutationId: String
+
+  """
+  The current state of the collection
+  """
+  designCollection: DesignCollection
+
+  """
+  Errors encountered during execution of the mutation.
+  """
+  errors: [String!]!
+}
+
 """
 Autogenerated input type of DesignManagementUpload
 """
@@ -8908,6 +8958,7 @@ type Mutation {
   dastSiteProfileDelete(input: DastSiteProfileDeleteInput!): DastSiteProfileDeletePayload
   deleteAnnotation(input: DeleteAnnotationInput!): DeleteAnnotationPayload
   designManagementDelete(input: DesignManagementDeleteInput!): DesignManagementDeletePayload
+  designManagementMove(input: DesignManagementMoveInput!): DesignManagementMovePayload
   designManagementUpload(input: DesignManagementUploadInput!): DesignManagementUploadPayload
   destroyNote(input: DestroyNoteInput!): DestroyNotePayload
   destroySnippet(input: DestroySnippetInput!): DestroySnippetPayload

doc/api/graphql/reference/gitlab_schema.json

@@ -8799,6 +8799,138 @@
           "enumValues": null,
           "possibleTypes": null
         },
+        {
+          "kind": "SCALAR",
+          "name": "DesignManagementDesignID",
+          "description": "Identifier of DesignManagement::Design",
+          "fields": null,
+          "inputFields": null,
+          "interfaces": null,
+          "enumValues": null,
+          "possibleTypes": null
+        },
+        {
+          "kind": "INPUT_OBJECT",
+          "name": "DesignManagementMoveInput",
+          "description": "Autogenerated input type of DesignManagementMove",
+          "fields": null,
+          "inputFields": [
+            {
+              "name": "id",
+              "description": "ID of the design to move",
+              "type": {
+                "kind": "NON_NULL",
+                "name": null,
+                "ofType": {
+                  "kind": "SCALAR",
+                  "name": "DesignManagementDesignID",
+                  "ofType": null
+                }
+              },
+              "defaultValue": null
+            },
+            {
+              "name": "previous",
+              "description": "ID of the immediately preceding design",
+              "type": {
+                "kind": "SCALAR",
+                "name": "DesignManagementDesignID",
+                "ofType": null
+              },
+              "defaultValue": null
+            },
+            {
+              "name": "next",
+              "description": "ID of the immediately following design",
+              "type": {
+                "kind": "SCALAR",
+                "name": "DesignManagementDesignID",
+                "ofType": null
+              },
+              "defaultValue": null
+            },
+            {
+              "name": "clientMutationId",
+              "description": "A unique identifier for the client performing the mutation.",
+              "type": {
+                "kind": "SCALAR",
+                "name": "String",
+                "ofType": null
+              },
+              "defaultValue": null
+            }
+          ],
+          "interfaces": null,
+          "enumValues": null,
+          "possibleTypes": null
+        },
+        {
+          "kind": "OBJECT",
+          "name": "DesignManagementMovePayload",
+          "description": "Autogenerated return type of DesignManagementMove",
+          "fields": [
+            {
+              "name": "clientMutationId",
+              "description": "A unique identifier for the client performing the mutation.",
+              "args": [
+
+              ],
+              "type": {
+                "kind": "SCALAR",
+                "name": "String",
+                "ofType": null
+              },
+              "isDeprecated": false,
+              "deprecationReason": null
+            },
+            {
+              "name": "designCollection",
+              "description": "The current state of the collection",
+              "args": [
+
+              ],
+              "type": {
+                "kind": "OBJECT",
+                "name": "DesignCollection",
+                "ofType": null
+              },
+              "isDeprecated": false,
+              "deprecationReason": null
+            },
+            {
+              "name": "errors",
+              "description": "Errors encountered during execution of the mutation.",
+              "args": [
+
+              ],
+              "type": {
+                "kind": "NON_NULL",
+                "name": null,
+                "ofType": {
+                  "kind": "LIST",
+                  "name": null,
+                  "ofType": {
+                    "kind": "NON_NULL",
+                    "name": null,
+                    "ofType": {
+                      "kind": "SCALAR",
+                      "name": "String",
+                      "ofType": null
+                    }
+                  }
+                }
+              },
+              "isDeprecated": false,
+              "deprecationReason": null
+            }
+          ],
+          "inputFields": null,
+          "interfaces": [
+
+          ],
+          "enumValues": null,
+          "possibleTypes": null
+        },
         {
           "kind": "INPUT_OBJECT",
           "name": "DesignManagementUploadInput",
@@ -25665,6 +25797,33 @@
               "isDeprecated": false,
               "deprecationReason": null
             },
+            {
+              "name": "designManagementMove",
+              "description": null,
+              "args": [
+                {
+                  "name": "input",
+                  "description": null,
+                  "type": {
+                    "kind": "NON_NULL",
+                    "name": null,
+                    "ofType": {
+                      "kind": "INPUT_OBJECT",
+                      "name": "DesignManagementMoveInput",
+                      "ofType": null
+                    }
+                  },
+                  "defaultValue": null
+                }
+              ],
+              "type": {
+                "kind": "OBJECT",
+                "name": "DesignManagementMovePayload",
+                "ofType": null
+              },
+              "isDeprecated": false,
+              "deprecationReason": null
+            },
             {
               "name": "designManagementUpload",
               "description": null,

doc/api/graphql/reference/index.md

@@ -547,6 +547,16 @@ Autogenerated return type of DesignManagementDelete
 | `errors` | String! => Array | Errors encountered during execution of the mutation. |
 | `version` | DesignVersion | The new version in which the designs are deleted |
 
+## DesignManagementMovePayload
+
+Autogenerated return type of DesignManagementMove
+
+| Name  | Type  | Description |
+| ---   |  ---- | ----------  |
+| `clientMutationId` | String | A unique identifier for the client performing the mutation. |
+| `designCollection` | DesignCollection | The current state of the collection |
+| `errors` | String! => Array | Errors encountered during execution of the mutation. |
+
 ## DesignManagementUploadPayload
 
 Autogenerated return type of DesignManagementUpload

spec/factories/design_management/designs.rb

@@ -22,6 +22,10 @@ FactoryBot.define do
       imported { true }
     end
 
+    trait :with_relative_position do
+      sequence(:relative_position) { |n| n * 1000 }
+    end
+
     create_versions = ->(design, evaluator, commit_version) do
       unless evaluator.versions_count == 0
         project = design.project

spec/graphql/mutations/design_management/move_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Mutations::DesignManagement::Move do
+  include DesignManagementTestHelpers
+
+  let_it_be(:issue) { create(:issue) }
+  let_it_be(:designs) { create_list(:design, 3, issue: issue) }
+  let_it_be(:developer) { create(:user, developer_projects: [issue.project]) }
+
+  let(:user) { developer }
+
+  let(:mutation) { described_class.new(object: nil, context: { current_user: user }, field: nil) }
+
+  let(:current_design) { designs.first }
+  let(:previous_design) { designs.second }
+  let(:next_design) { designs.third }
+
+  before do
+    enable_design_management
+  end
+
+  describe "#resolve" do
+    subject(:resolve) do
+      args = {
+        current_design: current_design.to_global_id,
+        previous_design: previous_design&.to_global_id,
+        next_design: next_design&.to_global_id
+      }.compact
+
+      mutation.resolve(args)
+    end
+
+    shared_examples "resource not available" do
+      it "raises an error" do
+        expect { resolve }.to raise_error(Gitlab::Graphql::Errors::ResourceNotAvailable)
+      end
+    end
+
+    context 'when the feature is not available' do
+      before do
+        enable_design_management(false)
+      end
+
+      it_behaves_like 'resource not available'
+    end
+
+    %i[current_design previous_design next_design].each do |binding|
+      context "When #{binding} cannot be found" do
+        let(binding) { build_stubbed(:design) }
+
+        it_behaves_like 'resource not available'
+      end
+    end
+
+    context 'the service runs' do
+      before do
+        expect_next_instance_of(::DesignManagement::MoveDesignsService) do |service|
+          expect(service).to receive(:execute).and_return(service_result)
+        end
+      end
+
+      context 'raising an error' do
+        let(:service_result) { ServiceResponse.error(message: 'bang!') }
+
+        it 'reports the service-level error' do
+          expect(resolve).to include(errors: ['bang!'], design_collection: eq(issue.design_collection))
+        end
+      end
+
+      context 'successfully' do
+        let(:service_result) { ServiceResponse.success }
+
+        it 'reports the service-level error' do
+          expect(resolve).to include(errors: be_empty, design_collection: eq(issue.design_collection))
+        end
+      end
+    end
+  end
+end

spec/models/design_management/design_spec.rb

@@ -603,4 +603,25 @@ RSpec.describe DesignManagement::Design do
       end
     end
   end
+
+  describe '#immediately_before' do
+    let_it_be(:design) { create(:design, issue: issue, relative_position: 100) }
+    let_it_be(:next_design) { create(:design, issue: issue, relative_position: 200) }
+
+    it 'is true when there is no element positioned between this item and the next' do
+      expect(design.immediately_before?(next_design)).to be true
+    end
+
+    it 'is false when there is an element positioned between this item and the next' do
+      create(:design, issue: issue, relative_position: 150)
+
+      expect(design.immediately_before?(next_design)).to be false
+    end
+
+    it 'is false when the next design is to the left of this design' do
+      further_left = create(:design, issue: issue, relative_position: 50)
+
+      expect(design.immediately_before?(further_left)).to be false
+    end
+  end
 end

spec/services/design_management/move_designs_service_spec.rb

+# frozen_string_literal: true
+require 'spec_helper'
+
+RSpec.describe DesignManagement::MoveDesignsService do
+  include DesignManagementTestHelpers
+
+  let_it_be(:issue) { create(:issue) }
+  let_it_be(:developer) { create(:user, developer_projects: [issue.project]) }
+  let_it_be(:designs) { create_list(:design, 3, :with_relative_position, issue: issue) }
+
+  let(:project) { issue.project }
+
+  let(:service) { described_class.new(current_user, params) }
+
+  let(:params) do
+    {
+      current_design: current_design,
+      previous_design: previous_design,
+      next_design: next_design
+    }
+  end
+
+  let(:current_user) { developer }
+  let(:current_design) { nil }
+  let(:previous_design) { nil }
+  let(:next_design) { nil }
+
+  before do
+    enable_design_management
+  end
+
+  describe '#execute' do
+    subject { service.execute }
+
+    context 'the feature is unavailable' do
+      let(:current_design) { designs.first }
+      let(:previous_design) { designs.second }
+      let(:next_design) { designs.third }
+
+      before do
+        stub_feature_flags(reorder_designs: false)
+      end
+
+      it 'raises cannot_move' do
+        expect(subject).to be_error.and(have_attributes(message: :cannot_move))
+      end
+
+      context 'but it is available on the current project' do
+        before do
+          stub_feature_flags(reorder_designs: issue.project)
+        end
+
+        it 'is successful' do
+          expect(subject).to be_success
+        end
+      end
+    end
+
+    context 'the user cannot move designs' do
+      let(:current_design) { designs.first }
+      let(:current_user) { build_stubbed(:user) }
+
+      it 'raises cannot_move' do
+        expect(subject).to be_error.and(have_attributes(message: :cannot_move))
+      end
+    end
+
+    context 'the designs are not distinct' do
+      let(:current_design) { designs.first }
+      let(:previous_design) { designs.first }
+
+      it 'raises not_distinct' do
+        expect(subject).to be_error.and(have_attributes(message: :not_distinct))
+      end
+    end
+
+    context 'the designs are not on the same issue' do
+      let(:current_design) { designs.first }
+      let(:previous_design) { create(:design) }
+
+      it 'raises not_same_issue' do
+        expect(subject).to be_error.and(have_attributes(message: :not_same_issue))
+      end
+    end
+
+    context 'no focus is passed' do
+      let(:previous_design) { designs.second }
+      let(:next_design) { designs.third }
+
+      it 'raises no_focus' do
+        expect(subject).to be_error.and(have_attributes(message: :no_focus))
+      end
+    end
+
+    context 'no neighbours are passed' do
+      let(:current_design) { designs.first }
+
+      it 'raises no_neighbors' do
+        expect(subject).to be_error.and(have_attributes(message: :no_neighbors))
+      end
+    end
+
+    context 'the designs are not adjacent' do
+      let(:current_design) { designs.first }
+      let(:previous_design) { designs.second }
+      let(:next_design) { designs.third }
+
+      it 'raises not_adjacent' do
+        create(:design, issue: issue, relative_position: next_design.relative_position - 1)
+
+        expect(subject).to be_error.and(have_attributes(message: :not_adjacent))
+      end
+    end
+
+    context 'moving a design with neighbours' do
+      let(:current_design) { designs.first }
+      let(:previous_design) { designs.second }
+      let(:next_design) { designs.third }
+
+      it 'calls move_between and is successful' do
+        expect(current_design).to receive(:move_between).with(previous_design, next_design)
+        expect(subject).to be_success
+      end
+    end
+  end
+end