Commit d9ef41cf authored by Sean McGivern's avatar Sean McGivern

Merge branch '23589-open-issue-for-mr' into 'master'

Create an issue for all unresolved discussions in an MR

See merge request !7180
parents e1198d4f 1123057a
......@@ -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),
......
......@@ -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
......
......@@ -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
......
......@@ -476,6 +476,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?
......
......@@ -99,7 +99,7 @@ class Note < ActiveRecord::Base
end
def discussions
Discussion.for_notes(all)
Discussion.for_notes(fresh)
end
def grouped_diff_discussions
......
module Discussions
class BaseService < ::BaseService
end
end
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
......@@ -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
......
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)
......
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
......@@ -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
......
......@@ -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`
......
......@@ -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.
......@@ -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?
......
---
title: Resolve all discussions in a merge request by creating an issue collecting
them
merge_request: 7180
author: Bob Van Landuyt
......@@ -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.
......
......@@ -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
......
......@@ -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?
......
......@@ -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)
......
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
......@@ -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
......
......@@ -1127,6 +1127,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
......
......@@ -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')
......
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
require 'spec_helper.rb'
describe Issues::BuildService, services: true do
let(:project) { create(:project) }
let(:user) { create(:user) }