Merge branch '207334-reposition_note_graphql' into 'master'

Add updateImageDiffNote mutation to reposition notes on images

See merge request gitlab-org/gitlab!45958

app/graphql/mutations/notes/reposition_image_diff_note.rb

+# frozen_string_literal: true
+
+module Mutations
+  module Notes
+    # This mutation differs from the update note mutations as it checks the
+    # `reposition_note` permission, and doesn't allow updating a note's `body`.
+    class RepositionImageDiffNote < Mutations::Notes::Base
+      graphql_name 'RepositionImageDiffNote'
+
+      description 'Repositions a DiffNote on an image (a `Note` where the `position.positionType` is `"image"`)'
+
+      authorize :reposition_note
+
+      argument :id,
+                Types::GlobalIDType[DiffNote],
+                loads: Types::Notes::NoteType,
+                as: :note,
+                required: true,
+                description: 'The global id of the DiffNote to update'
+
+      argument :position,
+                Types::Notes::UpdateDiffImagePositionInputType,
+                required: true,
+                description: copy_field_description(Types::Notes::NoteType, :position)
+
+      def resolve(note:, position:)
+        authorize!(note)
+
+        pre_update_checks!(note, position)
+
+        updated_note = ::Notes::UpdateService.new(
+          note.project,
+          current_user,
+          note_params(note.position, position)
+        ).execute(note)
+
+        {
+          note: updated_note.reset,
+          errors: errors_on_object(updated_note)
+        }
+      end
+
+      private
+
+      # An ImageDiffNote does not exist as a class itself, but is instead
+      # just a `DiffNote` with a particular kind of `Gitlab::Diff::Position`.
+      # In addition to accepting a `DiffNote` Global ID we also need to
+      # perform this check.
+      def pre_update_checks!(note, position)
+        unless note.position&.on_image?
+          raise Gitlab::Graphql::Errors::ResourceNotAvailable,
+                'Resource is not an ImageDiffNote'
+        end
+      end
+
+      def note_params(old_position, new_position)
+        position = old_position.to_h.merge(new_position)
+
+        {
+          position: Gitlab::Diff::Position.new(position)
+        }
+      end
+    end
+  end
+end

app/graphql/mutations/notes/update/image_diff_note.rb

@@ -47,12 +47,11 @@ module Mutations
         end
 
         def position_params(note, args)
-          new_position = args[:position]&.to_h&.compact
-          return unless new_position
+          return unless args[:position]
 
           original_position = note.position.to_h
 
-          Gitlab::Diff::Position.new(original_position.merge(new_position))
+          Gitlab::Diff::Position.new(original_position.merge(args[:position]))
         end
       end
     end

app/graphql/types/mutation_type.rb

@@ -61,6 +61,7 @@ module Types
                    description: 'Updates a DiffNote on an image (a `Note` where the `position.positionType` is `"image"`). ' \
                                 'If the body of the Note contains only quick actions, the Note will be ' \
                                 'destroyed during the update, and no Note will be returned'
+    mount_mutation Mutations::Notes::RepositionImageDiffNote
     mount_mutation Mutations::Notes::Destroy
     mount_mutation Mutations::Terraform::State::Delete
     mount_mutation Mutations::Terraform::State::Lock

app/graphql/types/notes/update_diff_image_position_input_type.rb

@@ -23,6 +23,14 @@ module Types
       argument :height, GraphQL::INT_TYPE,
                required: false,
                description: copy_field_description(Types::Notes::DiffPositionType, :height)
+
+      def prepare
+        to_h.compact.tap do |properties|
+          if properties.empty?
+            raise GraphQL::ExecutionError, "At least one property of `#{self.class.graphql_name}` must be set"
+          end
+        end
+      end
     end
     # rubocop: enable Graphql/AuthorizeTypes
   end

app/graphql/types/permission_types/note.rb

@@ -5,7 +5,7 @@ module Types
     class Note < BasePermissionType
       graphql_name 'NotePermissions'
 
-      abilities :read_note, :create_note, :admin_note, :resolve_note, :award_emoji
+      abilities :read_note, :create_note, :admin_note, :resolve_note, :reposition_note, :award_emoji
     end
   end
 end

changelogs/unreleased/207334-reposition_note_graphql.yml

+---
+title: Add repositionImageDiffNote GraphQL mutation to specifically reposition DiffNotes
+  on images
+merge_request: 45958
+author:
+type: added

doc/api/graphql/reference/gitlab_schema.graphql

@@ -6143,6 +6143,11 @@ input DiffImagePositionInput {
   y: Int!
 }
 
