Merge branch '201886-remove-repo-type-from-after-import' into 'master'

Remove main users of Repository#repo_type

See merge request gitlab-org/gitlab!26507

app/models/commit.rb

@@ -242,14 +242,6 @@ class Commit
     data
   end
 
-  # Discover issues should be closed when this commit is pushed to a project's
-  # default branch.
-  def closes_issues(current_user = self.committer)
-    return unless repository.repo_type.project?
-
-    Gitlab::ClosingIssueExtractor.new(project, current_user).closed_by_message(safe_message)
-  end
-
   def lazy_author
     BatchLoader.for(author_email.downcase).batch do |emails, loader|
       users = User.by_any_email(emails, confirmed: true).includes(:emails)
@@ -299,14 +291,6 @@ class Commit
     notes.includes(:author, :award_emoji)
   end
 
-  def merge_requests
-    strong_memoize(:merge_requests) do
-      next MergeRequest.none unless repository.repo_type.project? && project
-
-      project.merge_requests.by_commit_sha(sha)
-    end
-  end
-
   def method_missing(method, *args, &block)
     @raw.__send__(method, *args, &block) # rubocop:disable GitlabSecurity/PublicSend
   end

app/models/merge_request.rb

@@ -257,6 +257,10 @@ class MergeRequest < ApplicationRecord
     with_state(:opened).where(auto_merge_enabled: true)
   end
 
+  scope :including_metrics, -> do
+    includes(:metrics)
+  end
+
   ignore_column :state, remove_with: '12.7', remove_after: '2019-12-22'
 
   after_save :keep_around_commit, unless: :importing?

app/models/note.rb

@@ -290,6 +290,19 @@ class Note < ApplicationRecord
     @commit ||= project.commit(commit_id) if commit_id.present?
   end
 
+  # Notes on merge requests and commits can be traced back to one or several
+  # MRs. This method returns a relation if the note is for one of these types,
+  # or nil if it is a note on some other object.
+  def merge_requests
+    if for_commit?
+      project.merge_requests.by_commit_sha(commit_id)
+    elsif for_merge_request?
+      MergeRequest.id_in(noteable_id)
+    else
+      nil
+    end
+  end
+
   # override to return commits, which are not active record
   def noteable
     return commit if for_commit?

app/models/project.rb

@@ -1787,8 +1787,10 @@ class Project < ApplicationRecord
   # rubocop:enable Gitlab/RailsLogger
 
   def after_import
-    repository.after_import
-    wiki.repository.after_import
+    repository.expire_content_cache
+    wiki.repository.expire_content_cache
+
+    DetectRepositoryLanguagesWorker.perform_async(id)
 
     # The import assigns iid values on its own, e.g. by re-using GitHub ids.
     # Flush existing InternalId records for this project for consistency reasons.

app/models/repository.rb

@@ -437,15 +437,6 @@ class Repository
     expire_all_method_caches
   end
 
-  # Runs code after a repository has been forked/imported.
-  def after_import
-    expire_content_cache
-
-    return unless repo_type.project?
-
-    DetectRepositoryLanguagesWorker.perform_async(project.id)
-  end
-
   # Runs code after a new commit has been pushed.
   def after_push_commit(branch_name)
     expire_statistics_caches

app/workers/process_commit_worker.rb

@@ -19,13 +19,12 @@ class ProcessCommitWorker # rubocop:disable Scalability/IdempotentWorker
   # commit_hash - Hash containing commit details to use for constructing a
   #               Commit object without having to use the Git repository.
   # default - The data was pushed to the default branch.
-  # rubocop: disable CodeReuse/ActiveRecord
   def perform(project_id, user_id, commit_hash, default = false)
-    project = Project.find_by(id: project_id)
+    project = Project.id_in(project_id).first
 
     return unless project
 
-    user = User.find_by(id: user_id)
+    user = User.id_in(user_id).first
 
     return unless user
 
@@ -35,12 +34,11 @@ class ProcessCommitWorker # rubocop:disable Scalability/IdempotentWorker
     process_commit_message(project, commit, user, author, default)
     update_issue_metrics(commit, author)
   end
-  # rubocop: enable CodeReuse/ActiveRecord
 
   def process_commit_message(project, commit, user, author, default = false)
     # Ignore closing references from GitLab-generated commit messages.
     find_closing_issues = default && !commit.merged_merge_request?(user)
