diff options
author | Bob Van Landuyt <bob@vanlanduyt.co> | 2016-12-22 15:31:12 +0300 |
---|---|---|
committer | Bob Van Landuyt <bob@gitlab.com> | 2017-03-13 10:27:51 +0300 |
commit | 0267b83898d604181e70c5ea8ac4a84108d2e6d6 (patch) | |
tree | 3ce8cc81ba90185c976f3b19b7e548c67856370f /app | |
parent | 9ed3db915026c6e0cd266a1c276386e3e96d2151 (diff) |
Delegate a single discussion to a new issue
Delegate a discussion in a merge request into a new issue.
The discussion wil be marked as resolved and a system note will be
added linking to the newly created issue.
Diffstat (limited to 'app')
-rw-r--r-- | app/assets/javascripts/diff_notes/components/new_issue_for_discussion.js.es6 | 29 | ||||
-rw-r--r-- | app/assets/javascripts/diff_notes/diff_notes_bundle.js | 3 | ||||
-rw-r--r-- | app/assets/stylesheets/pages/note_form.scss | 21 | ||||
-rw-r--r-- | app/controllers/projects/issues_controller.rb | 24 | ||||
-rw-r--r-- | app/finders/notes_finder.rb | 10 | ||||
-rw-r--r-- | app/services/discussions/resolve_service.rb | 8 | ||||
-rw-r--r-- | app/services/issues/base_service.rb | 25 | ||||
-rw-r--r-- | app/services/issues/build_service.rb | 21 | ||||
-rw-r--r-- | app/services/issues/create_service.rb | 17 | ||||
-rw-r--r-- | app/views/discussions/_new_issue_for_discussion.html.haml | 8 | ||||
-rw-r--r-- | app/views/discussions/_notes.html.haml | 4 | ||||
-rw-r--r-- | app/views/shared/icons/_icon_mr_issue.svg | 1 | ||||
-rw-r--r-- | app/views/shared/issuable/_form.html.haml | 18 |
13 files changed, 160 insertions, 29 deletions
diff --git a/app/assets/javascripts/diff_notes/components/new_issue_for_discussion.js.es6 b/app/assets/javascripts/diff_notes/components/new_issue_for_discussion.js.es6 new file mode 100644 index 00000000000..e86bef47172 --- /dev/null +++ b/app/assets/javascripts/diff_notes/components/new_issue_for_discussion.js.es6 @@ -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/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb index 1151555b8fa..d7eb73d7ff5 100644 --- a/app/controllers/projects/issues_controller.rb +++ b/app/controllers/projects/issues_controller.rb @@ -64,7 +64,12 @@ class Projects::IssuesController < Projects::ApplicationController params[:issue] ||= ActionController::Parameters.new( assignee_id: "" ) - build_params = issue_params.merge(merge_request_for_resolving_discussions: merge_request_for_resolving_discussions) + + build_params = issue_params.merge( + merge_request_for_resolving_discussions: merge_request_for_resolving_discussions, + discussion_to_resolve: discussion_to_resolve + ) + @issue = @noteable = Issues::BuildService.new(project, current_user, build_params).execute respond_with(@issue) @@ -94,10 +99,12 @@ class Projects::IssuesController < Projects::ApplicationController end def create - create_params = issue_params - .merge(merge_request_for_resolving_discussions: merge_request_for_resolving_discussions) - .merge(spammable_params) - + create_params = issue_params. + merge( + merge_request_for_resolving_discussions: merge_request_for_resolving_discussions, + discussion_to_resolve: discussion_to_resolve + ). + merge(spammable_params) @issue = Issues::CreateService.new(project, current_user, create_params).execute respond_to do |format| @@ -193,6 +200,13 @@ class Projects::IssuesController < Projects::ApplicationController find_by(iid: merge_request_iid) end + def discussion_to_resolve + return unless discussion_id = params[:discussion_to_resolve] + + @discussion_to_resolve ||= NotesFinder.new(project, current_user, discussion_id: discussion_id). + first_discussion + end + def authorize_read_issue! return render_404 unless can?(current_user, :read_issue, @issue) end diff --git a/app/finders/notes_finder.rb b/app/finders/notes_finder.rb index 6630c6384f2..c93e6b05a67 100644 --- a/app/finders/notes_finder.rb +++ b/app/finders/notes_finder.rb @@ -11,6 +11,7 @@ class NotesFinder # target_type: string # target_id: integer # last_fetched_at: time + # discussion_id: string # search: string # def initialize(project, current_user, params = {}) @@ -22,9 +23,14 @@ class NotesFinder def execute @notes = since_fetch_at(@params[:last_fetched_at]) if @params[:last_fetched_at] + @notes = for_discussion(@params[:discussion_id]) if @params[:discussion_id] @notes end + def first_discussion + execute.discussions.first + end + private def init_collection @@ -100,4 +106,8 @@ class NotesFinder @notes.where('updated_at > ?', last_fetched_at - FETCH_OVERLAP).fresh end + + def for_discussion(discussion_id) + @notes.where(Note.arel_table[:discussion_id].eq(discussion_id)) + end end diff --git a/app/services/discussions/resolve_service.rb b/app/services/discussions/resolve_service.rb index 0437195f588..4a803b47bba 100644 --- a/app/services/discussions/resolve_service.rb +++ b/app/services/discussions/resolve_service.rb @@ -9,7 +9,13 @@ module Discussions discussion.resolve!(current_user) - MergeRequests::ResolvedDiscussionNotificationService.new(project, current_user).execute(merge_request) + notify_discussion_resolved(discussion) + end + + def notify_discussion_resolved(discussion) + noteable = merge_request || discussion.noteable + + MergeRequests::ResolvedDiscussionNotificationService.new(project, current_user).execute(noteable) SystemNoteService.discussion_continued_in_issue(discussion, project, current_user, follow_up_issue) if follow_up_issue end diff --git a/app/services/issues/base_service.rb b/app/services/issues/base_service.rb index 35af867a098..3aa842eb9a8 100644 --- a/app/services/issues/base_service.rb +++ b/app/services/issues/base_service.rb @@ -1,11 +1,12 @@ module Issues class BaseService < ::IssuableBaseService - attr_reader :merge_request_for_resolving_discussions + attr_reader :merge_request_for_resolving_discussions, :discussion_to_resolve def initialize(*args) super @merge_request_for_resolving_discussions ||= params.delete(:merge_request_for_resolving_discussions) + @discussion_to_resolve ||= params.delete(:discussion_to_resolve) end def hook_data(issue, action) @@ -15,6 +16,28 @@ module Issues issue_data end + def merge_request_for_resolving_discussions + @merge_request_for_resolving_discussions ||= discussion_to_resolve.try(:noteable) + end + + def for_all_discussions_in_a_merge_request? + discussion_to_resolve.nil? && merge_request_for_resolving_discussions + end + + def for_single_discussion? + discussion_to_resolve && discussion_to_resolve.noteable == merge_request_for_resolving_discussions + end + + def discussions_to_resolve + @discussions_to_resolve ||= if for_all_discussions_in_a_merge_request? + merge_request_for_resolving_discussions.resolvable_discussions + elsif for_single_discussion? + Array(discussion_to_resolve) + else + [] + end + end + private def execute_hooks(issue, action = 'open') diff --git a/app/services/issues/build_service.rb b/app/services/issues/build_service.rb index 7cd927d8005..1bcc5abd3f2 100644 --- a/app/services/issues/build_service.rb +++ b/app/services/issues/build_service.rb @@ -4,32 +4,35 @@ module Issues @issue = project.issues.new(issue_params) end - def issue_params_with_info_from_merge_request + def issue_params_with_info_from_discussions return {} unless merge_request_for_resolving_discussions - { title: title_from_merge_request, description: description_from_merge_request } + { title: title_for_merge_request, description: description_for_discussions } end - def title_from_merge_request + def title_for_merge_request "Follow-up from \"#{merge_request_for_resolving_discussions.title}\"" end - def description_from_merge_request - if merge_request_for_resolving_discussions.resolvable_discussions.empty? + def description_for_discussions + if discussions_to_resolve.empty? return "There are no unresolved discussions. "\ "Review the conversation in #{merge_request_for_resolving_discussions.to_reference}" end - description = "The following discussions from #{merge_request_for_resolving_discussions.to_reference} should be addressed:" + description = "The following #{'discussion'.pluralize(discussions_to_resolve.size)} "\ + "from #{merge_request_for_resolving_discussions.to_reference} "\ + "should be addressed:" + [description, *items_for_discussions].join("\n\n") end def items_for_discussions - merge_request_for_resolving_discussions.resolvable_discussions.map { |discussion| item_for_discussion(discussion) } + discussions_to_resolve.map { |discussion| item_for_discussion(discussion) } end def item_for_discussion(discussion) - first_note = discussion.first_note_to_resolve + first_note = discussion.first_note_to_resolve || discussion.first_note other_note_count = discussion.notes.size - 1 creation_time = first_note.created_at.to_s(:medium) note_url = Gitlab::UrlBuilder.build(first_note) @@ -44,7 +47,7 @@ module Issues end def issue_params - @issue_params ||= issue_params_with_info_from_merge_request.merge(whitelisted_issue_params) + @issue_params ||= issue_params_with_info_from_discussions.merge(whitelisted_issue_params) end def whitelisted_issue_params diff --git a/app/services/issues/create_service.rb b/app/services/issues/create_service.rb index 85b6eb3fe3d..d2cc70ed0dc 100644 --- a/app/services/issues/create_service.rb +++ b/app/services/issues/create_service.rb @@ -5,7 +5,11 @@ module Issues def execute filter_spam_check_params - issue_attributes = params.merge(merge_request_for_resolving_discussions: merge_request_for_resolving_discussions) + issue_attributes = params.merge( + merge_request_for_resolving_discussions: merge_request_for_resolving_discussions, + discussion_to_resolve: discussion_to_resolve + ) + @issue = BuildService.new(project, current_user, issue_attributes).execute create(@issue) @@ -21,17 +25,16 @@ module Issues notification_service.new_issue(issuable, current_user) todo_service.new_issue(issuable, current_user) user_agent_detail_service.create - - if merge_request_for_resolving_discussions.try(:discussions_can_be_resolved_by?, current_user) - resolve_discussions_in_merge_request(issuable) - end + resolve_discussions_with_issue(issuable) end - def resolve_discussions_in_merge_request(issue) + def resolve_discussions_with_issue(issue) + return if discussions_to_resolve.empty? + Discussions::ResolveService.new(project, current_user, merge_request: merge_request_for_resolving_discussions, follow_up_issue: issue). - execute(merge_request_for_resolving_discussions.resolvable_discussions) + execute(discussions_to_resolve) end private 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..de28ded9820 --- /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, 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..4a1e93cec3a 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 + = render "discussions/jump_to_next", discussion: discussion - else = link_to_reply_discussion(discussion) 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..e93fe0f6cb5 100644 --- a/app/views/shared/issuable/_form.html.haml +++ b/app/views/shared/issuable/_form.html.haml @@ -48,18 +48,32 @@ - if @merge_request_for_resolving_discussions .form-group .col-sm-10.col-sm-offset-2 + = icon('exclamation-triangle') - if @merge_request_for_resolving_discussions.discussions_can_be_resolved_by?(current_user) - = icon('exclamation-triangle') Creating this issue will mark all discussions in = link_to @merge_request_for_resolving_discussions.to_reference, merge_request_path(@merge_request_for_resolving_discussions) as resolved. = hidden_field_tag 'merge_request_for_resolving_discussions', @merge_request_for_resolving_discussions.iid - else - = icon('exclamation-triangle') You can't automatically mark all discussions in = link_to @merge_request_for_resolving_discussions.to_reference, merge_request_path(@merge_request_for_resolving_discussions) as resolved. Ask someone with sufficient rights to resolve the them. +- if @discussion_to_resolve + .form-group + .col-sm-10.col-sm-offset-2 + = icon('exclamation-triangle') + - if @discussion_to_resolve.can_resolve?(current_user) + Creating this issue will mark the discussion at + = link_to @discussion_to_resolve.noteable.to_reference, Gitlab::UrlBuilder.build(@discussion_to_resolve.first_note) + as resolved. + = hidden_field_tag 'discussion_to_resolve', @discussion_to_resolve.id + - else + You can't automatically mark the discussion at + = link_to @discussion_to_resolve.noteable.to_reference, Gitlab::UrlBuilder.build(@discussion_to_resolve.first_note) + as resolved. Ask someone with sufficient rights to resolve it. + + - is_footer = !(issuable.is_a?(MergeRequest) && issuable.new_record?) .row-content-block{ class: (is_footer ? "footer-block" : "middle-block") } .pull-right |