Commit 874ece00 authored by Luke Duncalfe's avatar Luke Duncalfe Committed by Bob Van Landuyt

Add updateImageDiffNote mutation

This adds the ability to update an `ImageDiffNote`'s `x` and `y`
positions as well as its `body`.

The existing `updateNote` mutation was refactored to inherit from a
new `Base` class, in the same way that the `Notes::Create` mutations
do.

https://gitlab.com/gitlab-org/gitlab/issues/34353
parent e2fb96a9
# frozen_string_literal: true
module Mutations
module Notes
class Update < Base
graphql_name 'UpdateNote'
authorize :admin_note
argument :id,
GraphQL::ID_TYPE,
required: true,
description: 'The global id of the note to update'
argument :body,
GraphQL::STRING_TYPE,
required: true,
description: copy_field_description(Types::Notes::NoteType, :body)
def resolve(args)
note = authorized_find!(id: args[:id])
check_object_is_note!(note)
note = ::Notes::UpdateService.new(
note.project,
current_user,
{ note: args[:body] }
).execute(note)
{
note: note.reset,
errors: errors_on_object(note)
}
end
end
end
end
# frozen_string_literal: true
module Mutations
module Notes
module Update
# This is a Base class for the Note update mutations and is not
# mounted as a GraphQL mutation itself.
class Base < Mutations::Notes::Base
authorize :admin_note
argument :id,
GraphQL::ID_TYPE,
required: true,
description: 'The global id of the note to update'
def resolve(args)
note = authorized_find!(id: args[:id])
pre_update_checks!(note, args)
updated_note = ::Notes::UpdateService.new(
note.project,
current_user,
note_params(note, args)
).execute(note)
# It's possible for updated_note to be `nil`, in the situation
# where the note is deleted within `Notes::UpdateService` due to
# the body of the note only containing Quick Actions.
{
note: updated_note&.reset,
errors: updated_note ? errors_on_object(updated_note) : []
}
end
private
def pre_update_checks!(_note, _args)
raise NotImplementedError
end
def note_params(_note, args)
{ note: args[:body] }.compact
end
end
end
end
end
# frozen_string_literal: true
module Mutations
module Notes
module Update
class ImageDiffNote < Mutations::Notes::Update::Base
graphql_name 'UpdateImageDiffNote'
argument :body,
GraphQL::STRING_TYPE,
required: false,
description: copy_field_description(Types::Notes::NoteType, :body)
argument :position,
Types::Notes::UpdateDiffImagePositionInputType,
required: false,
description: copy_field_description(Types::Notes::NoteType, :position)
def ready?(**args)
# As both arguments are optional, validate here that one of the
# arguments are present.
#
# This may be able to be done using InputUnions in the future
# if this RFC is merged:
# https://github.com/graphql/graphql-spec/blob/master/rfcs/InputUnion.md
if args.values_at(:body, :position).compact.blank?
raise Gitlab::Graphql::Errors::ArgumentError,
'body or position arguments are required'
end
super(args)
end
private
def pre_update_checks!(note, args)
unless note.is_a?(DiffNote) && note.position.on_image?
raise Gitlab::Graphql::Errors::ResourceNotAvailable,
'Resource is not an ImageDiffNote'
end
end
def note_params(note, args)
super(note, args).merge(
position: position_params(note, args)
).compact
end
def position_params(note, args)
new_position = args[:position]&.to_h&.compact
return unless new_position
original_position = note.position.to_h
Gitlab::Diff::Position.new(original_position.merge(new_position))
end
end
end
end
end
# frozen_string_literal: true
module Mutations
module Notes
module Update
class Note < Mutations::Notes::Update::Base
graphql_name 'UpdateNote'
argument :body,
GraphQL::STRING_TYPE,
required: true,
description: copy_field_description(Types::Notes::NoteType, :body)
private
def pre_update_checks!(note, _args)
check_object_is_note!(note)
end
end
end
end
end
......@@ -4,7 +4,7 @@ module Types
class MutationType < BaseObject
include Gitlab::Graphql::MountMutation
graphql_name "Mutation"
graphql_name 'Mutation'
mount_mutation Mutations::AwardEmojis::Add
mount_mutation Mutations::AwardEmojis::Remove
......@@ -20,7 +20,14 @@ module Types
mount_mutation Mutations::Notes::Create::Note, calls_gitaly: true
mount_mutation Mutations::Notes::Create::DiffNote, calls_gitaly: true
mount_mutation Mutations::Notes::Create::ImageDiffNote, calls_gitaly: true
mount_mutation Mutations::Notes::Update
mount_mutation Mutations::Notes::Update::Note,
description: 'Updates a Note. 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::Update::ImageDiffNote,
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::Destroy
mount_mutation Mutations::Todos::MarkDone
mount_mutation Mutations::Todos::Restore
......
......@@ -29,10 +29,10 @@ module Types
# Fields for image positions
field :x, GraphQL::INT_TYPE, null: true,
description: 'X position on which the comment was made',
description: 'X position of the note',
resolve: -> (position, _args, _ctx) { position.x if position.on_image? }
field :y, GraphQL::INT_TYPE, null: true,
description: 'Y position on which the comment was made',
description: 'Y position of the note',
resolve: -> (position, _args, _ctx) { position.y if position.on_image? }
field :width, GraphQL::INT_TYPE, null: true,
description: 'Total width of the image',
......
# frozen_string_literal: true
module Types
module Notes
# InputType used for updateImageDiffNote mutation.
#
# rubocop: disable Graphql/AuthorizeTypes
class UpdateDiffImagePositionInputType < BaseInputObject
graphql_name 'UpdateDiffImagePositionInput'
argument :x, GraphQL::INT_TYPE,
required: false,
description: copy_field_description(Types::Notes::DiffPositionType, :x)
argument :y, GraphQL::INT_TYPE,
required: false,
description: copy_field_description(Types::Notes::DiffPositionType, :y)
argument :width, GraphQL::INT_TYPE,
required: false,
description: copy_field_description(Types::Notes::DiffPositionType, :width)
argument :height, GraphQL::INT_TYPE,
required: false,
description: copy_field_description(Types::Notes::DiffPositionType, :height)
end
# rubocop: enable Graphql/AuthorizeTypes
end
end
---
title: Add updateImageDiffNote mutation
merge_request: 24027
author:
type: added
......@@ -1406,12 +1406,12 @@ input DiffImagePositionInput {
width: Int!
"""
X position on which the comment was made
X position of the note
"""
x: Int!
"""
Y position on which the comment was made
Y position of the note
"""
y: Int!
}
......@@ -1475,12 +1475,12 @@ type DiffPosition {
width: Int
"""
X position on which the comment was made
X position of the note
"""
x: Int
"""
Y position on which the comment was made
Y position of the note
"""
y: Int
}
......@@ -4660,6 +4660,18 @@ type Mutation {
todosMarkAllDone(input: TodosMarkAllDoneInput!): TodosMarkAllDonePayload
toggleAwardEmoji(input: ToggleAwardEmojiInput!): ToggleAwardEmojiPayload
updateEpic(input: UpdateEpicInput!): UpdateEpicPayload
"""
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
"""
updateImageDiffNote(input: UpdateImageDiffNoteInput!): UpdateImageDiffNotePayload
"""
Updates a Note. If the body of the Note contains only quick actions, the Note
will be destroyed during the update, and no Note will be returned
"""
updateNote(input: UpdateNoteInput!): UpdateNotePayload
updateSnippet(input: UpdateSnippetInput!): UpdateSnippetPayload
}
......@@ -7533,6 +7545,28 @@ enum TypeEnum {
project
}
input UpdateDiffImagePositionInput {
"""
Total height of the image
"""
height: Int
"""
Total width of the image
"""
width: Int
"""
X position of the note
"""
x: Int
"""
Y position of the note
"""
y: Int
}
"""
Autogenerated input type of UpdateEpic
"""
......@@ -7618,6 +7652,51 @@ type UpdateEpicPayload {
errors: [String!]!
}
"""
Autogenerated input type of UpdateImageDiffNote
"""
input UpdateImageDiffNoteInput {
"""
Content of the note
"""
body: String
"""
A unique identifier for the client performing the mutation.
"""
clientMutationId: String
"""
The global id of the note to update
"""
id: ID!
"""
The position of this note on a diff
"""
position: UpdateDiffImagePositionInput
}
"""
Autogenerated return type of UpdateImageDiffNote
"""
type UpdateImageDiffNotePayload {
"""
A unique identifier for the client performing the mutation.
"""
clientMutationId: String
"""
Reasons why the mutation failed.
"""
errors: [String!]!
"""
The note after mutation
"""
note: Note
}
"""
Autogenerated input type of UpdateNote
"""
......
......@@ -8063,7 +8063,7 @@
},
{
"name": "x",
"description": "X position on which the comment was made",
"description": "X position of the note",
"args": [
],
......@@ -8077,7 +8077,7 @@
},
{
"name": "y",
"description": "Y position on which the comment was made",
"description": "Y position of the note",
"args": [
],
......@@ -19426,9 +19426,36 @@
"isDeprecated": false,
"deprecationReason": null
},
{
"name": "updateImageDiffNote",
"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",
"args": [
{
"name": "input",
"description": null,
"type": {
"kind": "NON_NULL",
"name": null,
"ofType": {
"kind": "INPUT_OBJECT",
"name": "UpdateImageDiffNoteInput",
"ofType": null
}
},
"defaultValue": null
}
],
"type": {
"kind": "OBJECT",
"name": "UpdateImageDiffNotePayload",
"ofType": null
},
"isDeprecated": false,
"deprecationReason": null
},
{
"name": "updateNote",
"description": null,
"description": "Updates a Note. If the body of the Note contains only quick actions, the Note will be destroyed during the update, and no Note will be returned",
"args": [
{
"name": "input",
......@@ -21640,7 +21667,7 @@
},
{
"name": "x",
"description": "X position on which the comment was made",
"description": "X position of the note",
"type": {
"kind": "NON_NULL",
"name": null,
......@@ -21654,7 +21681,7 @@
},
{
"name": "y",
"description": "Y position on which the comment was made",
"description": "Y position of the note",
"type": {
"kind": "NON_NULL",
"name": null,
......@@ -21815,6 +21842,179 @@
"enumValues": null,
"possibleTypes": null
},
{
"kind": "OBJECT",
"name": "UpdateImageDiffNotePayload",
"description": "Autogenerated return type of UpdateImageDiffNote",
"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": "Reasons why the mutation failed.",
"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": "INPUT_OBJECT",
"name": "UpdateImageDiffNoteInput",
"description": "Autogenerated input type of UpdateImageDiffNote",
"fields": null,
"inputFields": [
{
"name": "id",
"description": "The global id of the note to update",
"type": {
"kind": "NON_NULL",
"name": null,
"ofType": {
"kind": "SCALAR",
"name": "ID",
"ofType": null
}
},
"defaultValue": null
},
{
"name": "body",
"description": "Content of the note",
"type": {
"kind": "SCALAR",
"name": "String",
"ofType": null
},
"defaultValue": null
},
{
"name": "position",
"description": "The position of this note on a diff",
"type": {
"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": "INPUT_OBJECT",
"name": "UpdateDiffImagePositionInput",
"description": null,
"fields": null,
"inputFields": [
{
"name": "x",
"description": "X position of the note",
"type": {
"kind": "SCALAR",
"name": "Int",
"ofType": null
},
"defaultValue": null
},
{
"name": "y",
"description": "Y position of the note",
"type": {
"kind": "SCALAR",
"name": "Int",
"ofType": null
},
"defaultValue": null
},
{
"name": "width",
"description": "Total width of the image",
"type": {
"kind": "SCALAR",
"name": "Int",
"ofType": null
},
"defaultValue": null
},
{
"name": "height",
"description": "Total height of the image",
"type": {
"kind": "SCALAR",
"name": "Int",
"ofType": null
},
"defaultValue": null
}
],
"interfaces": null,
"enumValues": null,
"possibleTypes": null
},
{
"kind": "OBJECT",
"name": "DestroyNotePayload",
......
......@@ -246,8 +246,8 @@ Autogenerated return type of DestroySnippet
| `oldPath` | String | Path of the file on the start SHA |
| `positionType` | DiffPositionType! | Type of file the position refers to |
| `width` | Int | Total width of the image |
| `x` | Int | X position on which the comment was made |
| `y` | Int | Y position on which the comment was made |
| `x` | Int | X position of the note |
| `y` | Int | Y position of the note |
## DiffRefs
......@@ -1230,6 +1230,16 @@ Autogenerated return type of UpdateEpic
| `epic` | Epic | The epic after mutation |
| `errors` | String! => Array | Reasons why the mutation failed. |
## UpdateImageDiffNotePayload
Autogenerated return type of UpdateImageDiffNote
| Name | Type | Description |
| --- | ---- | ---------- |
| `clientMutationId` | String | A unique identifier for the client performing the mutation. |
| `errors` | String! => Array | Reasons why the mutation failed. |
| `note` | Note | The note after mutation |
## UpdateNotePayload
Autogenerated return type of UpdateNote
......
# frozen_string_literal: true
require 'spec_helper'
describe 'Updating an image DiffNote' do
include GraphqlHelpers
using RSpec::Parameterized::TableSyntax
let_it_be(:noteable) { create(:merge_request, :with_diffs) }
let_it_be(:original_body) { 'Original body' }
let_it_be(:original_position) do
Gitlab::Diff::Position.new(
old_path: 'files/images/any_image.png',
new_path: 'files/images/any_image.png',
width: 10,
height: 20,
x: 1,
y: 2,
diff_refs: noteable.diff_refs,
position_type: 'image'
)
end
let_it_be(:updated_body) { 'Updated body' }
let_it_be(:updated_width) { 50 }
let_it_be(:updated_height) { 100 }
let_it_be(:updated_x) { 5 }
let_it_be(:updated_y) { 10 }
let(:updated_position) do
{
width: updated_width,
height: updated_height,
x: updated_x,
y: updated_y
}
end
let!(:diff_note) do
create(:image_diff_note_on_merge_request,
noteable: noteable,
project: noteable.project,
note: original_body,
position: original_position)
end
let(:mutation) do
variables = {
id: GitlabSchema.id_from_object(diff_note).to_s,
body: updated_body,
position: updated_position
}
graphql_mutation(:update_image_diff_note, variables)
end
def mutation_response
graphql_mutation_response(:update_image_diff_note)
end
context 'when the user does not have permission' do
let_it_be(:current_user) { create(:user) }
it_behaves_like 'a mutation that returns top-level errors',
errors: ['The resource that you are attempting to access does not exist or you don\'t have permission to perform this action']
it 'does not update the DiffNote' do
post_graphql_mutation(mutation, current_user: current_user)
diff_note.reload
expect(diff_note).to have_attributes(
note: original_body,
position: have_attributes(
width: original_position.width,
height: original_position.height,
x: original_position.x,
y: original_position.y
)
)
end
end
context 'when the user has permission' do
let(:current_user) { diff_note.author }
it 'updates the DiffNote' do
post_graphql_mutation(mutation, current_user: current_user)
diff_note.reload
expect(diff_note).to have_attributes(
note: updated_body,
position: have_attributes(
width: updated_width,
height: updated_height,
x: updated_x,
y: updated_y
)
)
end
it 'returns the updated DiffNote' do
post_graphql_mutation(mutation, current_user: current_user)
expect(mutation_response['note']).to include(
'body' => updated_body,
'position' => hash_including(
'width' => updated_width,
'height' => updated_height,
'x' => updated_x,
'y' => updated_y
)
)
end
describe 'updating single properties at a time' do
where(:property, :new_value) do
:body | 'foo'
:width | 19
:height | 18
:x | 17
:y | 16
end
with_them do
# Properties that will be POSTed:
let(:updated_body) { value(:body) }
let(:updated_width) { value(:width) }
let(:updated_height) { value(:height) }
let(:updated_x) { value(:x) }
let(:updated_y) { value(:y) }
# Expectations of the properties:
let(:expected_body) { value(:body) || original_body }
let(:expected_width) { value(:width) || original_position.width }
let(:expected_height) { value(:height) || original_position.height }
let(:expected_x) { value(:x) || original_position.x }
let(:expected_y) { value(:y) || original_position.y }
def value(prop)
new_value if property == prop
end
it 'updates the DiffNote correctly' do
post_graphql_mutation(mutation, current_user: current_user)
diff_note.reload
expect(diff_note).to have_attributes(
note: expected_body,
position: have_attributes(
width: expected_width,
height: expected_height,
x: expected_x,
y: expected_y
)
)
end
end
context 'when position is nil' do
let(:updated_position) { nil }
it 'updates the DiffNote correctly' do
post_graphql_mutation(mutation, current_user: current_user)
diff_note.reload
expect(diff_note).to have_attributes(
note: updated_body,
position: original_position
)
end
end
end
context 'when both body and position args are blank' do
let(:updated_body) { nil }
let(:updated_position) { nil }
it_behaves_like 'a mutation that returns top-level errors', errors: ['body or position arguments are required']
end
context 'when resource is not a DiffNote on an image' do
let!(:diff_note) { create(:diff_note_on_merge_request, note: original_body) }
it_behaves_like 'a mutation that returns top-level errors', errors: ['Resource is not an ImageDiffNote']
end
context 'when there are ActiveRecord validation errors' do
before do
expect(diff_note).to receive_message_chain(
:errors,
:full_messages
).and_return(['Error 1', 'Error 2'])
expect_next_instance_of(Notes::UpdateService) do |service|
expect(service).to receive(:execute).and_return(diff_note)
end
end
it_behaves_like 'a mutation that returns errors in the response', errors: ['Error 1', 'Error 2']
it 'does not update the DiffNote' do
post_graphql_mutation(mutation, current_user: current_user)
diff_note.reload
expect(diff_note).to have_attributes(
note: original_body,
position: have_attributes(
width: original_position.width,
height: original_position.height,
x: original_position.x,
y: original_position.y
)
)
end
it 'returns the DiffNote with its original body' do
post_graphql_mutation(mutation, current_user: current_user)
expect(mutation_response['note']).to include(
'body' => original_body,
'position' => hash_including(
'width' => original_position.width,
'height' => original_position.height,
'x' => original_position.x,
'y' => original_position.y
)
)
end
end
context 'when body only contains quick actions' do
let(:updated_body) { '/close' }
it 'returns a nil note and empty errors' do
post_graphql_mutation(mutation, current_user: current_user)
expect(mutation_response).to include(
'errors' => [],
'note' => nil
)
end
end
end
end
......@@ -22,7 +22,7 @@ describe 'Updating a Note' do
end
context 'when the user does not have permission' do
let(:current_user) { create(:user) }
let_it_be(:current_user) { create(:user) }
it_behaves_like 'a mutation that returns top-level errors',
errors: ['The resource that you are attempting to access does not exist or you don\'t have permission to perform this action']
......@@ -68,5 +68,18 @@ describe 'Updating a Note' do
expect(mutation_response['note']['body']).to eq(original_body)
end
end
context 'when body only contains quick actions' do
let(:updated_body) { '/close' }
it 'returns a nil note and empty errors' do
post_graphql_mutation(mutation, current_user: current_user)
expect(mutation_response).to include(
'errors' => [],
'note' => nil
)
end
end
end
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