Commit 0267b838 authored by Bob Van Landuyt's avatar Bob Van Landuyt Committed by Bob Van Landuyt

Delegate a single discussion to a new issue

Delegate a discussion in a merge request into a new issue.
The discussion wil be marked as resolved and a system note will be
added linking to the newly created issue.
parent 9ed3db91
/* global Vue */
/* global CommentsStore */
(() => {
const NewIssueForDiscussion = Vue.extend({
props: {
discussionId: {
type: String,
required: true,
},
},
data() {
return {
discussions: CommentsStore.state,
};
},
computed: {
discussion() {
return this.discussions[this.discussionId];
},
showButton() {
if (this.discussion) return !this.discussion.isResolved();
return false;
},
},
});
Vue.component('new-issue-for-discussion-btn', NewIssueForDiscussion);
})();
......@@ -14,10 +14,11 @@ require('./components/resolve_btn');
require('./components/resolve_count');
require('./components/resolve_discussion_btn');
require('./components/diff_note_avatars');
require('./components/new_issue_for_discussion');
$(() => {
const projectPath = document.querySelector('.merge-request').dataset.projectPath;
const COMPONENT_SELECTOR = 'resolve-btn, resolve-discussion-btn, jump-to-discussion, comment-and-resolve-btn';
const COMPONENT_SELECTOR = 'resolve-btn, resolve-discussion-btn, jump-to-discussion, comment-and-resolve-btn, new-issue-for-discussion-btn';
window.gl = window.gl || {};
window.gl.diffNoteApps = {};
......
......@@ -178,8 +178,25 @@
padding-right: 5px;
}
&:last-child {
padding-left: 5px;
}
.discussion-actions {
display: table;
.new-issue-for-discussion path {
fill: $gray-darkest;
}
.btn-group {
display: table-cell;
&:first-child {
padding-right: 0;
}
&:first-child:not(:last-child) > div {
border-right: 0;
}
}
}
......
......@@ -64,7 +64,12 @@ 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)
build_params = issue_params.merge(
merge_request_for_resolving_discussions: merge_request_for_resolving_discussions,
discussion_to_resolve: discussion_to_resolve
)
@issue = @noteable = Issues::BuildService.new(project, current_user, build_params).execute
respond_with(@issue)
......@@ -94,10 +99,12 @@ class Projects::IssuesController < Projects::ApplicationController
end
def create
create_params = issue_params
.merge(merge_request_for_resolving_discussions: merge_request_for_resolving_discussions)
.merge(spammable_params)
create_params = issue_params.
merge(
merge_request_for_resolving_discussions: merge_request_for_resolving_discussions,
discussion_to_resolve: discussion_to_resolve
).
merge(spammable_params)
@issue = Issues::CreateService.new(project, current_user, create_params).execute
respond_to do |format|
......@@ -193,6 +200,13 @@ class Projects::IssuesController < Projects::ApplicationController
find_by(iid: merge_request_iid)
end
def discussion_to_resolve
return unless discussion_id = params[:discussion_to_resolve]
@discussion_to_resolve ||= NotesFinder.new(project, current_user, discussion_id: discussion_id).
first_discussion
end
def authorize_read_issue!
return render_404 unless can?(current_user, :read_issue, @issue)
end
......
......@@ -11,6 +11,7 @@ class NotesFinder
# target_type: string
# target_id: integer
# last_fetched_at: time
# discussion_id: string
# search: string
#
def initialize(project, current_user, params = {})
......@@ -22,9 +23,14 @@ class NotesFinder
def execute
@notes = since_fetch_at(@params[:last_fetched_at]) if @params[:last_fetched_at]
@notes = for_discussion(@params[:discussion_id]) if @params[:discussion_id]
@notes
end
def first_discussion
execute.discussions.first
end
private
def init_collection
......@@ -100,4 +106,8 @@ class NotesFinder
@notes.where('updated_at > ?', last_fetched_at - FETCH_OVERLAP).fresh
end
def for_discussion(discussion_id)
@notes.where(Note.arel_table[:discussion_id].eq(discussion_id))
end
end
......@@ -9,7 +9,13 @@ module Discussions
discussion.resolve!(current_user)
MergeRequests::ResolvedDiscussionNotificationService.new(project, current_user).execute(merge_request)
notify_discussion_resolved(discussion)
end
def notify_discussion_resolved(discussion)
noteable = merge_request || discussion.noteable
MergeRequests::ResolvedDiscussionNotificationService.new(project, current_user).execute(noteable)
SystemNoteService.discussion_continued_in_issue(discussion, project, current_user, follow_up_issue) if follow_up_issue
end
......
module Issues
class BaseService < ::IssuableBaseService
attr_reader :merge_request_for_resolving_discussions
attr_reader :merge_request_for_resolving_discussions, :discussion_to_resolve
def initialize(*args)
super
@merge_request_for_resolving_discussions ||= params.delete(:merge_request_for_resolving_discussions)
@discussion_to_resolve ||= params.delete(:discussion_to_resolve)
end
def hook_data(issue, action)
......@@ -15,6 +16,28 @@ module Issues
issue_data
end
def merge_request_for_resolving_discussions
@merge_request_for_resolving_discussions ||= discussion_to_resolve.try(:noteable)
end
def for_all_discussions_in_a_merge_request?
discussion_to_resolve.nil? && merge_request_for_resolving_discussions
end
def for_single_discussion?
discussion_to_resolve && discussion_to_resolve.noteable == merge_request_for_resolving_discussions
end
def discussions_to_resolve
@discussions_to_resolve ||= if for_all_discussions_in_a_merge_request?
merge_request_for_resolving_discussions.resolvable_discussions
elsif for_single_discussion?
Array(discussion_to_resolve)
else
[]
end
end
private
def execute_hooks(issue, action = 'open')
......
......@@ -4,32 +4,35 @@ module Issues
@issue = project.issues.new(issue_params)
end
def issue_params_with_info_from_merge_request
def issue_params_with_info_from_discussions
return {} unless merge_request_for_resolving_discussions
{ title: title_from_merge_request, description: description_from_merge_request }
{ title: title_for_merge_request, description: description_for_discussions }
end
def title_from_merge_request
def title_for_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?
def description_for_discussions
if discussions_to_resolve.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 = "The following #{'discussion'.pluralize(discussions_to_resolve.size)} "\
"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) }
discussions_to_resolve.map { |discussion| item_for_discussion(discussion) }
end
def item_for_discussion(discussion)
first_note = discussion.first_note_to_resolve
first_note = discussion.first_note_to_resolve || discussion.first_note
other_note_count = discussion.notes.size - 1
creation_time = first_note.created_at.to_s(:medium)
note_url = Gitlab::UrlBuilder.build(first_note)
......@@ -44,7 +47,7 @@ module Issues
end
def issue_params
@issue_params ||= issue_params_with_info_from_merge_request.merge(whitelisted_issue_params)
@issue_params ||= issue_params_with_info_from_discussions.merge(whitelisted_issue_params)
end
def whitelisted_issue_params
......
......@@ -5,7 +5,11 @@ module Issues
def execute
filter_spam_check_params
issue_attributes = params.merge(merge_request_for_resolving_discussions: merge_request_for_resolving_discussions)
issue_attributes = params.merge(
merge_request_for_resolving_discussions: merge_request_for_resolving_discussions,
discussion_to_resolve: discussion_to_resolve
)
@issue = BuildService.new(project, current_user, issue_attributes).execute
create(@issue)
......@@ -21,17 +25,16 @@ 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
resolve_discussions_with_issue(issuable)
end
def resolve_discussions_in_merge_request(issue)
def resolve_discussions_with_issue(issue)
return if discussions_to_resolve.empty?
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)
execute(discussions_to_resolve)
end
private
......
- if discussion.can_resolve?(current_user) && can?(current_user, :create_issue, @project)
%new-issue-for-discussion-btn{ ":discussion-id" => "'#{discussion.id}'",
"inline-template" => true }
.btn-group{ role: "group", "v-if" => "showButton" }
.btn.btn-default.discussion-create-issue-btn.has-tooltip{ title: "Resolve this discussion in a new issue",
"aria-label" => "Resolve this discussion in a new issue",
"data-container" => "body" }
= link_to custom_icon('icon_mr_issue'), new_namespace_project_issue_path(@project.namespace, @project, discussion_to_resolve: discussion.id), title: "Resolve this discussion in a new issue", class: 'new-issue-for-discussion'
......@@ -11,6 +11,8 @@
= link_to_reply_discussion(discussion, line_type)
= render "discussions/resolve_all", discussion: discussion
- if discussion.for_merge_request?
= render "discussions/jump_to_next", discussion: discussion
.btn-group.discussion-actions
= render "discussions/new_issue_for_discussion", discussion: discussion
= render "discussions/jump_to_next", discussion: discussion
- else
= link_to_reply_discussion(discussion)
<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 16 16"><g fill-rule="evenodd"><path d="m8.411 1.012c-.136-.008-.273-.012-.411-.012-3.866 0-7 3.134-7 7 0 3.866 3.134 7 7 7 3.866 0 7-3.134 7-7 0-.138-.004-.275-.012-.411-.464.201-.964.334-1.488.386 0 .008 0 .016 0 .025 0 3.038-2.462 5.5-5.5 5.5-3.038 0-5.5-2.462-5.5-5.5 0-3.038 2.462-5.5 5.5-5.5.008 0 .016 0 .025 0 .052-.524.185-1.024.386-1.488"/><path d="m12 2h-1.01c-.54 0-.991.448-.991 1 0 .556.444 1 .991 1h1.01v1.01c0 .54.448.991 1 .991.556 0 1-.444 1-.991v-1.01h1.01c.54 0 .991-.448.991-1 0-.556-.444-1-.991-1h-1.01v-1.01c0-.54-.448-.991-1-.991-.556 0-1 .444-1 .991v1.01m-5 4.01c0-.557.444-1.01 1-1.01.552 0 1 .443 1 1.01v1.981c0 .557-.444 1.01-1 1.01-.552 0-1-.443-1-1.01v-1.981m1 5.991c.552 0 1-.448 1-1 0-.552-.448-1-1-1-.552 0-1 .448-1 1 0 .552.448 1 1 1"/></g></svg>
\ No newline at end of file
......@@ -48,18 +48,32 @@
- if @merge_request_for_resolving_discussions
.form-group
.col-sm-10.col-sm-offset-2
= icon('exclamation-triangle')
- 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.
- if @discussion_to_resolve
.form-group
.col-sm-10.col-sm-offset-2
= icon('exclamation-triangle')
- if @discussion_to_resolve.can_resolve?(current_user)
Creating this issue will mark the discussion at
= link_to @discussion_to_resolve.noteable.to_reference, Gitlab::UrlBuilder.build(@discussion_to_resolve.first_note)
as resolved.
= hidden_field_tag 'discussion_to_resolve', @discussion_to_resolve.id
- else
You can't automatically mark the discussion at
= link_to @discussion_to_resolve.noteable.to_reference, Gitlab::UrlBuilder.build(@discussion_to_resolve.first_note)
as resolved. Ask someone with sufficient rights to resolve it.
- is_footer = !(issuable.is_a?(MergeRequest) && issuable.new_record?)
.row-content-block{ class: (is_footer ? "footer-block" : "middle-block") }
.pull-right
......
---
title: Create a new issue for a single discussion in a Merge Request
merge_request: 8266
author: Bob Van Landuyt
......@@ -331,16 +331,17 @@ POST /projects/:id/issues
| Attribute | Type | Required | Description |
| --------- | ---- | -------- | ----------- |
| `id` | integer | yes | The ID of a project |
| `title` | string | yes | The title of an issue |
| `description` | string | no | The description of an issue |
| `confidential` | boolean | no | Set an issue to be confidential. Default is `false`. |
| `assignee_id` | integer | no | The ID of a user to assign issue |
| `milestone_id` | integer | no | The ID of a milestone to assign issue |
| `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` |
| `id` | integer | yes | The ID of a project |
| `title` | string | yes | The title of an issue |
| `description` | string | no | The description of an issue |
| `confidential` | boolean | no | Set an issue to be confidential. Default is `false`. |
| `assignee_id` | integer | no | The ID of a user to assign issue |
| `milestone_id` | integer | no | The ID of a milestone to assign issue |
| `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. |
| `discussion_to_resolve` | string | no | The ID of a discussion to resolve. This will fill in the issue with a default description and mark the discussion as resolved. |
```bash
curl --request POST --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v4/projects/4/issues?title=Issues%20with%20auth&labels=bug
......
......@@ -72,9 +72,28 @@ add a note referring to the newly created issue.
You can now proceed to merge the merge request from the UI.
## Moving a single discussion to a new issue
> [Introduced][ce-8266]
To create a new issue for a single discussion, you can use the **Resolve this
discussion in a new issue** button.
![Create issue for discussion](img/new_issue_for_discussion.png)
This will direct you to a new issue prefilled with the content of the
discussion, similar to the issues created for delegating multiple
discussions at once.
![New issue for a single discussion](img/preview_issue_for_discussion.png)
Saving the issue will mark the discussion as resolved and add a note
to the discussion referencing the new issue.
[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
[ce-8266]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/8266
[resolve-discussion-button]: img/resolve_discussion_button.png
[resolve-comment-button]: img/resolve_comment_button.png
[discussion-view]: img/discussion_view.png
......
......@@ -118,6 +118,8 @@ module API
desc: 'Date time when the issue was created. Available only for admins and project owners.'
optional :merge_request_for_resolving_discussions, type: Integer,
desc: 'The IID of a merge request for which to resolve discussions'
optional :discussion_to_resolve, type: String,
desc: 'The ID of a discussion to resolve'
use :issue_params
end
post ':id/issues' do
......@@ -134,6 +136,11 @@ module API
find_by(iid: merge_request_iid)
end
if discussion_id = params[:discussion_to_resolve]
issue_params[:discussion_to_resolve] = NotesFinder.new(user_project, current_user, discussion_id: discussion_id).
first_discussion
end
issue = ::Issues::CreateService.new(user_project,
current_user,
issue_params.merge(request: request, api: true)).execute
......
......@@ -109,6 +109,15 @@ describe Projects::IssuesController do
expect(assigns(:issue).title).not_to be_empty
expect(assigns(:issue).description).not_to be_empty
end
it 'fills in an issue for a discussion' do
note = create(:note_on_merge_request, project: project)
get :new, namespace_id: project.namespace.path, project_id: project, discussion_to_resolve: note.discussion_id
expect(assigns(:issue).title).not_to be_empty
expect(assigns(:issue).description).not_to be_empty
end
end
context 'external issue tracker' do
......
......@@ -26,12 +26,17 @@ FactoryGirl.define do
factory :diff_note_on_merge_request, traits: [:on_merge_request], class: DiffNote do
association :project, :repository
transient do
line_number 14
end
position do
Gitlab::Diff::Position.new(
old_path: "files/ruby/popen.rb",
new_path: "files/ruby/popen.rb",
old_line: nil,
new_line: 14,
new_line: line_number,
diff_refs: noteable.diff_refs
)
end
......
......@@ -42,35 +42,13 @@ feature 'Resolving all open discussions in a merge request from an issue', featu
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
it 'has a hidden field for the discussion' 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
it_behaves_like 'creating an issue for a discussion'
end
end
end
require 'rails_helper'
feature 'Resolve an open discussion in a merge request by creating 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
visit namespace_project_merge_request_path(project.namespace, project, merge_request)
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 'Resolve this discussion in a new issue'
end
end
context 'resolving the discussion', js: true do
before do
click_button 'Resolve discussion'
end
it 'hides the link for creating a new issue' do
expect(page).not_to have_link 'Resolve this discussion in a new issue'
end
it 'shows the link for creating a new issue when unresolving a discussion' do
page.within '.diff-content' do
click_button 'Unresolve discussion'
end
expect(page).to have_link 'Resolve this discussion in a new issue'
end
end
it 'has a link to create a new issue for a discussion' do
new_issue_link = new_namespace_project_issue_path(project.namespace, project, discussion_to_resolve: discussion.id)
expect(page).to have_link 'Resolve this discussion in a new issue', href: new_issue_link
end
context 'creating the issue' do
before do
click_link 'Resolve this discussion in a new issue', href: new_namespace_project_issue_path(project.namespace, project, discussion_to_resolve: discussion.id)
end
it 'has a hidden field for the discussion' do
discussion_field = find('#discussion_to_resolve', visible: false)
expect(discussion_field.value).to eq(discussion.id.to_s)
end
it_behaves_like 'creating an issue for a discussion'
end
end
......@@ -41,6 +41,17 @@ describe NotesFinder do
expect(notes.count).to eq(0)
end
context 'for a certain discussion' do
let!(:note_in_a_commit_discussion) { create(:note_on_commit, project: project) }
let!(:other_note) { create(:note_on_merge_request, project: project) }
it 'finds the only the notes for a certain discussion id' do
notes = described_class.new(project, user, discussion_id: note_in_a_commit_discussion.discussion_id).execute
expect(notes).to contain_exactly(note_in_a_commit_discussion)
end
end
context 'on restricted projects' do
let(:project) do
create(:empty_project,
......@@ -146,6 +157,28 @@ describe NotesFinder do
end
end
describe "#first_discussion" do
let!(:discussion_note) { create(:note_on_merge_request, project: project) }
it 'returns a discussion' do
discussion = described_class.new(project, user, discussion_id: discussion_note.discussion_id).first_discussion
expect(discussion).is_a? Discussion
end
it 'includes the notes in the discussion' do
discussion = described_class.new(project, user, discussion_id: discussion_note.discussion_id).first_discussion
expect(discussion.notes).to include(discussion_note)
end
it 'returns nil when no notes are found' do
discussion = described_class.new(project, user, discussion_id: 'non-existant').first_discussion
expect(discussion).to be(nil)