-    closed_issues = find_closing_issues ? commit.closes_issues(user) : []
+    closed_issues = find_closing_issues ? issues_to_close(project, commit, user) : []
 
     close_issues(project, user, author, commit, closed_issues) if closed_issues.any?
     commit.create_cross_references!(author, closed_issues)
@@ -56,6 +54,12 @@ class ProcessCommitWorker # rubocop:disable Scalability/IdempotentWorker
     end
   end
 
+  def issues_to_close(project, commit, user)
+    Gitlab::ClosingIssueExtractor
+      .new(project, user)
+      .closed_by_message(commit.safe_message)
+  end
+
   def update_issue_metrics(commit, author)
     mentioned_issues = commit.all_references(author).issues
 

ee/lib/analytics/refresh_comments_data.rb

@@ -4,19 +4,13 @@ module Analytics
   class RefreshCommentsData
     include MergeRequestMetricsRefresh
 
-    # rubocop: disable CodeReuse/ActiveRecord
-    def self.for_note(note)
-      if note.for_commit?
-        merge_requests = note.noteable.merge_requests.includes(:metrics)
-      elsif note.for_merge_request?
-        merge_requests = [note.noteable]
-      else
-        return
-      end
+    class << self
+      def for_note(note)
+        merge_requests = note.merge_requests&.including_metrics
 
-      new(*merge_requests)
+        new(*merge_requests) unless merge_requests.nil?
+      end
     end
-    # rubocop: enable CodeReuse/ActiveRecord
 
     private
 

ee/spec/models/project_import_state_spec.rb

@@ -12,8 +12,7 @@ describe ProjectImportState, type: :model do
 
       # Works around https://github.com/rspec/rspec-mocks/issues/910
       allow(Project).to receive(:find).with(project.id).and_return(project)
-      expect(project.repository).to receive(:after_import).and_call_original
-      expect(project.wiki.repository).to receive(:after_import).and_call_original
+      expect(project).to receive(:after_import).and_call_original
     end
 
     context 'with a mirrored project' do

spec/features/merge_request/user_sees_diff_spec.rb

@@ -61,10 +61,6 @@ describe 'Merge request > User sees diff', :js do
     let(:merge_request) { create(:merge_request_with_diffs, source_project: forked_project, target_project: project, author: author_user) }
     let(:changelog_id) { Digest::SHA1.hexdigest("CHANGELOG") }
 
-    before do
-      forked_project.repository.after_import
-    end
-
     context 'as author' do
       it 'shows direct edit link', :sidekiq_might_not_need_inline do
         sign_in(author_user)

spec/models/commit_spec.rb

@@ -444,61 +444,6 @@ eos
     it { is_expected.to respond_to(:id) }
   end
 
-  describe '#closes_issues' do
-    let(:issue) { create :issue, project: project }
-    let(:other_project) { create(:project, :public) }
-    let(:other_issue) { create :issue, project: other_project }
-    let(:committer) { create :user }
-
-    before do
-      project.add_developer(committer)
-      other_project.add_developer(committer)
-    end
-
-    it 'detects issues that this commit is marked as closing' do
-      ext_ref = "#{other_project.full_path}##{other_issue.iid}"
-
-      allow(commit).to receive_messages(
-        safe_message: "Fixes ##{issue.iid} and #{ext_ref}",
-        committer_email: committer.email
-      )
-
-      expect(commit.closes_issues).to include(issue)
-      expect(commit.closes_issues).to include(other_issue)
-    end
-
-    it 'ignores referenced issues when auto-close is disabled' do
-      project.update!(autoclose_referenced_issues: false)
-
-      allow(commit).to receive_messages(
-        safe_message: "Fixes ##{issue.iid}",
-        committer_email: committer.email
-      )
-
-      expect(commit.closes_issues).to be_empty
-    end
-
-    context 'with personal snippet' do
-      let(:commit) { personal_snippet.commit }
-
-      it 'does not call Gitlab::ClosingIssueExtractor' do
-        expect(Gitlab::ClosingIssueExtractor).not_to receive(:new)
-
-        commit.closes_issues
-      end
-    end
-
-    context 'with project snippet' do
-      let(:commit) { project_snippet.commit }
-
-      it 'does not call Gitlab::ClosingIssueExtractor' do
-        expect(Gitlab::ClosingIssueExtractor).not_to receive(:new)
-
-        commit.closes_issues
-      end
-    end
-  end
-
   it_behaves_like 'a mentionable' do
     subject { create(:project, :repository).commit }
 
