diff options
31 files changed, 571 insertions, 113 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 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/doc/api/issues.md b/doc/api/issues.md index e25841926f8..ddc304ea128 100644 --- a/doc/api/issues.md +++ b/doc/api/issues.md @@ -331,16 +331,17 @@ 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` | +| `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. | +| `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. | ```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/new_issue_for_discussion.png b/doc/user/project/merge_requests/img/new_issue_for_discussion.png Binary files differnew file mode 100644 index 00000000000..93c9dad8921 --- /dev/null +++ b/doc/user/project/merge_requests/img/new_issue_for_discussion.png 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 Binary files differnew file mode 100644 index 00000000000..798365aa4db --- /dev/null +++ b/doc/user/project/merge_requests/img/preview_issue_for_discussion.png 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..63274c86bbb 100644 --- a/doc/user/project/merge_requests/merge_request_discussion_resolution.md +++ b/doc/user/project/merge_requests/merge_request_discussion_resolution.md @@ -72,9 +72,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. + + + +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. + + + +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..9aac3c8fe1a 100644 --- a/lib/api/issues.rb +++ b/lib/api/issues.rb @@ -118,6 +118,8 @@ module API desc: 'Date time when the issue was created. Available only for admins and project owners.' optional :merge_request_for_resolving_discussions, 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' use :issue_params end post ':id/issues' do @@ -134,6 +136,11 @@ module API find_by(iid: merge_request_iid) end + if discussion_id = params[:discussion_to_resolve] + issue_params[:discussion_to_resolve] = NotesFinder.new(user_project, current_user, discussion_id: discussion_id). + first_discussion + end + issue = ::Issues::CreateService.new(user_project, current_user, issue_params.merge(request: request, api: true)).execute diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb index 46c758b4654..336e3b50a72 100644 --- a/spec/controllers/projects/issues_controller_spec.rb +++ b/spec/controllers/projects/issues_controller_spec.rb @@ -109,6 +109,15 @@ describe Projects::IssuesController do 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, discussion_to_resolve: note.discussion_id + + expect(assigns(:issue).title).not_to be_empty + expect(assigns(:issue).description).not_to be_empty + end end context 'external issue tracker' 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..62ecd0100ae 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 @@ -42,35 +42,13 @@ feature 'Resolving all open discussions in a merge request from an issue', featu 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 + it 'has a hidden field for the discussion' 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 + it_behaves_like 'creating an issue for a discussion' 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..6cf67639b7e --- /dev/null +++ b/spec/features/issues/create_issue_for_single_discussion_in_merge_request.rb @@ -0,0 +1,63 @@ +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 } + + 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) + + 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) + 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 diff --git a/spec/finders/notes_finder_spec.rb b/spec/finders/notes_finder_spec.rb index 77a04507be1..5fc2e3dd527 100644 --- a/spec/finders/notes_finder_spec.rb +++ b/spec/finders/notes_finder_spec.rb @@ -41,6 +41,17 @@ describe NotesFinder do expect(notes.count).to eq(0) end + context 'for a certain discussion' do + let!(:note_in_a_commit_discussion) { create(:note_on_commit, project: project) } + let!(:other_note) { create(:note_on_merge_request, project: project) } + + it 'finds the only the notes for a certain discussion id' do + notes = described_class.new(project, user, discussion_id: note_in_a_commit_discussion.discussion_id).execute + + expect(notes).to contain_exactly(note_in_a_commit_discussion) + end + end + context 'on restricted projects' do let(:project) do create(:empty_project, @@ -146,6 +157,28 @@ describe NotesFinder do end end + describe "#first_discussion" do + let!(:discussion_note) { create(:note_on_merge_request, project: project) } + + it 'returns a discussion' do + discussion = described_class.new(project, user, discussion_id: discussion_note.discussion_id).first_discussion + + expect(discussion).is_a? Discussion + end + + it 'includes the notes in the discussion' do + discussion = described_class.new(project, user, discussion_id: discussion_note.discussion_id).first_discussion + + expect(discussion.notes).to include(discussion_note) + end + + it 'returns nil when no notes are found' do + discussion = described_class.new(project, user, discussion_id: 'non-existant').first_discussion + + expect(discussion).to be(nil) + end + end + describe '.search' do let(:project) { create(:empty_project, :public) } let(:note) { create(:note_on_issue, note: 'WoW', project: project) } diff --git a/spec/requests/api/issues_spec.rb b/spec/requests/api/issues_spec.rb index 2fc11a3b782..4d52ab2aaa2 100644 --- a/spec/requests/api/issues_spec.rb +++ b/spec/requests/api/issues_spec.rb @@ -928,29 +928,33 @@ 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_for_resolving_discussions: 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', + discussion_to_resolve: discussion.id + end + + it_behaves_like 'creating an issue resolving discussions through the API' end end diff --git a/spec/services/discussions/resolve_service_spec.rb b/spec/services/discussions/resolve_service_spec.rb index 12c3cdf28c6..5eb5b2b04d7 100644 --- a/spec/services/discussions/resolve_service_spec.rb +++ b/spec/services/discussions/resolve_service_spec.rb @@ -6,7 +6,7 @@ describe Discussions::ResolveService do 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) } + let(:service) { described_class.new(discussion.noteable.project, user) } before do project.team << [user, :master] diff --git a/spec/services/issues/base_service_spec.rb b/spec/services/issues/base_service_spec.rb new file mode 100644 index 00000000000..bfeefe3bcc5 --- /dev/null +++ b/spec/services/issues/base_service_spec.rb @@ -0,0 +1,112 @@ +require 'spec_helper.rb' + +describe Issues::BaseService, 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 "#for_single_discussion" do + it "is true when only a discussion is passed" do + service = described_class.new(project, user, discussion_to_resolve: discussion) + + expect(service.for_single_discussion?).to be_truthy + end + + it "is true when matching merge request and discussion are passed" do + service = described_class.new( + project, + user, + discussion_to_resolve: discussion, + merge_request_for_resolving_discussions: merge_request + ) + + expect(service.for_single_discussion?).to be_truthy + end + + it "is false when a discussion and another merge request are passed" do + service = described_class.new( + project, + user, + discussion_to_resolve: discussion, + merge_request_for_resolving_discussions: other_merge_request + ) + + expect(service.for_single_discussion?).to be_falsy + end + end + + describe "#for_all_discussions_in_a_merge_request" do + it "is true when only a merge request is passed" do + service = described_class.new(project, user, merge_request_for_resolving_discussions: merge_request) + + expect(service.for_all_discussions_in_a_merge_request?).to be_truthy + end + + it "is false when matching merge request and discussion are passed" do + service = described_class.new( + project, + user, + discussion_to_resolve: discussion, + merge_request_for_resolving_discussions: merge_request + ) + + expect(service.for_all_discussions_in_a_merge_request?).to be_falsy + end + end + + describe "#discussions_to_resolve" do + it "only contains a single discussion when only a discussion is passed" do + service = described_class.new(project, user, discussion_to_resolve: discussion) + + expect(service.discussions_to_resolve).to contain_exactly(discussion) + end + + it "is contains a single discussion when matching merge request and discussion are passed" do + service = described_class.new( + project, + user, + discussion_to_resolve: discussion, + merge_request_for_resolving_discussions: merge_request + ) + + expect(service.discussions_to_resolve).to contain_exactly(discussion) + 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_for_resolving_discussions: merge_request + ) + # 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 "is empty when a discussion and another merge request are passed" do + service = described_class.new( + project, + user, + discussion_to_resolve: discussion, + merge_request_for_resolving_discussions: other_merge_request + ) + + expect(service.discussions_to_resolve).to be_empty + end + end + end +end diff --git a/spec/services/issues/build_service_spec.rb b/spec/services/issues/build_service_spec.rb index 09807e5d35b..cca7cdbdd2d 100644 --- a/spec/services/issues/build_service_spec.rb +++ b/spec/services/issues/build_service_spec.rb @@ -8,23 +8,36 @@ describe Issues::BuildService, services: true do project.team << [user, :developer] end + context 'for a single discussion' do + describe '#execute' do + it 'references the noteable title in the issue title' do + merge_request = create(:merge_request, title: "Hello world", source_project: project) + discussion = Discussion.new([create(:note_on_merge_request, project: project, noteable: merge_request)]) + service = described_class.new(project, user, discussion_to_resolve: discussion) + + issue = service.execute + + expect(issue.title).to include('Hello world') + end + + it 'adds the note content to the description' do + discussion = Discussion.new([create(:note_on_merge_request, project: project, note: "Almost done")]) + service = described_class.new(project, user, discussion_to_resolve: discussion) + + 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 - 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)) + 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_for_resolving_discussions: merge_request) service.execute @@ -82,7 +95,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 +112,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 diff --git a/spec/services/issues/create_service_spec.rb b/spec/services/issues/create_service_spec.rb index 6045d00ff09..4a5780f257a 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 } } - 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_for_resolving_discussions: 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_for_resolving_discussions: merge_request } } + + 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_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 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..8712e688d33 --- /dev/null +++ b/spec/support/features/resolving_discussions_in_issues_shared_examples.rb @@ -0,0 +1,25 @@ +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 +end |
