summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorDouwe Maan <douwe@gitlab.com>2017-03-13 18:25:25 +0000
committerDouwe Maan <douwe@gitlab.com>2017-03-13 18:25:25 +0000
commitcc4ca1f3fe2e2f7470c4fae9a9c5676bbebb18b6 (patch)
treed146fcb55dd01f9579ae1120c69056c36c3a36af
parent06c3c71bb8b15e4cad43cf890012ac869248b861 (diff)
parentb55936bf0beb2b21de793e3dfc350a6ca1445db1 (diff)
downloadgitlab-ce-cc4ca1f3fe2e2f7470c4fae9a9c5676bbebb18b6.tar.gz
Merge branch '25515-delegate-single-discussion-to-new-issue' into 'master'
Create a new issue for a single discussion See merge request !8266
-rw-r--r--app/assets/javascripts/diff_notes/components/new_issue_for_discussion.js29
-rw-r--r--app/assets/javascripts/diff_notes/diff_notes_bundle.js3
-rw-r--r--app/assets/stylesheets/pages/note_form.scss21
-rw-r--r--app/assets/stylesheets/pages/notes.scss22
-rw-r--r--app/controllers/projects/issues_controller.rb37
-rw-r--r--app/helpers/issues_helper.rb14
-rw-r--r--app/services/concerns/issues/resolve_discussions.rb32
-rw-r--r--app/services/issues/base_service.rb8
-rw-r--r--app/services/issues/build_service.rb34
-rw-r--r--app/services/issues/create_service.rb20
-rw-r--r--app/views/discussions/_new_issue_for_all_discussions.html.haml6
-rw-r--r--app/views/discussions/_new_issue_for_discussion.html.haml8
-rw-r--r--app/views/discussions/_notes.html.haml4
-rw-r--r--app/views/projects/issues/verify.html.haml3
-rw-r--r--app/views/projects/merge_requests/_show.html.haml1
-rw-r--r--app/views/projects/merge_requests/widget/open/_unresolved_discussions.html.haml2
-rw-r--r--app/views/shared/icons/_icon_mr_issue.svg1
-rw-r--r--app/views/shared/issuable/_form.html.haml27
-rw-r--r--changelogs/unreleased/25515-delegate-single-discussion-to-new-issue.yml4
-rw-r--r--config/application.rb3
-rw-r--r--config/initializers/8_metrics.rb18
-rw-r--r--doc/api/issues.md25
-rw-r--r--doc/user/project/merge_requests/img/btn_new_issue_for_all_discussions.pngbin0 -> 29007 bytes
-rw-r--r--doc/user/project/merge_requests/img/new_issue_for_discussion.pngbin0 -> 39563 bytes
-rw-r--r--doc/user/project/merge_requests/img/preview_issue_for_discussion.pngbin0 -> 82412 bytes
-rw-r--r--doc/user/project/merge_requests/img/preview_issue_for_discussions.pngbin178361 -> 143871 bytes
-rw-r--r--doc/user/project/merge_requests/img/resolve_discussion_issue_notice.pngbin11123 -> 10307 bytes
-rw-r--r--doc/user/project/merge_requests/merge_request_discussion_resolution.md33
-rw-r--r--lib/api/issues.rb10
-rw-r--r--lib/api/v3/issues.rb7
-rw-r--r--spec/controllers/projects/issues_controller_spec.rb38
-rw-r--r--spec/factories/notes.rb7
-rw-r--r--spec/features/issues/create_issue_for_discussions_in_merge_request_spec.rb101
-rw-r--r--spec/features/issues/create_issue_for_single_discussion_in_merge_request.rb81
-rw-r--r--spec/helpers/issues_helper_spec.rb32
-rw-r--r--spec/requests/api/issues_spec.rb31
-rw-r--r--spec/services/issues/build_service_spec.rb56
-rw-r--r--spec/services/issues/create_service_spec.rb85
-rw-r--r--spec/services/issues/resolve_discussions_spec.rb106
-rw-r--r--spec/support/api/issues_resolving_discussions_shared_examples.rb15
-rw-r--r--spec/support/features/resolving_discussions_in_issues_shared_examples.rb41
41 files changed, 759 insertions, 206 deletions
diff --git a/app/assets/javascripts/diff_notes/components/new_issue_for_discussion.js b/app/assets/javascripts/diff_notes/components/new_issue_for_discussion.js
new file mode 100644
index 00000000000..e86bef47172
--- /dev/null
+++ b/app/assets/javascripts/diff_notes/components/new_issue_for_discussion.js
@@ -0,0 +1,29 @@
+/* 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);
+})();
diff --git a/app/assets/javascripts/diff_notes/diff_notes_bundle.js b/app/assets/javascripts/diff_notes/diff_notes_bundle.js
index 7d8316dfd63..4f6b86a917c 100644
--- a/app/assets/javascripts/diff_notes/diff_notes_bundle.js
+++ b/app/assets/javascripts/diff_notes/diff_notes_bundle.js
@@ -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 = {};
diff --git a/app/assets/stylesheets/pages/note_form.scss b/app/assets/stylesheets/pages/note_form.scss
index f984b469609..c2156a5ac69 100644
--- a/app/assets/stylesheets/pages/note_form.scss
+++ b/app/assets/stylesheets/pages/note_form.scss
@@ -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;
+ }
}
}
diff --git a/app/assets/stylesheets/pages/notes.scss b/app/assets/stylesheets/pages/notes.scss
index c8ad88e9351..e238f0865f6 100644
--- a/app/assets/stylesheets/pages/notes.scss
+++ b/app/assets/stylesheets/pages/notes.scss
@@ -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 {
diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb
index 1151555b8fa..f2fee62ebd6 100644
--- a/app/controllers/projects/issues_controller.rb
+++ b/app/controllers/projects/issues_controller.rb
@@ -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]
+ )
+
+ service = Issues::CreateService.new(project, current_user, create_params)
+ @issue = service.execute
- @issue = Issues::CreateService.new(project, current_user, create_params).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
diff --git a/app/helpers/issues_helper.rb b/app/helpers/issues_helper.rb
index 4bdf07fe1ad..6978b0c89fd 100644
--- a/app/helpers/issues_helper.rb
+++ b/app/helpers/issues_helper.rb
@@ -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
diff --git a/app/services/concerns/issues/resolve_discussions.rb b/app/services/concerns/issues/resolve_discussions.rb
new file mode 100644
index 00000000000..297c7d696c3
--- /dev/null
+++ b/app/services/concerns/issues/resolve_discussions.rb
@@ -0,0 +1,32 @@
+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
diff --git a/app/services/issues/base_service.rb b/app/services/issues/base_service.rb
index 35af867a098..ee1b40db718 100644
--- a/app/services/issues/base_service.rb
+++ b/app/services/issues/base_service.rb
@@ -1,13 +1,5 @@
module Issues
class BaseService < ::IssuableBaseService
- attr_reader :merge_request_for_resolving_discussions
-
- def initialize(*args)
- super
-
- @merge_request_for_resolving_discussions ||= params.delete(:merge_request_for_resolving_discussions)
- end
-
def hook_data(issue, action)
issue_data = issue.to_hook_data(current_user)
issue_url = Gitlab::UrlBuilder.build(issue)
diff --git a/app/services/issues/build_service.rb b/app/services/issues/build_service.rb
index 7cd927d8005..77bced4bd5c 100644
--- a/app/services/issues/build_service.rb
+++ b/app/services/issues/build_service.rb
@@ -1,50 +1,56 @@
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
diff --git a/app/services/issues/create_service.rb b/app/services/issues/create_service.rb
index 85b6eb3fe3d..3cf4b82b9f2 100644
--- a/app/services/issues/create_service.rb
+++ b/app/services/issues/create_service.rb
@@ -1,12 +1,13 @@
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
diff --git a/app/views/discussions/_new_issue_for_all_discussions.html.haml b/app/views/discussions/_new_issue_for_all_discussions.html.haml
new file mode 100644
index 00000000000..ca9e0e8728a
--- /dev/null
+++ b/app/views/discussions/_new_issue_for_all_discussions.html.haml
@@ -0,0 +1,6 @@
+- 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'
diff --git a/app/views/discussions/_new_issue_for_discussion.html.haml b/app/views/discussions/_new_issue_for_discussion.html.haml
new file mode 100644
index 00000000000..df5546a1e32
--- /dev/null
+++ b/app/views/discussions/_new_issue_for_discussion.html.haml
@@ -0,0 +1,8 @@
+- 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'
diff --git a/app/views/discussions/_notes.html.haml b/app/views/discussions/_notes.html.haml
index dfdbdf1f969..2789391819c 100644
--- a/app/views/discussions/_notes.html.haml
+++ b/app/views/discussions/_notes.html.haml
@@ -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, merge_request: discussion.noteable
+ = render "discussions/jump_to_next", discussion: discussion
- else
= link_to_reply_discussion(discussion)
diff --git a/app/views/projects/issues/verify.html.haml b/app/views/projects/issues/verify.html.haml
index 09aa401e44a..6da7c317f3a 100644
--- a/app/views/projects/issues/verify.html.haml
+++ b/app/views/projects/issues/verify.html.haml
@@ -1,4 +1,5 @@
- 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])
diff --git a/app/views/projects/merge_requests/_show.html.haml b/app/views/projects/merge_requests/_show.html.haml
index 17be0490a86..c8f097c69da 100644
--- a/app/views/projects/merge_requests/_show.html.haml
+++ b/app/views/projects/merge_requests/_show.html.haml
@@ -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
diff --git a/app/views/projects/merge_requests/widget/open/_unresolved_discussions.html.haml b/app/views/projects/merge_requests/widget/open/_unresolved_discussions.html.haml
index e094f97f3b6..ec9346ce89b 100644
--- a/app/views/projects/merge_requests/widget/open/_unresolved_discussions.html.haml
+++ b/app/views/projects/merge_requests/widget/open/_unresolved_discussions.html.haml
@@ -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.
diff --git a/app/views/shared/icons/_icon_mr_issue.svg b/app/views/shared/icons/_icon_mr_issue.svg
new file mode 100644
index 00000000000..ae219a3ded2
--- /dev/null
+++ b/app/views/shared/icons/_icon_mr_issue.svg
@@ -0,0 +1 @@
+<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
diff --git a/app/views/shared/issuable/_form.html.haml b/app/views/shared/issuable/_form.html.haml
index 70470c83c51..0b0f2c9cd1a 100644
--- a/app/views/shared/issuable/_form.html.haml
+++ b/app/views/shared/issuable/_form.html.haml
@@ -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
+ Creating this issue will resolve all discussions in
+ = link_to_discussions_to_resolve(@merge_request_to_resolve_discussions_of, @discussion_to_resolve)
- 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.
+ 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") }
diff --git a/changelogs/unreleased/25515-delegate-single-discussion-to-new-issue.yml b/changelogs/unreleased/25515-delegate-single-discussion-to-new-issue.yml
new file mode 100644
index 00000000000..5b755a8bc32
--- /dev/null
+++ b/changelogs/unreleased/25515-delegate-single-discussion-to-new-issue.yml
@@ -0,0 +1,4 @@
+---
+title: Create a new issue for a single discussion in a Merge Request
+merge_request: 8266
+author: Bob Van Landuyt
diff --git a/config/application.rb b/config/application.rb
index 1cc092c4da1..98b2759a8a7 100644
--- a/config/application.rb
+++ b/config/application.rb
@@ -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")
diff --git a/config/initializers/8_metrics.rb b/config/initializers/8_metrics.rb
index a1517e6afc8..3e1657b8382 100644
--- a/config/initializers/8_metrics.rb
+++ b/config/initializers/8_metrics.rb
@@ -20,13 +20,17 @@ def instrument_classes(instrumentation)
# Path to search => prefix to strip from constant
paths_to_instrument = {
- %w(app finders) => %w(app finders),
- %w(app mailers emails) => %w(app mailers),
- %w(app services **) => %w(app services),
- %w(lib gitlab conflicts) => ['lib'],
- %w(lib gitlab diff) => ['lib'],
- %w(lib gitlab email message) => ['lib'],
- %w(lib gitlab checks) => ['lib']
+ %w(app finders) => %w(app finders),
+ %w(app mailers emails) => %w(app mailers),
+ # 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'],
+ %w(lib gitlab checks) => ['lib']
}
paths_to_instrument.each do |(path, prefix)|
diff --git a/doc/api/issues.md b/doc/api/issues.md
index e25841926f8..cb437ffb174 100644
--- a/doc/api/issues.md
+++ b/doc/api/issues.md
@@ -329,18 +329,19 @@ Creates a new project issue.
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` |
-| `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. |
+| 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` |
+| `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
diff --git a/doc/user/project/merge_requests/img/btn_new_issue_for_all_discussions.png b/doc/user/project/merge_requests/img/btn_new_issue_for_all_discussions.png
new file mode 100644
index 00000000000..b15447ec290
--- /dev/null
+++ b/doc/user/project/merge_requests/img/btn_new_issue_for_all_discussions.png
Binary files differ
diff --git a/doc/user/project/merge_requests/img/new_issue_for_discussion.png b/doc/user/project/merge_requests/img/new_issue_for_discussion.png
new file mode 100644
index 00000000000..93c9dad8921
--- /dev/null
+++ b/doc/user/project/merge_requests/img/new_issue_for_discussion.png
Binary files differ
diff --git a/doc/user/project/merge_requests/img/preview_issue_for_discussion.png b/doc/user/project/merge_requests/img/preview_issue_for_discussion.png
new file mode 100644
index 00000000000..2ee0653b2ba
--- /dev/null
+++ b/doc/user/project/merge_requests/img/preview_issue_for_discussion.png
Binary files differ
diff --git a/doc/user/project/merge_requests/img/preview_issue_for_discussions.png b/doc/user/project/merge_requests/img/preview_issue_for_discussions.png
index 9fdd387676c..3fe0a666678 100644
--- a/doc/user/project/merge_requests/img/preview_issue_for_discussions.png
+++ b/doc/user/project/merge_requests/img/preview_issue_for_discussions.png
Binary files differ
diff --git a/doc/user/project/merge_requests/img/resolve_discussion_issue_notice.png b/doc/user/project/merge_requests/img/resolve_discussion_issue_notice.png
index 8c7ce215ae0..e0ee6a39ffd 100644
--- a/doc/user/project/merge_requests/img/resolve_discussion_issue_notice.png
+++ b/doc/user/project/merge_requests/img/resolve_discussion_issue_notice.png
Binary files differ
diff --git a/doc/user/project/merge_requests/merge_request_discussion_resolution.md b/doc/user/project/merge_requests/merge_request_discussion_resolution.md
index d4b85676d19..230e957f045 100644
--- a/doc/user/project/merge_requests/merge_request_discussion_resolution.md
+++ b/doc/user/project/merge_requests/merge_request_discussion_resolution.md
@@ -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
diff --git a/lib/api/issues.rb b/lib/api/issues.rb
index 4a9f2b26fb2..1abe8639445 100644
--- a/lib/api/issues.rb
+++ b/lib/api/issues.rb
@@ -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
diff --git a/lib/api/v3/issues.rb b/lib/api/v3/issues.rb
index 5d7dfabfcd6..258cbfed022 100644
--- a/lib/api/v3/issues.rb
+++ b/lib/api/v3/issues.rb
@@ -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,
diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb
index 46c758b4654..6ceaf96f78f 100644
--- a/spec/controllers/projects/issues_controller_spec.rb
+++ b/spec/controllers/projects/issues_controller_spec.rb
@@ -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
diff --git a/spec/factories/notes.rb b/spec/factories/notes.rb
index 5c50cd7f4ad..fe19a404e16 100644
--- a/spec/factories/notes.rb
+++ b/spec/factories/notes.rb
@@ -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
diff --git a/spec/features/issues/create_issue_for_discussions_in_merge_request_spec.rb b/spec/features/issues/create_issue_for_discussions_in_merge_request_spec.rb
index 762cab0c0e1..572bca3de21 100644
--- a/spec/features/issues/create_issue_for_discussions_in_merge_request_spec.rb
+++ b/spec/features/issues/create_issue_for_discussions_in_merge_request_spec.rb
@@ -1,76 +1,93 @@
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 }
- before do
- project.team << [user, :master]
- login_as user
- end
-
- context 'with the internal tracker disabled' do
+ describe 'as a user with access to the project' do
before do
- project.project_feature.update_attribute(:issues_access_level, ProjectFeature::DISABLED)
+ project.team << [user, :master]
+ login_as user
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)
+ 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
- it 'shows a warning that the merge request contains unresolved discussions' do
- expect(page).to have_content 'This merge request has unresolved discussions'
- end
+ context 'resolving the discussion' do
+ before do
+ click_button 'Resolve discussion'
+ 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)
+ 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
- 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)
+ 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 'shows an issue with the title filled in' do
- title_field = page.find_field('issue[title]')
+ it_behaves_like 'creating an issue for a discussion'
+ end
- expect(title_field.value).to include(merge_request.title)
+ 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
- it 'has a mention of the discussion in the description' do
- description_field = page.find_field('issue[description]')
+ 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
- expect(description_field.value).to include(discussion.first_note.note)
+ 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
- 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
+ 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 'can create a new issue for the project' do
- expect { click_button 'Submit issue' }.to change { project.issues.reload.size }.by(1)
- 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 'resolves the discussion in the merge request' do
- click_button 'Submit issue'
+ 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_to_resolve_discussions_of: merge_request.iid)
+ end
+ end
- discussion.first_note.reload
+ 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_to_resolve_discussions_of: merge_request.iid)
+ end
- expect(discussion.resolved?).to eq(true)
+ it_behaves_like 'creating an issue for a discussion'
+ end
end
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)
+ 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
diff --git a/spec/features/issues/create_issue_for_single_discussion_in_merge_request.rb b/spec/features/issues/create_issue_for_single_discussion_in_merge_request.rb
new file mode 100644
index 00000000000..88e2cc60d79
--- /dev/null
+++ b/spec/features/issues/create_issue_for_single_discussion_in_merge_request.rb
@@ -0,0 +1,81 @@
+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
diff --git a/spec/helpers/issues_helper_spec.rb b/spec/helpers/issues_helper_spec.rb
index 88d853935c7..f0554cc068d 100644
--- a/spec/helpers/issues_helper_spec.rb
+++ b/spec/helpers/issues_helper_spec.rb
@@ -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
diff --git a/spec/requests/api/issues_spec.rb b/spec/requests/api/issues_spec.rb
index 2fc11a3b782..de7dbca0b22 100644
--- a/spec/requests/api/issues_spec.rb
+++ b/spec/requests/api/issues_spec.rb
@@ -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]
- 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
+ context 'resolving all discussions in a merge request' do
+ before do
+ post api("/projects/#{project.id}/issues", user),
+ title: 'New Issue',
+ merge_request_to_resolve_discussions_of: merge_request.iid
+ end
- expect(discussion.resolved?).to be(true)
+ it_behaves_like 'creating an issue resolving discussions through the API'
end
- it 'assigns a description to the issue mentioning the merge request' do
- expect(json_response['description']).to include(merge_request.to_reference)
+ 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_behaves_like 'creating an issue resolving discussions through the API'
end
end
diff --git a/spec/services/issues/build_service_spec.rb b/spec/services/issues/build_service_spec.rb
index 09807e5d35b..1dd53236fbd 100644
--- a/spec/services/issues/build_service_spec.rb
+++ b/spec/services/issues/build_service_spec.rb
@@ -8,24 +8,34 @@ describe Issues::BuildService, services: true do
project.team << [user, :developer]
end
+ 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
+
+ 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_for_resolving_discussions: merge_request).execute }
-
- def position_on_line(line_number)
- Gitlab::Diff::Position.new(
- old_path: "files/ruby/popen.rb",
- new_path: "files/ruby/popen.rb",
- old_line: nil,
- new_line: line_number,
- diff_refs: merge_request.diff_refs
- )
- end
+ 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
diff --git a/spec/services/issues/create_service_spec.rb b/spec/services/issues/create_service_spec.rb
index 6045d00ff09..776cbc4296b 100644
--- a/spec/services/issues/create_service_spec.rb
+++ b/spec/services/issues/create_service_spec.rb
@@ -140,46 +140,85 @@ 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
- described_class.new(project, user, opts).execute
- discussion.first_note.reload
+ describe 'for a single discussion' do
+ let(:opts) { { discussion_to_resolve: discussion.id, merge_request_to_resolve_discussions_of: merge_request.iid } }
- expect(discussion.resolved?).to be(true)
- end
+ it 'resolves the discussion' do
+ described_class.new(project, user, opts).execute
+ discussion.first_note.reload
- it 'added a system note to the discussion' do
- described_class.new(project, user, opts).execute
+ expect(discussion.resolved?).to be(true)
+ end
- reloaded_discussion = MergeRequest.find(merge_request.id).discussions.first
+ it 'added a system note to the discussion' do
+ described_class.new(project, user, opts).execute
- expect(reloaded_discussion.last_note.system).to eq(true)
- end
+ 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 'assigns the title and description for the issue' do
- issue = described_class.new(project, user, opts).execute
+ 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.title).not_to be_nil
- expect(issue.description).not_to be_nil
+ expect(issue.description).to be_nil
+ expect(issue.title).to be_nil
+ end
end
- it 'can set nil explicityly to the title and description' do
- issue = described_class.new(project, user,
- merge_request_for_resolving_discussions: merge_request,
- description: nil,
- title: nil).execute
+ 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(issue.description).to be_nil
- expect(issue.title).to be_nil
+ 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
diff --git a/spec/services/issues/resolve_discussions_spec.rb b/spec/services/issues/resolve_discussions_spec.rb
new file mode 100644
index 00000000000..6cc738aec08
--- /dev/null
+++ b/spec/services/issues/resolve_discussions_spec.rb
@@ -0,0 +1,106 @@
+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
diff --git a/spec/support/api/issues_resolving_discussions_shared_examples.rb b/spec/support/api/issues_resolving_discussions_shared_examples.rb
new file mode 100644
index 00000000000..d26d279363c
--- /dev/null
+++ b/spec/support/api/issues_resolving_discussions_shared_examples.rb
@@ -0,0 +1,15 @@
+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
diff --git a/spec/support/features/resolving_discussions_in_issues_shared_examples.rb b/spec/support/features/resolving_discussions_in_issues_shared_examples.rb
new file mode 100644
index 00000000000..4a946995f84
--- /dev/null
+++ b/spec/support/features/resolving_discussions_in_issues_shared_examples.rb
@@ -0,0 +1,41 @@
+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