diff options
author | Bob Van Landuyt <bob@vanlanduyt.co> | 2019-06-07 19:13:26 +0200 |
---|---|---|
committer | Bob Van Landuyt <bob@vanlanduyt.co> | 2019-06-14 12:36:27 +0200 |
commit | b6ff5f1e141162e701c33647aae5015e5d42cc11 (patch) | |
tree | fb6ec57e96dc811d07c75d6b32f9471263c85166 | |
parent | 8934ddbb47d24dac937351588bc28551bd7654e7 (diff) | |
download | gitlab-ce-b6ff5f1e141162e701c33647aae5015e5d42cc11.tar.gz |
Expose comments on Noteables in GraphQL
This exposes `Note`s on Issues & MergeRequests using a
`Types::Notes::NoteableType` in GraphQL.
Exposing notes on a new type can be done by implementing the
`NoteableType` interface on the type. The presented object should
be a `Noteable`.
27 files changed, 469 insertions, 12 deletions
diff --git a/app/graphql/gitlab_schema.rb b/app/graphql/gitlab_schema.rb index 2e5bdbd79c8..5615909c4ec 100644 --- a/app/graphql/gitlab_schema.rb +++ b/app/graphql/gitlab_schema.rb @@ -7,8 +7,8 @@ class GitlabSchema < GraphQL::Schema AUTHENTICATED_COMPLEXITY = 250 ADMIN_COMPLEXITY = 300 - DEFAULT_MAX_DEPTH = 10 - AUTHENTICATED_MAX_DEPTH = 15 + DEFAULT_MAX_DEPTH = 15 + AUTHENTICATED_MAX_DEPTH = 20 use BatchLoader::GraphQL use Gitlab::Graphql::Authorize diff --git a/app/graphql/types/issue_type.rb b/app/graphql/types/issue_type.rb index dd5133189dc..c762aa69e43 100644 --- a/app/graphql/types/issue_type.rb +++ b/app/graphql/types/issue_type.rb @@ -4,6 +4,8 @@ module Types class IssueType < BaseObject graphql_name 'Issue' + implements(Types::Notes::NoteableType) + authorize :read_issue expose_permissions Types::PermissionTypes::Issue diff --git a/app/graphql/types/merge_request_type.rb b/app/graphql/types/merge_request_type.rb index 85ac3102442..662503d447b 100644 --- a/app/graphql/types/merge_request_type.rb +++ b/app/graphql/types/merge_request_type.rb @@ -4,6 +4,8 @@ module Types class MergeRequestType < BaseObject graphql_name 'MergeRequest' + implements(Types::Notes::NoteableType) + authorize :read_merge_request expose_permissions Types::PermissionTypes::MergeRequest diff --git a/app/graphql/types/notes/diff_position_type.rb b/app/graphql/types/notes/diff_position_type.rb new file mode 100644 index 00000000000..104ccb79bbb --- /dev/null +++ b/app/graphql/types/notes/diff_position_type.rb @@ -0,0 +1,46 @@ +# frozen_string_literal: true + +module Types + module Notes + 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 :file_path, GraphQL::STRING_TYPE, null: false, + description: "The path of the file that was changed" + field :old_path, GraphQL::STRING_TYPE, null: true, + description: "The path of the file on the start sha." + field :new_path, GraphQL::STRING_TYPE, null: true, + description: "The path of the file on the head sha." + field :position_type, Types::Notes::PositionTypeEnum, null: false + + # Fields for text positions + field :old_line, GraphQL::INT_TYPE, null: true, + description: "The line on start sha that was changed", + resolve: -> (position, _args, _ctx) { position.old_line if position.on_text? } + field :new_line, GraphQL::INT_TYPE, null: true, + description: "The line on head sha that was changed", + resolve: -> (position, _args, _ctx) { position.new_line if position.on_text? } + + # Fields for image positions + field :x, GraphQL::INT_TYPE, null: true, + description: "The X postion on which the comment was made", + resolve: -> (position, _args, _ctx) { position.x if position.on_image? } + field :y, GraphQL::INT_TYPE, null: true, + description: "The Y position on which the comment was made", + resolve: -> (position, _args, _ctx) { position.y if position.on_image? } + field :width, GraphQL::INT_TYPE, null: true, + description: "The total width of the image", + resolve: -> (position, _args, _ctx) { position.width if position.on_image? } + field :height, GraphQL::INT_TYPE, null: true, + description: "The total height of the image", + resolve: -> (position, _args, _ctx) { position.height if position.on_image? } + end + end +end diff --git a/app/graphql/types/notes/discussion_type.rb b/app/graphql/types/notes/discussion_type.rb new file mode 100644 index 00000000000..c4691942f2d --- /dev/null +++ b/app/graphql/types/notes/discussion_type.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +module Types + module Notes + class DiscussionType < BaseObject + graphql_name 'Discussion' + + authorize :read_note + + field :id, GraphQL::ID_TYPE, null: false + field :created_at, Types::TimeType, null: false + field :notes, Types::Notes::NoteType.connection_type, null: false, description: "All notes in the discussion" + end + end +end diff --git a/app/graphql/types/notes/note_type.rb b/app/graphql/types/notes/note_type.rb new file mode 100644 index 00000000000..85c55d16ac2 --- /dev/null +++ b/app/graphql/types/notes/note_type.rb @@ -0,0 +1,46 @@ +# frozen_string_literal: true + +module Types + module Notes + class NoteType < BaseObject + graphql_name 'Note' + + authorize :read_note + + expose_permissions Types::PermissionTypes::Note + + field :id, GraphQL::ID_TYPE, null: false + + field :project, Types::ProjectType, + null: true, + description: "The project this note is associated to", + resolve: -> (note, args, context) { Gitlab::Graphql::Loaders::BatchModelLoader.new(Project, note.project_id).find } + + field :author, Types::UserType, + null: false, + description: "The user who wrote this note", + resolve: -> (note, args, context) { Gitlab::Graphql::Loaders::BatchModelLoader.new(User, note.author_id).find } + + field :resolved_by, Types::UserType, + null: true, + description: "The user that resolved the discussion", + resolve: -> (note, _args, _context) { Gitlab::Graphql::Loaders::BatchModelLoader.new(User, note.resolved_by_id).find } + + field :system, GraphQL::BOOLEAN_TYPE, + null: false, + description: "Whether or not this note was created by the system or by a user" + + field :body, GraphQL::STRING_TYPE, + null: false, + method: :note, + description: "The content note itself" + + field :created_at, Types::TimeType, null: false + field :updated_at, Types::TimeType, null: false + field :discussion, Types::Notes::DiscussionType, null: true, description: "The discussion this note is a part of" + field :resolvable, GraphQL::BOOLEAN_TYPE, null: false, method: :resolvable? + field :resolved_at, Types::TimeType, null: true, description: "The time the discussion was resolved" + field :position, Types::Notes::DiffPositionType, null: true, description: "The position of this note on a diff" + end + end +end diff --git a/app/graphql/types/notes/noteable_type.rb b/app/graphql/types/notes/noteable_type.rb new file mode 100644 index 00000000000..9f126d67b0d --- /dev/null +++ b/app/graphql/types/notes/noteable_type.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +module Types + module Notes + module NoteableType + include Types::BaseInterface + + field :notes, Types::Notes::NoteType.connection_type, null: false, description: "All notes on this noteable" + field :discussions, Types::Notes::DiscussionType.connection_type, null: false, description: "All discussions on this noteable" + + definition_methods do + def resolve_type(object, context) + case object + when Issue + Types::IssueType + when MergeRequest + Types::MergeRequestType + else + raise "Unknown GraphQL type for #{object}" + end + end + end + end + end +end diff --git a/app/graphql/types/notes/position_type_enum.rb b/app/graphql/types/notes/position_type_enum.rb new file mode 100644 index 00000000000..abdb2cfc804 --- /dev/null +++ b/app/graphql/types/notes/position_type_enum.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +module Types + module Notes + class PositionTypeEnum < BaseEnum + graphql_name 'DiffPositionType' + description 'Type of file the position refers to' + + value 'text' + value 'image' + end + end +end diff --git a/app/graphql/types/permission_types/note.rb b/app/graphql/types/permission_types/note.rb new file mode 100644 index 00000000000..a585d3daaa8 --- /dev/null +++ b/app/graphql/types/permission_types/note.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +module Types + module PermissionTypes + class Note < BasePermissionType + graphql_name 'NotePermissions' + + abilities :read_note, :create_note, :admin_note, :resolve_note, :award_emoji + end + end +end diff --git a/app/models/concerns/diff_positionable_note.rb b/app/models/concerns/diff_positionable_note.rb index b61bf29e6ad..2d09eff0111 100644 --- a/app/models/concerns/diff_positionable_note.rb +++ b/app/models/concerns/diff_positionable_note.rb @@ -3,6 +3,7 @@ module DiffPositionableNote extend ActiveSupport::Concern included do + delegate :on_text?, :on_image?, to: :position, allow_nil: true before_validation :set_original_position, on: :create before_validation :update_position, on: :create, if: :on_text? @@ -28,14 +29,6 @@ module DiffPositionableNote end end - def on_text? - position&.position_type == "text" - end - - def on_image? - position&.position_type == "image" - end - def supported? for_commit? || self.noteable.has_complete_diff_refs? end diff --git a/app/models/discussion.rb b/app/models/discussion.rb index 32529ebf71d..ae13cdfd85f 100644 --- a/app/models/discussion.rb +++ b/app/models/discussion.rb @@ -4,6 +4,7 @@ # # A discussion of this type can be resolvable. class Discussion + include GlobalID::Identification include ResolvableDiscussion attr_reader :notes, :context_noteable @@ -11,14 +12,19 @@ class Discussion delegate :created_at, :project, :author, - :noteable, :commit_id, :for_commit?, :for_merge_request?, + :to_ability_name, + :editable?, to: :first_note + def declarative_policy_delegate + first_note + end + def project_id project&.id end diff --git a/app/models/note.rb b/app/models/note.rb index 081d6f91230..15271c68a9e 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -342,7 +342,7 @@ class Note < ApplicationRecord end def to_ability_name - for_snippet? ? noteable.class.name.underscore : noteable_type.underscore + for_snippet? ? noteable.class.name.underscore : noteable_type.demodulize.underscore end def can_be_discussion_note? diff --git a/changelogs/unreleased/bvl-comments-graphql.yml b/changelogs/unreleased/bvl-comments-graphql.yml new file mode 100644 index 00000000000..9f510a910a3 --- /dev/null +++ b/changelogs/unreleased/bvl-comments-graphql.yml @@ -0,0 +1,5 @@ +--- +title: Expose notes and discussions in GraphQL +merge_request: 29212 +author: +type: added diff --git a/lib/gitlab/diff/position.rb b/lib/gitlab/diff/position.rb index e8f98f52111..d349c378e53 100644 --- a/lib/gitlab/diff/position.rb +++ b/lib/gitlab/diff/position.rb @@ -134,6 +134,14 @@ module Gitlab @line_code ||= diff_file(repository)&.line_code_for_position(self) end + def on_image? + position_type == 'image' + end + + def on_text? + position_type == 'text' + end + private def find_diff_file(repository) diff --git a/spec/graphql/types/issue_type_spec.rb b/spec/graphql/types/issue_type_spec.rb index bae560829cc..210932f8488 100644 --- a/spec/graphql/types/issue_type_spec.rb +++ b/spec/graphql/types/issue_type_spec.rb @@ -7,6 +7,8 @@ describe GitlabSchema.types['Issue'] do it { expect(described_class).to require_graphql_authorizations(:read_issue) } + it { expect(described_class.interfaces).to include(Types::Notes::NoteableType.to_graphql) } + it 'has specific fields' do %i[relative_position web_path web_url reference].each do |field_name| expect(described_class).to have_graphql_field(field_name) diff --git a/spec/graphql/types/merge_request_type_spec.rb b/spec/graphql/types/merge_request_type_spec.rb index 89c12879074..fd1c782bcc5 100644 --- a/spec/graphql/types/merge_request_type_spec.rb +++ b/spec/graphql/types/merge_request_type_spec.rb @@ -5,6 +5,8 @@ describe GitlabSchema.types['MergeRequest'] do it { expect(described_class).to require_graphql_authorizations(:read_merge_request) } + it { expect(described_class.interfaces).to include(Types::Notes::NoteableType.to_graphql) } + describe 'nested head pipeline' do it { expect(described_class).to have_graphql_field(:head_pipeline) } end diff --git a/spec/graphql/types/notes/diff_position_type_spec.rb b/spec/graphql/types/notes/diff_position_type_spec.rb new file mode 100644 index 00000000000..2f8724d7f0d --- /dev/null +++ b/spec/graphql/types/notes/diff_position_type_spec.rb @@ -0,0 +1,12 @@ +# frozen_string_literal: true +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, + :new_path, :position_type, :old_line, :new_line, :x, :y, + :width, :height] + + is_expected.to have_graphql_field(*expected_fields) + end +end diff --git a/spec/graphql/types/notes/discussion_type_spec.rb b/spec/graphql/types/notes/discussion_type_spec.rb new file mode 100644 index 00000000000..2a1eb0efd35 --- /dev/null +++ b/spec/graphql/types/notes/discussion_type_spec.rb @@ -0,0 +1,8 @@ +# frozen_string_literal: true +require 'spec_helper' + +describe GitlabSchema.types['Discussion'] do + it { is_expected.to have_graphql_fields(:id, :created_at, :notes) } + + it { is_expected.to require_graphql_authorizations(:read_note) } +end diff --git a/spec/graphql/types/notes/note_type_spec.rb b/spec/graphql/types/notes/note_type_spec.rb new file mode 100644 index 00000000000..8022b20f9dd --- /dev/null +++ b/spec/graphql/types/notes/note_type_spec.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true +require 'spec_helper' + +describe GitlabSchema.types['Note'] do + it 'exposes the expected fields' do + expected_fields = [:id, :project, :author, :body, :created_at, + :updated_at, :discussion, :resolvable, :position, :user_permissions, + :resolved_by, :resolved_at, :system] + + is_expected.to have_graphql_fields(*expected_fields) + end + + it { is_expected.to expose_permissions_using(Types::PermissionTypes::Note) } + it { is_expected.to require_graphql_authorizations(:read_note) } +end diff --git a/spec/graphql/types/notes/noteable_type_spec.rb b/spec/graphql/types/notes/noteable_type_spec.rb new file mode 100644 index 00000000000..d10c79b5344 --- /dev/null +++ b/spec/graphql/types/notes/noteable_type_spec.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true +require 'spec_helper' + +describe Types::Notes::NoteableType do + it { is_expected.to have_graphql_fields(:notes, :discussions) } + + describe ".resolve_type" do + it 'knows the correct type for objects' do + expect(described_class.resolve_type(build(:issue), {})).to eq(Types::IssueType) + expect(described_class.resolve_type(build(:merge_request), {})).to eq(Types::MergeRequestType) + end + end +end diff --git a/spec/graphql/types/permission_types/note_spec.rb b/spec/graphql/types/permission_types/note_spec.rb new file mode 100644 index 00000000000..32d56eb1f7a --- /dev/null +++ b/spec/graphql/types/permission_types/note_spec.rb @@ -0,0 +1,11 @@ +require 'spec_helper' + +describe GitlabSchema.types['NotePermissions'] do + it 'has the expected fields' do + expected_permissions = [ + :read_note, :create_note, :admin_note, :resolve_note, :award_emoji + ] + + is_expected.to have_graphql_fields(expected_permissions) + end +end diff --git a/spec/lib/gitlab/diff/position_spec.rb b/spec/lib/gitlab/diff/position_spec.rb index cc4faf6f10b..aea02d21048 100644 --- a/spec/lib/gitlab/diff/position_spec.rb +++ b/spec/lib/gitlab/diff/position_spec.rb @@ -46,6 +46,9 @@ describe Gitlab::Diff::Position do ) end + it { is_expected.to be_on_text } + it { is_expected.not_to be_on_image } + describe "#diff_file" do it "returns the correct diff file" do diff_file = subject.diff_file(project.repository) @@ -91,6 +94,9 @@ describe Gitlab::Diff::Position do ) end + it { is_expected.not_to be_on_text } + it { is_expected.to be_on_image } + it "returns the correct diff file" do diff_file = subject.diff_file(project.repository) diff --git a/spec/models/discussion_spec.rb b/spec/models/discussion_spec.rb index 0d02165787a..22d4dab0617 100644 --- a/spec/models/discussion_spec.rb +++ b/spec/models/discussion_spec.rb @@ -29,4 +29,12 @@ describe Discussion do ]) end end + + describe 'authorization' do + it 'delegates to the first note' do + policy = DeclarativePolicy.policy_for(instance_double(User, id: 1), subject) + + expect(policy).to be_a(NotePolicy) + end + end end diff --git a/spec/policies/note_policy_spec.rb b/spec/policies/note_policy_spec.rb index 4be7a0266d1..bcf021f1dfd 100644 --- a/spec/policies/note_policy_spec.rb +++ b/spec/policies/note_policy_spec.rb @@ -133,6 +133,25 @@ describe NotePolicy do end end end + + context 'for discussions' do + let(:policy) { described_class.new(user, note.discussion) } + + it 'allows the author to manage the discussion' do + expect(policy).to be_allowed(:admin_note) + expect(policy).to be_allowed(:resolve_note) + expect(policy).to be_allowed(:read_note) + expect(policy).to be_allowed(:award_emoji) + end + + context 'when the user does not have access to the noteable' do + before do + noteable.update_attribute(:confidential, true) + end + + it_behaves_like 'a discussion with a private noteable' + end + end end end end diff --git a/spec/requests/api/graphql/project/issue/notes_spec.rb b/spec/requests/api/graphql/project/issue/notes_spec.rb new file mode 100644 index 00000000000..bfc89434370 --- /dev/null +++ b/spec/requests/api/graphql/project/issue/notes_spec.rb @@ -0,0 +1,24 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe 'getting notes for an issue' do + include GraphqlHelpers + + let(:noteable) { create(:issue) } + let(:noteable_data) { graphql_data['project']['issue'] } + + def noteable_query(noteable_fields) + <<~QRY + { + project(fullPath: "#{noteable.project.full_path}") { + issue(iid: "#{noteable.iid}") { + #{noteable_fields} + } + } + } + QRY + end + + it_behaves_like 'exposing regular notes on a noteable in GraphQL' +end diff --git a/spec/requests/api/graphql/project/merge_request/diff_notes_spec.rb b/spec/requests/api/graphql/project/merge_request/diff_notes_spec.rb new file mode 100644 index 00000000000..e260e4463f4 --- /dev/null +++ b/spec/requests/api/graphql/project/merge_request/diff_notes_spec.rb @@ -0,0 +1,90 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe 'getting notes for a merge request' do + include GraphqlHelpers + + let(:noteable) { create(:merge_request) } + + def noteable_query(noteable_fields) + <<~QRY + { + project(fullPath: "#{noteable.project.full_path}") { + id + mergeRequest(iid: "#{noteable.iid}") { + #{noteable_fields} + } + } + } + QRY + end + let(:noteable_data) { graphql_data['project']['mergeRequest'] } + + it_behaves_like "exposing regular notes on a noteable in GraphQL" + + context 'diff notes on a merge request' do + let(:project) { noteable.project } + let!(:note) { create(:diff_note_on_merge_request, noteable: noteable, project: project) } + let(:user) { note.author } + + let(:query) do + noteable_query( + <<~NOTES + notes { + edges { + node { + #{all_graphql_fields_for('Note')} + } + } + } + NOTES + ) + end + + it_behaves_like 'a working graphql query' do + before do + post_graphql(query, current_user: user) + end + end + + it 'includes the note' do + post_graphql(query, current_user: user) + + expect(graphql_data['project']['mergeRequest']['notes']['edges'].last['node']['body']) + .to eq(note.note) + end + + context 'the position of the diffnote' do + it 'includes a correct position' do + post_graphql(query, current_user: user) + + note_data = noteable_data['notes']['edges'].last['node'] + + expect(note_data['position']['positionType']).to eq('text') + expect(note_data['position']['newLine']).to be_present + expect(note_data['position']['x']).not_to be_present + expect(note_data['position']['y']).not_to be_present + expect(note_data['position']['width']).not_to be_present + expect(note_data['position']['height']).not_to be_present + end + + context 'with a note on an image' do + let(:note) { create(:image_diff_note_on_merge_request, noteable: noteable, project: project) } + + it 'includes a correct position' do + post_graphql(query, current_user: user) + + note_data = noteable_data['notes']['edges'].last['node'] + + expect(note_data['position']['positionType']).to eq('image') + expect(note_data['position']['x']).to be_present + expect(note_data['position']['y']).to be_present + expect(note_data['position']['width']).to be_present + expect(note_data['position']['height']).to be_present + expect(note_data['position']['newLine']).not_to be_present + end + end + end + end +end diff --git a/spec/support/shared_examples/graphql/notes_on_noteables_shared_examples.rb b/spec/support/shared_examples/graphql/notes_on_noteables_shared_examples.rb new file mode 100644 index 00000000000..323d1c51ffd --- /dev/null +++ b/spec/support/shared_examples/graphql/notes_on_noteables_shared_examples.rb @@ -0,0 +1,75 @@ +# frozen_string_literal: true +require 'spec_helper' + +shared_context 'exposing regular notes on a noteable in GraphQL' do + include GraphqlHelpers + + let(:note) do + create(:note, + noteable: noteable, + project: (noteable.project if noteable.respond_to?(:project))) + end + let(:user) { note.author } + + context 'for regular notes' do + let(:query) do + note_fields = <<~NOTES + notes { + edges { + node { + #{all_graphql_fields_for('Note')} + } + } + } + NOTES + + noteable_query(note_fields) + end + + it_behaves_like 'a working graphql query' do + before do + post_graphql(query, current_user: user) + end + end + + it 'includes the note' do + post_graphql(query, current_user: user) + + expect(noteable_data['notes']['edges'].first['node']['body']) + .to eq(note.note) + end + end + + context "for discussions" do + let(:query) do + discussion_fields = <<~DISCUSSIONS + discussions { + edges { + node { + #{all_graphql_fields_for('Discussion')} + } + } + } + DISCUSSIONS + + noteable_query(discussion_fields) + end + + let!(:reply) { create(:note, noteable: noteable, in_reply_to: note, discussion_id: note.discussion_id) } + + it_behaves_like 'a working graphql query' do + before do + post_graphql(query, current_user: user) + end + end + + it 'includes all discussion notes' do + post_graphql(query, current_user: user) + + discussion = noteable_data['discussions']['edges'].first['node'] + ids = discussion['notes']['edges'].map { |note_edge| note_edge['node']['id'] } + + expect(ids).to eq([note.to_global_id.to_s, reply.to_global_id.to_s]) + end + end +end |