Commit cc4ca1f3 authored by Douwe Maan's avatar Douwe Maan

Merge branch '25515-delegate-single-discussion-to-new-issue' into 'master'

Create a new issue for a single discussion

See merge request !8266
parents 06c3c71b b55936bf
/* 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;
}
}
}
......
......@@ -509,6 +509,7 @@ ul.notes {
}
.line-resolve-all-container {
.btn-group {
margin-left: -4px;
}
......@@ -517,6 +518,27 @@ ul.notes {
border-top-left-radius: 0;
border-bottom-left-radius: 0;
}
.btn.discussion-create-issue-btn {
margin-left: -4px;
border-radius: 0;
border-right: 0;
a {
padding: 0;
line-height: 0;
&:hover {
text-decoration: none;
border: 0;
}
}
.new-issue-for-discussion path {
fill: $gray-darkest;
}
}
}
.line-resolve-all {
......
......@@ -64,8 +64,15 @@ 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
build_params = issue_params.merge(
merge_request_to_resolve_discussions_of: params[:merge_request_to_resolve_discussions_of],
discussion_to_resolve: params[:discussion_to_resolve]
)
service = Issues::BuildService.new(project, current_user, build_params)
@issue = @noteable = service.execute
@merge_request_to_resolve_discussions_of = service.merge_request_to_resolve_discussions_of
@discussion_to_resolve = service.discussions_to_resolve.first if params[:discussion_to_resolve]
respond_with(@issue)
end
......@@ -94,11 +101,21 @@ 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(spammable_params).merge(
merge_request_to_resolve_discussions_of: params[:merge_request_to_resolve_discussions_of],
discussion_to_resolve: params[:discussion_to_resolve]
)
@issue = Issues::CreateService.new(project, current_user, create_params).execute
service = Issues::CreateService.new(project, current_user, create_params)
@issue = service.execute
if service.discussions_to_resolve.count(&:resolved?) > 0
flash[:notice] = if service.discussion_to_resolve_id
"Resolved 1 discussion."
else
"Resolved all discussions."
end
end
respond_to do |format|
format.html do
......@@ -185,14 +202,6 @@ 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
......
......@@ -134,6 +134,20 @@ module IssuesHelper
options_from_collection_for_select(options, 'name', 'title', params[:due_date])
end
def link_to_discussions_to_resolve(merge_request, single_discussion = nil)
link_text = merge_request.to_reference
link_text += " (discussion #{single_discussion.first_note.id})" if single_discussion
path = if single_discussion
Gitlab::UrlBuilder.build(single_discussion.first_note)
else
project = merge_request.project
namespace_project_merge_request_path(project.namespace, project, merge_request)
end
link_to link_text, path
end
# Required for Banzai::Filter::IssueReferenceFilter
module_function :url_for_issue
end
module Issues
module ResolveDiscussions
attr_reader :merge_request_to_resolve_discussions_of_iid, :discussion_to_resolve_id
def filter_resolve_discussion_params
@merge_request_to_resolve_discussions_of_iid ||= params.delete(:merge_request_to_resolve_discussions_of)
@discussion_to_resolve_id ||= params.delete(:discussion_to_resolve)
end
def merge_request_to_resolve_discussions_of
return @merge_request_to_resolve_discussions_of if defined?(@merge_request_to_resolve_discussions_of)
@merge_request_to_resolve_discussions_of = MergeRequestsFinder.new(current_user, project_id: project.id).
execute.
find_by(iid: merge_request_to_resolve_discussions_of_iid)
end
def discussions_to_resolve
return [] unless merge_request_to_resolve_discussions_of
@discussions_to_resolve ||=
if discussion_to_resolve_id
discussion_or_nil = merge_request_to_resolve_discussions_of
.find_diff_discussion(discussion_to_resolve_id)
Array(discussion_or_nil)
else
merge_request_to_resolve_discussions_of
.resolvable_discussions
end
end
end
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
include ResolveDiscussions
def execute
filter_resolve_discussion_params
@issue = project.issues.new(issue_params)
end
def issue_params_with_info_from_merge_request
return {} unless merge_request_for_resolving_discussions
def issue_params_with_info_from_discussions
return {} unless merge_request_to_resolve_discussions_of
{ title: title_from_merge_request, description: description_from_merge_request }
{ title: title_from_merge_request, description: description_for_discussions }
end
def title_from_merge_request
"Follow-up from \"#{merge_request_for_resolving_discussions.title}\""
"Follow-up from \"#{merge_request_to_resolve_discussions_of.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}"
"Review the conversation in #{merge_request_to_resolve_discussions_of.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_to_resolve_discussions_of.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)
discussion_info = "- [ ] #{first_note.author.to_reference} commented in a discussion on [#{creation_time}](#{note_url}): "
discussion_info = "- [ ] #{first_note.author.to_reference} commented on a [discussion](#{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>>>"
spaces = ' ' * 4
quote = note_without_block_quotes.lines.map { |line| "#{spaces}> #{line}" }.join
[discussion_info, quote].join("\n\n")
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
......
module Issues
class CreateService < Issues::BaseService
include SpamCheckService
include ResolveDiscussions
def execute
filter_spam_check_params
@issue = BuildService.new(project, current_user, params).execute
issue_attributes = params.merge(merge_request_for_resolving_discussions: merge_request_for_resolving_discussions)
@issue = BuildService.new(project, current_user, issue_attributes).execute
filter_spam_check_params
filter_resolve_discussion_params
create(@issue)
end
......@@ -21,17 +22,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,
merge_request: merge_request_to_resolve_discussions_of,
follow_up_issue: issue).
execute(merge_request_for_resolving_discussions.resolvable_discussions)
execute(discussions_to_resolve)
end
private
......
- if merge_request.discussions_can_be_resolved_by?(current_user) && can?(current_user, :create_issue, @project)
.btn-group{ role: "group", "v-if" => "unresolvedDiscussionCount > 0" }
.btn.btn-default.discussion-create-issue-btn.has-tooltip{ title: "Resolve all discussions in new issue",
"aria-label" => "Resolve all discussions in a new issue",
"data-container" => "body" }
= link_to custom_icon('icon_mr_issue'), new_namespace_project_issue_path(@project.namespace, @project, merge_request_to_resolve_discussions_of: merge_request.iid), title: "Resolve all discussions in new issue", class: 'new-issue-for-discussion'
- 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, merge_request_to_resolve_discussions_of: merge_request.iid, 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?
.btn-group.discussion-actions
= render "discussions/new_issue_for_discussion", discussion: discussion, merge_request: discussion.noteable
= render "discussions/jump_to_next", discussion: discussion
- else
= link_to_reply_discussion(discussion)
- form = [@project.namespace.becomes(Namespace), @project, @issue]
= render layout: 'layouts/recaptcha_verification', locals: { spammable: @issue, form: form } do
= hidden_field_tag(:merge_request_for_resolving_discussions, params[:merge_request_for_resolving_discussions])
= hidden_field_tag(:merge_request_to_resolve_discussions_of, params[:merge_request_to_resolve_discussions_of])
= hidden_field_tag(:discussion_to_resolve, params[:discussion_to_resolve])
......@@ -82,6 +82,7 @@
= render "shared/icons/icon_status_success.svg"
%span.line-resolve-text
{{ resolvedDiscussionCount }}/{{ discussionCount }} {{ resolvedCountText }} resolved
= render "discussions/new_issue_for_all_discussions", merge_request: @merge_request
= render "discussions/jump_to_next"
.tab-content#diff-notes-app
......
......@@ -6,5 +6,5 @@
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)
= link_to "open an issue to resolve them later", new_namespace_project_issue_path(@project.namespace, @project, merge_request_to_resolve_discussions_of: @merge_request.iid)
to allow this merge request to be merged.
<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
......@@ -45,20 +45,25 @@
= render 'shared/issuable/form/merge_params', issuable: issuable
- if @merge_request_for_resolving_discussions
- if @merge_request_to_resolve_discussions_of
.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
= icon('info-circle')
- if @merge_request_to_resolve_discussions_of.discussions_can_be_resolved_by?(current_user)
= hidden_field_tag 'merge_request_to_resolve_discussions_of', @merge_request_to_resolve_discussions_of.iid
- if @discussion_to_resolve
= hidden_field_tag 'discussion_to_resolve', @discussion_to_resolve.id
Creating this issue will resolve the discussion in
- 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.
Creating this issue will resolve all discussions in
= link_to_discussions_to_resolve(@merge_request_to_resolve_discussions_of, @discussion_to_resolve)
- else
The
= @discussion_to_resolve ? 'discussion' : 'discussions'
at
= link_to_discussions_to_resolve(@merge_request_to_resolve_discussions_of, @discussion_to_resolve)
will stay unresolved. Ask someone with permission to resolve
= @discussion_to_resolve ? 'it.' : 'them.'
- is_footer = !(issuable.is_a?(MergeRequest) && issuable.new_record?)
.row-content-block{ class: (is_footer ? "footer-block" : "middle-block") }
......
---
title: Create a new issue for a single discussion in a Merge Request
merge_request: 8266
author: Bob Van Landuyt
......@@ -26,7 +26,8 @@ module Gitlab
#{config.root}/app/models/hooks
#{config.root}/app/models/members
#{config.root}/app/models/project_services
#{config.root}/app/workers/concerns))
#{config.root}/app/workers/concerns
#{config.root}/app/services/concerns))
config.generators.templates.push("#{config.root}/generator_templates")
......
......@@ -22,7 +22,11 @@ def instrument_classes(instrumentation)
paths_to_instrument = {
%w(app finders) => %w(app finders),
%w(app mailers emails) => %w(app mailers),
%w(app services **) => %w(app services),
# Don't instrument `app/services/concerns`
# It contains modules that are included in the services.
# The services themselves are instrumented so the methods from the modules
# are included.
%w(app services [^concerns]**) => %w(app services),
%w(lib gitlab conflicts) => ['lib'],
%w(lib gitlab diff) => ['lib'],
%w(lib gitlab email message) => ['lib'],
......
......@@ -340,7 +340,8 @@ 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. |
| `merge_request_to_resolve_discussions_of` | 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. Use in combination with `merge_request_to_resolve_discussions_of`. |
```bash
curl --request POST --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v4/projects/4/issues?title=Issues%20with%20auth&labels=bug
......
......@@ -53,12 +53,18 @@ are resolved.
## Move all unresolved discussions in a merge request to an issue
> [Introduced][ce-7180] in GitLab 8.15.
> [Introduced][ce-8266]
To delegate unresolved discussions to a new issue you can click the link **open
an issue to resolve them later**.
To continue all open discussions in a merge request, click the button **Resolve
all discussions in new issue**
![Open new issue from unresolved discussions](img/resolve_discussion_open_issue.png)
![Open new issue for all unresolved discussions](img/btn_new_issue_for_all_discussions.png)
Alternatively, when your project only accepts merge requests when all discussions
are resolved, there will be an **open an issue to resolve them later** link in
the merge request-widget.
![Link in merge request widget](img/resolve_discussion_open_issue.png)
This will prepare an issue with content referring to the merge request and
discussions.
......@@ -72,9 +78,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
......
......@@ -116,8 +116,10 @@ module API
requires :title, type: String, desc: 'The title of an issue'
optional :created_at, type: DateTime,
desc: 'Date time when the issue was created. Available only for admins and project owners.'
optional :merge_request_for_resolving_discussions, type: Integer,
optional :merge_request_to_resolve_discussions_of, 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, also pass `merge_request_to_resolve_discussions_of`'
use :issue_params
end
post ':id/issues' do
......@@ -128,12 +130,6 @@ module API
issue_params = declared_params(include_missing: false)
if merge_request_iid = params[:merge_request_for_resolving_discussions]
issue_params[:merge_request_for_resolving_discussions] = MergeRequestsFinder.new(current_user, project_id: user_project.id).
execute.
find_by(iid: merge_request_iid)
end
issue = ::Issues::CreateService.new(user_project,
current_user,
issue_params.merge(request: request, api: true)).execute
......
......@@ -139,12 +139,7 @@ module API
end
issue_params = declared_params(include_missing: false)
if merge_request_iid = params[:merge_request_for_resolving_discussions]
issue_params[:merge_request_for_resolving_discussions] = MergeRequestsFinder.new(current_user, project_id: user_project.id).
execute.
find_by(iid: merge_request_iid)
end
issue_params = issue_params.merge(merge_request_to_resolve_discussions_of: issue_params.delete(:merge_request_for_resolving_discussions))
issue = ::Issues::CreateService.new(user_project,
current_user,
......
......@@ -104,7 +104,16 @@ describe Projects::IssuesController do
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, project_id: project_with_repository, merge_request_for_resolving_discussions: mr.iid
get :new, namespace_id: project_with_repository.namespace, project_id: project_with_repository, merge_request_to_resolve_discussions_of: mr.iid
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, merge_request_to_resolve_discussions_of: note.noteable.iid, discussion_to_resolve: note.discussion_id
expect(assigns(:issue).title).not_to be_empty
expect(assigns(:issue).description).not_to be_empty
......@@ -462,11 +471,11 @@ describe Projects::IssuesController do
end
let(:merge_request_params) do
{ merge_request_for_resolving_discussions: merge_request.iid }
{ merge_request_to_resolve_discussions_of: merge_request.iid }
end
def post_issue(issue_params)
post :create, namespace_id: project.namespace.to_param, project_id: project, issue: issue_params, merge_request_for_resolving_discussions: merge_request.iid
def post_issue(issue_params, other_params: {})
post :create, { namespace_id: project.namespace.to_param, project_id: project, issue: issue_params, merge_request_to_resolve_discussions_of: merge_request.iid }.merge(other_params)
end
it 'creates an issue for the project' do
......@@ -485,6 +494,27 @@ describe Projects::IssuesController do
expect(discussion.resolved?).to eq(true)
end
it 'sets a flash message' do
post_issue(title: 'Hello')
expect(flash[:notice]).to eq('Resolved all discussions.')
end
describe "resolving a single discussion" do
before do
post_issue({ title: 'Hello' }, other_params: { discussion_to_resolve: discussion.id })
end
it 'resolves a single discussion' do
discussion.first_note.reload
expect(discussion.resolved?).to eq(true)
end
it 'sets a flash message that one discussion was resolved' do
expect(flash[:notice]).to eq('Resolved 1 discussion.')
end
end
end
context 'Akismet is enabled' 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
......
require 'rails_helper'
feature 'Resolving all open discussions in a merge request from an issue', feature: true do
feature 'Resolving all open discussions in a merge request from an issue', feature: true, js: true do
let(:user) { create(:user) }
let(:project) { create(:project, only_allow_merge_if_all_discussions_are_resolved: true) }
let(:project) { create(:project) }
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 }
describe 'as a user with access to the project' do
before do
project.team << [user, :master]
login_as user
visit namespace_project_merge_request_path(project.namespace, project, merge_request)
end
it 'shows a button to resolve all discussions by creating a new issue' do
within('li#resolve-count-app') do
expect(page).to have_link "Resolve all discussions in new issue", href: new_namespace_project_issue_path(project.namespace, project, merge_request_to_resolve_discussions_of: merge_request.iid)
end
end
context 'resolving the discussion' 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 all discussions in new issue", href: new_namespace_project_issue_path(project.namespace, project, merge_request_to_resolve_discussions_of: merge_request.iid)
end
end
context 'creating an issue for discussions' do
before do
click_link "Resolve all discussions in new issue", href: new_namespace_project_issue_path(project.namespace, project, merge_request_to_resolve_discussions_of: merge_request.iid)
end
it_behaves_like 'creating an issue for a discussion'
end
context 'for a project where all discussions need to be resolved before merging' do
before do
project.update_attribute(:only_allow_merge_if_all_discussions_are_resolved, true)
end
context 'with the internal tracker disabled' do
......@@ -33,44 +64,30 @@ feature 'Resolving all open discussions in a merge request from an issue', featu
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)
expect(page).to have_link 'open an issue to resolve them later', href: new_namespace_project_issue_path(project.namespace, project, merge_request_to_resolve_discussions_of: 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)
page.click_link 'open an issue to resolve them later', href: new_namespace_project_issue_path(project.namespace, project, merge_request_to_resolve_discussions_of: 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)
it_behaves_like 'creating an issue for a discussion'
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)
describe 'as a reporter' do
before do
project.team << [user, :reporter]
login_as user
visit new_namespace_project_issue_path(project.namespace, project, merge_request_to_resolve_discussions_of: merge_request.iid)
end
it 'Shows a notice to ask someone else to resolve the discussions' do
expect(page).to have_content("The discussions at #{merge_request.to_reference} will stay unresolved. Ask someone with permission to resolve them.")
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 }
describe 'As a user with access to the project' do
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, merge_request_to_resolve_discussions_of: merge_request.iid)
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, merge_request_to_resolve_discussions_of: merge_request.iid)
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
describe 'as a reporter' do
before do
project.team << [user, :reporter]
login_as user
visit new_namespace_project_issue_path(project.namespace, project,
merge_request_to_resolve_discussions_of: merge_request.iid,
discussion_to_resolve: discussion.id)
end
it 'Shows a notice to ask someone else to resolve the discussions' do
expect(page).to have_content("The discussion at #{merge_request.to_reference}"\
"(discussion #{discussion.first_note.id}) will stay unresolved."\
"Ask someone with permission to resolve it.")
end
end
end
......@@ -131,4 +131,36 @@ describe IssuesHelper do
expect(options).to have_selector('option', text: milestone2.title)
end
end
describe "#link_to_discussions_to_resolve" do
describe "passing only a merge request" do
let(:merge_request) { create(:merge_request) }
it "links just the merge request" do
expected_path = namespace_project_merge_request_path(merge_request.project.namespace, merge_request.project, merge_request)
expect(link_to_discussions_to_resolve(merge_request, nil)).to include(expected_path)
end
it "containst the reference to the merge request" do
expect(link_to_discussions_to_resolve(merge_request, nil)).to include(merge_request.to_reference)
end
end
describe "when passing a discussion" do
let(:diff_note) { create(:diff_note_on_merge_request) }
let(:merge_request) { diff_note.noteable }
let(:discussion) { Discussion.new([diff_note]) }
it "links to the merge request with first note if a single discussion was passed" do
expected_path = Gitlab::UrlBuilder.build(diff_note)
expect(link_to_discussions_to_resolve(merge_request, discussion)).to include(expected_path)
end
it "contains both the reference to the merge request and a mention of the discussion" do
expect(link_to_discussions_to_resolve(merge_request, discussion)).to include("#{merge_request.to_reference} (discussion #{diff_note.id})")
end
end
end
end
......@@ -928,29 +928,34 @@ describe API::Issues, api: true do
])
end
context 'resolving issues in a merge request' do
context 'resolving discussions' 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]
end
context 'resolving all discussions in a merge request' do
before do
post api("/projects/#{project.id}/issues", user),
title: 'New Issue',
merge_request_for_resolving_discussions: merge_request.iid
merge_request_to_resolve_discussions_of: merge_request.iid
end
it 'creates a new project issue' do
expect(response).to have_http_status(:created)
it_behaves_like 'creating an issue resolving discussions through the API'
end
it 'resolves the discussions in a merge request' do
discussion.first_note.reload
expect(discussion.resolved?).to be(true)
context 'resolving a single discussion' do
before do
post api("/projects/#{project.id}/issues", user),
title: 'New Issue',
merge_request_to_resolve_discussions_of: merge_request.iid,
discussion_to_resolve: discussion.id
end
it 'assigns a description to the issue mentioning the merge request' do
expect(json_response['description']).to include(merge_request.to_reference)
it_behaves_like 'creating an issue resolving discussions through the API'
end
end
......
......@@ -8,24 +8,34 @@ describe Issues::BuildService, services: true 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 }
context 'for a single discussion' do
describe '#execute' do
let(:merge_request) { create(:merge_request, title: "Hello world", source_project: project) }
let(:discussion) { Discussion.new([create(:diff_note_on_merge_request, project: project, noteable: merge_request, note: "Almost done")]) }
let(:service) { described_class.new(project, user, merge_request_to_resolve_discussions_of: merge_request.iid, discussion_to_resolve: discussion.id) }
it 'references the noteable title in the issue title' do
issue = service.execute
expect(issue.title).to include('Hello world')
end
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
)
it 'adds the note content to the description' do
issue = service.execute
expect(issue.description).to include('Almost done')
end
end
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_to_resolve_discussions_of: merge_request.iid).execute }
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)
create(:diff_note_on_merge_request, noteable: merge_request, project: merge_request.source_project, line_number: 13)
service = described_class.new(project, user, merge_request_to_resolve_discussions_of: merge_request.iid)
service.execute
......@@ -34,7 +44,7 @@ describe Issues::BuildService, services: true do
end
describe '#item_for_discussion' do
let(:service) { described_class.new(project, user, merge_request_for_resolving_discussions: merge_request) }
let(:service) { described_class.new(project, user, merge_request_to_resolve_discussions_of: merge_request.iid) }
it 'mentions the author of the note' do
discussion = Discussion.new([create(:diff_note_on_merge_request, author: create(:user, username: 'author'))])
......@@ -47,11 +57,11 @@ describe Issues::BuildService, services: true do
"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"
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>>>")
expect(service.item_for_discussion(discussion)).to include(note_result)
end
end
......@@ -66,7 +76,7 @@ describe Issues::BuildService, services: true do
it 'does not assign title when a title was given' do
issue = described_class.new(project, user,
merge_request_for_resolving_discussions: merge_request,
merge_request_to_resolve_discussions_of: merge_request,
title: 'What an issue').execute
expect(issue.title).to eq('What an issue')
......@@ -74,7 +84,7 @@ describe Issues::BuildService, services: true do
it 'does not assign description when a description was given' do
issue = described_class.new(project, user,
merge_request_for_resolving_discussions: merge_request,
merge_request_to_resolve_discussions_of: merge_request,
description: 'Fix at your earliest conveignance').execute
expect(issue.description).to eq('Fix at your earliest conveignance')
......@@ -82,7 +92,7 @@ describe Issues::BuildService, services: true do
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))
create(:diff_note_on_merge_request, noteable: merge_request, project: merge_request.target_project, line_number: 15)
end
it 'mentions all the authors in the description' do
......@@ -99,7 +109,7 @@ describe Issues::BuildService, services: true do
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))
create_list(:diff_note_on_merge_request, 2, noteable: merge_request, project: merge_request.target_project, line_number: 15)
expect(issue.description).to include('(+2 comments)')
end
......@@ -112,7 +122,7 @@ describe Issues::BuildService, services: true do
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
issue = described_class.new(project, user, merge_request_to_resolve_discussions_of: merge_request.iid).execute
expect(issue.description).to include("Review the conversation in #{merge_request.to_reference}")
end
......
......@@ -140,17 +140,19 @@ describe Issues::CreateService, services: true do
it_behaves_like 'new issuable record that supports slash commands'
context 'for a merge request' do
context 'resolving discussions' 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
describe 'for a single discussion' do
let(:opts) { { discussion_to_resolve: discussion.id, merge_request_to_resolve_discussions_of: merge_request.iid } }
it 'resolves the discussion' do
described_class.new(project, user, opts).execute
discussion.first_note.reload
......@@ -172,9 +174,9 @@ describe Issues::CreateService, services: true do
expect(issue.description).not_to be_nil
end
it 'can set nil explicityly to the title and description' do
it 'can set nil explicitly to the title and description' do
issue = described_class.new(project, user,
merge_request_for_resolving_discussions: merge_request,
merge_request_to_resolve_discussions_of: merge_request,
description: nil,
title: nil).execute
......@@ -183,6 +185,43 @@ describe Issues::CreateService, services: true do
end
end
describe 'for a merge request' do
let(:opts) { { merge_request_to_resolve_discussions_of: merge_request.iid } }
it 'resolves the discussion' 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 explicitly to the title and description' do
issue = described_class.new(project, user,
merge_request_to_resolve_discussions_of: merge_request,
description: nil,
title: nil).execute
expect(issue.description).to be_nil
expect(issue.title).to be_nil
end
end
end
context 'checking spam' do
let(:opts) do
{
......
require 'spec_helper.rb'
class DummyService < Issues::BaseService
include ::Issues::ResolveDiscussions
def initialize(*args)
super
filter_resolve_discussion_params
end
end
describe DummyService, services: true do
let(:project) { create(:project) }
let(:user) { create(:user) }
before do
project.team << [user, :developer]
end
describe "for resolving discussions" do
let(:discussion) { Discussion.new([create(:diff_note_on_merge_request, project: project, note: "Almost done")]) }
let(:merge_request) { discussion.noteable }
let(:other_merge_request) { create(:merge_request, source_project: project, source_branch: "other") }
describe "#merge_request_for_resolving_discussion" do
let(:service) { described_class.new(project, user, merge_request_to_resolve_discussions_of: merge_request.iid) }
it "finds the merge request" do
expect(service.merge_request_to_resolve_discussions_of).to eq(merge_request)
end
it "only queries for the merge request once" do
fake_finder = double
fake_results = double
expect(fake_finder).to receive(:execute).and_return(fake_results).exactly(1)
expect(fake_results).to receive(:find_by).exactly(1)
expect(MergeRequestsFinder).to receive(:new).and_return(fake_finder).exactly(1)
2.times { service.merge_request_to_resolve_discussions_of }
end
end
describe "#discussions_to_resolve" do
it "contains a single discussion when matching merge request and discussion are passed" do
service = described_class.new(
project,
user,
discussion_to_resolve: discussion.id,
merge_request_to_resolve_discussions_of: merge_request.iid
)
# We need to compare discussion id's because the Discussion-objects are rebuilt
# which causes the object-id's not to be different.
discussion_ids = service.discussions_to_resolve.map(&:id)
expect(discussion_ids).to contain_exactly(discussion.id)
end
it "contains all discussions when only a merge request is passed" do
second_discussion = Discussion.new([create(:diff_note_on_merge_request,
noteable: merge_request,
project: merge_request.target_project,
line_number: 15)])
service = described_class.new(
project,
user,
merge_request_to_resolve_discussions_of: merge_request.iid
)
# We need to compare discussion id's because the Discussion-objects are rebuilt
# which causes the object-id's not to be different.
discussion_ids = service.discussions_to_resolve.map(&:id)
expect(discussion_ids).to contain_exactly(discussion.id, second_discussion.id)
end
it "contains only unresolved discussions" do
_second_discussion = Discussion.new([create(:diff_note_on_merge_request, :resolved,
noteable: merge_request,
project: merge_request.target_project,
line_number: 15,
)])
service = described_class.new(
project,
user,
merge_request_to_resolve_discussions_of: merge_request.iid
)
# We need to compare discussion id's because the Discussion-objects are rebuilt
# which causes the object-id's not to be different.
discussion_ids = service.discussions_to_resolve.map(&:id)
expect(discussion_ids).to contain_exactly(discussion.id)
end
it "is empty when a discussion and another merge request are passed" do
service = described_class.new(
project,
user,
discussion_to_resolve: discussion.id,
merge_request_to_resolve_discussions_of: other_merge_request.iid
)
expect(service.discussions_to_resolve).to be_empty
end
end
end
end
shared_examples 'creating an issue resolving discussions through the API' do
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
shared_examples 'creating an issue for a discussion' do
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 '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 'shows a flash messaage after resolving a discussion' do
click_button 'Submit issue'
page.within '.flash-notice' do
# Only check for the word 'Resolved' since the spec might have resolved
# multiple discussions
expect(page).to have_content('Resolved')
end
end
it 'has a hidden field for the merge request' do
merge_request_field = find('#merge_request_to_resolve_discussions_of', visible: false)
expect(merge_request_field.value).to eq(merge_request.iid.to_s)
end
end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment