summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorJan Provaznik <jprovaznik@gitlab.com>2019-07-10 05:52:18 +0000
committerJan Provaznik <jprovaznik@gitlab.com>2019-07-10 05:52:18 +0000
commit810df4fb51bf3db4016c5f7458599331d4586300 (patch)
tree6d034939842822b4173d99df698aed00a47fbe68
parent227d8b4445fa1257b525945a34a2e301369736a4 (diff)
parent073c8b25ea36b6b96eab05eb675e8726b1d5318e (diff)
downloadgitlab-ce-810df4fb51bf3db4016c5f7458599331d4586300.tar.gz
Merge branch '62826-graphql-note-mutations' into 'master'
GraphQL mutations for managing Notes See merge request gitlab-org/gitlab-ce!30210
-rw-r--r--app/graphql/gitlab_schema.rb2
-rw-r--r--app/graphql/mutations/notes/base.rb34
-rw-r--r--app/graphql/mutations/notes/create/base.rb49
-rw-r--r--app/graphql/mutations/notes/create/diff_note.rb33
-rw-r--r--app/graphql/mutations/notes/create/image_diff_note.rb33
-rw-r--r--app/graphql/mutations/notes/create/note.rb40
-rw-r--r--app/graphql/mutations/notes/destroy.rb28
-rw-r--r--app/graphql/mutations/notes/update.rb38
-rw-r--r--app/graphql/types/base_input_object.rb1
-rw-r--r--app/graphql/types/diff_paths_input_type.rb12
-rw-r--r--app/graphql/types/diff_refs_type.rb14
-rw-r--r--app/graphql/types/merge_request_type.rb1
-rw-r--r--app/graphql/types/mutation_type.rb5
-rw-r--r--app/graphql/types/notes/diff_image_position_input_type.rb20
-rw-r--r--app/graphql/types/notes/diff_position_base_input_type.rb22
-rw-r--r--app/graphql/types/notes/diff_position_input_type.rb16
-rw-r--r--app/graphql/types/notes/diff_position_type.rb7
-rw-r--r--app/graphql/types/notes/discussion_type.rb8
-rw-r--r--app/models/discussion.rb11
-rw-r--r--app/models/note.rb2
-rw-r--r--changelogs/unreleased/62826-graphql-note-mutations.yml5
-rw-r--r--lib/gitlab/global_id.rb14
-rw-r--r--spec/graphql/gitlab_schema_spec.rb20
-rw-r--r--spec/graphql/types/diff_refs_type_spec.rb9
-rw-r--r--spec/graphql/types/merge_request_type_spec.rb2
-rw-r--r--spec/graphql/types/notes/diff_position_type_spec.rb2
-rw-r--r--spec/graphql/types/notes/discussion_type_spec.rb2
-rw-r--r--spec/lib/gitlab/global_id_spec.rb37
-rw-r--r--spec/models/discussion_spec.rb14
-rw-r--r--spec/requests/api/graphql/mutations/notes/create/diff_note_spec.rb64
-rw-r--r--spec/requests/api/graphql/mutations/notes/create/image_diff_note_spec.rb70
-rw-r--r--spec/requests/api/graphql/mutations/notes/create/note_spec.rb64
-rw-r--r--spec/requests/api/graphql/mutations/notes/destroy_spec.rb52
-rw-r--r--spec/requests/api/graphql/mutations/notes/update_spec.rb72
-rw-r--r--spec/support/helpers/graphql_helpers.rb15
-rw-r--r--spec/support/shared_examples/graphql/notes_creation_shared_examples.rb61
36 files changed, 869 insertions, 10 deletions
diff --git a/app/graphql/gitlab_schema.rb b/app/graphql/gitlab_schema.rb
index 152ebb930e2..7edd14e48f7 100644
--- a/app/graphql/gitlab_schema.rb
+++ b/app/graphql/gitlab_schema.rb
@@ -66,6 +66,8 @@ class GitlabSchema < GraphQL::Schema
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)
+ gid.model_class.lazy_find(gid.model_id)
else
gid.find
end
diff --git a/app/graphql/mutations/notes/base.rb b/app/graphql/mutations/notes/base.rb
new file mode 100644
index 00000000000..a7198f5fba6
--- /dev/null
+++ b/app/graphql/mutations/notes/base.rb
@@ -0,0 +1,34 @@
+# frozen_string_literal: true
+
+module Mutations
+ module Notes
+ class Base < BaseMutation
+ include Gitlab::Graphql::Authorize::AuthorizeResource
+
+ field :note,
+ Types::Notes::NoteType,
+ null: true,
+ description: 'The note after mutation'
+
+ private
+
+ def find_object(id:)
+ GitlabSchema.object_from_id(id)
+ end
+
+ def check_object_is_noteable!(object)
+ unless object.is_a?(Noteable)
+ raise Gitlab::Graphql::Errors::ResourceNotAvailable,
+ 'Cannot add notes to this resource'
+ end
+ end
+
+ def check_object_is_note!(object)
+ unless object.is_a?(Note)
+ raise Gitlab::Graphql::Errors::ResourceNotAvailable,
+ 'Resource is not a note'
+ end
+ end
+ end
+ end
+end
diff --git a/app/graphql/mutations/notes/create/base.rb b/app/graphql/mutations/notes/create/base.rb
new file mode 100644
index 00000000000..d3a5dae2188
--- /dev/null
+++ b/app/graphql/mutations/notes/create/base.rb
@@ -0,0 +1,49 @@
+# frozen_string_literal: true
+
+module Mutations
+ module Notes
+ module Create
+ # This is a Base class for the Note creation Mutations and is not
+ # mounted as a GraphQL mutation itself.
+ class Base < Mutations::Notes::Base
+ authorize :create_note
+
+ argument :noteable_id,
+ GraphQL::ID_TYPE,
+ required: true,
+ description: 'The global id of the resource to add a note to'
+
+ argument :body,
+ GraphQL::STRING_TYPE,
+ required: true,
+ description: copy_field_description(Types::Notes::NoteType, :body)
+
+ private
+
+ def resolve(args)
+ noteable = authorized_find!(id: args[:noteable_id])
+
+ check_object_is_noteable!(noteable)
+
+ note = ::Notes::CreateService.new(
+ noteable.project,
+ current_user,
+ create_note_params(noteable, args)
+ ).execute
+
+ {
+ note: (note if note.persisted?),
+ errors: errors_on_object(note)
+ }
+ end
+
+ def create_note_params(noteable, args)
+ {
+ noteable: noteable,
+ note: args[:body]
+ }
+ end
+ end
+ end
+ end
+end
diff --git a/app/graphql/mutations/notes/create/diff_note.rb b/app/graphql/mutations/notes/create/diff_note.rb
new file mode 100644
index 00000000000..9b5f3092006
--- /dev/null
+++ b/app/graphql/mutations/notes/create/diff_note.rb
@@ -0,0 +1,33 @@
+# frozen_string_literal: true
+
+module Mutations
+ module Notes
+ module Create
+ class DiffNote < Base
+ graphql_name 'CreateDiffNote'
+
+ argument :position,
+ Types::Notes::DiffPositionInputType,
+ required: true,
+ description: copy_field_description(Types::Notes::NoteType, :position)
+
+ private
+
+ def create_note_params(noteable, args)
+ super(noteable, args).merge({
+ type: 'DiffNote',
+ position: position(noteable, args)
+ })
+ end
+
+ def position(noteable, args)
+ position = args[:position].to_h
+ position[:position_type] = 'text'
+ position.merge!(position[:paths].to_h)
+
+ Gitlab::Diff::Position.new(position)
+ end
+ end
+ end
+ end
+end
diff --git a/app/graphql/mutations/notes/create/image_diff_note.rb b/app/graphql/mutations/notes/create/image_diff_note.rb
new file mode 100644
index 00000000000..d94fd4d6ff8
--- /dev/null
+++ b/app/graphql/mutations/notes/create/image_diff_note.rb
@@ -0,0 +1,33 @@
+# frozen_string_literal: true
+
+module Mutations
+ module Notes
+ module Create
+ class ImageDiffNote < Base
+ graphql_name 'CreateImageDiffNote'
+
+ argument :position,
+ Types::Notes::DiffImagePositionInputType,
+ required: true,
+ description: copy_field_description(Types::Notes::NoteType, :position)
+
+ private
+
+ def create_note_params(noteable, args)
+ super(noteable, args).merge({
+ type: 'DiffNote',
+ position: position(noteable, args)
+ })
+ end
+
+ def position(noteable, args)
+ position = args[:position].to_h
+ position[:position_type] = 'image'
+ position.merge!(position[:paths].to_h)
+
+ Gitlab::Diff::Position.new(position)
+ end
+ end
+ end
+ end
+end
diff --git a/app/graphql/mutations/notes/create/note.rb b/app/graphql/mutations/notes/create/note.rb
new file mode 100644
index 00000000000..5236e48026e
--- /dev/null
+++ b/app/graphql/mutations/notes/create/note.rb
@@ -0,0 +1,40 @@
+# frozen_string_literal: true
+
+module Mutations
+ module Notes
+ module Create
+ class Note < Base
+ graphql_name 'CreateNote'
+
+ argument :discussion_id,
+ GraphQL::ID_TYPE,
+ required: false,
+ description: 'The global id of the discussion this note is in reply to'
+
+ private
+
+ def create_note_params(noteable, args)
+ discussion_id = nil
+
+ if args[:discussion_id]
+ discussion = GitlabSchema.object_from_id(args[:discussion_id])
+ authorize_discussion!(discussion)
+
+ discussion_id = discussion.id
+ end
+
+ super(noteable, args).merge({
+ in_reply_to_discussion_id: discussion_id
+ })
+ end
+
+ def authorize_discussion!(discussion)
+ unless Ability.allowed?(current_user, :read_note, discussion, scope: :user)
+ raise Gitlab::Graphql::Errors::ResourceNotAvailable,
+ "The discussion does not exist or you don't have permission to perform this action"
+ end
+ end
+ end
+ end
+ end
+end
diff --git a/app/graphql/mutations/notes/destroy.rb b/app/graphql/mutations/notes/destroy.rb
new file mode 100644
index 00000000000..a81322bc9b7
--- /dev/null
+++ b/app/graphql/mutations/notes/destroy.rb
@@ -0,0 +1,28 @@
+# frozen_string_literal: true
+
+module Mutations
+ module Notes
+ class Destroy < Base
+ graphql_name 'DestroyNote'
+
+ authorize :admin_note
+
+ argument :id,
+ GraphQL::ID_TYPE,
+ required: true,
+ description: 'The global id of the note to destroy'
+
+ def resolve(id:)
+ note = authorized_find!(id: id)
+
+ check_object_is_note!(note)
+
+ ::Notes::DestroyService.new(note.project, current_user).execute(note)
+
+ {
+ errors: []
+ }
+ end
+ end
+ end
+end
diff --git a/app/graphql/mutations/notes/update.rb b/app/graphql/mutations/notes/update.rb
new file mode 100644
index 00000000000..ebf57b800c0
--- /dev/null
+++ b/app/graphql/mutations/notes/update.rb
@@ -0,0 +1,38 @@
+# 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
diff --git a/app/graphql/types/base_input_object.rb b/app/graphql/types/base_input_object.rb
index aebed035d3b..90a29b0cfb8 100644
--- a/app/graphql/types/base_input_object.rb
+++ b/app/graphql/types/base_input_object.rb
@@ -2,5 +2,6 @@
module Types
class BaseInputObject < GraphQL::Schema::InputObject
+ prepend Gitlab::Graphql::CopyFieldDescription
end
end
diff --git a/app/graphql/types/diff_paths_input_type.rb b/app/graphql/types/diff_paths_input_type.rb
new file mode 100644
index 00000000000..43feddd9827
--- /dev/null
+++ b/app/graphql/types/diff_paths_input_type.rb
@@ -0,0 +1,12 @@
+# frozen_string_literal: true
+
+module Types
+ # rubocop: disable Graphql/AuthorizeTypes
+ class DiffPathsInputType < BaseInputObject
+ argument :old_path, GraphQL::STRING_TYPE, required: false,
+ description: 'The path of the file on the start sha'
+ argument :new_path, GraphQL::STRING_TYPE, required: false,
+ description: 'The path of the file on the head sha'
+ end
+ # rubocop: enable Graphql/AuthorizeTypes
+end
diff --git a/app/graphql/types/diff_refs_type.rb b/app/graphql/types/diff_refs_type.rb
new file mode 100644
index 00000000000..33a5780cd68
--- /dev/null
+++ b/app/graphql/types/diff_refs_type.rb
@@ -0,0 +1,14 @@
+# frozen_string_literal: true
+
+module Types
+ # rubocop: disable Graphql/AuthorizeTypes
+ # Types that use DiffRefsType should have their own authorization
+ class DiffRefsType < BaseObject
+ graphql_name 'DiffRefs'
+
+ field :head_sha, GraphQL::STRING_TYPE, null: false, description: 'The sha of the head at the time the comment was made'
+ field :base_sha, GraphQL::STRING_TYPE, null: false, description: 'The merge base of the branch the comment was made on'
+ field :start_sha, GraphQL::STRING_TYPE, null: false, description: 'The sha of the branch being compared against'
+ end
+ # rubocop: enable Graphql/AuthorizeTypes
+end
diff --git a/app/graphql/types/merge_request_type.rb b/app/graphql/types/merge_request_type.rb
index 6734d4761c2..b8f63a750c5 100644
--- a/app/graphql/types/merge_request_type.rb
+++ b/app/graphql/types/merge_request_type.rb
@@ -23,6 +23,7 @@ module Types
field :updated_at, Types::TimeType, null: false
field :source_project, Types::ProjectType, null: true
field :target_project, Types::ProjectType, null: false
+ field :diff_refs, Types::DiffRefsType, null: true
# Alias for target_project
field :project, Types::ProjectType, null: false
field :project_id, GraphQL::INT_TYPE, null: false, method: :target_project_id
diff --git a/app/graphql/types/mutation_type.rb b/app/graphql/types/mutation_type.rb
index bc5fb709522..f843d6ad86f 100644
--- a/app/graphql/types/mutation_type.rb
+++ b/app/graphql/types/mutation_type.rb
@@ -10,5 +10,10 @@ module Types
mount_mutation Mutations::AwardEmojis::Remove
mount_mutation Mutations::AwardEmojis::Toggle
mount_mutation Mutations::MergeRequests::SetWip, calls_gitaly: true
+ 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::Destroy
end
end
diff --git a/app/graphql/types/notes/diff_image_position_input_type.rb b/app/graphql/types/notes/diff_image_position_input_type.rb
new file mode 100644
index 00000000000..23b53b20815
--- /dev/null
+++ b/app/graphql/types/notes/diff_image_position_input_type.rb
@@ -0,0 +1,20 @@
+# frozen_string_literal: true
+
+module Types
+ module Notes
+ # rubocop: disable Graphql/AuthorizeTypes
+ class DiffImagePositionInputType < DiffPositionBaseInputType
+ graphql_name 'DiffImagePositionInput'
+
+ argument :x, GraphQL::INT_TYPE, required: true,
+ description: copy_field_description(Types::Notes::DiffPositionType, :x)
+ argument :y, GraphQL::INT_TYPE, required: true,
+ description: copy_field_description(Types::Notes::DiffPositionType, :y)
+ argument :width, GraphQL::INT_TYPE, required: true,
+ description: copy_field_description(Types::Notes::DiffPositionType, :width)
+ argument :height, GraphQL::INT_TYPE, required: true,
+ description: copy_field_description(Types::Notes::DiffPositionType, :height)
+ end
+ # rubocop: enable Graphql/AuthorizeTypes
+ end
+end
diff --git a/app/graphql/types/notes/diff_position_base_input_type.rb b/app/graphql/types/notes/diff_position_base_input_type.rb
new file mode 100644
index 00000000000..a9b4e1a8948
--- /dev/null
+++ b/app/graphql/types/notes/diff_position_base_input_type.rb
@@ -0,0 +1,22 @@
+# frozen_string_literal: true
+
+module Types
+ module Notes
+ # rubocop: disable Graphql/AuthorizeTypes
+ class DiffPositionBaseInputType < BaseInputObject
+ argument :head_sha, GraphQL::STRING_TYPE, required: true,
+ description: copy_field_description(Types::DiffRefsType, :head_sha)
+ argument :base_sha, GraphQL::STRING_TYPE, required: false,
+ description: copy_field_description(Types::DiffRefsType, :base_sha)
+ argument :start_sha, GraphQL::STRING_TYPE, required: true,
+ description: copy_field_description(Types::DiffRefsType, :start_sha)
+
+ argument :paths,
+ Types::DiffPathsInputType,
+ required: true,
+ description: 'The paths of the file that was changed. ' \
+ 'Both of the properties of this input are optional, but at least one of them is required'
+ end
+ # rubocop: enable Graphql/AuthorizeTypes
+ end
+end
diff --git a/app/graphql/types/notes/diff_position_input_type.rb b/app/graphql/types/notes/diff_position_input_type.rb
new file mode 100644
index 00000000000..02c91e173cb
--- /dev/null
+++ b/app/graphql/types/notes/diff_position_input_type.rb
@@ -0,0 +1,16 @@
+# frozen_string_literal: true
+
+module Types
+ module Notes
+ # rubocop: disable Graphql/AuthorizeTypes
+ class DiffPositionInputType < DiffPositionBaseInputType
+ graphql_name 'DiffPositionInput'
+
+ argument :old_line, GraphQL::INT_TYPE, required: false,
+ description: copy_field_description(Types::Notes::DiffPositionType, :old_line)
+ argument :new_line, GraphQL::INT_TYPE, required: true,
+ description: copy_field_description(Types::Notes::DiffPositionType, :new_line)
+ end
+ # rubocop: enable Graphql/AuthorizeTypes
+ end
+end
diff --git a/app/graphql/types/notes/diff_position_type.rb b/app/graphql/types/notes/diff_position_type.rb
index ebc24451715..6a0377fbfdf 100644
--- a/app/graphql/types/notes/diff_position_type.rb
+++ b/app/graphql/types/notes/diff_position_type.rb
@@ -7,12 +7,7 @@ module Types
class DiffPositionType < BaseObject
graphql_name 'DiffPosition'
- field :head_sha, GraphQL::STRING_TYPE, null: false,
- description: "The sha of the head at the time the comment was made"
- field :base_sha, GraphQL::STRING_TYPE, null: true,
- description: "The merge base of the branch the comment was made on"
- field :start_sha, GraphQL::STRING_TYPE, null: false,
- description: "The sha of the branch being compared against"
+ field :diff_refs, Types::DiffRefsType, null: false
field :file_path, GraphQL::STRING_TYPE, null: false,
description: "The path of the file that was changed"
diff --git a/app/graphql/types/notes/discussion_type.rb b/app/graphql/types/notes/discussion_type.rb
index c4691942f2d..a3fb28298f6 100644
--- a/app/graphql/types/notes/discussion_type.rb
+++ b/app/graphql/types/notes/discussion_type.rb
@@ -8,8 +8,16 @@ module Types
authorize :read_note
field :id, GraphQL::ID_TYPE, null: false
+ field :reply_id, GraphQL::ID_TYPE, null: false, description: 'The ID used to reply to this discussion'
field :created_at, Types::TimeType, null: false
field :notes, Types::Notes::NoteType.connection_type, null: false, description: "All notes in the discussion"
+
+ # The gem we use to generate Global IDs is hard-coded to work with
+ # `id` properties. To generate a GID for the `reply_id` property,
+ # we must use the ::Gitlab::GlobalId module.
+ def reply_id
+ ::Gitlab::GlobalId.build(object, id: object.reply_id)
+ end
end
end
end
diff --git a/app/models/discussion.rb b/app/models/discussion.rb
index ae13cdfd85f..dd896f77084 100644
--- a/app/models/discussion.rb
+++ b/app/models/discussion.rb
@@ -38,6 +38,17 @@ class Discussion
grouped_notes.values.map { |notes| build(notes, context_noteable) }
end
+ def self.lazy_find(discussion_id)
+ BatchLoader.for(discussion_id).batch do |discussion_ids, loader|
+ results = Note.where(discussion_id: discussion_ids).fresh.to_a.group_by(&:discussion_id)
+ results.each do |discussion_id, notes|
+ next if notes.empty?
+
+ loader.call(discussion_id, Discussion.build(notes))
+ end
+ end
+ end
+
# Returns an alphanumeric discussion ID based on `build_discussion_id`
def self.discussion_id(note)
Digest::SHA1.hexdigest(build_discussion_id(note).join("-"))
diff --git a/app/models/note.rb b/app/models/note.rb
index 4e9ea146485..5c31cff9816 100644
--- a/app/models/note.rb
+++ b/app/models/note.rb
@@ -158,6 +158,8 @@ class Note < ApplicationRecord
Discussion.build_collection(all.includes(:noteable).fresh, context_noteable)
end
+ # Note: Where possible consider using Discussion#lazy_find to return
+ # Discussions in order to benefit from having records batch loaded.
def find_discussion(discussion_id)
notes = where(discussion_id: discussion_id).fresh.to_a
diff --git a/changelogs/unreleased/62826-graphql-note-mutations.yml b/changelogs/unreleased/62826-graphql-note-mutations.yml
new file mode 100644
index 00000000000..85273186bb6
--- /dev/null
+++ b/changelogs/unreleased/62826-graphql-note-mutations.yml
@@ -0,0 +1,5 @@
+---
+title: GraphQL mutations for managing Notes
+merge_request: 30210
+author:
+type: added
diff --git a/lib/gitlab/global_id.rb b/lib/gitlab/global_id.rb
new file mode 100644
index 00000000000..cc82b6c5897
--- /dev/null
+++ b/lib/gitlab/global_id.rb
@@ -0,0 +1,14 @@
+# frozen_string_literal: true
+
+module Gitlab
+ module GlobalId
+ def self.build(object = nil, model_name: nil, id: nil, params: nil)
+ if object
+ model_name ||= object.class.name
+ id ||= object.id
+ end
+
+ ::URI::GID.build(app: GlobalID.app, model_name: model_name, model_id: id, params: params)
+ end
+ end
+end
diff --git a/spec/graphql/gitlab_schema_spec.rb b/spec/graphql/gitlab_schema_spec.rb
index 93b86b9b812..dec6b23d72a 100644
--- a/spec/graphql/gitlab_schema_spec.rb
+++ b/spec/graphql/gitlab_schema_spec.rb
@@ -143,6 +143,26 @@ describe GitlabSchema do
end
end
+ context 'for classes that are not ActiveRecord subclasses and have implemented .lazy_find' do
+ it 'returns the correct record' do
+ note = create(:discussion_note_on_merge_request)
+
+ result = described_class.object_from_id(note.to_global_id)
+
+ expect(result.__sync).to eq(note)
+ end
+
+ it 'batchloads the queries' do
+ note1 = create(:discussion_note_on_merge_request)
+ note2 = create(:discussion_note_on_merge_request)
+
+ expect do
+ [described_class.object_from_id(note1.to_global_id),
+ described_class.object_from_id(note2.to_global_id)].map(&:__sync)
+ end.not_to exceed_query_limit(1)
+ end
+ end
+
context 'for other classes' do
# We cannot use an anonymous class here as `GlobalID` expects `.name` not
# to return `nil`
diff --git a/spec/graphql/types/diff_refs_type_spec.rb b/spec/graphql/types/diff_refs_type_spec.rb
new file mode 100644
index 00000000000..91017c827ad
--- /dev/null
+++ b/spec/graphql/types/diff_refs_type_spec.rb
@@ -0,0 +1,9 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe GitlabSchema.types['DiffRefs'] do
+ it { expect(described_class.graphql_name).to eq('DiffRefs') }
+
+ it { expect(described_class).to have_graphql_fields(:base_sha, :head_sha, :start_sha) }
+end
diff --git a/spec/graphql/types/merge_request_type_spec.rb b/spec/graphql/types/merge_request_type_spec.rb
index f73bd062369..59bd0123d88 100644
--- a/spec/graphql/types/merge_request_type_spec.rb
+++ b/spec/graphql/types/merge_request_type_spec.rb
@@ -13,7 +13,7 @@ describe GitlabSchema.types['MergeRequest'] do
description_html state created_at updated_at source_project target_project
project project_id source_project_id target_project_id source_branch
target_branch work_in_progress merge_when_pipeline_succeeds diff_head_sha
- merge_commit_sha user_notes_count should_remove_source_branch
+ merge_commit_sha user_notes_count should_remove_source_branch diff_refs
force_remove_source_branch merge_status in_progress_merge_commit_sha
merge_error allow_collaboration should_be_rebased rebase_commit_sha
rebase_in_progress merge_commit_message default_merge_commit_message
diff --git a/spec/graphql/types/notes/diff_position_type_spec.rb b/spec/graphql/types/notes/diff_position_type_spec.rb
index 2f8724d7f0d..345bca8f702 100644
--- a/spec/graphql/types/notes/diff_position_type_spec.rb
+++ b/spec/graphql/types/notes/diff_position_type_spec.rb
@@ -3,7 +3,7 @@ require 'spec_helper'
describe GitlabSchema.types['DiffPosition'] do
it 'exposes the expected fields' do
- expected_fields = [:head_sha, :base_sha, :start_sha, :file_path, :old_path,
+ expected_fields = [:diff_refs, :file_path, :old_path,
:new_path, :position_type, :old_line, :new_line, :x, :y,
:width, :height]
diff --git a/spec/graphql/types/notes/discussion_type_spec.rb b/spec/graphql/types/notes/discussion_type_spec.rb
index 2a1eb0efd35..ba7fc961212 100644
--- a/spec/graphql/types/notes/discussion_type_spec.rb
+++ b/spec/graphql/types/notes/discussion_type_spec.rb
@@ -2,7 +2,7 @@
require 'spec_helper'
describe GitlabSchema.types['Discussion'] do
- it { is_expected.to have_graphql_fields(:id, :created_at, :notes) }
+ it { is_expected.to have_graphql_fields(:id, :created_at, :notes, :reply_id) }
it { is_expected.to require_graphql_authorizations(:read_note) }
end
diff --git a/spec/lib/gitlab/global_id_spec.rb b/spec/lib/gitlab/global_id_spec.rb
new file mode 100644
index 00000000000..d35b5da0b75
--- /dev/null
+++ b/spec/lib/gitlab/global_id_spec.rb
@@ -0,0 +1,37 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::GlobalId do
+ describe '.build' do
+ set(:object) { create(:issue) }
+
+ it 'returns a standard GlobalId if only object is passed' do
+ expect(described_class.build(object).to_s).to eq(object.to_global_id.to_s)
+ end
+
+ it 'returns a GlobalId from params' do
+ expect(described_class.build(model_name: 'MyModel', id: 'myid').to_s).to eq(
+ 'gid://gitlab/MyModel/myid'
+ )
+ end
+
+ it 'returns a GlobalId from object and `id` param' do
+ expect(described_class.build(object, id: 'myid').to_s).to eq(
+ 'gid://gitlab/Issue/myid'
+ )
+ end
+
+ it 'returns a GlobalId from object and `model_name` param' do
+ expect(described_class.build(object, model_name: 'MyModel').to_s).to eq(
+ "gid://gitlab/MyModel/#{object.id}"
+ )
+ end
+
+ it 'returns an error if model_name and id are not able to be determined' do
+ expect { described_class.build(id: 'myid') }.to raise_error(URI::InvalidComponentError)
+ expect { described_class.build(model_name: 'MyModel') }.to raise_error(URI::InvalidComponentError)
+ expect { described_class.build }.to raise_error(URI::InvalidComponentError)
+ end
+ end
+end
diff --git a/spec/models/discussion_spec.rb b/spec/models/discussion_spec.rb
index 22d4dab0617..950bdec4d00 100644
--- a/spec/models/discussion_spec.rb
+++ b/spec/models/discussion_spec.rb
@@ -10,6 +10,20 @@ describe Discussion do
let(:second_note) { create(:diff_note_on_merge_request, in_reply_to: first_note) }
let(:third_note) { create(:diff_note_on_merge_request) }
+ describe '.lazy_find' do
+ let!(:note1) { create(:discussion_note_on_merge_request).to_discussion }
+ let!(:note2) { create(:discussion_note_on_merge_request, in_reply_to: note1).to_discussion }
+
+ subject { [note1, note2].map { |note| described_class.lazy_find(note.discussion_id) } }
+
+ it 'batches requests' do
+ expect do
+ [described_class.lazy_find(note1.id),
+ described_class.lazy_find(note2.id)].map(&:__sync)
+ end.not_to exceed_query_limit(1)
+ end
+ end
+
describe '.build' do
it 'returns a discussion of the right type' do
discussion = described_class.build([first_note, second_note], merge_request)
diff --git a/spec/requests/api/graphql/mutations/notes/create/diff_note_spec.rb b/spec/requests/api/graphql/mutations/notes/create/diff_note_spec.rb
new file mode 100644
index 00000000000..b04fcb9aece
--- /dev/null
+++ b/spec/requests/api/graphql/mutations/notes/create/diff_note_spec.rb
@@ -0,0 +1,64 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe 'Adding a DiffNote' do
+ include GraphqlHelpers
+
+ set(:current_user) { create(:user) }
+ let(:noteable) { create(:merge_request, source_project: project, target_project: project) }
+ let(:project) { create(:project, :repository) }
+ let(:diff_refs) { noteable.diff_refs }
+ let(:mutation) do
+ variables = {
+ noteable_id: GitlabSchema.id_from_object(noteable).to_s,
+ body: 'Body text',
+ position: {
+ paths: {
+ old_path: 'files/ruby/popen.rb',
+ new_path: 'files/ruby/popen2.rb'
+ },
+ new_line: 14,
+ base_sha: diff_refs.base_sha,
+ head_sha: diff_refs.head_sha,
+ start_sha: diff_refs.start_sha
+ }
+ }
+
+ graphql_mutation(:create_diff_note, variables)
+ end
+
+ def mutation_response
+ graphql_mutation_response(:create_diff_note)
+ end
+
+ it_behaves_like 'a Note mutation when the user does not have permission'
+
+ context 'when the user has permission' do
+ before do
+ project.add_developer(current_user)
+ end
+
+ it_behaves_like 'a Note mutation that creates a Note'
+
+ it_behaves_like 'a Note mutation when there are active record validation errors', model: DiffNote
+
+ context do
+ let(:diff_refs) { build(:merge_request).diff_refs } # Allow fake diff refs so arguments are valid
+
+ it_behaves_like 'a Note mutation when the given resource id is not for a Noteable'
+ end
+
+ it 'returns the note with the correct position' do
+ post_graphql_mutation(mutation, current_user: current_user)
+
+ expect(mutation_response['note']['body']).to eq('Body text')
+ mutation_position_response = mutation_response['note']['position']
+ expect(mutation_position_response['positionType']).to eq('text')
+ expect(mutation_position_response['filePath']).to eq('files/ruby/popen2.rb')
+ expect(mutation_position_response['oldPath']).to eq('files/ruby/popen.rb')
+ expect(mutation_position_response['newPath']).to eq('files/ruby/popen2.rb')
+ expect(mutation_position_response['newLine']).to eq(14)
+ end
+ end
+end
diff --git a/spec/requests/api/graphql/mutations/notes/create/image_diff_note_spec.rb b/spec/requests/api/graphql/mutations/notes/create/image_diff_note_spec.rb
new file mode 100644
index 00000000000..3ba6c689024
--- /dev/null
+++ b/spec/requests/api/graphql/mutations/notes/create/image_diff_note_spec.rb
@@ -0,0 +1,70 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe 'Adding an image DiffNote' do
+ include GraphqlHelpers
+
+ set(:current_user) { create(:user) }
+ let(:noteable) { create(:merge_request, source_project: project, target_project: project) }
+ let(:project) { create(:project, :repository) }
+ let(:diff_refs) { noteable.diff_refs }
+ let(:mutation) do
+ variables = {
+ noteable_id: GitlabSchema.id_from_object(noteable).to_s,
+ body: 'Body text',
+ position: {
+ paths: {
+ old_path: 'files/images/any_image.png',
+ new_path: 'files/images/any_image2.png'
+ },
+ width: 100,
+ height: 200,
+ x: 1,
+ y: 2,
+ base_sha: diff_refs.base_sha,
+ head_sha: diff_refs.head_sha,
+ start_sha: diff_refs.start_sha
+ }
+ }
+
+ graphql_mutation(:create_image_diff_note, variables)
+ end
+
+ def mutation_response
+ graphql_mutation_response(:create_image_diff_note)
+ end
+
+ it_behaves_like 'a Note mutation when the user does not have permission'
+
+ context 'when the user has permission' do
+ before do
+ project.add_developer(current_user)
+ end
+
+ it_behaves_like 'a Note mutation that creates a Note'
+
+ it_behaves_like 'a Note mutation when there are active record validation errors', model: DiffNote
+
+ context do
+ let(:diff_refs) { build(:merge_request).diff_refs } # Allow fake diff refs so arguments are valid
+
+ it_behaves_like 'a Note mutation when the given resource id is not for a Noteable'
+ end
+
+ it 'returns the note with the correct position' do
+ post_graphql_mutation(mutation, current_user: current_user)
+
+ expect(mutation_response['note']['body']).to eq('Body text')
+ mutation_position_response = mutation_response['note']['position']
+ expect(mutation_position_response['filePath']).to eq('files/images/any_image2.png')
+ expect(mutation_position_response['oldPath']).to eq('files/images/any_image.png')
+ expect(mutation_position_response['newPath']).to eq('files/images/any_image2.png')
+ expect(mutation_position_response['positionType']).to eq('image')
+ expect(mutation_position_response['width']).to eq(100)
+ expect(mutation_position_response['height']).to eq(200)
+ expect(mutation_position_response['x']).to eq(1)
+ expect(mutation_position_response['y']).to eq(2)
+ end
+ end
+end
diff --git a/spec/requests/api/graphql/mutations/notes/create/note_spec.rb b/spec/requests/api/graphql/mutations/notes/create/note_spec.rb
new file mode 100644
index 00000000000..14aaa430ac9
--- /dev/null
+++ b/spec/requests/api/graphql/mutations/notes/create/note_spec.rb
@@ -0,0 +1,64 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe 'Adding a Note' do
+ include GraphqlHelpers
+
+ set(:current_user) { create(:user) }
+ let(:noteable) { create(:merge_request, source_project: project, target_project: project) }
+ let(:project) { create(:project) }
+ let(:discussion) { nil }
+ let(:mutation) do
+ variables = {
+ noteable_id: GitlabSchema.id_from_object(noteable).to_s,
+ discussion_id: (GitlabSchema.id_from_object(discussion).to_s if discussion),
+ body: 'Body text'
+ }
+
+ graphql_mutation(:create_note, variables)
+ end
+
+ def mutation_response
+ graphql_mutation_response(:create_note)
+ end
+
+ it_behaves_like 'a Note mutation when the user does not have permission'
+
+ context 'when the user has permission' do
+ before do
+ project.add_developer(current_user)
+ end
+
+ it_behaves_like 'a Note mutation that creates a Note'
+
+ it_behaves_like 'a Note mutation when there are active record validation errors'
+
+ it_behaves_like 'a Note mutation when the given resource id is not for a Noteable'
+
+ it 'returns the note' do
+ post_graphql_mutation(mutation, current_user: current_user)
+
+ expect(mutation_response['note']['body']).to eq('Body text')
+ end
+
+ describe 'creating Notes in reply to a discussion' do
+ context 'when the user does not have permission to create notes on the discussion' do
+ let(:discussion) { create(:discussion_note).to_discussion }
+
+ it_behaves_like 'a mutation that returns top-level errors',
+ errors: ["The discussion does not exist or you don't have permission to perform this action"]
+ end
+
+ context 'when the user has permission to create notes on the discussion' do
+ let(:discussion) { create(:discussion_note, project: project).to_discussion }
+
+ it 'creates a Note in a discussion' do
+ post_graphql_mutation(mutation, current_user: current_user)
+
+ expect(mutation_response['note']['discussion']['id']).to eq(discussion.to_global_id.to_s)
+ end
+ end
+ end
+ end
+end
diff --git a/spec/requests/api/graphql/mutations/notes/destroy_spec.rb b/spec/requests/api/graphql/mutations/notes/destroy_spec.rb
new file mode 100644
index 00000000000..337a6e6f6e6
--- /dev/null
+++ b/spec/requests/api/graphql/mutations/notes/destroy_spec.rb
@@ -0,0 +1,52 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe 'Destroying a Note' do
+ include GraphqlHelpers
+
+ let!(:note) { create(:note) }
+ let(:mutation) do
+ variables = {
+ id: GitlabSchema.id_from_object(note).to_s
+ }
+
+ graphql_mutation(:destroy_note, variables)
+ end
+
+ def mutation_response
+ graphql_mutation_response(:destroy_note)
+ end
+
+ context 'when the user does not have permission' do
+ let(: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 destroy the Note' do
+ expect do
+ post_graphql_mutation(mutation, current_user: current_user)
+ end.not_to change { Note.count }
+ end
+ end
+
+ context 'when the user has permission' do
+ let(:current_user) { note.author }
+
+ it_behaves_like 'a Note mutation when the given resource id is not for a Note'
+
+ it 'destroys the Note' do
+ expect do
+ post_graphql_mutation(mutation, current_user: current_user)
+ end.to change { Note.count }.by(-1)
+ end
+
+ it 'returns an empty Note' do
+ post_graphql_mutation(mutation, current_user: current_user)
+
+ expect(mutation_response).to have_key('note')
+ expect(mutation_response['note']).to be_nil
+ end
+ end
+end
diff --git a/spec/requests/api/graphql/mutations/notes/update_spec.rb b/spec/requests/api/graphql/mutations/notes/update_spec.rb
new file mode 100644
index 00000000000..958f640995a
--- /dev/null
+++ b/spec/requests/api/graphql/mutations/notes/update_spec.rb
@@ -0,0 +1,72 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe 'Updating a Note' do
+ include GraphqlHelpers
+
+ let!(:note) { create(:note, note: original_body) }
+ let(:original_body) { 'Initial body text' }
+ let(:updated_body) { 'Updated body text' }
+ let(:mutation) do
+ variables = {
+ id: GitlabSchema.id_from_object(note).to_s,
+ body: updated_body
+ }
+
+ graphql_mutation(:update_note, variables)
+ end
+
+ def mutation_response
+ graphql_mutation_response(:update_note)
+ end
+
+ context 'when the user does not have permission' do
+ let(: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 Note' do
+ post_graphql_mutation(mutation, current_user: current_user)
+
+ expect(note.reload.note).to eq(original_body)
+ end
+ end
+
+ context 'when the user has permission' do
+ let(:current_user) { note.author }
+
+ it_behaves_like 'a Note mutation when the given resource id is not for a Note'
+
+ it 'updates the Note' do
+ post_graphql_mutation(mutation, current_user: current_user)
+
+ expect(note.reload.note).to eq(updated_body)
+ end
+
+ it 'returns the updated Note' do
+ post_graphql_mutation(mutation, current_user: current_user)
+
+ expect(mutation_response['note']['body']).to eq(updated_body)
+ end
+
+ context 'when there are ActiveRecord validation errors' do
+ let(:updated_body) { '' }
+
+ it_behaves_like 'a mutation that returns errors in the response', errors: ["Note can't be blank"]
+
+ it 'does not update the Note' do
+ post_graphql_mutation(mutation, current_user: current_user)
+
+ expect(note.reload.note).to eq(original_body)
+ end
+
+ it 'returns the Note with its original body' do
+ post_graphql_mutation(mutation, current_user: current_user)
+
+ expect(mutation_response['note']['body']).to eq(original_body)
+ end
+ end
+ end
+end
diff --git a/spec/support/helpers/graphql_helpers.rb b/spec/support/helpers/graphql_helpers.rb
index 1a09d48f4cd..ec3c460cd37 100644
--- a/spec/support/helpers/graphql_helpers.rb
+++ b/spec/support/helpers/graphql_helpers.rb
@@ -57,7 +57,8 @@ module GraphqlHelpers
end
def variables_for_mutation(name, input)
- graphql_input = input.map { |name, value| [GraphqlHelpers.fieldnamerize(name), value] }.to_h
+ graphql_input = prepare_input_for_mutation(input)
+
result = { input_variable_name_for_mutation(name) => graphql_input }
# Avoid trying to serialize multipart data into JSON
@@ -68,6 +69,18 @@ module GraphqlHelpers
end
end
+ # Recursively convert a Hash with Ruby-style keys to GraphQL fieldname-style keys
+ #
+ # prepare_input_for_mutation({ 'my_key' => 1 })
+ # => { 'myKey' => 1}
+ def prepare_input_for_mutation(input)
+ input.map do |name, value|
+ value = prepare_input_for_mutation(value) if value.is_a?(Hash)
+
+ [GraphqlHelpers.fieldnamerize(name), value]
+ end.to_h
+ end
+
def input_variable_name_for_mutation(mutation_name)
mutation_name = GraphqlHelpers.fieldnamerize(mutation_name)
mutation_field = GitlabSchema.mutation.fields[mutation_name]
diff --git a/spec/support/shared_examples/graphql/notes_creation_shared_examples.rb b/spec/support/shared_examples/graphql/notes_creation_shared_examples.rb
new file mode 100644
index 00000000000..f2e1a95345b
--- /dev/null
+++ b/spec/support/shared_examples/graphql/notes_creation_shared_examples.rb
@@ -0,0 +1,61 @@
+# frozen_string_literal: true
+
+RSpec.shared_examples 'a Note mutation that does not create a Note' do
+ it do
+ expect do
+ post_graphql_mutation(mutation, current_user: current_user)
+ end.not_to change { Note.count }
+ end
+end
+
+RSpec.shared_examples 'a Note mutation that creates a Note' do
+ it do
+ expect do
+ post_graphql_mutation(mutation, current_user: current_user)
+ end.to change { Note.count }.by(1)
+ end
+end
+
+RSpec.shared_examples 'a Note mutation when the user does not have permission' do
+ it_behaves_like 'a Note mutation that does not create a Note'
+
+ 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']
+end
+
+RSpec.shared_examples 'a Note mutation when there are active record validation errors' do |model: Note|
+ before do
+ expect_next_instance_of(model) do |note|
+ expect(note).to receive(:valid?).at_least(:once).and_return(false)
+ expect(note).to receive_message_chain(
+ :errors,
+ :full_messages
+ ).and_return(['Error 1', 'Error 2'])
+ end
+ end
+
+ it_behaves_like 'a Note mutation that does not create a Note'
+
+ it_behaves_like 'a mutation that returns errors in the response', errors: ['Error 1', 'Error 2']
+
+ it 'returns an empty Note' do
+ post_graphql_mutation(mutation, current_user: current_user)
+
+ expect(mutation_response).to have_key('note')
+ expect(mutation_response['note']).to be_nil
+ end
+end
+
+RSpec.shared_examples 'a Note mutation when the given resource id is not for a Noteable' do
+ let(:noteable) { create(:label, project: project) }
+
+ it_behaves_like 'a Note mutation that does not create a Note'
+
+ it_behaves_like 'a mutation that returns top-level errors', errors: ['Cannot add notes to this resource']
+end
+
+RSpec.shared_examples 'a Note mutation when the given resource id is not for a Note' do
+ let(:note) { create(:issue) }
+
+ it_behaves_like 'a mutation that returns top-level errors', errors: ['Resource is not a note']
+end