@@ -775,32 +720,6 @@ eos
     end
   end
 
-  describe '#merge_requests' do
-    let!(:project) { create(:project, :repository) }
-    let!(:merge_request1) { create(:merge_request, source_project: project, source_branch: 'master', target_branch: 'feature') }
-    let!(:merge_request2) { create(:merge_request, source_project: project, source_branch: 'merged-target', target_branch: 'feature') }
-    let(:commit1) { merge_request1.merge_request_diff.commits.last }
-    let(:commit2) { merge_request1.merge_request_diff.commits.first }
-
-    it 'returns merge_requests that introduced that commit' do
-      expect(commit1.merge_requests).to contain_exactly(merge_request1, merge_request2)
-      expect(commit2.merge_requests).to contain_exactly(merge_request1)
-    end
-
-    context 'with personal snippet' do
-      it 'returns empty relation' do
-        expect(personal_snippet.repository.commit.merge_requests).to eq MergeRequest.none
-      end
-    end
-
-    context 'with project snippet' do
-      it 'returns empty relation' do
-        expect(project_snippet.project).not_to receive(:merge_requests)
-        expect(project_snippet.repository.commit.merge_requests).to eq MergeRequest.none
-      end
-    end
-  end
-
   describe 'signed commits' do
     let(:gpg_signed_commit) { project.commit_by(oid: '0b4bc9a49b562e85de7cc9e834518ea6828729b9') }
     let(:x509_signed_commit) { project.commit_by(oid: '189a6c924013fc3fe40d6f1ec1dc20214183bc97') }

spec/models/project_import_state_spec.rb

@@ -23,8 +23,7 @@ describe ProjectImportState, type: :model do
 
       # Works around https://github.com/rspec/rspec-mocks/issues/910
       allow(Project).to receive(:find).with(project.id).and_return(project)
-      expect(project.repository).to receive(:after_import).and_call_original
-      expect(project.wiki.repository).to receive(:after_import).and_call_original
+      expect(project).to receive(:after_import).and_call_original
     end
 
     it 'imports a project', :sidekiq_might_not_need_inline do

spec/models/project_spec.rb

@@ -4618,13 +4618,14 @@ describe Project do
     let(:import_state) { create(:import_state, project: project) }
 
     it 'runs the correct hooks' do
-      expect(project.repository).to receive(:after_import)
-      expect(project.wiki.repository).to receive(:after_import)
+      expect(project.repository).to receive(:expire_content_cache)
+      expect(project.wiki.repository).to receive(:expire_content_cache)
       expect(import_state).to receive(:finish)
       expect(project).to receive(:update_project_counter_caches)
       expect(project).to receive(:after_create_default_branch)
       expect(project).to receive(:refresh_markdown_cache!)
       expect(InternalId).to receive(:flush_records!).with(project: project)
+      expect(DetectRepositoryLanguagesWorker).to receive(:perform_async).with(project.id)
 
       project.after_import
     end

spec/models/repository_spec.rb

@@ -1929,32 +1929,6 @@ describe Repository do
     end
   end
 
-  describe '#after_import' do
-    subject { repository.after_import }
-
-    it 'flushes and builds the cache' do
-      expect(repository).to receive(:expire_content_cache)
-
-      subject
-    end
-
-    it 'calls DetectRepositoryLanguagesWorker' do
-      expect(DetectRepositoryLanguagesWorker).to receive(:perform_async)
-
-      subject
-    end
-
-    context 'with a wiki repository' do
-      let(:repository) { project.wiki.repository }
-
-      it 'does not call DetectRepositoryLanguagesWorker' do
-        expect(DetectRepositoryLanguagesWorker).not_to receive(:perform_async)
-
-        subject
-      end
-    end
-  end
-
   describe '#after_push_commit' do
     it 'expires statistics caches' do
       expect(repository).to receive(:expire_statistics_caches)

spec/support/helpers/project_forks_helper.rb

@@ -59,7 +59,7 @@ module ProjectForksHelper
         bare_repo: TestEnv.forked_repo_path_bare,
         refs: TestEnv::FORKED_BRANCH_SHA
       )
-      forked_project.repository.after_import
+      forked_project.repository.expire_content_cache
 
       forked_project
     end