diff options
author | Bob Van Landuyt <bob@vanlanduyt.co> | 2017-02-27 10:23:36 +0100 |
---|---|---|
committer | Bob Van Landuyt <bob@gitlab.com> | 2017-03-13 08:27:51 +0100 |
commit | f86928953d2d79f40f10813a6e244c1da0779d16 (patch) | |
tree | 7396889bce42589175ec3c2fa314fdf621ef73b1 /app | |
parent | 51253b2dd0bb073d271ddcbd172f7c204d4639db (diff) | |
download | gitlab-ce-f86928953d2d79f40f10813a6e244c1da0779d16.tar.gz |
Always require MR-iid for resolving discussions
And deduplicate the finding of MR's & discussions. Now the searching
is done in the service, istead of the controller & the API.
Diffstat (limited to 'app')
-rw-r--r-- | app/controllers/projects/issues_controller.rb | 36 | ||||
-rw-r--r-- | app/finders/notes_finder.rb | 4 | ||||
-rw-r--r-- | app/services/issues/base_service.rb | 36 | ||||
-rw-r--r-- | app/services/issues/create_service.rb | 4 | ||||
-rw-r--r-- | app/views/discussions/_new_issue_for_discussion.html.haml | 2 | ||||
-rw-r--r-- | app/views/discussions/_notes.html.haml | 2 | ||||
-rw-r--r-- | app/views/shared/issuable/_form.html.haml | 26 |
7 files changed, 40 insertions, 70 deletions
diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb index d7eb73d7ff5..d76643ff875 100644 --- a/app/controllers/projects/issues_controller.rb +++ b/app/controllers/projects/issues_controller.rb @@ -66,11 +66,14 @@ class Projects::IssuesController < Projects::ApplicationController ) build_params = issue_params.merge( - merge_request_for_resolving_discussions: merge_request_for_resolving_discussions, - discussion_to_resolve: discussion_to_resolve + merge_request_for_resolving_discussions: params[:merge_request_for_resolving_discussions], + discussion_to_resolve: params[:discussion_to_resolve] ) + service = Issues::BuildService.new(project, current_user, build_params) + @merge_request_for_resolving_discussions = service.merge_request_for_resolving_discussions + @discussion_to_resolve = service.discussions_to_resolve.first if params[:discussion_to_resolve] - @issue = @noteable = Issues::BuildService.new(project, current_user, build_params).execute + @issue = @noteable = service.execute respond_with(@issue) end @@ -99,12 +102,11 @@ class Projects::IssuesController < Projects::ApplicationController end def create - create_params = issue_params. - merge( - merge_request_for_resolving_discussions: merge_request_for_resolving_discussions, - discussion_to_resolve: discussion_to_resolve - ). - merge(spammable_params) + create_params = issue_params.merge(spammable_params).merge( + merge_request_for_resolving_discussions: params[:merge_request_for_resolving_discussions], + discussion_to_resolve: params[:discussion_to_resolve] + ) + @issue = Issues::CreateService.new(project, current_user, create_params).execute respond_to do |format| @@ -192,21 +194,6 @@ class Projects::IssuesController < Projects::ApplicationController alias_method :awardable, :issue alias_method :spammable, :issue - def merge_request_for_resolving_discussions - return unless merge_request_iid = params[:merge_request_for_resolving_discussions] - - @merge_request_for_resolving_discussions ||= MergeRequestsFinder.new(current_user, project_id: project.id). - execute. - find_by(iid: merge_request_iid) - end - - def 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 @@ -253,6 +240,7 @@ class Projects::IssuesController < Projects::ApplicationController def issue_params params.require(:issue).permit( :title, :assignee_id, :position, :description, :confidential, + :discussion_to_resolve, :merge_request_for_resolving_discussions, :milestone_id, :due_date, :state_event, :task_num, :lock_version, label_ids: [] ) end diff --git a/app/finders/notes_finder.rb b/app/finders/notes_finder.rb index 6d0ff1798ce..98c66dfa021 100644 --- a/app/finders/notes_finder.rb +++ b/app/finders/notes_finder.rb @@ -27,10 +27,6 @@ class NotesFinder @notes end - def first_discussion - execute.discussions.first - end - private def init_collection diff --git a/app/services/issues/base_service.rb b/app/services/issues/base_service.rb index 3aa842eb9a8..058c398df02 100644 --- a/app/services/issues/base_service.rb +++ b/app/services/issues/base_service.rb @@ -1,12 +1,11 @@ module Issues class BaseService < ::IssuableBaseService - attr_reader :merge_request_for_resolving_discussions, :discussion_to_resolve - + attr_reader :merge_request_for_resolving_discussions_iid, :discussion_to_resolve_id def initialize(*args) super - @merge_request_for_resolving_discussions ||= params.delete(:merge_request_for_resolving_discussions) - @discussion_to_resolve ||= params.delete(:discussion_to_resolve) + @merge_request_for_resolving_discussions_iid ||= params.delete(:merge_request_for_resolving_discussions) + @discussion_to_resolve_id ||= params.delete(:discussion_to_resolve) end def hook_data(issue, action) @@ -17,25 +16,22 @@ module Issues 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 + @merge_request_for_resolving_discussions ||= MergeRequestsFinder.new(current_user, project_id: project.id). + execute. + find_by(iid: merge_request_for_resolving_discussions_iid) 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 + return [] unless merge_request_for_resolving_discussions + + @discussions_to_resolve ||= NotesFinder.new(project, current_user, { + discussion_id: discussion_to_resolve_id, + target_type: MergeRequest.name.underscore, + target_id: merge_request_for_resolving_discussions.id + }). + execute. + discussions. + select(&:to_be_resolved?) end private diff --git a/app/services/issues/create_service.rb b/app/services/issues/create_service.rb index d2cc70ed0dc..007c8354c8c 100644 --- a/app/services/issues/create_service.rb +++ b/app/services/issues/create_service.rb @@ -6,8 +6,8 @@ module Issues filter_spam_check_params issue_attributes = params.merge( - merge_request_for_resolving_discussions: merge_request_for_resolving_discussions, - discussion_to_resolve: discussion_to_resolve + merge_request_for_resolving_discussions: merge_request_for_resolving_discussions_iid, + discussion_to_resolve: discussion_to_resolve_id ) @issue = BuildService.new(project, current_user, issue_attributes).execute diff --git a/app/views/discussions/_new_issue_for_discussion.html.haml b/app/views/discussions/_new_issue_for_discussion.html.haml index de28ded9820..c21ec0ecc3d 100644 --- a/app/views/discussions/_new_issue_for_discussion.html.haml +++ b/app/views/discussions/_new_issue_for_discussion.html.haml @@ -5,4 +5,4 @@ .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' + = link_to custom_icon('icon_mr_issue'), new_namespace_project_issue_path(@project.namespace, @project, merge_request_for_resolving_discussions: merge_request.iid, discussion_to_resolve: discussion.id), title: "Resolve this discussion in a new issue", class: 'new-issue-for-discussion' diff --git a/app/views/discussions/_notes.html.haml b/app/views/discussions/_notes.html.haml index 4a1e93cec3a..2789391819c 100644 --- a/app/views/discussions/_notes.html.haml +++ b/app/views/discussions/_notes.html.haml @@ -12,7 +12,7 @@ = render "discussions/resolve_all", discussion: discussion - if discussion.for_merge_request? .btn-group.discussion-actions - = render "discussions/new_issue_for_discussion", discussion: discussion + = render "discussions/new_issue_for_discussion", discussion: discussion, merge_request: discussion.noteable = render "discussions/jump_to_next", discussion: discussion - else = link_to_reply_discussion(discussion) diff --git a/app/views/shared/issuable/_form.html.haml b/app/views/shared/issuable/_form.html.haml index e93fe0f6cb5..a10f9caf3c1 100644 --- a/app/views/shared/issuable/_form.html.haml +++ b/app/views/shared/issuable/_form.html.haml @@ -50,30 +50,20 @@ .col-sm-10.col-sm-offset-2 = icon('exclamation-triangle') - if @merge_request_for_resolving_discussions.discussions_can_be_resolved_by?(current_user) - 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 + - if @discussion_to_resolve + = hidden_field_tag 'discussion_to_resolve', @discussion_to_resolve.id + Creating this issue will mark the discussion at + = link_to @discussion_to_resolve.noteable.to_reference, Gitlab::UrlBuilder.build(@discussion_to_resolve.first_note) + - else + 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. - else 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 |