From 142edf0afcb83f220175d02ea74b71d90753a875 Mon Sep 17 00:00:00 2001 From: "micael.bergeron" Date: Wed, 8 Nov 2017 15:39:29 -0500 Subject: [PATCH] diff notes created in merge request on a commit have the right context add a spec for commit merge request diff notes --- .../merge_requests/application_controller.rb | 7 +++++- .../merge_requests/diffs_controller.rb | 8 +------ app/helpers/merge_requests_helper.rb | 24 +++++++++++++++++++ .../merge_requests/refresh_service.rb | 2 +- app/services/system_note_service.rb | 4 ++-- .../_not_all_comments_displayed.html.haml | 2 +- .../projects/merge_requests/show.html.haml | 8 +++---- .../projects/commit_controller_spec.rb | 15 ++++++++++++ 8 files changed, 54 insertions(+), 16 deletions(-) diff --git a/app/controllers/projects/merge_requests/application_controller.rb b/app/controllers/projects/merge_requests/application_controller.rb index 1269759fc2b..3b764433c01 100644 --- a/app/controllers/projects/merge_requests/application_controller.rb +++ b/app/controllers/projects/merge_requests/application_controller.rb @@ -1,6 +1,7 @@ class Projects::MergeRequests::ApplicationController < Projects::ApplicationController before_action :check_merge_requests_available! before_action :merge_request + before_action :commit before_action :authorize_read_merge_request! private @@ -9,6 +10,11 @@ class Projects::MergeRequests::ApplicationController < Projects::ApplicationCont @issuable = @merge_request ||= @project.merge_requests.find_by!(iid: params[:id]) end + def commit + return nil unless commit_id = params[:commit_id].presence + @commit ||= merge_request.target_project.commit(commit_id) + end + def merge_request_params params.require(:merge_request).permit(merge_request_params_attributes) end @@ -28,7 +34,6 @@ class Projects::MergeRequests::ApplicationController < Projects::ApplicationCont :task_num, :title, :discussion_locked, - label_ids: [] ] end diff --git a/app/controllers/projects/merge_requests/diffs_controller.rb b/app/controllers/projects/merge_requests/diffs_controller.rb index 42a6e5be14f..1312c83373f 100644 --- a/app/controllers/projects/merge_requests/diffs_controller.rb +++ b/app/controllers/projects/merge_requests/diffs_controller.rb @@ -22,13 +22,7 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic def define_diff_vars @merge_request_diffs = @merge_request.merge_request_diffs.viewable.select_without_diff.order_id_desc - if commit_id = params[:commit_id].presence - @commit = @merge_request.target_project.commit(commit_id) - @compare = @commit - else - @compare = find_merge_request_diff_compare - end - + @compare = commit || find_merge_request_diff_compare return render_404 unless @compare @diffs = @compare.diffs(diff_options) diff --git a/app/helpers/merge_requests_helper.rb b/app/helpers/merge_requests_helper.rb index 5b2c58d193d..004aaeb2c56 100644 --- a/app/helpers/merge_requests_helper.rb +++ b/app/helpers/merge_requests_helper.rb @@ -101,6 +101,30 @@ module MergeRequestsHelper }.merge(merge_params_ee(merge_request)) end + def tab_link_for(tab, options={}, &block) + data_attrs = { + action: tab.to_s, + target: "##{tab.to_s}", + toggle: options.fetch(:force_link, false) ? '' : 'tab' + } + + url = case tab + when :show + data_attrs.merge!(target: '#notes') + project_merge_request_path(@project, @merge_request) + when :commits + commits_project_merge_request_path(@project, @merge_request) + when :pipelines + pipelines_project_merge_request_path(@project, @merge_request) + when :diffs + diffs_project_merge_request_path(@project, @merge_request) + else + raise "Cannot create tab #{tab}." + end + + link_to(url, data: data_attrs, &block) + end + def merge_params_ee(merge_request) {} end diff --git a/app/services/merge_requests/refresh_service.rb b/app/services/merge_requests/refresh_service.rb index 434dda89db0..9f05535d4d4 100644 --- a/app/services/merge_requests/refresh_service.rb +++ b/app/services/merge_requests/refresh_service.rb @@ -6,7 +6,7 @@ module MergeRequests @oldrev, @newrev = oldrev, newrev @branch_name = Gitlab::Git.ref_name(ref) - find_new_commits + Gitlab::GitalyClient.allow_n_plus_1_calls(&method(:find_new_commits)) # Be sure to close outstanding MRs before reloading them to avoid generating an # empty diff during a manual merge close_merge_requests diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb index f90c6fcafb8..385b34120ef 100644 --- a/app/services/system_note_service.rb +++ b/app/services/system_note_service.rb @@ -671,8 +671,8 @@ module SystemNoteService def merge_request_commit_url(merge_request, commit) url_helpers.diffs_namespace_project_merge_request_url( - project.namespace, - project, + merge_request.target_project.namespace, + merge_request.target_project, merge_request.iid, commit_id: commit.id ) diff --git a/app/views/projects/merge_requests/diffs/_not_all_comments_displayed.html.haml b/app/views/projects/merge_requests/diffs/_not_all_comments_displayed.html.haml index 26988d34917..60c419a3cda 100644 --- a/app/views/projects/merge_requests/diffs/_not_all_comments_displayed.html.haml +++ b/app/views/projects/merge_requests/diffs/_not_all_comments_displayed.html.haml @@ -13,7 +13,7 @@ - else viewing an old version of the diff - .pull-right + .text-right = link_to diffs_namespace_project_merge_request_path(@project.namespace, @project, @merge_request), class: 'btn btn-sm' do Show latest version = "of the diff" if @commit diff --git a/app/views/projects/merge_requests/show.html.haml b/app/views/projects/merge_requests/show.html.haml index 4e3f6f9edc9..0d7abe8137f 100644 --- a/app/views/projects/merge_requests/show.html.haml +++ b/app/views/projects/merge_requests/show.html.haml @@ -38,21 +38,21 @@ .nav-links.scrolling-tabs %ul.merge-request-tabs %li.notes-tab - = link_to project_merge_request_path(@project, @merge_request), data: { target: 'div#notes', action: 'show', toggle: 'tab' } do + = tab_link_for :show, force_link: @commit.present? do Discussion %span.badge= @merge_request.related_notes.user.count - if @merge_request.source_project %li.commits-tab - = link_to commits_project_merge_request_path(@project, @merge_request), data: { target: 'div#commits', action: 'commits', toggle: 'tab' } do + = tab_link_for :commits do Commits %span.badge= @commits_count - if @pipelines.any? %li.pipelines-tab - = link_to pipelines_project_merge_request_path(@project, @merge_request), data: { target: '#pipelines', action: 'pipelines', toggle: 'tab' } do + = tab_link_for :pipelines do Pipelines %span.badge.js-pipelines-mr-count= @pipelines.size %li.diffs-tab - = link_to diffs_project_merge_request_path(@project, @merge_request), data: { target: 'div#diffs', action: 'diffs', toggle: 'tab' } do + = tab_link_for :diffs do Changes %span.badge= @merge_request.diff_size #resolve-count-app.line-resolve-all-container.prepend-top-10{ "v-cloak" => true } diff --git a/spec/controllers/projects/commit_controller_spec.rb b/spec/controllers/projects/commit_controller_spec.rb index fd90c0d8bad..a5b603d6bff 100644 --- a/spec/controllers/projects/commit_controller_spec.rb +++ b/spec/controllers/projects/commit_controller_spec.rb @@ -132,6 +132,21 @@ describe Projects::CommitController do expect(response).to be_success end end + + context 'in the context of a merge_request' do + let(:merge_request) { create(:merge_request, source_project: project) } + let(:commit) { merge_request.commits.first } + + it 'prepare diff notes in the context of the merge request' do + go(id: commit.id, merge_request_iid: merge_request.iid) + + expect(assigns(:new_diff_note_attrs)).to eq({ noteable_type: 'MergeRequest', + noteable_id: merge_request.id, + commit_id: commit.id + }) + expect(response).to be_ok + end + end end describe 'GET branches' do -- GitLab