Merge branch '28154-move-models-outside-ee' into 'master'

[RUN AS-IF-FOSS] Move DraftNote outside EE

See merge request gitlab-org/gitlab!32631

app/models/merge_request.rb

@@ -88,6 +88,9 @@ class MergeRequest < ApplicationRecord
   has_many :deployments,
     through: :deployment_merge_requests
 
+  has_many :draft_notes
+  has_many :reviews, inverse_of: :merge_request
+
   KNOWN_MERGE_PARAMS = [
     :auto_merge_strategy,
     :should_remove_source_branch,
@@ -541,13 +544,21 @@ class MergeRequest < ApplicationRecord
     merge_request_diffs.where.not(id: merge_request_diff.id)
   end
 
-  # Overwritten in EE
-  def note_positions_for_paths(paths, _user = nil)
+  def note_positions_for_paths(paths, user = nil)
     positions = notes.new_diff_notes.joins(:note_diff_file)
       .where('note_diff_files.old_path IN (?) OR note_diff_files.new_path IN (?)', paths, paths)
       .positions
 
-    Gitlab::Diff::PositionCollection.new(positions, diff_head_sha)
+    collection = Gitlab::Diff::PositionCollection.new(positions, diff_head_sha)
+
+    return collection unless user
+
+    positions = draft_notes
+      .authored_by(user)
+      .positions
+      .select { |pos| paths.include?(pos.file_path) }
+
+    collection.concat(positions)
   end
 
   def preloads_discussion_diff_highlighting?

app/models/note.rb

@@ -72,6 +72,7 @@ class Note < ApplicationRecord
   belongs_to :author, class_name: "User"
   belongs_to :updated_by, class_name: "User"
   belongs_to :last_edited_by, class_name: 'User'
+  belongs_to :review, inverse_of: :notes
 
   has_many :todos
 

app/models/project.rb

@@ -329,6 +329,7 @@ class Project < ApplicationRecord
   has_many :repository_storage_moves, class_name: 'ProjectRepositoryStorageMove'
 
   has_many :webide_pipelines, -> { webide_source }, class_name: 'Ci::Pipeline', inverse_of: :project
+  has_many :reviews, inverse_of: :project
 
   accepts_nested_attributes_for :variables, allow_destroy: true
   accepts_nested_attributes_for :project_feature, update_only: true

app/models/user.rb

@@ -180,6 +180,8 @@ class User < ApplicationRecord
   has_one :user_highest_role
   has_one :user_canonical_email
 
+  has_many :reviews, foreign_key: :author_id, inverse_of: :author
+
   #
   # Validations
   #

ee/app/models/ee/merge_request.rb

@@ -15,14 +15,12 @@ module EE
       include DeprecatedApprovalsBeforeMerge
       include UsageStatistics
 
-      has_many :reviews, inverse_of: :merge_request
       has_many :approvals, dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent
       has_many :approved_by_users, through: :approvals, source: :user
       has_many :approvers, as: :target, dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent
       has_many :approver_users, through: :approvers, source: :user
       has_many :approver_groups, as: :target, dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent
       has_many :approval_rules, class_name: 'ApprovalMergeRequestRule', inverse_of: :merge_request
-      has_many :draft_notes
       has_one :merge_train, inverse_of: :merge_request, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
 
       has_many :blocks_as_blocker,
@@ -150,18 +148,6 @@ module EE
       hidden.count
     end
 
-    override :note_positions_for_paths
-    def note_positions_for_paths(file_paths, user = nil)
-      return super unless user
-
-      positions = draft_notes
-        .authored_by(user)
-        .positions
-        .select { |pos| file_paths.include?(pos.file_path) }
-
-      super.concat(positions)
-    end
-
     def enabled_reports
       {
         sast: report_type_enabled?(:sast),

ee/app/models/ee/note.rb

@@ -10,8 +10,6 @@ module EE
       include Elastic::ApplicationVersionedSearch
       include UsageStatistics
 
-      belongs_to :review, inverse_of: :notes
-
       scope :searchable, -> { where(system: false).includes(:noteable) }
       scope :by_humans, -> { user.joins(:author).merge(::User.humans) }
       scope :with_suggestions, -> { joins(:suggestions) }

ee/app/models/ee/project.rb

@@ -47,7 +47,6 @@ module EE
       has_one :status_page_setting, inverse_of: :project, class_name: 'StatusPage::ProjectSetting'
       has_one :compliance_framework_setting, class_name: 'ComplianceManagement::ComplianceFramework::ProjectSettings', inverse_of: :project
 
-      has_many :reviews, inverse_of: :project
       has_many :approvers, as: :target, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
       has_many :approver_users, through: :approvers, source: :user
       has_many :approver_groups, as: :target, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent

ee/app/models/ee/user.rb

@@ -32,7 +32,6 @@ module EE
                :extra_shared_runners_minutes_limit, :extra_shared_runners_minutes_limit=,
                to: :namespace
 
-      has_many :reviews,                  foreign_key: :author_id, inverse_of: :author
       has_many :epics,                    foreign_key: :author_id
       has_many :requirements,             foreign_key: :author_id, inverse_of: :author, class_name: 'RequirementsManagement::Requirement'
       has_many :test_reports,             foreign_key: :author_id, inverse_of: :author, class_name: 'RequirementsManagement::TestReport'

ee/spec/models/merge_request_spec.rb

@@ -16,7 +16,6 @@ describe MergeRequest do
   subject(:merge_request) { create(:merge_request, source_project: project, target_project: project) }
 
   describe 'associations' do
-    it { is_expected.to have_many(:reviews).inverse_of(:merge_request) }
     it { is_expected.to have_many(:approvals).dependent(:delete_all) }
     it { is_expected.to have_many(:approvers).dependent(:delete_all) }
     it { is_expected.to have_many(:approver_users).through(:approvers) }
@@ -50,48 +49,6 @@ describe MergeRequest do
     end
   end
 
-  describe '#note_positions_for_paths' do
-    let(:user) { create(:user) }
-    let(:merge_request) { create(:merge_request, :with_diffs) }
-    let(:project) { merge_request.project }
-    let!(:diff_note) do
-      create(:diff_note_on_merge_request, project: project, noteable: merge_request)
-    end
-    let!(:draft_note) do
-      create(:draft_note_on_text_diff, author: user, merge_request: merge_request)
-    end
-
-    let(:file_paths) { merge_request.diffs.diff_files.map(&:file_path) }
-
-    subject do
-      merge_request.note_positions_for_paths(file_paths)
-    end
-
-    it 'returns a Gitlab::Diff::PositionCollection' do
-      expect(subject).to be_a(Gitlab::Diff::PositionCollection)
-    end
-
-    context 'when user is given' do
-      subject do
-        merge_request.note_positions_for_paths(file_paths, user)
-      end
-
-      it 'returns notes and draft notes positions' do
-        expect(subject).to match_array([draft_note.position, diff_note.position])
-      end
-    end
-
-    context 'when user is not given' do
-      subject do
-        merge_request.note_positions_for_paths(file_paths)
-      end
-
-      it 'returns notes positions' do
-        expect(subject).to match_array([diff_note.position])
-      end
-    end
-  end
-
   describe '#participants' do
     context 'with approval rule' do
       before do

ee/spec/models/note_spec.rb

@@ -91,10 +91,6 @@ describe Note do
     end
   end
 
-  describe 'associations' do
-    it { is_expected.to belong_to(:review).inverse_of(:notes) }
-  end
-
   describe 'scopes' do
     describe '.with_suggestions' do
       it 'returns the correct note' do

ee/spec/models/project_spec.rb

@@ -28,7 +28,6 @@ describe Project do
     it { is_expected.to have_one(:status_page_setting).class_name('StatusPage::ProjectSetting') }
     it { is_expected.to have_one(:compliance_framework_setting).class_name('ComplianceManagement::ComplianceFramework::ProjectSettings') }
 
-    it { is_expected.to have_many(:reviews).inverse_of(:project) }
     it { is_expected.to have_many(:path_locks) }
     it { is_expected.to have_many(:vulnerability_feedback) }
     it { is_expected.to have_many(:vulnerability_exports) }

ee/spec/models/user_spec.rb

@@ -21,7 +21,6 @@ describe User do
   describe 'associations' do
     subject { build(:user) }
 
-    it { is_expected.to have_many(:reviews) }
     it { is_expected.to have_many(:vulnerability_feedback) }
     it { is_expected.to have_many(:path_locks).dependent(:destroy) }
     it { is_expected.to have_many(:users_security_dashboard_projects) }

spec/models/merge_request_spec.rb

@@ -22,6 +22,8 @@ describe MergeRequest do
     it { is_expected.to belong_to(:iteration) }
     it { is_expected.to have_many(:resource_milestone_events) }
     it { is_expected.to have_many(:resource_state_events) }
+    it { is_expected.to have_many(:draft_notes) }
+    it { is_expected.to have_many(:reviews).inverse_of(:merge_request) }
 
     context 'for forks' do
       let!(:project) { create(:project) }
@@ -721,11 +723,15 @@ describe MergeRequest do
   end
 
   describe '#note_positions_for_paths' do
+    let(:user) { create(:user) }
     let(:merge_request) { create(:merge_request, :with_diffs) }
     let(:project) { merge_request.project }
     let!(:diff_note) do
       create(:diff_note_on_merge_request, project: project, noteable: merge_request)
     end
+    let!(:draft_note) do
+      create(:draft_note_on_text_diff, author: user, merge_request: merge_request)
+    end
 
     let(:file_paths) { merge_request.diffs.diff_files.map(&:file_path) }
 
@@ -758,6 +764,26 @@ describe MergeRequest do
         expect(subject.to_a).to be_empty
       end
     end
+
+    context 'when user is given' do
+      subject do
+        merge_request.note_positions_for_paths(file_paths, user)
+      end
+
+      it 'returns notes and draft notes positions' do
+        expect(subject).to match_array([draft_note.position, diff_note.position])
+      end
+    end
+
+    context 'when user is not given' do
+      subject do
+        merge_request.note_positions_for_paths(file_paths)
+      end
+
+      it 'returns notes positions' do
+        expect(subject).to match_array([diff_note.position])
+      end
+    end
   end
 
   describe '#discussions_diffs' do

spec/models/note_spec.rb

@@ -11,6 +11,7 @@ describe Note do
     it { is_expected.to belong_to(:author).class_name('User') }
 
     it { is_expected.to have_many(:todos) }
+    it { is_expected.to belong_to(:review).inverse_of(:notes) }
   end
 
   describe 'modules' do

spec/models/project_spec.rb

@@ -117,6 +117,7 @@ describe Project do
     it { is_expected.to have_many(:jira_imports) }
     it { is_expected.to have_many(:metrics_users_starred_dashboards).inverse_of(:project) }
     it { is_expected.to have_many(:repository_storage_moves) }
+    it { is_expected.to have_many(:reviews).inverse_of(:project) }
 
     it_behaves_like 'model with repository' do
       let_it_be(:container) { create(:project, :repository, path: 'somewhere') }

spec/models/user_spec.rb

@@ -56,6 +56,7 @@ describe User do
     it { is_expected.to have_many(:custom_attributes).class_name('UserCustomAttribute') }
     it { is_expected.to have_many(:releases).dependent(:nullify) }
     it { is_expected.to have_many(:metrics_users_starred_dashboards).inverse_of(:user) }
+    it { is_expected.to have_many(:reviews).inverse_of(:author) }
 
     describe "#bio" do
       it 'syncs bio with `user_details.bio` on create' do