From 67b579c54e77a28db2736401e698f609a713d0f3 Mon Sep 17 00:00:00 2001 From: Mark Chao Date: Thu, 25 Oct 2018 21:46:04 +0800 Subject: [PATCH] Allow getting all paths (old & new) involved in MR Fetch database columns directly if available --- app/models/compare.rb | 11 ++++++++ app/models/merge_request.rb | 12 ++++++++ app/models/merge_request_diff.rb | 7 +++++ spec/models/compare_spec.rb | 29 ++++++++++++++++++++ spec/models/merge_request_diff_spec.rb | 13 +++++++++ spec/models/merge_request_spec.rb | 38 ++++++++++++++++++++++++++ 6 files changed, 110 insertions(+) diff --git a/app/models/compare.rb b/app/models/compare.rb index b2d46ada831..f1ed84ab5a5 100644 --- a/app/models/compare.rb +++ b/app/models/compare.rb @@ -1,5 +1,7 @@ # frozen_string_literal: true +require 'set' + class Compare include Gitlab::Utils::StrongMemoize @@ -77,4 +79,13 @@ class Compare head_sha: head_commit_sha ) end + + def modified_paths + paths = Set.new + diffs.diff_files.each do |diff| + paths.add diff.old_path + paths.add diff.new_path + end + paths.to_a + end end diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 735d9fba966..df5678ec2f1 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -409,6 +409,18 @@ class MergeRequest < ActiveRecord::Base merge_request_diff&.real_size || diffs.real_size end + def modified_paths(past_merge_request_diff: nil) + diffs = if past_merge_request_diff + past_merge_request_diff + elsif compare + compare + else + self.merge_request_diff + end + + diffs.modified_paths + end + def diff_base_commit if persisted? merge_request_diff.base_commit diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb index bb6ff8921df..74583af1a29 100644 --- a/app/models/merge_request_diff.rb +++ b/app/models/merge_request_diff.rb @@ -6,6 +6,7 @@ class MergeRequestDiff < ActiveRecord::Base include ManualInverseAssociation include IgnorableColumn include EachBatch + include Gitlab::Utils::StrongMemoize # Don't display more than 100 commits at once COMMITS_SAFE_SIZE = 100 @@ -234,6 +235,12 @@ class MergeRequestDiff < ActiveRecord::Base end # rubocop: enable CodeReuse/ServiceClass + def modified_paths + strong_memoize(:modified_paths) do + merge_request_diff_files.pluck(:new_path, :old_path).flatten.uniq + end + end + private def create_merge_request_diff_files(diffs) diff --git a/spec/models/compare_spec.rb b/spec/models/compare_spec.rb index 8e88bb81162..0bc3ee014e6 100644 --- a/spec/models/compare_spec.rb +++ b/spec/models/compare_spec.rb @@ -92,4 +92,33 @@ describe Compare do expect(subject.diff_refs.head_sha).to eq(head_commit.id) end end + + describe '#modified_paths' do + context 'changes are present' do + let(:raw_compare) do + Gitlab::Git::Compare.new( + project.repository.raw_repository, 'before-create-delete-modify-move', 'after-create-delete-modify-move' + ) + end + + it 'returns affected file paths, without duplication' do + expect(subject.modified_paths).to contain_exactly(*%w{ + foo/for_move.txt + foo/bar/for_move.txt + foo/for_create.txt + foo/for_delete.txt + foo/for_edit.txt + }) + end + end + + context 'changes are absent' do + let(:start_commit) { sample_commit } + let(:head_commit) { sample_commit } + + it 'returns empty array' do + expect(subject.modified_paths).to eq([]) + end + end + end end diff --git a/spec/models/merge_request_diff_spec.rb b/spec/models/merge_request_diff_spec.rb index 47e8f04e728..cbe60b3a4a5 100644 --- a/spec/models/merge_request_diff_spec.rb +++ b/spec/models/merge_request_diff_spec.rb @@ -232,4 +232,17 @@ describe MergeRequestDiff do expect(commits.map(&:sha)).to match_array(commit_shas) end end + + describe '#modified_paths' do + subject do + diff = create(:merge_request_diff) + create(:merge_request_diff_file, :new_file, merge_request_diff: diff) + create(:merge_request_diff_file, :renamed_file, merge_request_diff: diff) + diff + end + + it 'returns affected file paths' do + expect(subject.modified_paths).to eq(%w{foo bar baz}) + end + end end diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 3a54725c7ec..c7202b481d3 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -631,6 +631,44 @@ describe MergeRequest do end end + describe '#modified_paths' do + let(:paths) { double(:paths) } + subject(:merge_request) { build(:merge_request) } + + before do + expect(diff).to receive(:modified_paths).and_return(paths) + end + + context 'when past_merge_request_diff is specified' do + let(:another_diff) { double(:merge_request_diff) } + let(:diff) { another_diff } + + it 'returns affected file paths from specified past_merge_request_diff' do + expect(merge_request.modified_paths(past_merge_request_diff: another_diff)).to eq(paths) + end + end + + context 'when compare is present' do + let(:compare) { double(:compare) } + let(:diff) { compare } + + it 'returns affected file paths from compare' do + merge_request.compare = compare + + expect(merge_request.modified_paths).to eq(paths) + end + end + + context 'when no arguments provided' do + let(:diff) { merge_request.merge_request_diff } + subject(:merge_request) { create(:merge_request, source_branch: 'feature', target_branch: 'master') } + + it 'returns affected file paths for merge_request_diff' do + expect(merge_request.modified_paths).to eq(paths) + end + end + end + describe "#related_notes" do let!(:merge_request) { create(:merge_request) }