Commit bdc20840 authored by Douwe Maan's avatar Douwe Maan

Merge branch 'jprovazn-commit-notes-api' into 'master'

Merge request and commit discussions API

Closes #20901

See merge request gitlab-org/gitlab-ce!18537
parents 3fcb9c11 7a76caa5
...@@ -33,9 +33,7 @@ class Projects::NotesController < Projects::ApplicationController ...@@ -33,9 +33,7 @@ class Projects::NotesController < Projects::ApplicationController
def resolve def resolve
return render_404 unless note.resolvable? return render_404 unless note.resolvable?
note.resolve!(current_user) Notes::ResolveService.new(project, current_user).execute(note)
MergeRequests::ResolvedDiscussionNotificationService.new(project, current_user).execute(note.noteable)
discussion = note.discussion discussion = note.discussion
......
...@@ -105,6 +105,10 @@ class Commit ...@@ -105,6 +105,10 @@ class Commit
end end
end end
end end
def parent_class
::Project
end
end end
attr_accessor :raw attr_accessor :raw
......
module Notes
class ResolveService < ::BaseService
def execute(note)
note.resolve!(current_user)
::MergeRequests::ResolvedDiscussionNotificationService.new(project, current_user).execute(note.noteable)
end
end
end
---
title: Add discussion API for merge requests and commits
merge_request:
author:
type: added
This diff is collapsed.
...@@ -39,7 +39,8 @@ GET /projects/:id/issues/:issue_iid/notes?sort=asc&order_by=updated_at ...@@ -39,7 +39,8 @@ GET /projects/:id/issues/:issue_iid/notes?sort=asc&order_by=updated_at
"system": true, "system": true,
"noteable_id": 377, "noteable_id": 377,
"noteable_type": "Issue", "noteable_type": "Issue",
"noteable_iid": 377 "noteable_iid": 377,
"resolvable": false
}, },
{ {
"id": 305, "id": 305,
...@@ -58,7 +59,8 @@ GET /projects/:id/issues/:issue_iid/notes?sort=asc&order_by=updated_at ...@@ -58,7 +59,8 @@ GET /projects/:id/issues/:issue_iid/notes?sort=asc&order_by=updated_at
"system": true, "system": true,
"noteable_id": 121, "noteable_id": 121,
"noteable_type": "Issue", "noteable_type": "Issue",
"noteable_iid": 121 "noteable_iid": 121,
"resolvable": false
} }
] ]
``` ```
...@@ -314,7 +316,8 @@ Parameters: ...@@ -314,7 +316,8 @@ Parameters:
"system": false, "system": false,
"noteable_id": 2, "noteable_id": 2,
"noteable_type": "MergeRequest", "noteable_type": "MergeRequest",
"noteable_iid": 2 "noteable_iid": 2,
"resolvable": false
} }
``` ```
......
...@@ -5,11 +5,12 @@ module API ...@@ -5,11 +5,12 @@ module API
before { authenticate! } before { authenticate! }
NOTEABLE_TYPES = [Issue, Snippet].freeze NOTEABLE_TYPES = [Issue, Snippet, MergeRequest, Commit].freeze
NOTEABLE_TYPES.each do |noteable_type| NOTEABLE_TYPES.each do |noteable_type|
parent_type = noteable_type.parent_class.to_s.underscore parent_type = noteable_type.parent_class.to_s.underscore
noteables_str = noteable_type.to_s.underscore.pluralize noteables_str = noteable_type.to_s.underscore.pluralize
noteables_path = noteable_type == Commit ? "repository/#{noteables_str}" : noteables_str
params do params do
requires :id, type: String, desc: "The ID of a #{parent_type}" requires :id, type: String, desc: "The ID of a #{parent_type}"
...@@ -19,14 +20,12 @@ module API ...@@ -19,14 +20,12 @@ module API
success Entities::Discussion success Entities::Discussion
end end
params do params do
requires :noteable_id, type: Integer, desc: 'The ID of the noteable' requires :noteable_id, types: [Integer, String], desc: 'The ID of the noteable'
use :pagination use :pagination
end end
get ":id/#{noteables_str}/:noteable_id/discussions" do get ":id/#{noteables_path}/:noteable_id/discussions" do
noteable = find_noteable(parent_type, noteables_str, params[:noteable_id]) noteable = find_noteable(parent_type, noteables_str, params[:noteable_id])
break not_found!("Discussions") unless can?(current_user, noteable_read_ability_name(noteable), noteable)
notes = noteable.notes notes = noteable.notes
.inc_relations_for_view .inc_relations_for_view
.includes(:noteable) .includes(:noteable)
...@@ -43,13 +42,13 @@ module API ...@@ -43,13 +42,13 @@ module API
end end
params do params do
requires :discussion_id, type: String, desc: 'The ID of a discussion' requires :discussion_id, type: String, desc: 'The ID of a discussion'
requires :noteable_id, type: Integer, desc: 'The ID of the noteable' requires :noteable_id, types: [Integer, String], desc: 'The ID of the noteable'
end end
get ":id/#{noteables_str}/:noteable_id/discussions/:discussion_id" do get ":id/#{noteables_path}/:noteable_id/discussions/:discussion_id" do
noteable = find_noteable(parent_type, noteables_str, params[:noteable_id]) noteable = find_noteable(parent_type, noteables_str, params[:noteable_id])
notes = readable_discussion_notes(noteable, params[:discussion_id]) notes = readable_discussion_notes(noteable, params[:discussion_id])
if notes.empty? || !can?(current_user, noteable_read_ability_name(noteable), noteable) if notes.empty?
break not_found!("Discussion") break not_found!("Discussion")
end end
...@@ -62,19 +61,36 @@ module API ...@@ -62,19 +61,36 @@ module API
success Entities::Discussion success Entities::Discussion
end end
params do params do
requires :noteable_id, type: Integer, desc: 'The ID of the noteable' requires :noteable_id, types: [Integer, String], desc: 'The ID of the noteable'
requires :body, type: String, desc: 'The content of a note' requires :body, type: String, desc: 'The content of a note'
optional :created_at, type: String, desc: 'The creation date of the note' optional :created_at, type: String, desc: 'The creation date of the note'
optional :position, type: Hash do
requires :base_sha, type: String, desc: 'Base commit SHA in the source branch'
requires :start_sha, type: String, desc: 'SHA referencing commit in target branch'
requires :head_sha, type: String, desc: 'SHA referencing HEAD of this merge request'
requires :position_type, type: String, desc: 'Type of the position reference', values: %w(text image)
optional :new_path, type: String, desc: 'File path after change'
optional :new_line, type: Integer, desc: 'Line number after change'
optional :old_path, type: String, desc: 'File path before change'
optional :old_line, type: Integer, desc: 'Line number before change'
optional :width, type: Integer, desc: 'Width of the image'
optional :height, type: Integer, desc: 'Height of the image'
optional :x, type: Integer, desc: 'X coordinate in the image'
optional :y, type: Integer, desc: 'Y coordinate in the image'
end
end end
post ":id/#{noteables_str}/:noteable_id/discussions" do post ":id/#{noteables_path}/:noteable_id/discussions" do
noteable = find_noteable(parent_type, noteables_str, params[:noteable_id]) noteable = find_noteable(parent_type, noteables_str, params[:noteable_id])
type = params[:position] ? 'DiffNote' : 'DiscussionNote'
id_key = noteable.is_a?(Commit) ? :commit_id : :noteable_id
opts = { opts = {
note: params[:body], note: params[:body],
created_at: params[:created_at], created_at: params[:created_at],
type: 'DiscussionNote', type: type,
noteable_type: noteables_str.classify, noteable_type: noteables_str.classify,
noteable_id: noteable.id position: params[:position],
id_key => noteable.id
} }
note = create_note(noteable, opts) note = create_note(noteable, opts)
...@@ -91,13 +107,13 @@ module API ...@@ -91,13 +107,13 @@ module API
end end
params do params do
requires :discussion_id, type: String, desc: 'The ID of a discussion' requires :discussion_id, type: String, desc: 'The ID of a discussion'
requires :noteable_id, type: Integer, desc: 'The ID of the noteable' requires :noteable_id, types: [Integer, String], desc: 'The ID of the noteable'
end end
get ":id/#{noteables_str}/:noteable_id/discussions/:discussion_id/notes" do get ":id/#{noteables_path}/:noteable_id/discussions/:discussion_id/notes" do
noteable = find_noteable(parent_type, noteables_str, params[:noteable_id]) noteable = find_noteable(parent_type, noteables_str, params[:noteable_id])
notes = readable_discussion_notes(noteable, params[:discussion_id]) notes = readable_discussion_notes(noteable, params[:discussion_id])
if notes.empty? || !can?(current_user, noteable_read_ability_name(noteable), noteable) if notes.empty?
break not_found!("Notes") break not_found!("Notes")
end end
...@@ -108,12 +124,12 @@ module API ...@@ -108,12 +124,12 @@ module API
success Entities::Note success Entities::Note
end end
params do params do
requires :noteable_id, type: Integer, desc: 'The ID of the noteable' requires :noteable_id, types: [Integer, String], desc: 'The ID of the noteable'
requires :discussion_id, type: String, desc: 'The ID of a discussion' requires :discussion_id, type: String, desc: 'The ID of a discussion'
requires :body, type: String, desc: 'The content of a note' requires :body, type: String, desc: 'The content of a note'
optional :created_at, type: String, desc: 'The creation date of the note' optional :created_at, type: String, desc: 'The creation date of the note'
end end
post ":id/#{noteables_str}/:noteable_id/discussions/:discussion_id/notes" do post ":id/#{noteables_path}/:noteable_id/discussions/:discussion_id/notes" do
noteable = find_noteable(parent_type, noteables_str, params[:noteable_id]) noteable = find_noteable(parent_type, noteables_str, params[:noteable_id])
notes = readable_discussion_notes(noteable, params[:discussion_id]) notes = readable_discussion_notes(noteable, params[:discussion_id])
...@@ -139,11 +155,11 @@ module API ...@@ -139,11 +155,11 @@ module API
success Entities::Note success Entities::Note
end end
params do params do
requires :noteable_id, type: Integer, desc: 'The ID of the noteable' requires :noteable_id, types: [Integer, String], desc: 'The ID of the noteable'
requires :discussion_id, type: String, desc: 'The ID of a discussion' requires :discussion_id, type: String, desc: 'The ID of a discussion'
requires :note_id, type: Integer, desc: 'The ID of a note' requires :note_id, type: Integer, desc: 'The ID of a note'
end end
get ":id/#{noteables_str}/:noteable_id/discussions/:discussion_id/notes/:note_id" do get ":id/#{noteables_path}/:noteable_id/discussions/:discussion_id/notes/:note_id" do
noteable = find_noteable(parent_type, noteables_str, params[:noteable_id]) noteable = find_noteable(parent_type, noteables_str, params[:noteable_id])
get_note(noteable, params[:note_id]) get_note(noteable, params[:note_id])
...@@ -153,30 +169,52 @@ module API ...@@ -153,30 +169,52 @@ module API
success Entities::Note success Entities::Note
end end
params do params do
requires :noteable_id, type: Integer, desc: 'The ID of the noteable' requires :noteable_id, types: [Integer, String], desc: 'The ID of the noteable'
requires :discussion_id, type: String, desc: 'The ID of a discussion' requires :discussion_id, type: String, desc: 'The ID of a discussion'
requires :note_id, type: Integer, desc: 'The ID of a note' requires :note_id, type: Integer, desc: 'The ID of a note'
requires :body, type: String, desc: 'The content of a note' optional :body, type: String, desc: 'The content of a note'
optional :resolved, type: Boolean, desc: 'Mark note resolved/unresolved'
exactly_one_of :body, :resolved
end end
put ":id/#{noteables_str}/:noteable_id/discussions/:discussion_id/notes/:note_id" do put ":id/#{noteables_path}/:noteable_id/discussions/:discussion_id/notes/:note_id" do
noteable = find_noteable(parent_type, noteables_str, params[:noteable_id]) noteable = find_noteable(parent_type, noteables_str, params[:noteable_id])
update_note(noteable, params[:note_id]) if params[:resolved].nil?
update_note(noteable, params[:note_id])
else
resolve_note(noteable, params[:note_id], params[:resolved])
end
end end
desc "Delete a comment in a #{noteable_type.to_s.downcase} discussion" do desc "Delete a comment in a #{noteable_type.to_s.downcase} discussion" do
success Entities::Note success Entities::Note
end end
params do params do
requires :noteable_id, type: Integer, desc: 'The ID of the noteable' requires :noteable_id, types: [Integer, String], desc: 'The ID of the noteable'
requires :discussion_id, type: String, desc: 'The ID of a discussion' requires :discussion_id, type: String, desc: 'The ID of a discussion'
requires :note_id, type: Integer, desc: 'The ID of a note' requires :note_id, type: Integer, desc: 'The ID of a note'
end end
delete ":id/#{noteables_str}/:noteable_id/discussions/:discussion_id/notes/:note_id" do delete ":id/#{noteables_path}/:noteable_id/discussions/:discussion_id/notes/:note_id" do
noteable = find_noteable(parent_type, noteables_str, params[:noteable_id]) noteable = find_noteable(parent_type, noteables_str, params[:noteable_id])
delete_note(noteable, params[:note_id]) delete_note(noteable, params[:note_id])
end end
if Noteable::RESOLVABLE_TYPES.include?(noteable_type.to_s)
desc "Resolve/unresolve an existing #{noteable_type.to_s.downcase} discussion" do
success Entities::Discussion
end
params do
requires :noteable_id, types: [Integer, String], desc: 'The ID of the noteable'
requires :discussion_id, type: String, desc: 'The ID of a discussion'
requires :resolved, type: Boolean, desc: 'Mark discussion resolved/unresolved'
end
put ":id/#{noteables_path}/:noteable_id/discussions/:discussion_id" do
noteable = find_noteable(parent_type, noteables_str, params[:noteable_id])
resolve_discussion(noteable, params[:discussion_id], params[:resolved])
end
end
end end
end end
......
...@@ -286,6 +286,10 @@ module API ...@@ -286,6 +286,10 @@ module API
end end
end end
class DiffRefs < Grape::Entity
expose :base_sha, :head_sha, :start_sha
end
class Commit < Grape::Entity class Commit < Grape::Entity
expose :id, :short_id, :title, :created_at expose :id, :short_id, :title, :created_at
expose :parent_ids expose :parent_ids
...@@ -601,6 +605,8 @@ module API ...@@ -601,6 +605,8 @@ module API
merge_request.metrics&.pipeline merge_request.metrics&.pipeline
end end
expose :diff_refs, using: Entities::DiffRefs
def build_available?(options) def build_available?(options)
options[:project]&.feature_available?(:builds, options[:current_user]) options[:project]&.feature_available?(:builds, options[:current_user])
end end
...@@ -642,6 +648,11 @@ module API ...@@ -642,6 +648,11 @@ module API
expose :id, :key, :created_at expose :id, :key, :created_at
end end
class DiffPosition < Grape::Entity
expose :base_sha, :start_sha, :head_sha, :old_path, :new_path,
:position_type
end
class Note < Grape::Entity class Note < Grape::Entity
# Only Issue and MergeRequest have iid # Only Issue and MergeRequest have iid
NOTEABLE_TYPES_WITH_IID = %w(Issue MergeRequest).freeze NOTEABLE_TYPES_WITH_IID = %w(Issue MergeRequest).freeze
...@@ -655,6 +666,14 @@ module API ...@@ -655,6 +666,14 @@ module API
expose :system?, as: :system expose :system?, as: :system
expose :noteable_id, :noteable_type expose :noteable_id, :noteable_type
expose :position, if: ->(note, options) { note.diff_note? } do |note|
note.position.to_h
end
expose :resolvable?, as: :resolvable
expose :resolved?, as: :resolved, if: ->(note, options) { note.resolvable? }
expose :resolved_by, using: Entities::UserBasic, if: ->(note, options) { note.resolvable? }
# Avoid N+1 queries as much as possible # Avoid N+1 queries as much as possible
expose(:noteable_iid) { |note| note.noteable.iid if NOTEABLE_TYPES_WITH_IID.include?(note.noteable_type) } expose(:noteable_iid) { |note| note.noteable.iid if NOTEABLE_TYPES_WITH_IID.include?(note.noteable_type) }
end end
......
...@@ -171,6 +171,10 @@ module API ...@@ -171,6 +171,10 @@ module API
MergeRequestsFinder.new(current_user, project_id: user_project.id).find_by!(iid: iid) MergeRequestsFinder.new(current_user, project_id: user_project.id).find_by!(iid: iid)
end end
def find_project_commit(id)
user_project.commit_by(oid: id)
end
def find_project_snippet(id) def find_project_snippet(id)
finder_params = { project: user_project } finder_params = { project: user_project }
SnippetsFinder.new(current_user, finder_params).find(id) SnippetsFinder.new(current_user, finder_params).find(id)
......
...@@ -21,6 +21,23 @@ module API ...@@ -21,6 +21,23 @@ module API
end end
end end
def resolve_note(noteable, note_id, resolved)
note = noteable.notes.find(note_id)
authorize! :resolve_note, note
bad_request!("Note is not resolvable") unless note.resolvable?
if resolved
parent = noteable_parent(noteable)
::Notes::ResolveService.new(parent, current_user).execute(note)
else
note.unresolve!
end
present note, with: Entities::Note
end
def delete_note(noteable, note_id) def delete_note(noteable, note_id)
note = noteable.notes.find(note_id) note = noteable.notes.find(note_id)
...@@ -35,7 +52,7 @@ module API ...@@ -35,7 +52,7 @@ module API
def get_note(noteable, note_id) def get_note(noteable, note_id)
note = noteable.notes.with_metadata.find(params[:note_id]) note = noteable.notes.with_metadata.find(params[:note_id])
can_read_note = can?(current_user, noteable_read_ability_name(noteable), noteable) && !note.cross_reference_not_visible_for?(current_user) can_read_note = !note.cross_reference_not_visible_for?(current_user)
if can_read_note if can_read_note
present note, with: Entities::Note present note, with: Entities::Note
...@@ -49,7 +66,20 @@ module API ...@@ -49,7 +66,20 @@ module API
end end
def find_noteable(parent, noteables_str, noteable_id) def find_noteable(parent, noteables_str, noteable_id)
public_send("find_#{parent}_#{noteables_str.singularize}", noteable_id) # rubocop:disable GitlabSecurity/PublicSend noteable = public_send("find_#{parent}_#{noteables_str.singularize}", noteable_id) # rubocop:disable GitlabSecurity/PublicSend
readable =
if noteable.is_a?(Commit)
# for commits there is not :read_commit policy, check if user
# has :read_note permission on the commit's project
can?(current_user, :read_note, user_project)
else
can?(current_user, noteable_read_ability_name(noteable), noteable)
end
return not_found!(noteables_str) unless readable
noteable
end end
def noteable_parent(noteable) def noteable_parent(noteable)
...@@ -57,11 +87,8 @@ module API ...@@ -57,11 +87,8 @@ module API
end end
def create_note(noteable, opts) def create_note(noteable, opts)
noteables_str = noteable.model_name.to_s.underscore.pluralize policy_object = noteable.is_a?(Commit) ? user_project : noteable
authorize!(:create_note, policy_object)
return not_found!(noteables_str) unless can?(current_user, noteable_read_ability_name(noteable), noteable)
authorize! :create_note, noteable
parent = noteable_parent(noteable) parent = noteable_parent(noteable)
...@@ -73,6 +100,21 @@ module API ...@@ -73,6 +100,21 @@ module API
project = parent if parent.is_a?(Project) project = parent if parent.is_a?(Project)
::Notes::CreateService.new(project, current_user, opts).execute ::Notes::CreateService.new(project, current_user, opts).execute
end end
def resolve_discussion(noteable, discussion_id, resolved)
discussion = noteable.find_discussion(discussion_id)
forbidden! unless discussion.can_resolve?(current_user)
if resolved
parent = noteable_parent(noteable)
::Discussions::ResolveService.new(parent, current_user, merge_request: noteable).execute(discussion)
else
discussion.unresolve!
end
present discussion, with: Entities::Discussion
end
end end
end end
end end
...@@ -31,23 +31,19 @@ module API ...@@ -31,23 +31,19 @@ module API
get ":id/#{noteables_str}/:noteable_id/notes" do get ":id/#{noteables_str}/:noteable_id/notes" do
noteable = find_noteable(parent_type, noteables_str, params[:noteable_id]) noteable = find_noteable(parent_type, noteables_str, params[:noteable_id])
if can?(current_user, noteable_read_ability_name(noteable), noteable) # We exclude notes that are cross-references and that cannot be viewed
# We exclude notes that are cross-references and that cannot be viewed # by the current user. By doing this exclusion at this level and not
# by the current user. By doing this exclusion at this level and not # at the DB query level (which we cannot in that case), the current
# at the DB query level (which we cannot in that case), the current # page can have less elements than :per_page even if
# page can have less elements than :per_page even if # there's more than one page.
# there's more than one page. raw_notes = noteable.notes.with_metadata.reorder(params[:order_by] => params[:sort])
raw_notes = noteable.notes.with_metadata.reorder(params[:order_by] => params[:sort]) notes =
notes = # paginate() only works with a relation. This could lead to a
# paginate() only works with a relation. This could lead to a # mismatch between the pagination headers info and the actual notes
# mismatch between the pagination headers info and the actual notes # array returned, but this is really a edge-case.
# array returned, but this is really a edge-case. paginate(raw_notes)
paginate(raw_notes) .reject { |n| n.cross_reference_not_visible_for?(current_user) }
.reject { |n| n.cross_reference_not_visible_for?(current_user) } present notes, with: Entities::Note
present notes, with: Entities::Note
else
not_found!("Notes")
end
end end
desc "Get a single #{noteable_type.to_s.downcase} note" do desc "Get a single #{noteable_type.to_s.downcase} note" do
......
...@@ -12,6 +12,10 @@ module Gitlab ...@@ -12,6 +12,10 @@ module Gitlab
:head_sha, :head_sha,
:old_line, :old_line,
:new_line, :new_line,
:width,
:height,
:x,
:y,
:position_type, to: :formatter :position_type, to: :formatter
# A position can belong to a text line or to an image coordinate # A position can belong to a text line or to an image coordinate
......
...@@ -24,7 +24,10 @@ ...@@ -24,7 +24,10 @@
"system": { "type": "boolean" }, "system": { "type": "boolean" },
"noteable_id": { "type": "integer" }, "noteable_id": { "type": "integer" },
"noteable_iid": { "type": "integer" }, "noteable_iid": { "type": "integer" },
"noteable_type": { "type": "string" } "noteable_type": { "type": "string" },
"resolved": { "type": "boolean" },
"resolvable": { "type": "boolean" },
"resolved_by": { "type": ["string", "null"] }
}, },
"required": [ "required": [
"id", "body", "attachment", "author", "created_at", "updated_at", "id", "body", "attachment", "author", "created_at", "updated_at",
......
...@@ -2,32 +2,53 @@ require 'spec_helper' ...@@ -2,32 +2,53 @@ require 'spec_helper'
describe API::Discussions do describe API::Discussions do
let(:user) { create(:user) } let(:user) { create(:user) }
let!(:project) { create(:project, :public, namespace: user.namespace) } let!(:project) { create(:project, :public, :repository, namespace: user.namespace) }
let(:private_user) { create(:user) } let(:private_user) { create(:user) }
before do before do
project.add_reporter(user) project.add_developer(user)
end end
context "when noteable is an Issue" do context 'when noteable is an Issue' do
let!(:issue) { create(:issue, project: project, author: user) } let!(:issue) { create(:issue, project: project, author: user) }
let!(:issue_note) { create(:discussion_note_on_issue, noteable: issue, project: project, author: user) } let!(:issue_note) { create(:discussion_note_on_issue, noteable: issue, project: project, author: user) }
it_behaves_like "discussions API", 'projects', 'issues', 'iid' do it_behaves_like 'discussions API', 'projects', 'issues', 'iid' do
let(:parent) { project } let(:parent) { project }
let(:noteable) { issue } let(:noteable) { issue }
let(:note) { issue_note } let(:note) { issue_note }
end end
end end
context "when noteable is a Snippet" do context 'when noteable is a Snippet' do
let!(:snippet) { create(:project_snippet, project: project, author: user) } let!(:snippet) { create(:project_snippet, project: project, author: user) }
let!(:snippet_note) { create(:discussion_note_on_snippet, noteable: snippet, project: project, author: user) } let!(:snippet_note) { create(:discussion_note_on_snippet, noteable: snippet, project: project, author: user) }
it_behaves_like "discussions API", 'projects', 'snippets', 'id' do it_behaves_like 'discussions API', 'projects', 'snippets', 'id' do
let(:parent) { project } let(:parent) { project }
let(:noteable) { snippet } let(:noteable) { snippet }
let(:note) { snippet_note } let(:note) { snippet_note }
end end
end end
context 'when noteable is a Merge Request' do
let!(:noteable) { create(:merge_request_with_diffs, source_project: project, target_project: project, author: user) }
let!(:note) { create(:discussion_note_on_merge_request, noteable: noteable, project: project, author: user) }
let!(:diff_note) { create(:diff_note_on_merge_request, noteable: noteable, project: project, author: user) }
let(:parent) { project }
it_behaves_like 'discussions API', 'projects', 'merge_requests', 'iid'
it_behaves_like 'diff discussions API', 'projects', 'merge_requests', 'iid'
it_behaves_like 'resolvable discussions API', 'projects', 'merge_requests', 'iid'
end
context 'when noteable is a Commit' do
let!(:noteable) { create(:commit, project: project, author: user) }
let!(:note) { create(:discussion_note_on_commit, commit_id: noteable.id, project: project, author: user) }
let!(:diff_note) { create(:diff_note_on_commit, commit_id: noteable.id, project: project, author: user) }
let(:parent) { project }
it_behaves_like 'discussions API', 'projects', 'repository/commits', 'id'
it_behaves_like 'diff discussions API', 'projects', 'repository/commits', 'id'
end
end end
require 'spec_helper'
describe Notes::ResolveService do
let(:merge_request) { create(:merge_request) }
let(:note) { create(:diff_note_on_merge_request, noteable: merge_request, project: merge_request.project) }
let(:user) { merge_request.author }
describe '#execute' do
it "resolves the note" do
described_class.new(merge_request.project, user).execute(note)
note.reload
expect(note.resolved?).to be true
expect(note.resolved_by).to eq(user)
end
it "sends notifications if all discussions are resolved" do
expect_any_instance_of(MergeRequests::ResolvedDiscussionNotificationService).to receive(:execute).with(merge_request)
described_class.new(merge_request.project, user).execute(note)
end
end
end
shared_examples 'diff discussions API' do |parent_type, noteable_type, id_name|
describe "GET /#{parent_type}/:id/#{noteable_type}/:noteable_id/discussions" do
it "includes diff discussions" do
get api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/discussions", user)
discussion = json_response.find { |record| record['id'] == diff_note.discussion_id }
expect(response).to have_gitlab_http_status(200)
expect(discussion).not_to be_nil
expect(discussion['individual_note']).to eq(false)
expect(discussion['notes'].first['body']).to eq(diff_note.note)
end
end
describe "GET /#{parent_type}/:id/#{noteable_type}/:noteable_id/discussions/:discussion_id" do
it "returns a discussion by id" do
get api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/discussions/#{diff_note.discussion_id}", user)
expect(response).to have_gitlab_http_status(200)
expect(json_response['id']).to eq(diff_note.discussion_id)
expect(json_response['notes'].first['body']).to eq(diff_note.note)
expect(json_response['notes'].first['position']).to eq(diff_note.position.to_h.stringify_keys)
end
end
describe "POST /#{parent_type}/:id/#{noteable_type}/:noteable_id/discussions" do
it "creates a new diff note" do
position = diff_note.position.to_h
post api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/discussions", user), body: 'hi!', position: position
expect(response).to have_gitlab_http_status(201)
expect(json_response['notes'].first['body']).to eq('hi!')
expect(json_response['notes'].first['type']).to eq('DiffNote')
expect(json_response['notes'].first['position']).to eq(position.stringify_keys)
end
it "returns a 400 bad request error when position is invalid" do
position = diff_note.position.to_h.merge(new_line: '100000')
post api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/discussions", user), body: 'hi!', position: position
expect(response).to have_gitlab_http_status(400)
end
end
describe "POST /#{parent_type}/:id/#{noteable_type}/:noteable_id/discussions/:discussion_id/notes" do
it 'adds a new note to the diff discussion' do
post api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/"\
"discussions/#{diff_note.discussion_id}/notes", user), body: 'hi!'
expect(response).to have_gitlab_http_status(201)
expect(json_response['body']).to eq('hi!')
expect(json_response['type']).to eq('DiffNote')
end
end
end
shared_examples 'resolvable discussions API' do |parent_type, noteable_type, id_name|
describe "PUT /#{parent_type}/:id/#{noteable_type}/:noteable_id/discussions/:discussion_id" do
it "resolves discussion if resolved is true" do
put api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/"\
"discussions/#{note.discussion_id}", user), resolved: true
expect(response).to have_gitlab_http_status(200)
expect(json_response['notes'].size).to eq(1)
expect(json_response['notes'][0]['resolved']).to eq(true)
end
it "unresolves discussion if resolved is false" do
put api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/"\
"discussions/#{note.discussion_id}", user), resolved: false
expect(response).to have_gitlab_http_status(200)
expect(json_response['notes'].size).to eq(1)
expect(json_response['notes'][0]['resolved']).to eq(false)
end
it "returns a 400 bad request error if resolved parameter is not passed" do
put api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/"\
"discussions/#{note.discussion_id}", user)
expect(response).to have_gitlab_http_status(400)
end
it "returns a 401 unauthorized error if user is not authenticated" do
put api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/"\
"discussions/#{note.discussion_id}"), resolved: true
expect(response).to have_gitlab_http_status(401)
end
it "returns a 403 error if user resolves discussion of someone else" do
put api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/"\
"discussions/#{note.discussion_id}", private_user), resolved: true
expect(response).to have_gitlab_http_status(403)
end
context 'when user does not have access to read the discussion' do
before do
parent.update!(visibility_level: Gitlab::VisibilityLevel::PRIVATE)
end
it 'responds with 404' do
put api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/"\
"discussions/#{note.discussion_id}", private_user), resolved: true
expect(response).to have_gitlab_http_status(404)
end
end
end
describe "PUT /#{parent_type}/:id/#{noteable_type}/:noteable_id/discussions/:discussion_id/notes/:note_id" do
it 'returns resolved note when resolved parameter is true' do
put api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/"\
"discussions/#{note.discussion_id}/notes/#{note.id}", user), resolved: true
expect(response).to have_gitlab_http_status(200)
expect(json_response['resolved']).to eq(true)
end
it 'returns a 404 error when note id not found' do
put api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/"\
"discussions/#{note.discussion_id}/notes/12345", user),
body: 'Hello!'
expect(response).to have_gitlab_http_status(404)
end
it 'returns a 400 bad request error if neither body nor resolved parameter is given' do
put api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/"\
"discussions/#{note.discussion_id}/notes/#{note.id}", user)
expect(response).to have_gitlab_http_status(400)
end
it "returns a 403 error if user resolves note of someone else" do
put api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/"\
"discussions/#{note.discussion_id}/notes/#{note.id}", private_user), resolved: true
expect(response).to have_gitlab_http_status(403)
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