Commit 5ef5cf98 authored by Luke Duncalfe's avatar Luke Duncalfe Committed by Heinrich Lee Yu

Add repositionImageDiffNote mutation

This mutation allows repositioning a DiffNote.

It uses the special `reposition_note` permission, which is an alias of
`admin_note` unless the noteable is a Design, in which case it will be
true if the user can `create_note`.

This allows people who can comment on a Design to reposition the notes
of other people.

https://gitlab.com/gitlab-org/gitlab/-/issues/207334
parent fa883dba
# 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
......@@ -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
......
......@@ -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
......
......@@ -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
......
......@@ -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
---
title: Add repositionImageDiffNote GraphQL mutation to specifically reposition DiffNotes
on images
merge_request: 45958
author:
type: added
......@@ -6143,6 +6143,11 @@ input DiffImagePositionInput {
y: Int!
}
"""
Identifier of DiffNote
"""
scalar DiffNoteID
input DiffPathsInput {
"""
The path of the file on the head sha
......@@ -13251,6 +13256,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
......@@ -13738,6 +13748,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
"""
......@@ -17803,6 +17818,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
......
......@@ -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",
......@@ -38507,6 +38517,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,
......@@ -40531,6 +40568,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",
......@@ -51292,6 +51347,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",
......@@ -2069,6 +2069,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
......@@ -2490,6 +2491,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 |
......
# 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
......@@ -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
# 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
......@@ -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
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment