summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorBob Van Landuyt <bob@vanlanduyt.co>2016-10-26 23:21:50 +0200
committerBob Van Landuyt <bob@vanlanduyt.co>2016-12-05 20:55:45 +0100
commit1123057ab792ac73b1611f4d3a9faf79369dd6da (patch)
treeae88332a5d2d47d5a814583fbe874582ebaf859c
parent5fedc46343d054df28ce81a320f33d1d43d02bc8 (diff)
downloadgitlab-ce-1123057ab792ac73b1611f4d3a9faf79369dd6da.tar.gz
Feature: delegate all open discussions to Issue
When a merge request can only be merged when all discussions are resolved. This feature allows to easily delegate those discussions to a new issue, while marking them as resolved in the merge request. The user is presented with a new issue, prepared with mentions of all unresolved discussions, including the first unresolved note of the discussion, time and link to the note. When the issue is created, the discussions in the merge request will get a system note directing the user to the newly created issue.
-rw-r--r--app/controllers/projects/discussions_controller.rb4
-rw-r--r--app/controllers/projects/issues_controller.rb15
-rw-r--r--app/models/discussion.rb4
-rw-r--r--app/models/merge_request.rb8
-rw-r--r--app/models/note.rb2
-rw-r--r--app/services/discussions/base_service.rb4
-rw-r--r--app/services/discussions/resolve_service.rb24
-rw-r--r--app/services/issuable_base_service.rb5
-rw-r--r--app/services/issues/base_service.rb8
-rw-r--r--app/services/issues/build_service.rb50
-rw-r--r--app/services/issues/create_service.rb14
-rw-r--r--app/services/system_note_service.rb8
-rw-r--r--app/views/projects/merge_requests/widget/open/_unresolved_discussions.html.haml6
-rw-r--r--app/views/shared/issuable/_form.html.haml15
-rw-r--r--changelogs/unreleased/23589-open-issue-for-mr.yml5
-rw-r--r--doc/api/issues.md3
-rw-r--r--doc/user/project/merge_requests/img/preview_issue_for_discussions.pngbin0 -> 178361 bytes
-rw-r--r--doc/user/project/merge_requests/merge_request_discussion_resolution.md21
-rw-r--r--lib/api/issues.rb32
-rw-r--r--spec/controllers/projects/issues_controller_spec.rb60
-rw-r--r--spec/features/issues/create_issue_for_discussions_in_merge_request_spec.rb76
-rw-r--r--spec/models/discussion_spec.rb9
-rw-r--r--spec/models/merge_request_spec.rb40
-rw-r--r--spec/requests/api/issues_spec.rb26
-rw-r--r--spec/services/discussions/resolve_service_spec.rb52
-rw-r--r--spec/services/issues/build_service_spec.rb130
-rw-r--r--spec/services/issues/create_service_spec.rb43
-rw-r--r--spec/services/system_note_service_spec.rb28
28 files changed, 670 insertions, 22 deletions
diff --git a/app/controllers/projects/discussions_controller.rb b/app/controllers/projects/discussions_controller.rb
index d174e1145a7..148e39630e3 100644
--- a/app/controllers/projects/discussions_controller.rb
+++ b/app/controllers/projects/discussions_controller.rb
@@ -5,9 +5,7 @@ class Projects::DiscussionsController < Projects::ApplicationController
before_action :authorize_resolve_discussion!
def resolve
- discussion.resolve!(current_user)
-
- MergeRequests::ResolvedDiscussionNotificationService.new(project, current_user).execute(merge_request)
+ Discussions::ResolveService.new(project, current_user, merge_request: merge_request).execute(discussion)
render json: {
resolved_by: discussion.resolved_by.try(:name),
diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb
index 4aea7bb62c4..4f66e01e0f7 100644
--- a/app/controllers/projects/issues_controller.rb
+++ b/app/controllers/projects/issues_controller.rb
@@ -46,8 +46,9 @@ class Projects::IssuesController < Projects::ApplicationController
params[:issue] ||= ActionController::Parameters.new(
assignee_id: ""
)
+ build_params = issue_params.merge(merge_request_for_resolving_discussions: merge_request_for_resolving_discussions)
+ @issue = @noteable = Issues::BuildService.new(project, current_user, build_params).execute
- @issue = @noteable = @project.issues.new(issue_params)
respond_with(@issue)
end
@@ -75,7 +76,9 @@ class Projects::IssuesController < Projects::ApplicationController
end
def create
- @issue = Issues::CreateService.new(project, current_user, issue_params.merge(request: request)).execute
+ extra_params = { request: request,
+ merge_request_for_resolving_discussions: merge_request_for_resolving_discussions }
+ @issue = Issues::CreateService.new(project, current_user, issue_params.merge(extra_params)).execute
respond_to do |format|
format.html do
@@ -169,6 +172,14 @@ class Projects::IssuesController < Projects::ApplicationController
alias_method :awardable, :issue
alias_method :spammable, :issue
+ def merge_request_for_resolving_discussions
+ return unless merge_request_iid = params[:merge_request_for_resolving_discussions]
+
+ @merge_request_for_resolving_discussions ||= MergeRequestsFinder.new(current_user, project_id: project.id).
+ execute.
+ find_by(iid: merge_request_iid)
+ end
+
def authorize_read_issue!
return render_404 unless can?(current_user, :read_issue, @issue)
end
diff --git a/app/models/discussion.rb b/app/models/discussion.rb
index 75a85563235..bbe813db823 100644
--- a/app/models/discussion.rb
+++ b/app/models/discussion.rb
@@ -88,6 +88,10 @@ class Discussion
@first_note ||= @notes.first
end
+ def first_note_to_resolve
+ @first_note_to_resolve ||= notes.detect(&:to_be_resolved?)
+ end
+
def last_note
@last_note ||= @notes.last
end
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index bfb016df46d..a6fc9bb120d 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -480,6 +480,14 @@ class MergeRequest < ActiveRecord::Base
@diff_discussions ||= self.notes.diff_notes.discussions
end
+ def resolvable_discussions
+ @resolvable_discussions ||= diff_discussions.select(&:to_be_resolved?)
+ end
+
+ def discussions_can_be_resolved_by?(user)
+ resolvable_discussions.all? { |discussion| discussion.can_resolve?(user) }
+ end
+
def find_diff_discussion(discussion_id)
notes = self.notes.diff_notes.where(discussion_id: discussion_id).fresh.to_a
return if notes.empty?
diff --git a/app/models/note.rb b/app/models/note.rb
index 5b50ca285c3..08bd08743ef 100644
--- a/app/models/note.rb
+++ b/app/models/note.rb
@@ -99,7 +99,7 @@ class Note < ActiveRecord::Base
end
def discussions
- Discussion.for_notes(all)
+ Discussion.for_notes(fresh)
end
def grouped_diff_discussions
diff --git a/app/services/discussions/base_service.rb b/app/services/discussions/base_service.rb
new file mode 100644
index 00000000000..e4dfe6e71bb
--- /dev/null
+++ b/app/services/discussions/base_service.rb
@@ -0,0 +1,4 @@
+module Discussions
+ class BaseService < ::BaseService
+ end
+end
diff --git a/app/services/discussions/resolve_service.rb b/app/services/discussions/resolve_service.rb
new file mode 100644
index 00000000000..0437195f588
--- /dev/null
+++ b/app/services/discussions/resolve_service.rb
@@ -0,0 +1,24 @@
+module Discussions
+ class ResolveService < Discussions::BaseService
+ def execute(one_or_more_discussions)
+ Array(one_or_more_discussions).each { |discussion| resolve_discussion(discussion) }
+ end
+
+ def resolve_discussion(discussion)
+ return unless discussion.can_resolve?(current_user)
+
+ discussion.resolve!(current_user)
+
+ MergeRequests::ResolvedDiscussionNotificationService.new(project, current_user).execute(merge_request)
+ SystemNoteService.discussion_continued_in_issue(discussion, project, current_user, follow_up_issue) if follow_up_issue
+ end
+
+ def merge_request
+ params[:merge_request]
+ end
+
+ def follow_up_issue
+ params[:follow_up_issue]
+ end
+ end
+end
diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb
index ce68e433ab8..b5f63cc5a1a 100644
--- a/app/services/issuable_base_service.rb
+++ b/app/services/issuable_base_service.rb
@@ -120,9 +120,10 @@ class IssuableBaseService < BaseService
def merge_slash_commands_into_params!(issuable)
description, command_params =
SlashCommands::InterpretService.new(project, current_user).
- execute(params[:description], issuable)
+ execute(params[:description], issuable)
- params[:description] = description
+ # Avoid a description already set on an issuable to be overwritten by a nil
+ params[:description] = description if params.has_key?(:description)
params.merge!(command_params)
end
diff --git a/app/services/issues/base_service.rb b/app/services/issues/base_service.rb
index 9ea3ce084ba..742e834df97 100644
--- a/app/services/issues/base_service.rb
+++ b/app/services/issues/base_service.rb
@@ -1,5 +1,13 @@
module Issues
class BaseService < ::IssuableBaseService
+ attr_reader :merge_request_for_resolving_discussions
+
+ def initialize(*args)
+ super
+
+ @merge_request_for_resolving_discussions ||= params.delete(:merge_request_for_resolving_discussions)
+ end
+
def hook_data(issue, action)
issue_data = issue.to_hook_data(current_user)
issue_url = Gitlab::UrlBuilder.build(issue)
diff --git a/app/services/issues/build_service.rb b/app/services/issues/build_service.rb
new file mode 100644
index 00000000000..a63982f60c8
--- /dev/null
+++ b/app/services/issues/build_service.rb
@@ -0,0 +1,50 @@
+module Issues
+ class BuildService < Issues::BaseService
+ def execute
+ @issue = project.issues.new(issue_params)
+ end
+
+ def issue_params_with_info_from_merge_request
+ return {} unless merge_request_for_resolving_discussions
+
+ { title: title_from_merge_request, description: description_from_merge_request }
+ end
+
+ def title_from_merge_request
+ "Follow-up from \"#{merge_request_for_resolving_discussions.title}\""
+ end
+
+ def description_from_merge_request
+ if merge_request_for_resolving_discussions.resolvable_discussions.empty?
+ return "There are no unresolved discussions. "\
+ "Review the conversation in #{merge_request_for_resolving_discussions.to_reference}"
+ end
+
+ description = "The following discussions from #{merge_request_for_resolving_discussions.to_reference} should be addressed:"
+ [description, *items_for_discussions].join("\n\n")
+ end
+
+ def items_for_discussions
+ merge_request_for_resolving_discussions.resolvable_discussions.map { |discussion| item_for_discussion(discussion) }
+ end
+
+ def item_for_discussion(discussion)
+ first_note = discussion.first_note_to_resolve
+ other_note_count = discussion.notes.size - 1
+ creation_time = first_note.created_at.to_s(:medium)
+ note_url = Gitlab::UrlBuilder.build(first_note)
+
+ discussion_info = "- [ ] #{first_note.author.to_reference} commented in a discussion on [#{creation_time}](#{note_url}): "
+ discussion_info << " (+#{other_note_count} #{'comment'.pluralize(other_note_count)})" if other_note_count > 0
+
+ note_without_block_quotes = Banzai::Filter::BlockquoteFenceFilter.new(first_note.note).call
+ quote = ">>>\n#{note_without_block_quotes}\n>>>"
+
+ [discussion_info, quote].join("\n\n")
+ end
+
+ def issue_params
+ @issue_params ||= issue_params_with_info_from_merge_request.merge(params.slice(:title, :description))
+ end
+ end
+end
diff --git a/app/services/issues/create_service.rb b/app/services/issues/create_service.rb
index ea1690f3e38..d2eb46ac41b 100644
--- a/app/services/issues/create_service.rb
+++ b/app/services/issues/create_service.rb
@@ -4,7 +4,8 @@ module Issues
@request = params.delete(:request)
@api = params.delete(:api)
- @issue = project.issues.new
+ issue_attributes = params.merge(merge_request_for_resolving_discussions: merge_request_for_resolving_discussions)
+ @issue = BuildService.new(project, current_user, issue_attributes).execute
create(@issue)
end
@@ -18,6 +19,17 @@ module Issues
notification_service.new_issue(issuable, current_user)
todo_service.new_issue(issuable, current_user)
user_agent_detail_service.create
+
+ if merge_request_for_resolving_discussions.try(:discussions_can_be_resolved_by?, current_user)
+ resolve_discussions_in_merge_request(issuable)
+ end
+ end
+
+ def resolve_discussions_in_merge_request(issue)
+ Discussions::ResolveService.new(project, current_user,
+ merge_request: merge_request_for_resolving_discussions,
+ follow_up_issue: issue).
+ execute(merge_request_for_resolving_discussions.resolvable_discussions)
end
private
diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb
index 3cf6467804f..8b48d90f60b 100644
--- a/app/services/system_note_service.rb
+++ b/app/services/system_note_service.rb
@@ -163,6 +163,14 @@ module SystemNoteService
create_note(noteable: merge_request, project: project, author: author, note: body)
end
+ def discussion_continued_in_issue(discussion, project, author, issue)
+ body = "Added #{issue.to_reference} to continue this discussion"
+ note_attributes = discussion.reply_attributes.merge(project: project, author: author, note: body)
+ note_attributes[:type] = note_attributes.delete(:note_type)
+
+ create_note(note_attributes)
+ end
+
# Called when the title of a Noteable is changed
#
# noteable - Noteable object that responds to `title`
diff --git a/app/views/projects/merge_requests/widget/open/_unresolved_discussions.html.haml b/app/views/projects/merge_requests/widget/open/_unresolved_discussions.html.haml
index 35d5677ee37..e094f97f3b6 100644
--- a/app/views/projects/merge_requests/widget/open/_unresolved_discussions.html.haml
+++ b/app/views/projects/merge_requests/widget/open/_unresolved_discussions.html.haml
@@ -3,4 +3,8 @@
This merge request has unresolved discussions
%p
- Please resolve these discussions to allow this merge request to be merged. \ No newline at end of file
+ Please resolve these discussions
+ - if @project.issues_enabled? && can?(current_user, :create_issue, @project)
+ or
+ = link_to "open an issue to resolve them later", new_namespace_project_issue_path(@project.namespace, @project, merge_request_for_resolving_discussions: @merge_request.iid)
+ to allow this merge request to be merged.
diff --git a/app/views/shared/issuable/_form.html.haml b/app/views/shared/issuable/_form.html.haml
index 2f05093f435..bdb00bfa33c 100644
--- a/app/views/shared/issuable/_form.html.haml
+++ b/app/views/shared/issuable/_form.html.haml
@@ -42,6 +42,21 @@
= render 'shared/issuable/form/branch_chooser', issuable: issuable, form: form
+- if @merge_request_for_resolving_discussions
+ .form-group
+ .col-sm-10.col-sm-offset-2
+ - if @merge_request_for_resolving_discussions.discussions_can_be_resolved_by?(current_user)
+ = icon('exclamation-triangle')
+ Creating this issue will mark all discussions in
+ = link_to @merge_request_for_resolving_discussions.to_reference, merge_request_path(@merge_request_for_resolving_discussions)
+ as resolved.
+ = hidden_field_tag 'merge_request_for_resolving_discussions', @merge_request_for_resolving_discussions.iid
+ - else
+ = icon('exclamation-triangle')
+ You can't automatically mark all discussions in
+ = link_to @merge_request_for_resolving_discussions.to_reference, merge_request_path(@merge_request_for_resolving_discussions)
+ as resolved. Ask someone with sufficient rights to resolve the them.
+
- is_footer = !(issuable.is_a?(MergeRequest) && issuable.new_record?)
.row-content-block{class: (is_footer ? "footer-block" : "middle-block")}
- if issuable.new_record?
diff --git a/changelogs/unreleased/23589-open-issue-for-mr.yml b/changelogs/unreleased/23589-open-issue-for-mr.yml
new file mode 100644
index 00000000000..cea48b85254
--- /dev/null
+++ b/changelogs/unreleased/23589-open-issue-for-mr.yml
@@ -0,0 +1,5 @@
+---
+title: Resolve all discussions in a merge request by creating an issue collecting
+ them
+merge_request: 7180
+author: Bob Van Landuyt
diff --git a/doc/api/issues.md b/doc/api/issues.md
index 16f8e32c82a..119125bcd3d 100644
--- a/doc/api/issues.md
+++ b/doc/api/issues.md
@@ -330,6 +330,7 @@ POST /projects/:id/issues
| `labels` | string | no | Comma-separated label names for an issue |
| `created_at` | string | no | Date time string, ISO 8601 formatted, e.g. `2016-03-11T03:45:40Z` (requires admin or project owner rights) |
| `due_date` | string | no | Date time string in the format YEAR-MONTH-DAY, e.g. `2016-03-11` |
+| `merge_request_for_resolving_discussions` | integer | no | The IID of a merge request in which to resolve all issues. This will fill the issue with a default description and mark all discussions as resolved. When passing a description or title, these values will take precedence over the default values. |
```bash
curl --request POST --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v3/projects/4/issues?title=Issues%20with%20auth&labels=bug
@@ -506,7 +507,7 @@ Example response:
## Subscribe to an issue
-Subscribes the authenticated user to an issue to receive notifications.
+Subscribes the authenticated user to an issue to receive notifications.
If the user is already subscribed to the issue, the status code `304`
is returned.
diff --git a/doc/user/project/merge_requests/img/preview_issue_for_discussions.png b/doc/user/project/merge_requests/img/preview_issue_for_discussions.png
new file mode 100644
index 00000000000..9fdd387676c
--- /dev/null
+++ b/doc/user/project/merge_requests/img/preview_issue_for_discussions.png
Binary files differ
diff --git a/doc/user/project/merge_requests/merge_request_discussion_resolution.md b/doc/user/project/merge_requests/merge_request_discussion_resolution.md
index 285b1798ac5..f37f1ce4d21 100644
--- a/doc/user/project/merge_requests/merge_request_discussion_resolution.md
+++ b/doc/user/project/merge_requests/merge_request_discussion_resolution.md
@@ -37,7 +37,8 @@ resolved discussions tracker.
> [Introduced][ce-7125] in GitLab 8.14.
-You can prevent merge requests from being merged until all discussions are resolved.
+You can prevent merge requests from being merged until all discussions are
+resolved.
Navigate to your project's settings page, select the
**Only allow merge requests to be merged if all discussions are resolved** check
@@ -50,8 +51,26 @@ are resolved.
![Only allow merge if all the discussions are resolved message](img/only_allow_merge_if_all_discussions_are_resolved_msg.png)
+### Move all unresolved discussions in a merge request to an issue
+
+> [Introduced][ce-7180] (Currently on Backlog)
+
+To delegate unresolved discussions to a new issue you can click the link **open
+an issue to resolve them later**.
+
+This will prepare an issue with content referring to the merge request and
+discussions.
+
+![Issue mentioning discussions in a merge request](img/preview_issue_for_discussions.png)
+
+Hitting **Submit issue** will cause all discussions to be marked as resolved and
+add a note referring to the newly created issue.
+
+You can now proceed to merge the merge request from the UI.
+
[ce-5022]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5022
[ce-7125]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7125
+[ce-7180]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7180
[resolve-discussion-button]: img/resolve_discussion_button.png
[resolve-comment-button]: img/resolve_comment_button.png
[discussion-view]: img/discussion_view.png
diff --git a/lib/api/issues.rb b/lib/api/issues.rb
index 049b4fb214c..cfb7c45de8e 100644
--- a/lib/api/issues.rb
+++ b/lib/api/issues.rb
@@ -28,6 +28,14 @@ module API
new_params
end
+
+ def merge_request_for_resolving_discussions
+ return unless merge_request_iid = params[:merge_request_for_resolving_discussions]
+
+ @merge_request_for_resolving_discussions ||= MergeRequestsFinder.new(current_user, project_id: user_project.id).
+ execute.
+ find_by(iid: merge_request_iid)
+ end
end
resource :issues do
@@ -151,24 +159,28 @@ module API
# Create a new project issue
#
# Parameters:
- # id (required) - The ID of a project
- # title (required) - The title of an issue
- # description (optional) - The description of an issue
- # assignee_id (optional) - The ID of a user to assign issue
- # milestone_id (optional) - The ID of a milestone to assign issue
- # labels (optional) - The labels of an issue
- # created_at (optional) - Date time string, ISO 8601 formatted
- # due_date (optional) - Date time string in the format YEAR-MONTH-DAY
- # confidential (optional) - Boolean parameter if the issue should be confidential
+ # id (required) - The ID of a project
+ # title (required) - The title of an issue
+ # description (optional) - The description of an issue
+ # assignee_id (optional) - The ID of a user to assign issue
+ # milestone_id (optional) - The ID of a milestone to assign issue
+ # labels (optional) - The labels of an issue
+ # created_at (optional) - Date time string, ISO 8601 formatted
+ # due_date (optional) - Date time string in the format YEAR-MONTH-DAY
+ # confidential (optional) - Boolean parameter if the issue should be confidential
+ # merge_request_for_resolving_discussions (optional) - The IID of a merge request for which to resolve discussions
# Example Request:
# POST /projects/:id/issues
post ':id/issues' do
required_attributes! [:title]
- keys = [:title, :description, :assignee_id, :milestone_id, :due_date, :confidential, :labels]
+ keys = [:title, :description, :assignee_id, :milestone_id, :due_date, :confidential, :labels, :merge_request_for_resolving_discussions]
keys << :created_at if current_user.admin? || user_project.owner == current_user
attrs = attributes_for_keys(keys)
+ attrs[:labels] = params[:labels] if params[:labels]
+ attrs[:merge_request_for_resolving_discussions] = merge_request_for_resolving_discussions if params[:merge_request_for_resolving_discussions]
+
# Convert and filter out invalid confidential flags
attrs['confidential'] = to_boolean(attrs['confidential'])
attrs.delete('confidential') if attrs['confidential'].nil?
diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb
index 90419368f22..dbe5ddccbcf 100644
--- a/spec/controllers/projects/issues_controller_spec.rb
+++ b/spec/controllers/projects/issues_controller_spec.rb
@@ -55,6 +55,30 @@ describe Projects::IssuesController do
end
describe 'GET #new' do
+ context 'internal issue tracker' do
+ before do
+ sign_in(user)
+ project.team << [user, :developer]
+ end
+
+ it 'builds a new issue' do
+ get :new, namespace_id: project.namespace.path, project_id: project
+
+ expect(assigns(:issue)).to be_a_new(Issue)
+ end
+
+ it 'fills in an issue for a merge request' do
+ project_with_repository = create(:project)
+ project_with_repository.team << [user, :developer]
+ mr = create(:merge_request_with_diff_notes, source_project: project_with_repository)
+
+ get :new, namespace_id: project_with_repository.namespace.path, project_id: project_with_repository, merge_request_for_resolving_discussions: mr.iid
+
+ expect(assigns(:issue).title).not_to be_empty
+ expect(assigns(:issue).description).not_to be_empty
+ end
+ end
+
context 'external issue tracker' do
it 'redirects to the external issue tracker' do
external = double(new_issue_path: 'https://example.com/issues/new')
@@ -272,6 +296,42 @@ describe Projects::IssuesController do
end
describe 'POST #create' do
+ context 'resolving discussions in MergeRequest' do
+ let(:discussion) { Discussion.for_diff_notes([create(:diff_note_on_merge_request)]).first }
+ let(:merge_request) { discussion.noteable }
+ let(:project) { merge_request.source_project }
+
+ before do
+ project.team << [user, :master]
+ sign_in user
+ end
+
+ let(:merge_request_params) do
+ { merge_request_for_resolving_discussions: merge_request.iid }
+ end
+
+ def post_issue(issue_params)
+ post :create, namespace_id: project.namespace.to_param, project_id: project.to_param, issue: issue_params, merge_request_for_resolving_discussions: merge_request.iid
+ end
+
+ it 'creates an issue for the project' do
+ expect { post_issue({ title: 'Hello' }) }.to change { project.issues.reload.size }.by(1)
+ end
+
+ it "doesn't overwrite given params" do
+ post_issue(description: 'Manually entered description')
+
+ expect(assigns(:issue).description).to eq('Manually entered description')
+ end
+
+ it 'resolves the discussion in the merge_request' do
+ post_issue(title: 'Hello')
+ discussion.first_note.reload
+
+ expect(discussion.resolved?).to eq(true)
+ end
+ end
+
context 'Akismet is enabled' do
before do
allow_any_instance_of(SpamService).to receive(:check_for_spam?).and_return(true)
diff --git a/spec/features/issues/create_issue_for_discussions_in_merge_request_spec.rb b/spec/features/issues/create_issue_for_discussions_in_merge_request_spec.rb
new file mode 100644
index 00000000000..762cab0c0e1
--- /dev/null
+++ b/spec/features/issues/create_issue_for_discussions_in_merge_request_spec.rb
@@ -0,0 +1,76 @@
+require 'rails_helper'
+
+feature 'Resolving all open discussions in a merge request from an issue', feature: true do
+ let(:user) { create(:user) }
+ let(:project) { create(:project, only_allow_merge_if_all_discussions_are_resolved: true) }
+ let(:merge_request) { create(:merge_request, source_project: project) }
+ let!(:discussion) { Discussion.for_diff_notes([create(:diff_note_on_merge_request, noteable: merge_request, project: project)]).first }
+
+ before do
+ project.team << [user, :master]
+ login_as user
+ end
+
+ context 'with the internal tracker disabled' do
+ before do
+ project.project_feature.update_attribute(:issues_access_level, ProjectFeature::DISABLED)
+ visit namespace_project_merge_request_path(project.namespace, project, merge_request)
+ end
+
+ it 'does not show a link to create a new issue' do
+ expect(page).not_to have_link 'open an issue to resolve them later'
+ end
+ end
+
+ context 'merge request has discussions that need to be resolved' do
+ before do
+ visit namespace_project_merge_request_path(project.namespace, project, merge_request)
+ end
+
+ it 'shows a warning that the merge request contains unresolved discussions' do
+ expect(page).to have_content 'This merge request has unresolved discussions'
+ end
+
+ it 'has a link to resolve all discussions by creating an issue' do
+ page.within '.mr-widget-body' do
+ expect(page).to have_link 'open an issue to resolve them later', href: new_namespace_project_issue_path(project.namespace, project, merge_request_for_resolving_discussions: merge_request.iid)
+ end
+ end
+
+ context 'creating an issue for discussions' do
+ before do
+ page.click_link 'open an issue to resolve them later', href: new_namespace_project_issue_path(project.namespace, project, merge_request_for_resolving_discussions: merge_request.iid)
+ end
+
+ it 'shows an issue with the title filled in' do
+ title_field = page.find_field('issue[title]')
+
+ expect(title_field.value).to include(merge_request.title)
+ end
+
+ it 'has a mention of the discussion in the description' do
+ description_field = page.find_field('issue[description]')
+
+ expect(description_field.value).to include(discussion.first_note.note)
+ end
+
+ it 'has a hidden field for the merge request' do
+ merge_request_field = find('#merge_request_for_resolving_discussions', visible: false)
+
+ expect(merge_request_field.value).to eq(merge_request.iid.to_s)
+ end
+
+ it 'can create a new issue for the project' do
+ expect { click_button 'Submit issue' }.to change { project.issues.reload.size }.by(1)
+ end
+
+ it 'resolves the discussion in the merge request' do
+ click_button 'Submit issue'
+
+ discussion.first_note.reload
+
+ expect(discussion.resolved?).to eq(true)
+ end
+ end
+ end
+end
diff --git a/spec/models/discussion_spec.rb b/spec/models/discussion_spec.rb
index 2a67c60b978..bc32fadd391 100644
--- a/spec/models/discussion_spec.rb
+++ b/spec/models/discussion_spec.rb
@@ -521,6 +521,15 @@ describe Discussion, model: true do
end
end
+ describe "#first_note_to_resolve" do
+ it "returns the first not that still needs to be resolved" do
+ allow(first_note).to receive(:to_be_resolved?).and_return(false)
+ allow(second_note).to receive(:to_be_resolved?).and_return(true)
+
+ expect(subject.first_note_to_resolve).to eq(second_note)
+ end
+ end
+
describe "#collapsed?" do
context "when a diff discussion" do
before do
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index 2cc818af6c7..925232169f1 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -1124,6 +1124,46 @@ describe MergeRequest, models: true do
allow(subject).to receive(:diff_discussions).and_return([first_discussion, second_discussion, third_discussion])
end
+ describe '#resolvable_discussions' do
+ before do
+ allow(first_discussion).to receive(:to_be_resolved?).and_return(true)
+ allow(second_discussion).to receive(:to_be_resolved?).and_return(false)
+ allow(third_discussion).to receive(:to_be_resolved?).and_return(false)
+ end
+
+ it 'includes only discussions that need to be resolved' do
+ expect(subject.resolvable_discussions).to eq([first_discussion])
+ end
+ end
+
+ describe '#discussions_can_be_resolved_by? user' do
+ let(:user) { build(:user) }
+
+ context 'all discussions can be resolved by the user' do
+ before do
+ allow(first_discussion).to receive(:can_resolve?).with(user).and_return(true)
+ allow(second_discussion).to receive(:can_resolve?).with(user).and_return(true)
+ allow(third_discussion).to receive(:can_resolve?).with(user).and_return(true)
+ end
+
+ it 'allows a user to resolve the discussions' do
+ expect(subject.discussions_can_be_resolved_by?(user)).to be(true)
+ end
+ end
+
+ context 'one discussion cannot be resolved by the user' do
+ before do
+ allow(first_discussion).to receive(:can_resolve?).with(user).and_return(true)
+ allow(second_discussion).to receive(:can_resolve?).with(user).and_return(true)
+ allow(third_discussion).to receive(:can_resolve?).with(user).and_return(false)
+ end
+
+ it 'allows a user to resolve the discussions' do
+ expect(subject.discussions_can_be_resolved_by?(user)).to be(false)
+ end
+ end
+ end
+
describe "#discussions_resolvable?" do
context "when all discussions are unresolvable" do
before do
diff --git a/spec/requests/api/issues_spec.rb b/spec/requests/api/issues_spec.rb
index 5700f800c2e..553983575c4 100644
--- a/spec/requests/api/issues_spec.rb
+++ b/spec/requests/api/issues_spec.rb
@@ -692,6 +692,32 @@ describe API::Issues, api: true do
])
end
+ context 'resolving issues in a merge request' do
+ let(:discussion) { Discussion.for_diff_notes([create(:diff_note_on_merge_request)]).first }
+ let(:merge_request) { discussion.noteable }
+ let(:project) { merge_request.source_project }
+ before do
+ project.team << [user, :master]
+ post api("/projects/#{project.id}/issues", user),
+ title: 'New Issue',
+ merge_request_for_resolving_discussions: merge_request.iid
+ end
+
+ it 'creates a new project issue' do
+ expect(response).to have_http_status(:created)
+ end
+
+ it 'resolves the discussions in a merge request' do
+ discussion.first_note.reload
+
+ expect(discussion.resolved?).to be(true)
+ end
+
+ it 'assigns a description to the issue mentioning the merge request' do
+ expect(json_response['description']).to include(merge_request.to_reference)
+ end
+ end
+
context 'with due date' do
it 'creates a new project issue' do
due_date = 2.weeks.from_now.strftime('%Y-%m-%d')
diff --git a/spec/services/discussions/resolve_service_spec.rb b/spec/services/discussions/resolve_service_spec.rb
new file mode 100644
index 00000000000..12c3cdf28c6
--- /dev/null
+++ b/spec/services/discussions/resolve_service_spec.rb
@@ -0,0 +1,52 @@
+require 'spec_helper'
+
+describe Discussions::ResolveService do
+ describe '#execute' do
+ let(:discussion) { Discussion.for_diff_notes([create(:diff_note_on_merge_request)]).first }
+ let(:project) { merge_request.project }
+ let(:merge_request) { discussion.noteable }
+ let(:user) { create(:user) }
+ let(:service) { described_class.new(discussion.noteable.project, user, merge_request: merge_request) }
+
+ before do
+ project.team << [user, :master]
+ end
+
+ it "doesn't resolve discussions the user can't resolve" do
+ expect(discussion).to receive(:can_resolve?).with(user).and_return(false)
+
+ service.execute(discussion)
+
+ expect(discussion.resolved?).to be(false)
+ end
+
+ it 'resolves the discussion' do
+ service.execute(discussion)
+
+ expect(discussion.resolved?).to be(true)
+ end
+
+ it 'executes the notification service' do
+ expect_any_instance_of(MergeRequests::ResolvedDiscussionNotificationService).to receive(:execute).with(discussion.noteable)
+
+ service.execute(discussion)
+ end
+
+ it 'adds a system note to the discussion' do
+ issue = create(:issue, project: project)
+
+ expect(SystemNoteService).to receive(:discussion_continued_in_issue).with(discussion, project, user, issue)
+ service = described_class.new(project, user, merge_request: merge_request, follow_up_issue: issue)
+ service.execute(discussion)
+ end
+
+ it 'can resolve multiple discussions at once' do
+ other_discussion = Discussion.for_diff_notes([create(:diff_note_on_merge_request, noteable: discussion.noteable, project: discussion.noteable.source_project)]).first
+
+ service.execute([discussion, other_discussion])
+
+ expect(discussion.resolved?).to be(true)
+ expect(other_discussion.resolved?).to be(true)
+ end
+ end
+end
diff --git a/spec/services/issues/build_service_spec.rb b/spec/services/issues/build_service_spec.rb
new file mode 100644
index 00000000000..4cfba35c830
--- /dev/null
+++ b/spec/services/issues/build_service_spec.rb
@@ -0,0 +1,130 @@
+require 'spec_helper.rb'
+
+describe Issues::BuildService, services: true do
+ let(:project) { create(:project) }
+ let(:user) { create(:user) }
+
+ before do
+ project.team << [user, :developer]
+ end
+
+ context 'for discussions in a merge request' do
+ let(:merge_request) { create(:merge_request_with_diff_notes, source_project: project) }
+ let(:issue) { described_class.new(project, user, merge_request_for_resolving_discussions: merge_request).execute }
+
+ def position_on_line(line_number)
+ Gitlab::Diff::Position.new(
+ old_path: "files/ruby/popen.rb",
+ new_path: "files/ruby/popen.rb",
+ old_line: nil,
+ new_line: line_number,
+ diff_refs: merge_request.diff_refs
+ )
+ end
+
+ describe '#items_for_discussions' do
+ it 'has an item for each discussion' do
+ create(:diff_note_on_merge_request, noteable: merge_request, project: merge_request.source_project, position: position_on_line(13))
+ service = described_class.new(project, user, merge_request_for_resolving_discussions: merge_request)
+
+ service.execute
+
+ expect(service.items_for_discussions.size).to eq(2)
+ end
+ end
+
+ describe '#item_for_discussion' do
+ let(:service) { described_class.new(project, user, merge_request_for_resolving_discussions: merge_request) }
+
+ it 'mentions the author of the note' do
+ discussion = Discussion.new([create(:diff_note_on_merge_request, author: create(:user, username: 'author'))])
+ expect(service.item_for_discussion(discussion)).to include('@author')
+ end
+
+ it 'wraps the note in a blockquote' do
+ note_text = "This is a string\n"\
+ ">>>\n"\
+ "with a blockquote\n"\
+ "> That has a quote\n"\
+ ">>>\n"
+ note_result = "This is a string\n"\
+ "> with a blockquote\n"\
+ "> > That has a quote\n"
+ discussion = Discussion.new([create(:diff_note_on_merge_request, note: note_text)])
+ expect(service.item_for_discussion(discussion)).to include(">>>\n#{note_result}\n>>>")
+ end
+ end
+
+ describe '#execute' do
+ it 'has the merge request reference in the title' do
+ expect(issue.title).to include(merge_request.title)
+ end
+
+ it 'has the reference of the merge request in the description' do
+ expect(issue.description).to include(merge_request.to_reference)
+ end
+
+ it 'does not assign title when a title was given' do
+ issue = described_class.new(project, user,
+ merge_request_for_resolving_discussions: merge_request,
+ title: 'What an issue').execute
+
+ expect(issue.title).to eq('What an issue')
+ end
+
+ it 'does not assign description when a description was given' do
+ issue = described_class.new(project, user,
+ merge_request_for_resolving_discussions: merge_request,
+ description: 'Fix at your earliest conveignance').execute
+
+ expect(issue.description).to eq('Fix at your earliest conveignance')
+ end
+
+ describe 'with multiple discussions' do
+ before do
+ create(:diff_note_on_merge_request, noteable: merge_request, project: merge_request.target_project, position: position_on_line(15))
+ end
+
+ it 'mentions all the authors in the description' do
+ authors = merge_request.diff_discussions.map(&:author)
+
+ expect(issue.description).to include(*authors.map(&:to_reference))
+ end
+
+ it 'has a link for each unresolved discussion in the description' do
+ notes = merge_request.diff_discussions.map(&:first_note)
+ links = notes.map { |note| Gitlab::UrlBuilder.build(note) }
+
+ expect(issue.description).to include(*links)
+ end
+
+ it 'mentions additional notes' do
+ create_list(:diff_note_on_merge_request, 2, noteable: merge_request, project: merge_request.target_project, position: position_on_line(15))
+
+ expect(issue.description).to include('(+2 comments)')
+ end
+ end
+ end
+ end
+
+ context 'For a merge request without discussions' do
+ let(:merge_request) { create(:merge_request, source_project: project) }
+
+ describe '#execute' do
+ it 'mentions the merge request in the description' do
+ issue = described_class.new(project, user, merge_request_for_resolving_discussions: merge_request).execute
+
+ expect(issue.description).to include("Review the conversation in #{merge_request.to_reference}")
+ end
+ end
+ end
+
+ describe '#execute' do
+ it 'builds a new issues with given params' do
+ issue = described_class.new(project, user, title: 'Issue #1', description: 'Issue description').execute
+
+ expect(issue.title).to eq('Issue #1')
+ expect(issue.description).to eq('Issue description')
+ end
+ end
+end
diff --git a/spec/services/issues/create_service_spec.rb b/spec/services/issues/create_service_spec.rb
index 5c0331ebe66..8bde61ee336 100644
--- a/spec/services/issues/create_service_spec.rb
+++ b/spec/services/issues/create_service_spec.rb
@@ -136,5 +136,48 @@ describe Issues::CreateService, services: true do
end
it_behaves_like 'new issuable record that supports slash commands'
+
+ context 'for a merge request' do
+ let(:discussion) { Discussion.for_diff_notes([create(:diff_note_on_merge_request)]).first }
+ let(:merge_request) { discussion.noteable }
+ let(:project) { merge_request.source_project }
+ let(:opts) { { merge_request_for_resolving_discussions: merge_request } }
+
+ before do
+ project.team << [user, :master]
+ end
+
+ it 'resolves the discussion for the merge request' do
+ described_class.new(project, user, opts).execute
+ discussion.first_note.reload
+
+ expect(discussion.resolved?).to be(true)
+ end
+
+ it 'added a system note to the discussion' do
+ described_class.new(project, user, opts).execute
+
+ reloaded_discussion = MergeRequest.find(merge_request.id).discussions.first
+
+ expect(reloaded_discussion.last_note.system).to eq(true)
+ end
+
+ it 'assigns the title and description for the issue' do
+ issue = described_class.new(project, user, opts).execute
+
+ expect(issue.title).not_to be_nil
+ expect(issue.description).not_to be_nil
+ end
+
+ it 'can set nil explicityly to the title and description' do
+ issue = described_class.new(project, user,
+ merge_request_for_resolving_discussions: merge_request,
+ description: nil,
+ title: nil).execute
+
+ expect(issue.description).to be_nil
+ expect(issue.title).to be_nil
+ end
+ end
end
end
diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb
index 435cfb07292..07a9d8e1997 100644
--- a/spec/services/system_note_service_spec.rb
+++ b/spec/services/system_note_service_spec.rb
@@ -712,4 +712,32 @@ describe SystemNoteService, services: true do
end
end
end
+
+ describe '.discussion_continued_in_issue' do
+ let(:discussion) { Discussion.for_diff_notes([create(:diff_note_on_merge_request)]).first }
+ let(:merge_request) { discussion.noteable }
+ let(:project) { merge_request.source_project }
+ let(:issue) { create(:issue, project: project) }
+ let(:user) { create(:user) }
+
+ def reloaded_merge_request
+ MergeRequest.find(merge_request.id)
+ end
+
+ before do
+ project.team << [user, :developer]
+ end
+
+ it 'creates a new note in the discussion' do
+ # we need to completely rebuild the merge request object, or the `@discussions` on the merge request are not reloaded.
+ expect { SystemNoteService.discussion_continued_in_issue(discussion, project, user, issue) }.
+ to change { reloaded_merge_request.discussions.first.notes.size }.by(1)
+ end
+
+ it 'mentions the created issue in the system note' do
+ note = SystemNoteService.discussion_continued_in_issue(discussion, project, user, issue)
+
+ expect(note.note).to include(issue.to_reference)
+ end
+ end
end