Commit 1123057a authored by Bob Van Landuyt's avatar Bob Van Landuyt

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.
parent 5fedc463
......@@ -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 @@ def new
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 @@ def show
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 @@ def issue
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 @@ def first_note
@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
......
......@@ -480,6 +480,14 @@ def diff_discussions
@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 @@ def discussion_id(*args)
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 @@ def available_labels
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 @@ def execute
@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 @@ def after_create(issuable)
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 @@ def self.resolve_all_discussions(merge_request, project, author)
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 @@ def issue_params
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 @@ def issue_params
# 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 @@
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 @@ def go(id:)
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 @@
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
......
......@@ -1124,6 +1124,46 @@
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