diff options
author | Bob Van Landuyt <bob@vanlanduyt.co> | 2016-10-27 00:21:50 +0300 |
---|---|---|
committer | Bob Van Landuyt <bob@vanlanduyt.co> | 2016-12-05 22:55:45 +0300 |
commit | 1123057ab792ac73b1611f4d3a9faf79369dd6da (patch) | |
tree | ae88332a5d2d47d5a814583fbe874582ebaf859c /app | |
parent | 5fedc46343d054df28ce81a320f33d1d43d02bc8 (diff) |
Feature: delegate all open discussions to Issue
When a merge request can only be merged when all discussions are
resolved. This feature allows to easily delegate those discussions to a
new issue, while marking them as resolved in the merge request.
The user is presented with a new issue, prepared with mentions of all
unresolved discussions, including the first unresolved note of the
discussion, time and link to the note.
When the issue is created, the discussions in the merge request will get
a system note directing the user to the newly created issue.
Diffstat (limited to 'app')
-rw-r--r-- | app/controllers/projects/discussions_controller.rb | 4 | ||||
-rw-r--r-- | app/controllers/projects/issues_controller.rb | 15 | ||||
-rw-r--r-- | app/models/discussion.rb | 4 | ||||
-rw-r--r-- | app/models/merge_request.rb | 8 | ||||
-rw-r--r-- | app/models/note.rb | 2 | ||||
-rw-r--r-- | app/services/discussions/base_service.rb | 4 | ||||
-rw-r--r-- | app/services/discussions/resolve_service.rb | 24 | ||||
-rw-r--r-- | app/services/issuable_base_service.rb | 5 | ||||
-rw-r--r-- | app/services/issues/base_service.rb | 8 | ||||
-rw-r--r-- | app/services/issues/build_service.rb | 50 | ||||
-rw-r--r-- | app/services/issues/create_service.rb | 14 | ||||
-rw-r--r-- | app/services/system_note_service.rb | 8 | ||||
-rw-r--r-- | app/views/projects/merge_requests/widget/open/_unresolved_discussions.html.haml | 6 | ||||
-rw-r--r-- | app/views/shared/issuable/_form.html.haml | 15 |
14 files changed, 157 insertions, 10 deletions
diff --git a/app/controllers/projects/discussions_controller.rb b/app/controllers/projects/discussions_controller.rb index d174e1145a7..148e39630e3 100644 --- a/app/controllers/projects/discussions_controller.rb +++ b/app/controllers/projects/discussions_controller.rb @@ -5,9 +5,7 @@ class Projects::DiscussionsController < Projects::ApplicationController before_action :authorize_resolve_discussion! def resolve - discussion.resolve!(current_user) - - MergeRequests::ResolvedDiscussionNotificationService.new(project, current_user).execute(merge_request) + Discussions::ResolveService.new(project, current_user, merge_request: merge_request).execute(discussion) render json: { resolved_by: discussion.resolved_by.try(:name), diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb index 4aea7bb62c4..4f66e01e0f7 100644 --- a/app/controllers/projects/issues_controller.rb +++ b/app/controllers/projects/issues_controller.rb @@ -46,8 +46,9 @@ 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 - @issue = @noteable = @project.issues.new(issue_params) respond_with(@issue) end @@ -75,7 +76,9 @@ class Projects::IssuesController < Projects::ApplicationController end def create - @issue = Issues::CreateService.new(project, current_user, issue_params.merge(request: request)).execute + extra_params = { request: request, + merge_request_for_resolving_discussions: merge_request_for_resolving_discussions } + @issue = Issues::CreateService.new(project, current_user, issue_params.merge(extra_params)).execute respond_to do |format| format.html do @@ -169,6 +172,14 @@ 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/models/discussion.rb b/app/models/discussion.rb index 75a85563235..bbe813db823 100644 --- a/app/models/discussion.rb +++ b/app/models/discussion.rb @@ -88,6 +88,10 @@ class Discussion @first_note ||= @notes.first end + def first_note_to_resolve + @first_note_to_resolve ||= notes.detect(&:to_be_resolved?) + end + def last_note @last_note ||= @notes.last end diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index bfb016df46d..a6fc9bb120d 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -480,6 +480,14 @@ class MergeRequest < ActiveRecord::Base @diff_discussions ||= self.notes.diff_notes.discussions end + def resolvable_discussions + @resolvable_discussions ||= diff_discussions.select(&:to_be_resolved?) + end + + def discussions_can_be_resolved_by?(user) + resolvable_discussions.all? { |discussion| discussion.can_resolve?(user) } + end + def find_diff_discussion(discussion_id) notes = self.notes.diff_notes.where(discussion_id: discussion_id).fresh.to_a return if notes.empty? diff --git a/app/models/note.rb b/app/models/note.rb index 5b50ca285c3..08bd08743ef 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -99,7 +99,7 @@ class Note < ActiveRecord::Base end def discussions - Discussion.for_notes(all) + Discussion.for_notes(fresh) end def grouped_diff_discussions diff --git a/app/services/discussions/base_service.rb b/app/services/discussions/base_service.rb new file mode 100644 index 00000000000..e4dfe6e71bb --- /dev/null +++ b/app/services/discussions/base_service.rb @@ -0,0 +1,4 @@ +module Discussions + class BaseService < ::BaseService + end +end diff --git a/app/services/discussions/resolve_service.rb b/app/services/discussions/resolve_service.rb new file mode 100644 index 00000000000..0437195f588 --- /dev/null +++ b/app/services/discussions/resolve_service.rb @@ -0,0 +1,24 @@ +module Discussions + class ResolveService < Discussions::BaseService + def execute(one_or_more_discussions) + Array(one_or_more_discussions).each { |discussion| resolve_discussion(discussion) } + end + + def resolve_discussion(discussion) + return unless discussion.can_resolve?(current_user) + + discussion.resolve!(current_user) + + MergeRequests::ResolvedDiscussionNotificationService.new(project, current_user).execute(merge_request) + SystemNoteService.discussion_continued_in_issue(discussion, project, current_user, follow_up_issue) if follow_up_issue + end + + def merge_request + params[:merge_request] + end + + def follow_up_issue + params[:follow_up_issue] + end + end +end diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb index ce68e433ab8..b5f63cc5a1a 100644 --- a/app/services/issuable_base_service.rb +++ b/app/services/issuable_base_service.rb @@ -120,9 +120,10 @@ class IssuableBaseService < BaseService def merge_slash_commands_into_params!(issuable) description, command_params = SlashCommands::InterpretService.new(project, current_user). - execute(params[:description], issuable) + execute(params[:description], issuable) - params[:description] = description + # Avoid a description already set on an issuable to be overwritten by a nil + params[:description] = description if params.has_key?(:description) params.merge!(command_params) end diff --git a/app/services/issues/base_service.rb b/app/services/issues/base_service.rb index 9ea3ce084ba..742e834df97 100644 --- a/app/services/issues/base_service.rb +++ b/app/services/issues/base_service.rb @@ -1,5 +1,13 @@ 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 new file mode 100644 index 00000000000..a63982f60c8 --- /dev/null +++ b/app/services/issues/build_service.rb @@ -0,0 +1,50 @@ +module Issues + class BuildService < Issues::BaseService + def execute + @issue = project.issues.new(issue_params) + end + + def issue_params_with_info_from_merge_request + return {} unless merge_request_for_resolving_discussions + + { title: title_from_merge_request, description: description_from_merge_request } + end + + def title_from_merge_request + "Follow-up from \"#{merge_request_for_resolving_discussions.title}\"" + end + + def description_from_merge_request + if merge_request_for_resolving_discussions.resolvable_discussions.empty? + return "There are no unresolved discussions. "\ + "Review the conversation in #{merge_request_for_resolving_discussions.to_reference}" + end + + description = "The following discussions from #{merge_request_for_resolving_discussions.to_reference} should be addressed:" + [description, *items_for_discussions].join("\n\n") + end + + def items_for_discussions + merge_request_for_resolving_discussions.resolvable_discussions.map { |discussion| item_for_discussion(discussion) } + end + + def item_for_discussion(discussion) + first_note = discussion.first_note_to_resolve + other_note_count = discussion.notes.size - 1 + creation_time = first_note.created_at.to_s(:medium) + note_url = Gitlab::UrlBuilder.build(first_note) + + discussion_info = "- [ ] #{first_note.author.to_reference} commented in a discussion on [#{creation_time}](#{note_url}): " + discussion_info << " (+#{other_note_count} #{'comment'.pluralize(other_note_count)})" if other_note_count > 0 + + note_without_block_quotes = Banzai::Filter::BlockquoteFenceFilter.new(first_note.note).call + quote = ">>>\n#{note_without_block_quotes}\n>>>" + + [discussion_info, quote].join("\n\n") + end + + def issue_params + @issue_params ||= issue_params_with_info_from_merge_request.merge(params.slice(:title, :description)) + end + end +end diff --git a/app/services/issues/create_service.rb b/app/services/issues/create_service.rb index ea1690f3e38..d2eb46ac41b 100644 --- a/app/services/issues/create_service.rb +++ b/app/services/issues/create_service.rb @@ -4,7 +4,8 @@ module Issues @request = params.delete(:request) @api = params.delete(:api) - @issue = project.issues.new + issue_attributes = params.merge(merge_request_for_resolving_discussions: merge_request_for_resolving_discussions) + @issue = BuildService.new(project, current_user, issue_attributes).execute create(@issue) end @@ -18,6 +19,17 @@ 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 + end + + def resolve_discussions_in_merge_request(issue) + Discussions::ResolveService.new(project, current_user, + merge_request: merge_request_for_resolving_discussions, + follow_up_issue: issue). + execute(merge_request_for_resolving_discussions.resolvable_discussions) end private diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb index 3cf6467804f..8b48d90f60b 100644 --- a/app/services/system_note_service.rb +++ b/app/services/system_note_service.rb @@ -163,6 +163,14 @@ module SystemNoteService create_note(noteable: merge_request, project: project, author: author, note: body) end + def discussion_continued_in_issue(discussion, project, author, issue) + body = "Added #{issue.to_reference} to continue this discussion" + note_attributes = discussion.reply_attributes.merge(project: project, author: author, note: body) + note_attributes[:type] = note_attributes.delete(:note_type) + + create_note(note_attributes) + end + # Called when the title of a Noteable is changed # # noteable - Noteable object that responds to `title` 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 35d5677ee37..e094f97f3b6 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 @@ -3,4 +3,8 @@ This merge request has unresolved discussions %p - Please resolve these discussions to allow this merge request to be merged.
\ No newline at end of file + Please resolve these discussions + - if @project.issues_enabled? && can?(current_user, :create_issue, @project) + or + = link_to "open an issue to resolve them later", new_namespace_project_issue_path(@project.namespace, @project, merge_request_for_resolving_discussions: @merge_request.iid) + to allow this merge request to be merged. diff --git a/app/views/shared/issuable/_form.html.haml b/app/views/shared/issuable/_form.html.haml index 2f05093f435..bdb00bfa33c 100644 --- a/app/views/shared/issuable/_form.html.haml +++ b/app/views/shared/issuable/_form.html.haml @@ -42,6 +42,21 @@ = render 'shared/issuable/form/branch_chooser', issuable: issuable, form: form +- if @merge_request_for_resolving_discussions + .form-group + .col-sm-10.col-sm-offset-2 + - if @merge_request_for_resolving_discussions.discussions_can_be_resolved_by?(current_user) + = icon('exclamation-triangle') + Creating this issue will mark all discussions in + = link_to @merge_request_for_resolving_discussions.to_reference, merge_request_path(@merge_request_for_resolving_discussions) + as resolved. + = hidden_field_tag 'merge_request_for_resolving_discussions', @merge_request_for_resolving_discussions.iid + - else + = icon('exclamation-triangle') + You can't automatically mark all discussions in + = link_to @merge_request_for_resolving_discussions.to_reference, merge_request_path(@merge_request_for_resolving_discussions) + as resolved. Ask someone with sufficient rights to resolve the them. + - is_footer = !(issuable.is_a?(MergeRequest) && issuable.new_record?) .row-content-block{class: (is_footer ? "footer-block" : "middle-block")} - if issuable.new_record? |