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? diff --git a/changelogs/unreleased/23589-open-issue-for-mr.yml b/changelogs/unreleased/23589-open-issue-for-mr.yml new file mode 100644 index 00000000000..cea48b85254 --- /dev/null +++ b/changelogs/unreleased/23589-open-issue-for-mr.yml @@ -0,0 +1,5 @@ +--- +title: Resolve all discussions in a merge request by creating an issue collecting + them +merge_request: 7180 +author: Bob Van Landuyt diff --git a/doc/api/issues.md b/doc/api/issues.md index 16f8e32c82a..119125bcd3d 100644 --- a/doc/api/issues.md +++ b/doc/api/issues.md @@ -330,6 +330,7 @@ POST /projects/:id/issues | `labels` | string | no | Comma-separated label names for an issue | | `created_at` | string | no | Date time string, ISO 8601 formatted, e.g. `2016-03-11T03:45:40Z` (requires admin or project owner rights) | | `due_date` | string | no | Date time string in the format YEAR-MONTH-DAY, e.g. `2016-03-11` | +| `merge_request_for_resolving_discussions` | integer | no | The IID of a merge request in which to resolve all issues. This will fill the issue with a default description and mark all discussions as resolved. When passing a description or title, these values will take precedence over the default values. | ```bash curl --request POST --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v3/projects/4/issues?title=Issues%20with%20auth&labels=bug @@ -506,7 +507,7 @@ Example response: ## Subscribe to an issue -Subscribes the authenticated user to an issue to receive notifications. +Subscribes the authenticated user to an issue to receive notifications. If the user is already subscribed to the issue, the status code `304` is returned. 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 new file mode 100644 index 00000000000..9fdd387676c Binary files /dev/null and b/doc/user/project/merge_requests/img/preview_issue_for_discussions.png 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 285b1798ac5..f37f1ce4d21 100644 --- a/doc/user/project/merge_requests/merge_request_discussion_resolution.md +++ b/doc/user/project/merge_requests/merge_request_discussion_resolution.md @@ -37,7 +37,8 @@ resolved discussions tracker. > [Introduced][ce-7125] in GitLab 8.14. -You can prevent merge requests from being merged until all discussions are resolved. +You can prevent merge requests from being merged until all discussions are +resolved. Navigate to your project's settings page, select the **Only allow merge requests to be merged if all discussions are resolved** check @@ -50,8 +51,26 @@ are resolved. ![Only allow merge if all the discussions are resolved message](img/only_allow_merge_if_all_discussions_are_resolved_msg.png) +### Move all unresolved discussions in a merge request to an issue + +> [Introduced][ce-7180] (Currently on Backlog) + +To delegate unresolved discussions to a new issue you can click the link **open +an issue to resolve them later**. + +This will prepare an issue with content referring to the merge request and +discussions. + +![Issue mentioning discussions in a merge request](img/preview_issue_for_discussions.png) + +Hitting **Submit issue** will cause all discussions to be marked as resolved and +add a note referring to the newly created issue. + +You can now proceed to merge the merge request from the UI. + [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 [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 049b4fb214c..cfb7c45de8e 100644 --- a/lib/api/issues.rb +++ b/lib/api/issues.rb @@ -28,6 +28,14 @@ module API new_params end + + 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: user_project.id). + execute. + find_by(iid: merge_request_iid) + end end resource :issues do @@ -151,24 +159,28 @@ module API # Create a new project issue # # Parameters: - # id (required) - The ID of a project - # title (required) - The title of an issue - # description (optional) - The description of an issue - # assignee_id (optional) - The ID of a user to assign issue - # milestone_id (optional) - The ID of a milestone to assign issue - # labels (optional) - The labels of an issue - # created_at (optional) - Date time string, ISO 8601 formatted - # due_date (optional) - Date time string in the format YEAR-MONTH-DAY - # confidential (optional) - Boolean parameter if the issue should be confidential + # id (required) - The ID of a project + # title (required) - The title of an issue + # description (optional) - The description of an issue + # assignee_id (optional) - The ID of a user to assign issue + # milestone_id (optional) - The ID of a milestone to assign issue + # labels (optional) - The labels of an issue + # created_at (optional) - Date time string, ISO 8601 formatted + # due_date (optional) - Date time string in the format YEAR-MONTH-DAY + # confidential (optional) - Boolean parameter if the issue should be confidential + # merge_request_for_resolving_discussions (optional) - The IID of a merge request for which to resolve discussions # Example Request: # POST /projects/:id/issues post ':id/issues' do required_attributes! [:title] - keys = [:title, :description, :assignee_id, :milestone_id, :due_date, :confidential, :labels] + keys = [:title, :description, :assignee_id, :milestone_id, :due_date, :confidential, :labels, :merge_request_for_resolving_discussions] keys << :created_at if current_user.admin? || user_project.owner == current_user attrs = attributes_for_keys(keys) + attrs[:labels] = params[:labels] if params[:labels] + attrs[:merge_request_for_resolving_discussions] = merge_request_for_resolving_discussions if params[:merge_request_for_resolving_discussions] + # Convert and filter out invalid confidential flags attrs['confidential'] = to_boolean(attrs['confidential']) attrs.delete('confidential') if attrs['confidential'].nil? diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb index 90419368f22..dbe5ddccbcf 100644 --- a/spec/controllers/projects/issues_controller_spec.rb +++ b/spec/controllers/projects/issues_controller_spec.rb @@ -55,6 +55,30 @@ describe Projects::IssuesController do end describe 'GET #new' do + context 'internal issue tracker' do + before do + sign_in(user) + project.team << [user, :developer] + end + + it 'builds a new issue' do + get :new, namespace_id: project.namespace.path, project_id: project + + expect(assigns(:issue)).to be_a_new(Issue) + end + + it 'fills in an issue for a merge request' do + project_with_repository = create(:project) + 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.path, project_id: project_with_repository, merge_request_for_resolving_discussions: mr.iid + + expect(assigns(:issue).title).not_to be_empty + expect(assigns(:issue).description).not_to be_empty + end + end + context 'external issue tracker' do it 'redirects to the external issue tracker' do external = double(new_issue_path: 'https://example.com/issues/new') @@ -272,6 +296,42 @@ describe Projects::IssuesController do end describe 'POST #create' do + context 'resolving discussions in MergeRequest' 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] + sign_in user + end + + let(:merge_request_params) do + { merge_request_for_resolving_discussions: merge_request.iid } + end + + def post_issue(issue_params) + post :create, namespace_id: project.namespace.to_param, project_id: project.to_param, issue: issue_params, merge_request_for_resolving_discussions: merge_request.iid + end + + it 'creates an issue for the project' do + expect { post_issue({ title: 'Hello' }) }.to change { project.issues.reload.size }.by(1) + end + + it "doesn't overwrite given params" do + post_issue(description: 'Manually entered description') + + expect(assigns(:issue).description).to eq('Manually entered description') + end + + it 'resolves the discussion in the merge_request' do + post_issue(title: 'Hello') + discussion.first_note.reload + + expect(discussion.resolved?).to eq(true) + end + end + context 'Akismet is enabled' do before do allow_any_instance_of(SpamService).to receive(:check_for_spam?).and_return(true) 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 new file mode 100644 index 00000000000..762cab0c0e1 --- /dev/null +++ b/spec/features/issues/create_issue_for_discussions_in_merge_request_spec.rb @@ -0,0 +1,76 @@ +require 'rails_helper' + +feature 'Resolving all open discussions in a merge request from an issue', feature: true do + let(:user) { create(:user) } + let(:project) { create(:project, only_allow_merge_if_all_discussions_are_resolved: true) } + let(:merge_request) { create(:merge_request, source_project: project) } + let!(:discussion) { Discussion.for_diff_notes([create(:diff_note_on_merge_request, noteable: merge_request, project: project)]).first } + + before do + project.team << [user, :master] + login_as user + 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 '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) + 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 '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) + 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) + end + + it 'shows an issue with the title filled in' do + title_field = page.find_field('issue[title]') + + expect(title_field.value).to include(merge_request.title) + end + + it 'has a mention of the discussion in the description' do + description_field = page.find_field('issue[description]') + + expect(description_field.value).to include(discussion.first_note.note) + end + + it 'has a hidden field for the merge request' do + merge_request_field = find('#merge_request_for_resolving_discussions', visible: false) + + expect(merge_request_field.value).to eq(merge_request.iid.to_s) + end + + it 'can create a new issue for the project' do + expect { click_button 'Submit issue' }.to change { project.issues.reload.size }.by(1) + end + + it 'resolves the discussion in the merge request' do + click_button 'Submit issue' + + discussion.first_note.reload + + expect(discussion.resolved?).to eq(true) + end + end + end +end diff --git a/spec/models/discussion_spec.rb b/spec/models/discussion_spec.rb index 2a67c60b978..bc32fadd391 100644 --- a/spec/models/discussion_spec.rb +++ b/spec/models/discussion_spec.rb @@ -521,6 +521,15 @@ describe Discussion, model: true do end end + describe "#first_note_to_resolve" do + it "returns the first not that still needs to be resolved" do + allow(first_note).to receive(:to_be_resolved?).and_return(false) + allow(second_note).to receive(:to_be_resolved?).and_return(true) + + expect(subject.first_note_to_resolve).to eq(second_note) + end + end + describe "#collapsed?" do context "when a diff discussion" do before do diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 2cc818af6c7..925232169f1 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -1124,6 +1124,46 @@ describe MergeRequest, models: true do allow(subject).to receive(:diff_discussions).and_return([first_discussion, second_discussion, third_discussion]) end + describe '#resolvable_discussions' do + before do + allow(first_discussion).to receive(:to_be_resolved?).and_return(true) + allow(second_discussion).to receive(:to_be_resolved?).and_return(false) + allow(third_discussion).to receive(:to_be_resolved?).and_return(false) + end + + it 'includes only discussions that need to be resolved' do + expect(subject.resolvable_discussions).to eq([first_discussion]) + end + end + + describe '#discussions_can_be_resolved_by? user' do + let(:user) { build(:user) } + + context 'all discussions can be resolved by the user' do + before do + allow(first_discussion).to receive(:can_resolve?).with(user).and_return(true) + allow(second_discussion).to receive(:can_resolve?).with(user).and_return(true) + allow(third_discussion).to receive(:can_resolve?).with(user).and_return(true) + end + + it 'allows a user to resolve the discussions' do + expect(subject.discussions_can_be_resolved_by?(user)).to be(true) + end + end + + context 'one discussion cannot be resolved by the user' do + before do + allow(first_discussion).to receive(:can_resolve?).with(user).and_return(true) + allow(second_discussion).to receive(:can_resolve?).with(user).and_return(true) + allow(third_discussion).to receive(:can_resolve?).with(user).and_return(false) + end + + it 'allows a user to resolve the discussions' do + expect(subject.discussions_can_be_resolved_by?(user)).to be(false) + end + end + end + describe "#discussions_resolvable?" do context "when all discussions are unresolvable" do before do diff --git a/spec/requests/api/issues_spec.rb b/spec/requests/api/issues_spec.rb index 5700f800c2e..553983575c4 100644 --- a/spec/requests/api/issues_spec.rb +++ b/spec/requests/api/issues_spec.rb @@ -692,6 +692,32 @@ describe API::Issues, api: true do ]) end + context 'resolving issues in a merge request' 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 + + 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 + context 'with due date' do it 'creates a new project issue' do due_date = 2.weeks.from_now.strftime('%Y-%m-%d') diff --git a/spec/services/discussions/resolve_service_spec.rb b/spec/services/discussions/resolve_service_spec.rb new file mode 100644 index 00000000000..12c3cdf28c6 --- /dev/null +++ b/spec/services/discussions/resolve_service_spec.rb @@ -0,0 +1,52 @@ +require 'spec_helper' + +describe Discussions::ResolveService do + describe '#execute' do + let(:discussion) { Discussion.for_diff_notes([create(:diff_note_on_merge_request)]).first } + let(:project) { merge_request.project } + let(:merge_request) { discussion.noteable } + let(:user) { create(:user) } + let(:service) { described_class.new(discussion.noteable.project, user, merge_request: merge_request) } + + before do + project.team << [user, :master] + end + + it "doesn't resolve discussions the user can't resolve" do + expect(discussion).to receive(:can_resolve?).with(user).and_return(false) + + service.execute(discussion) + + expect(discussion.resolved?).to be(false) + end + + it 'resolves the discussion' do + service.execute(discussion) + + expect(discussion.resolved?).to be(true) + end + + it 'executes the notification service' do + expect_any_instance_of(MergeRequests::ResolvedDiscussionNotificationService).to receive(:execute).with(discussion.noteable) + + service.execute(discussion) + end + + it 'adds a system note to the discussion' do + issue = create(:issue, project: project) + + expect(SystemNoteService).to receive(:discussion_continued_in_issue).with(discussion, project, user, issue) + service = described_class.new(project, user, merge_request: merge_request, follow_up_issue: issue) + service.execute(discussion) + end + + it 'can resolve multiple discussions at once' do + other_discussion = Discussion.for_diff_notes([create(:diff_note_on_merge_request, noteable: discussion.noteable, project: discussion.noteable.source_project)]).first + + service.execute([discussion, other_discussion]) + + expect(discussion.resolved?).to be(true) + expect(other_discussion.resolved?).to be(true) + end + end +end diff --git a/spec/services/issues/build_service_spec.rb b/spec/services/issues/build_service_spec.rb new file mode 100644 index 00000000000..4cfba35c830 --- /dev/null +++ b/spec/services/issues/build_service_spec.rb @@ -0,0 +1,130 @@ +require 'spec_helper.rb' + +describe Issues::BuildService, services: true do + let(:project) { create(:project) } + let(:user) { create(:user) } + + before do + project.team << [user, :developer] + end + + context 'for discussions in a merge request' do + let(:merge_request) { create(:merge_request_with_diff_notes, source_project: project) } + let(:issue) { described_class.new(project, user, merge_request_for_resolving_discussions: merge_request).execute } + + 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 + + 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) + + service.execute + + expect(service.items_for_discussions.size).to eq(2) + end + end + + describe '#item_for_discussion' do + let(:service) { described_class.new(project, user, merge_request_for_resolving_discussions: merge_request) } + + it 'mentions the author of the note' do + discussion = Discussion.new([create(:diff_note_on_merge_request, author: create(:user, username: 'author'))]) + expect(service.item_for_discussion(discussion)).to include('@author') + end + + it 'wraps the note in a blockquote' do + note_text = "This is a string\n"\ + ">>>\n"\ + "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" + discussion = Discussion.new([create(:diff_note_on_merge_request, note: note_text)]) + expect(service.item_for_discussion(discussion)).to include(">>>\n#{note_result}\n>>>") + end + end + + describe '#execute' do + it 'has the merge request reference in the title' do + expect(issue.title).to include(merge_request.title) + end + + it 'has the reference of the merge request in the description' do + expect(issue.description).to include(merge_request.to_reference) + end + + it 'does not assign title when a title was given' do + issue = described_class.new(project, user, + merge_request_for_resolving_discussions: merge_request, + title: 'What an issue').execute + + expect(issue.title).to eq('What an issue') + end + + it 'does not assign description when a description was given' do + issue = described_class.new(project, user, + merge_request_for_resolving_discussions: merge_request, + description: 'Fix at your earliest conveignance').execute + + expect(issue.description).to eq('Fix at your earliest conveignance') + end + + 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)) + end + + it 'mentions all the authors in the description' do + authors = merge_request.diff_discussions.map(&:author) + + expect(issue.description).to include(*authors.map(&:to_reference)) + end + + it 'has a link for each unresolved discussion in the description' do + notes = merge_request.diff_discussions.map(&:first_note) + links = notes.map { |note| Gitlab::UrlBuilder.build(note) } + + expect(issue.description).to include(*links) + 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)) + + expect(issue.description).to include('(+2 comments)') + end + end + end + end + + context 'For a merge request without discussions' do + let(:merge_request) { create(:merge_request, source_project: project) } + + 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 + + expect(issue.description).to include("Review the conversation in #{merge_request.to_reference}") + end + end + end + + describe '#execute' do + it 'builds a new issues with given params' do + issue = described_class.new(project, user, title: 'Issue #1', description: 'Issue description').execute + + expect(issue.title).to eq('Issue #1') + expect(issue.description).to eq('Issue description') + end + end +end diff --git a/spec/services/issues/create_service_spec.rb b/spec/services/issues/create_service_spec.rb index 5c0331ebe66..8bde61ee336 100644 --- a/spec/services/issues/create_service_spec.rb +++ b/spec/services/issues/create_service_spec.rb @@ -136,5 +136,48 @@ describe Issues::CreateService, services: true do end it_behaves_like 'new issuable record that supports slash commands' + + context 'for a merge request' 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 + + 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 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 + + expect(issue.description).to be_nil + expect(issue.title).to be_nil + end + end end end diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb index 435cfb07292..07a9d8e1997 100644 --- a/spec/services/system_note_service_spec.rb +++ b/spec/services/system_note_service_spec.rb @@ -712,4 +712,32 @@ describe SystemNoteService, services: true do end end end + + describe '.discussion_continued_in_issue' 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(:issue) { create(:issue, project: project) } + let(:user) { create(:user) } + + def reloaded_merge_request + MergeRequest.find(merge_request.id) + end + + before do + project.team << [user, :developer] + end + + it 'creates a new note in the discussion' do + # we need to completely rebuild the merge request object, or the `@discussions` on the merge request are not reloaded. + expect { SystemNoteService.discussion_continued_in_issue(discussion, project, user, issue) }. + to change { reloaded_merge_request.discussions.first.notes.size }.by(1) + end + + it 'mentions the created issue in the system note' do + note = SystemNoteService.discussion_continued_in_issue(discussion, project, user, issue) + + expect(note.note).to include(issue.to_reference) + end + end end