+"""
+Identifier of DiffNote
+"""
+scalar DiffNoteID
+
 input DiffPathsInput {
   """
   The path of the file on the head sha
@@ -13297,6 +13302,11 @@ type Mutation {
   prometheusIntegrationUpdate(input: PrometheusIntegrationUpdateInput!): PrometheusIntegrationUpdatePayload
   removeAwardEmoji(input: RemoveAwardEmojiInput!): RemoveAwardEmojiPayload @deprecated(reason: "Use awardEmojiRemove. Deprecated in 13.2")
   removeProjectFromSecurityDashboard(input: RemoveProjectFromSecurityDashboardInput!): RemoveProjectFromSecurityDashboardPayload
+
+  """
+  Repositions a DiffNote on an image (a `Note` where the `position.positionType` is `"image"`)
+  """
+  repositionImageDiffNote(input: RepositionImageDiffNoteInput!): RepositionImageDiffNotePayload
   revertVulnerabilityToDetected(input: RevertVulnerabilityToDetectedInput!): RevertVulnerabilityToDetectedPayload @deprecated(reason: "Use vulnerabilityRevertToDetected. Deprecated in 13.5")
   runDastScan(input: RunDASTScanInput!): RunDASTScanPayload @deprecated(reason: "Use DastOnDemandScanCreate. Deprecated in 13.4")
   terraformStateDelete(input: TerraformStateDeleteInput!): TerraformStateDeletePayload
@@ -13784,6 +13794,11 @@ type NotePermissions {
   """
   readNote: Boolean!
 
+  """
+  Indicates the user can perform `reposition_note` on this resource
+  """
+  repositionNote: Boolean!
+
   """
   Indicates the user can perform `resolve_note` on this resource
   """
@@ -17849,6 +17864,46 @@ type RemoveProjectFromSecurityDashboardPayload {
   errors: [String!]!
 }
 
+"""
+Autogenerated input type of RepositionImageDiffNote
+"""
+input RepositionImageDiffNoteInput {
+  """
+  A unique identifier for the client performing the mutation.
+  """
+  clientMutationId: String
+
+  """
+  The global id of the DiffNote to update
+  """
+  id: DiffNoteID!
+
+  """
+  The position of this note on a diff
+  """
+  position: UpdateDiffImagePositionInput!
+}
+
+"""
+Autogenerated return type of RepositionImageDiffNote
+"""
+type RepositionImageDiffNotePayload {
+  """
+  A unique identifier for the client performing the mutation.
+  """
+  clientMutationId: String
+
+  """
+  Errors encountered during execution of the mutation.
+  """
+  errors: [String!]!
+
+  """
+  The note after mutation
+  """
+  note: Note
+}
+
 type Repository {
   """
   Indicates repository has no visible content

doc/api/graphql/reference/gitlab_schema.json

@@ -16860,6 +16860,16 @@
           "enumValues": null,
           "possibleTypes": null
         },
+        {
+          "kind": "SCALAR",
+          "name": "DiffNoteID",
+          "description": "Identifier of DiffNote",
+          "fields": null,
+          "inputFields": null,
+          "interfaces": null,
+          "enumValues": null,
+          "possibleTypes": null
+        },
         {
           "kind": "INPUT_OBJECT",
           "name": "DiffPathsInput",
@@ -38672,6 +38682,33 @@
               "isDeprecated": false,
               "deprecationReason": null
             },
+            {
+              "name": "repositionImageDiffNote",
+              "description": "Repositions a DiffNote on an image (a `Note` where the `position.positionType` is `\"image\"`)",
+              "args": [
+                {
+                  "name": "input",
+                  "description": null,
+                  "type": {
+                    "kind": "NON_NULL",
+                    "name": null,
+                    "ofType": {
+                      "kind": "INPUT_OBJECT",
+                      "name": "RepositionImageDiffNoteInput",
+                      "ofType": null
+                    }
+                  },
+                  "defaultValue": null
+                }
+              ],
+              "type": {
+                "kind": "OBJECT",
+                "name": "RepositionImageDiffNotePayload",
+                "ofType": null
+              },
+              "isDeprecated": false,
+              "deprecationReason": null
+            },
             {
               "name": "revertVulnerabilityToDetected",
               "description": null,
@@ -40696,6 +40733,24 @@
               "isDeprecated": false,
               "deprecationReason": null
             },
+            {
+              "name": "repositionNote",
+              "description": "Indicates the user can perform `reposition_note` on this resource",
+              "args": [
+
+              ],
+              "type": {
+                "kind": "NON_NULL",
+                "name": null,
+                "ofType": {
+                  "kind": "SCALAR",
+                  "name": "Boolean",
+                  "ofType": null
+                }
+              },
+              "isDeprecated": false,
+              "deprecationReason": null
+            },
             {
               "name": "resolveNote",
               "description": "Indicates the user can perform `resolve_note` on this resource",
@@ -51457,6 +51512,122 @@
           "enumValues": null,
           "possibleTypes": null
         },
+        {
+          "kind": "INPUT_OBJECT",
+          "name": "RepositionImageDiffNoteInput",
+          "description": "Autogenerated input type of RepositionImageDiffNote",
+          "fields": null,
+          "inputFields": [
+            {
+              "name": "id",
+              "description": "The global id of the DiffNote to update",
+              "type": {
+                "kind": "NON_NULL",
+                "name": null,
+                "ofType": {
+                  "kind": "SCALAR",
+                  "name": "DiffNoteID",
+                  "ofType": null
+                }
+              },
+              "defaultValue": null
+            },
+            {
+              "name": "position",
+              "description": "The position of this note on a diff",
+              "type": {
+                "kind": "NON_NULL",
+                "name": null,
+                "ofType": {
+                  "kind": "INPUT_OBJECT",
+                  "name": "UpdateDiffImagePositionInput",
+                  "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": "RepositionImageDiffNotePayload",
+          "description": "Autogenerated return type of RepositionImageDiffNote",
+          "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": "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
+            },
+            {
+              "name": "note",
+              "description": "The note after mutation",
+              "args": [
+
+              ],
+              "type": {
+                "kind": "OBJECT",
+                "name": "Note",
+                "ofType": null
+              },
+              "isDeprecated": false,
+              "deprecationReason": null
+            }
+          ],
+          "inputFields": null,
+          "interfaces": [
+
+          ],
+          "enumValues": null,
+          "possibleTypes": null
+        },
         {
           "kind": "OBJECT",
           "name": "Repository",

doc/api/graphql/reference/index.md

@@ -2080,6 +2080,7 @@ Autogenerated return type of NamespaceIncreaseStorageTemporarily.
 | `awardEmoji` | Boolean! | Indicates the user can perform `award_emoji` on this resource |
 | `createNote` | Boolean! | Indicates the user can perform `create_note` on this resource |
 | `readNote` | Boolean! | Indicates the user can perform `read_note` on this resource |
+| `repositionNote` | Boolean! | Indicates the user can perform `reposition_note` on this resource |
 | `resolveNote` | Boolean! | Indicates the user can perform `resolve_note` on this resource |
 
 ### Package
@@ -2501,6 +2502,16 @@ Autogenerated return type of RemoveProjectFromSecurityDashboard.
 | `clientMutationId` | String | A unique identifier for the client performing the mutation. |
 | `errors` | String! => Array | Errors encountered during execution of the mutation. |
 
+### RepositionImageDiffNotePayload
+
+Autogenerated return type of RepositionImageDiffNote.
+
+| Field | Type | Description |
+| ----- | ---- | ----------- |
+| `clientMutationId` | String | A unique identifier for the client performing the mutation. |
+| `errors` | String! => Array | Errors encountered during execution of the mutation. |
+| `note` | Note | The note after mutation |
+
 ### Repository
 
 | Field | Type | Description |

spec/graphql/mutations/notes/reposition_image_diff_note_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Mutations::Notes::RepositionImageDiffNote do
+  include GraphqlHelpers
+
+  describe '#resolve' do
+    subject do
+      mutation.resolve({ note: note, position: new_position })
+    end
+
+    let_it_be(:noteable) { create(:merge_request) }
+    let_it_be(:project) { noteable.project }
+    let(:note) { create(:image_diff_note_on_merge_request, noteable: noteable, project: project) }
+
+    let(:mutation) do
+      described_class.new(object: nil, context: { current_user: user }, field: nil)
+    end
+
+    let(:new_position) do
+      { x: 10, y: 11, width: 12, height: 13 }
+    end
+
+    context 'when the user does not have permission' do
+      let(:user) { nil }
+
+      it 'raises an error if the resource is not accessible to the user' do
+        expect { subject }.to raise_error(
+          Gitlab::Graphql::Errors::ResourceNotAvailable,
+          "The resource that you are attempting to access does not exist or you don't have permission to perform this action"
+        )
+      end
+    end
+
+    context 'when the user has permission' do
+      let(:user) { project.creator }
+      let(:mutated_note) { subject[:note] }
+      let(:errors) { subject[:errors] }
+
+      it 'mutates the note', :aggregate_failures do
+        expect { subject }.to change { note.reset.position.to_h }.to(include(new_position))
+
+        expect(mutated_note).to eq(note)
+        expect(errors).to be_empty
+      end
+
+      context 'when the note is a DiffNote, but not on an image' do
+        let(:note) { create(:diff_note_on_merge_request, noteable: noteable, project: project) }
+
+        it 'raises an error' do
+          expect { subject }.to raise_error(
+            Gitlab::Graphql::Errors::ResourceNotAvailable,
+            'Resource is not an ImageDiffNote'
+          )
+        end
+      end
+    end
+  end
+end

spec/graphql/types/permission_types/note_spec.rb

@@ -5,9 +5,9 @@ require 'spec_helper'
 RSpec.describe GitlabSchema.types['NotePermissions'] do
   it 'has the expected fields' do
     expected_permissions = [
-      :read_note, :create_note, :admin_note, :resolve_note, :award_emoji
+      :read_note, :create_note, :admin_note, :resolve_note, :reposition_note, :award_emoji
     ]
 
-    expect(described_class).to have_graphql_fields(expected_permissions)
+    expect(described_class).to have_graphql_fields(expected_permissions).only
   end
 end

spec/requests/api/graphql/mutations/notes/reposition_image_diff_note_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe 'Repositioning an ImageDiffNote' do
+  include GraphqlHelpers
+
+  let_it_be(:noteable) { create(:merge_request) }
+  let_it_be(:project) { noteable.project }
+  let(:note) { create(:image_diff_note_on_merge_request, noteable: noteable, project: project) }
+  let(:new_position) { { x: 10 } }
+  let(:current_user) { project.creator }
+
+  let(:mutation_variables) do
+    {
+      id: global_id_of(note),
+      position: new_position
+    }
+  end
+
+  let(:mutation) do
+    graphql_mutation(:reposition_image_diff_note, mutation_variables) do
+      <<~QL
+        note {
+          id
+        }
+        errors
+      QL
+    end
+  end
+
+  def mutation_response
+    graphql_mutation_response(:reposition_image_diff_note)
+  end
+
+  it 'updates the note', :aggregate_failures do
+    expect do
+      post_graphql_mutation(mutation, current_user: current_user)
+    end.to change { note.reset.position.x }.to(10)
+
+    expect(mutation_response['note']).to eq('id' => global_id_of(note))
+    expect(mutation_response['errors']).to be_empty
+  end
+
+  context 'when the note is not a DiffNote' do
+    let(:note) { project }
+
+    it_behaves_like 'a mutation that returns top-level errors' do
+      let(:match_errors) { include(/does not represent an instance of DiffNote/) }
+    end
+  end
+
+  context 'when a position arg is nil' do
+    let(:new_position) { { x: nil, y: 10 } }
+
+    it 'does not set the property to nil', :aggregate_failures do
+      expect do
+        post_graphql_mutation(mutation, current_user: current_user)
+      end.not_to change { note.reset.position.x }
+
+      expect(mutation_response['note']).to eq('id' => global_id_of(note))
+      expect(mutation_response['errors']).to be_empty
+    end
+  end
+
+  context 'when all position args are nil' do
+    let(:new_position) { { x: nil } }
+
+    it_behaves_like 'a mutation that returns top-level errors' do
+      let(:match_errors) { include(/RepositionImageDiffNoteInput! was provided invalid value/) }
+    end
+
+    it 'contains an explanation for the error' do
+      post_graphql_mutation(mutation, current_user: current_user)
+
+      explanation = graphql_errors.first['extensions']['problems'].first['explanation']
+
+      expect(explanation).to eq('At least one property of `UpdateDiffImagePositionInput` must be set')
+    end
+  end
+end

spec/requests/api/graphql/mutations/notes/update/image_diff_note_spec.rb

@@ -31,7 +31,7 @@ RSpec.describe 'Updating an image DiffNote' do
       height: updated_height,
       x: updated_x,
       y: updated_y
-    }
+    }.compact.presence
   end
 
   let!(:diff_note) do
@@ -45,10 +45,11 @@ RSpec.describe 'Updating an image DiffNote' do
   let(:mutation) do
     variables = {
       id: GitlabSchema.id_from_object(diff_note).to_s,
-      body: updated_body,
-      position: updated_position
+      body: updated_body
     }
 
+    variables[:position] = updated_position if updated_position
+
     graphql_mutation(:update_image_diff_note, variables)
   end