diff options
22 files changed, 104 insertions, 60 deletions
diff --git a/app/controllers/concerns/creates_commit.rb b/app/controllers/concerns/creates_commit.rb index dacb5679dd3..936d9bab57e 100644 --- a/app/controllers/concerns/creates_commit.rb +++ b/app/controllers/concerns/creates_commit.rb @@ -81,10 +81,8 @@ module CreatesCommit def merge_request_exists? return @merge_request if defined?(@merge_request) - @merge_request = @mr_target_project.merge_requests.opened.find_by( - source_branch: @mr_source_branch, - target_branch: @mr_target_branch - ) + @merge_request = MergeRequestsFinder.new(current_user, project_id: @mr_target_project.id).execute.opened. + find_by(source_branch: @mr_source_branch, target_branch: @mr_target_branch) end def different_project? diff --git a/app/controllers/projects/commit_controller.rb b/app/controllers/projects/commit_controller.rb index cdfc1ba7b92..8197d9e4c99 100644 --- a/app/controllers/projects/commit_controller.rb +++ b/app/controllers/projects/commit_controller.rb @@ -65,7 +65,7 @@ class Projects::CommitController < Projects::ApplicationController return render_404 if @target_branch.blank? - create_commit(Commits::RevertService, success_notice: "The #{@commit.change_type_title} has been successfully reverted.", + create_commit(Commits::RevertService, success_notice: "The #{@commit.change_type_title(current_user)} has been successfully reverted.", success_path: successful_change_path, failure_path: failed_change_path) end @@ -74,26 +74,24 @@ class Projects::CommitController < Projects::ApplicationController return render_404 if @target_branch.blank? - create_commit(Commits::CherryPickService, success_notice: "The #{@commit.change_type_title} has been successfully cherry-picked.", + create_commit(Commits::CherryPickService, success_notice: "The #{@commit.change_type_title(current_user)} has been successfully cherry-picked.", success_path: successful_change_path, failure_path: failed_change_path) end private def successful_change_path - return referenced_merge_request_url if @commit.merged_merge_request - - namespace_project_commits_url(@project.namespace, @project, @target_branch) + referenced_merge_request_url || namespace_project_commits_url(@project.namespace, @project, @target_branch) end def failed_change_path - return referenced_merge_request_url if @commit.merged_merge_request - - namespace_project_commit_url(@project.namespace, @project, params[:id]) + referenced_merge_request_url || namespace_project_commit_url(@project.namespace, @project, params[:id]) end def referenced_merge_request_url - namespace_project_merge_request_url(@project.namespace, @project, @commit.merged_merge_request) + if merge_request = @commit.merged_merge_request(current_user) + namespace_project_merge_request_url(@project.namespace, @project, merge_request) + end end def commit diff --git a/app/controllers/projects/commits_controller.rb b/app/controllers/projects/commits_controller.rb index aba87b6144b..ad92f05a42d 100644 --- a/app/controllers/projects/commits_controller.rb +++ b/app/controllers/projects/commits_controller.rb @@ -21,7 +21,7 @@ class Projects::CommitsController < Projects::ApplicationController @note_counts = project.notes.where(commit_id: @commits.map(&:id)). group(:commit_id).count - @merge_request = @project.merge_requests.opened. + @merge_request = MergeRequestsFinder.new(current_user, project_id: @project.id).execute.opened. find_by(source_project: @project, source_branch: @ref, target_branch: @repository.root_ref) respond_to do |format| diff --git a/app/controllers/projects/compare_controller.rb b/app/controllers/projects/compare_controller.rb index bee3d56076c..ec02fc15d35 100644 --- a/app/controllers/projects/compare_controller.rb +++ b/app/controllers/projects/compare_controller.rb @@ -53,7 +53,7 @@ class Projects::CompareController < Projects::ApplicationController end def merge_request - @merge_request ||= @project.merge_requests.opened. + @merge_request ||= MergeRequestsFinder.new(current_user, project_id: @project.id).execute.opened. find_by(source_project: @project, source_branch: @head_ref, target_branch: @start_ref) end end diff --git a/app/controllers/projects/discussions_controller.rb b/app/controllers/projects/discussions_controller.rb index 148e39630e3..1349b015a63 100644 --- a/app/controllers/projects/discussions_controller.rb +++ b/app/controllers/projects/discussions_controller.rb @@ -24,7 +24,7 @@ class Projects::DiscussionsController < Projects::ApplicationController private def merge_request - @merge_request ||= @project.merge_requests.find_by!(iid: params[:merge_request_id]) + @merge_request ||= MergeRequestsFinder.new(current_user, project_id: @project.id).find_by!(iid: params[:merge_request_id]) end def discussion diff --git a/app/controllers/projects/todos_controller.rb b/app/controllers/projects/todos_controller.rb index 52517381c65..a41fcb85c40 100644 --- a/app/controllers/projects/todos_controller.rb +++ b/app/controllers/projects/todos_controller.rb @@ -18,7 +18,7 @@ class Projects::TodosController < Projects::ApplicationController when "issue" IssuesFinder.new(current_user, project_id: @project.id).find(params[:issuable_id]) when "merge_request" - @project.merge_requests.find(params[:issuable_id]) + MergeRequestsFinder.new(current_user, project_id: @project.id).find(params[:issuable_id]) end end end diff --git a/app/finders/issuable_finder.rb b/app/finders/issuable_finder.rb index c9bee01b9ad..b4c14d05eaf 100644 --- a/app/finders/issuable_finder.rb +++ b/app/finders/issuable_finder.rb @@ -77,6 +77,10 @@ class IssuableFinder counts end + def find_by!(*params) + execute.find_by!(*params) + end + def group return @group if defined?(@group) diff --git a/app/finders/notes_finder.rb b/app/finders/notes_finder.rb index a653a6d59c6..2484339e3a4 100644 --- a/app/finders/notes_finder.rb +++ b/app/finders/notes_finder.rb @@ -14,7 +14,7 @@ class NotesFinder when "issue" IssuesFinder.new(current_user, project_id: project.id).find(target_id).notes.inc_author when "merge_request" - project.merge_requests.find(target_id).mr_and_commit_notes.inc_author + MergeRequestsFinder.new(current_user, project_id: project.id).find(target_id).mr_and_commit_notes.inc_author when "snippet", "project_snippet" project.snippets.find(target_id).notes else diff --git a/app/helpers/commits_helper.rb b/app/helpers/commits_helper.rb index ed402b698fb..66a720a9426 100644 --- a/app/helpers/commits_helper.rb +++ b/app/helpers/commits_helper.rb @@ -130,7 +130,7 @@ module CommitsHelper def revert_commit_link(commit, continue_to_path, btn_class: nil, has_tooltip: true) return unless current_user - tooltip = "Revert this #{commit.change_type_title} in a new merge request" if has_tooltip + tooltip = "Revert this #{commit.change_type_title(current_user)} in a new merge request" if has_tooltip if can_collaborate_with_project? btn_class = "btn btn-warning btn-#{btn_class}" unless btn_class.nil? @@ -154,7 +154,7 @@ module CommitsHelper def cherry_pick_commit_link(commit, continue_to_path, btn_class: nil, has_tooltip: true) return unless current_user - tooltip = "Cherry-pick this #{commit.change_type_title} in a new merge request" + tooltip = "Cherry-pick this #{commit.change_type_title(current_user)} in a new merge request" if can_collaborate_with_project? btn_class = "btn btn-default btn-#{btn_class}" unless btn_class.nil? diff --git a/app/models/commit.rb b/app/models/commit.rb index 248140f421b..1831cc7e175 100644 --- a/app/models/commit.rb +++ b/app/models/commit.rb @@ -245,44 +245,47 @@ class Commit project.repository.next_branch("cherry-pick-#{short_id}", mild: true) end - def revert_description - if merged_merge_request - "This reverts merge request #{merged_merge_request.to_reference}" + def revert_description(user) + if merged_merge_request?(user) + "This reverts merge request #{merged_merge_request(user).to_reference}" else "This reverts commit #{sha}" end end - def revert_message - %Q{Revert "#{title.strip}"\n\n#{revert_description}} + def revert_message(user) + %Q{Revert "#{title.strip}"\n\n#{revert_description(user)}} end - def reverts_commit?(commit) - description? && description.include?(commit.revert_description) + def reverts_commit?(commit, user) + description? && description.include?(commit.revert_description(user)) end def merge_commit? parents.size > 1 end - def merged_merge_request - return @merged_merge_request if defined?(@merged_merge_request) - - @merged_merge_request = project.merge_requests.find_by(merge_commit_sha: id) if merge_commit? + def merged_merge_request(current_user) + # Memoize with per-user access check + @merged_merge_request_hash ||= Hash.new do |hash, user| + hash[user] = merged_merge_request_no_cache(user) + end + + @merged_merge_request_hash[current_user] end - def has_been_reverted?(current_user = nil, noteable = self) + def has_been_reverted?(current_user, noteable = self) ext = all_references(current_user) noteable.notes_with_associations.system.each do |note| note.all_references(current_user, extractor: ext) end - ext.commits.any? { |commit_ref| commit_ref.reverts_commit?(self) } + ext.commits.any? { |commit_ref| commit_ref.reverts_commit?(self, current_user) } end - def change_type_title - merged_merge_request ? 'merge request' : 'commit' + def change_type_title(user) + merged_merge_request?(user) ? 'merge request' : 'commit' end # Get the URI type of the given path @@ -350,4 +353,12 @@ class Commit changes end + + def merged_merge_request?(user) + !!merged_merge_request(user) + end + + def merged_merge_request_no_cache(user) + MergeRequestsFinder.new(user, project_id: project.id).find_by(merge_commit_sha: id) if merge_commit? + end end diff --git a/app/models/concerns/milestoneish.rb b/app/models/concerns/milestoneish.rb index e65fc9eaa09..875e9834487 100644 --- a/app/models/concerns/milestoneish.rb +++ b/app/models/concerns/milestoneish.rb @@ -1,17 +1,17 @@ module Milestoneish - def closed_items_count(user = nil) + def closed_items_count(user) issues_visible_to_user(user).closed.size + merge_requests.closed_and_merged.size end - def total_items_count(user = nil) + def total_items_count(user) issues_visible_to_user(user).size + merge_requests.size end - def complete?(user = nil) + def complete?(user) total_items_count(user) > 0 && total_items_count(user) == closed_items_count(user) end - def percent_complete(user = nil) + def percent_complete(user) ((closed_items_count(user) * 100) / total_items_count(user)).abs rescue ZeroDivisionError 0 @@ -29,7 +29,7 @@ module Milestoneish (Date.today - start_date).to_i end - def issues_visible_to_user(user = nil) + def issues_visible_to_user(user) issues.visible_to_user(user) end diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 33b578e12c1..dd9f1a7c85b 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -805,7 +805,7 @@ class MergeRequest < ActiveRecord::Base @merge_commit ||= project.commit(merge_commit_sha) if merge_commit_sha end - def can_be_reverted?(current_user = nil) + def can_be_reverted?(current_user) merge_commit && !merge_commit.has_been_reverted?(current_user, self) end diff --git a/app/models/repository.rb b/app/models/repository.rb index 3c4b0212af7..1ccabdb7c1f 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -950,7 +950,7 @@ class Repository update_branch_with_hooks(user, base_branch) do committer = user_to_committer(user) source_sha = Rugged::Commit.create(rugged, - message: commit.revert_message, + message: commit.revert_message(user), author: committer, committer: committer, tree: revert_tree_id, diff --git a/app/services/commits/change_service.rb b/app/services/commits/change_service.rb index 1c82599c579..db5f2bf9b2e 100644 --- a/app/services/commits/change_service.rb +++ b/app/services/commits/change_service.rb @@ -34,7 +34,7 @@ module Commits repository.public_send(action, current_user, @commit, into, tree_id) success else - error_msg = "Sorry, we cannot #{action.to_s.dasherize} this #{@commit.change_type_title} automatically. + error_msg = "Sorry, we cannot #{action.to_s.dasherize} this #{@commit.change_type_title(current_user)} automatically. It may have already been #{action.to_s.dasherize}, or a more recent commit may have updated some of its content." raise ChangeError, error_msg end diff --git a/app/views/layouts/nav/_project.html.haml b/app/views/layouts/nav/_project.html.haml index 701bcd3ab71..7bd11f5727a 100644 --- a/app/views/layouts/nav/_project.html.haml +++ b/app/views/layouts/nav/_project.html.haml @@ -77,7 +77,7 @@ = link_to namespace_project_merge_requests_path(@project.namespace, @project), title: 'Merge Requests', class: 'shortcuts-merge_requests' do %span Merge Requests - %span.badge.count.merge_counter= number_with_delimiter(@project.merge_requests.opened.count) + %span.badge.count.merge_counter= number_with_delimiter(MergeRequestsFinder.new(current_user, project_id: @project.id).execute.opened.count) - if project_nav_tab? :wiki = nav_link(controller: :wikis) do diff --git a/app/views/projects/commit/_change.html.haml b/app/views/projects/commit/_change.html.haml index e4cd55b9f7a..f6e3d5e76f5 100644 --- a/app/views/projects/commit/_change.html.haml +++ b/app/views/projects/commit/_change.html.haml @@ -11,7 +11,7 @@ .modal-content .modal-header %a.close{href: "#", "data-dismiss" => "modal"} × - %h3.page-title== #{label} this #{commit.change_type_title} + %h3.page-title== #{label} this #{commit.change_type_title(current_user)} .modal-body = form_tag send("#{type.underscore}_namespace_project_commit_path", @project.namespace, @project, commit.id), method: :post, remote: false, class: 'form-horizontal js-#{type}-form js-requires-input' do .form-group.branch diff --git a/changelogs/unreleased/jej-23867-use-mr-finder-instead-of-access-check.yml b/changelogs/unreleased/jej-23867-use-mr-finder-instead-of-access-check.yml new file mode 100644 index 00000000000..5a4a44b9562 --- /dev/null +++ b/changelogs/unreleased/jej-23867-use-mr-finder-instead-of-access-check.yml @@ -0,0 +1,4 @@ +--- +title: Replace MR access checks with use of MergeRequestsFinder +merge_request: +author: diff --git a/lib/gitlab/search_results.rb b/lib/gitlab/search_results.rb index 47d8599e298..35212992698 100644 --- a/lib/gitlab/search_results.rb +++ b/lib/gitlab/search_results.rb @@ -68,7 +68,7 @@ module Gitlab end def merge_requests - merge_requests = MergeRequest.in_projects(project_ids_relation) + merge_requests = MergeRequestsFinder.new(current_user).execute.in_projects(project_ids_relation) if query =~ /[#!](\d+)\z/ merge_requests = merge_requests.where(iid: $1) else diff --git a/spec/controllers/projects/todo_controller_spec.rb b/spec/controllers/projects/todo_controller_spec.rb index 193a3f6b5a3..415c264e0dd 100644 --- a/spec/controllers/projects/todo_controller_spec.rb +++ b/spec/controllers/projects/todo_controller_spec.rb @@ -110,7 +110,7 @@ describe Projects::TodosController do end end - context 'when not authorized' do + context 'when not authorized for project' do it 'does not create todo for merge request user has no access to' do sign_in(user) expect do @@ -128,6 +128,19 @@ describe Projects::TodosController do expect(response).to have_http_status(302) end end + + context 'when not authorized for merge_request' do + before do + project.update!(visibility_level: Gitlab::VisibilityLevel::PUBLIC) + project.project_feature.update!(merge_requests_access_level: ProjectFeature::PRIVATE) + sign_in(user) + end + + it "doesn't create todo" do + expect{ go }.not_to change { user.todos.count } + expect(response).to have_http_status(404) + end + end end end end diff --git a/spec/lib/gitlab/search_results_spec.rb b/spec/lib/gitlab/search_results_spec.rb index f23e3522625..9614aad3e73 100644 --- a/spec/lib/gitlab/search_results_spec.rb +++ b/spec/lib/gitlab/search_results_spec.rb @@ -40,6 +40,15 @@ describe Gitlab::SearchResults do expect(results.milestones_count).to eq(1) end end + + it 'includes merge requests from source and target projects' do + forked_project = create(:empty_project, forked_from_project: project) + merge_request_2 = create(:merge_request, target_project: project, source_project: forked_project, title: 'foo') + + results = described_class.new(user, Project.where(id: forked_project.id), 'foo') + + expect(results.objects('merge_requests')).to include merge_request_2 + end end it 'does not list issues on private projects' do @@ -152,4 +161,11 @@ describe Gitlab::SearchResults do expect(results.issues_count).to eq 5 end end + + it 'does not list merge requests on projects with limited access' do + project.update!(visibility_level: Gitlab::VisibilityLevel::PUBLIC) + project.project_feature.update!(merge_requests_access_level: ProjectFeature::PRIVATE) + + expect(results.objects('merge_requests')).not_to include merge_request + end end diff --git a/spec/models/commit_range_spec.rb b/spec/models/commit_range_spec.rb index d89d4342dea..30782ca75a0 100644 --- a/spec/models/commit_range_spec.rb +++ b/spec/models/commit_range_spec.rb @@ -137,26 +137,25 @@ describe CommitRange, models: true do end describe '#has_been_reverted?' do - it 'returns true if the commit has been reverted' do - issue = create(:issue) + let(:issue) { create(:issue) } + let(:user) { issue.author } + it 'returns true if the commit has been reverted' do create(:note_on_issue, noteable: issue, system: true, - note: commit1.revert_description, + note: commit1.revert_description(user), project: issue.project) expect_any_instance_of(Commit).to receive(:reverts_commit?). - with(commit1). + with(commit1, user). and_return(true) - expect(commit1.has_been_reverted?(nil, issue)).to eq(true) + expect(commit1.has_been_reverted?(user, issue)).to eq(true) end it 'returns false a commit has not been reverted' do - issue = create(:issue) - - expect(commit1.has_been_reverted?(nil, issue)).to eq(false) + expect(commit1.has_been_reverted?(user, issue)).to eq(false) end end end diff --git a/spec/models/commit_spec.rb b/spec/models/commit_spec.rb index eb482c7f913..0935fc0561c 100644 --- a/spec/models/commit_spec.rb +++ b/spec/models/commit_spec.rb @@ -179,25 +179,26 @@ eos describe '#reverts_commit?' do let(:another_commit) { double(:commit, revert_description: "This reverts commit #{commit.sha}") } + let(:user) { commit.author } - it { expect(commit.reverts_commit?(another_commit)).to be_falsy } + it { expect(commit.reverts_commit?(another_commit, user)).to be_falsy } context 'commit has no description' do before { allow(commit).to receive(:description?).and_return(false) } - it { expect(commit.reverts_commit?(another_commit)).to be_falsy } + it { expect(commit.reverts_commit?(another_commit, user)).to be_falsy } end context "another_commit's description does not revert commit" do before { allow(commit).to receive(:description).and_return("Foo Bar") } - it { expect(commit.reverts_commit?(another_commit)).to be_falsy } + it { expect(commit.reverts_commit?(another_commit, user)).to be_falsy } end context "another_commit's description reverts commit" do before { allow(commit).to receive(:description).and_return("Foo #{another_commit.revert_description} Bar") } - it { expect(commit.reverts_commit?(another_commit)).to be_truthy } + it { expect(commit.reverts_commit?(another_commit, user)).to be_truthy } end context "another_commit's description reverts merged merge request" do @@ -207,7 +208,7 @@ eos allow(commit).to receive(:description).and_return("Foo #{another_commit.revert_description} Bar") end - it { expect(commit.reverts_commit?(another_commit)).to be_truthy } + it { expect(commit.reverts_commit?(another_commit, user)).to be_truthy } end end |