Merge branch 'elastic-indexer-remove-changed-fields' into 'master'

Remove ElasticIndexerWorker changed_fields

See merge request gitlab-org/gitlab!24873

ee/app/models/concerns/elastic/application_versioned_search.rb

@@ -53,8 +53,7 @@ module Elastic
         :update,
         self.class.to_s,
         self.id,
-        self.es_id,
-        changed_fields: self.previous_changes.keys
+        self.es_id
       )
     end
 

ee/app/models/ee/issue.rb

@@ -10,6 +10,7 @@ module EE
       WEIGHT_ALL = 'Everything'.freeze
       WEIGHT_ANY = 'Any'.freeze
       WEIGHT_NONE = 'None'.freeze
+      ELASTICSEARCH_PERMISSION_TRACKED_FIELDS = %w(assignee_ids author_id confidential).freeze
 
       include Elastic::ApplicationVersionedSearch
       include UsageStatistics
@@ -88,6 +89,24 @@ module EE
       super if supports_weight?
     end
 
+    # override
+    def maintain_elasticsearch_update
+      super
+
+      maintain_elasticsearch_issue_notes_update if elasticsearch_issue_notes_need_updating?
+    end
+
+    def maintain_elasticsearch_issue_notes_update
+      ::Note.searchable.where(noteable: self).find_each do |note|
+        note.maintain_elasticsearch_update
+      end
+    end
+
+    def elasticsearch_issue_notes_need_updating?
+      changed_fields = self.previous_changes.keys
+      changed_fields && (changed_fields & ELASTICSEARCH_PERMISSION_TRACKED_FIELDS).any?
+    end
+
     def supports_weight?
       project&.feature_available?(:issue_weights)
     end

ee/app/models/ee/issue_assignee.rb

@@ -8,14 +8,9 @@ module EE
     end
 
     def update_elasticsearch_index
-      if issue.project&.use_elasticsearch?
-        ::ElasticIndexerWorker.perform_async(
-          :update,
-          'Issue',
-          issue.id,
-          issue.es_id,
-          changed_fields: ['assignee_ids']
-        )
+      if issue.project&.use_elasticsearch? && issue.maintaining_elasticsearch?
+        issue.maintain_elasticsearch_update
+        issue.maintain_elasticsearch_issue_notes_update # we need to propagate new permissions to notes
       end
     end
   end

ee/app/services/ee/groups/transfer_service.rb

@@ -32,8 +32,7 @@ module EE
               :update,
               project.class.to_s,
               project.id,
-              project.es_id,
-              changed_fields: ['visibility_level']
+              project.es_id
             )
           end
         end

ee/app/services/elastic/index_record_service.rb

@@ -6,7 +6,6 @@ module Elastic
 
     ImportError = Class.new(StandardError)
 
-    ISSUE_TRACKED_FIELDS = %w(assignee_ids author_id confidential).freeze
     IMPORT_RETRY_COUNT = 3
 
     # @param indexing [Boolean] determines whether operation is "indexing" or "updating"
@@ -19,8 +18,6 @@ module Elastic
 
       initial_index_project(record) if record.class == Project && indexing
 
-      update_issue_notes(record, options["changed_fields"]) if record.class == Issue
-
       true
     rescue Elasticsearch::Transport::Transport::Errors::NotFound, ActiveRecord::RecordNotFound => e
       # These errors can happen in several cases, including:
@@ -37,14 +34,6 @@ module Elastic
 
     private
 
-    # rubocop: disable CodeReuse/ActiveRecord
-    def update_issue_notes(record, changed_fields)
-      if changed_fields && (changed_fields & ISSUE_TRACKED_FIELDS).any?
-        import_association(Note, query: -> { searchable.where(noteable: record) })
-      end
-    end
-    # rubocop: enable CodeReuse/ActiveRecord
-
     def initial_index_project(project)
       # Enqueue the repository indexing jobs immediately so they run in parallel
       # One for the project repository, one for the wiki repository

ee/spec/models/concerns/elastic/note_spec.rb

@@ -112,7 +112,7 @@ describe Note, :elastic do
     issue = create :issue, project: project, updated_at: 1.minute.ago
 
     # Only issue should be updated
-    expect(ElasticIndexerWorker).to receive(:perform_async).with(:update, 'Issue', anything, anything, anything)
+    expect(ElasticIndexerWorker).to receive(:perform_async).with(:update, 'Issue', anything, anything)
     create :note, :system, project: project, noteable: issue
   end
 

ee/spec/models/issue_spec.rb

@@ -303,6 +303,49 @@ describe Issue do
     end
   end
 
+  context 'ES related specs', :elastic do
+    before do
+      stub_ee_application_setting(elasticsearch_indexing: true)
+    end
+
+    context 'when updating an Issue' do
+      let(:project) { create(:project, :public) }
+      let(:issue) { create(:issue, project: project, confidential: true) }
+
+      before do
+        Sidekiq::Testing.disable! do
+          create(:note, note: 'the_normal_note', noteable: issue, project: project)
+        end
+        Sidekiq::Testing.inline! do
+          project.maintain_elasticsearch_update
+
+          issue.update!(update_field => update_value)
+          Gitlab::Elastic::Helper.refresh_index
+        end
+      end
+
+      context 'when changing the confidential value' do
+        let(:update_field) { :confidential }
+        let(:update_value) { false }
+
+        it 'updates issue notes excluding system notes' do
+          expect(issue.elasticsearch_issue_notes_need_updating?).to eq(true)
+          expect(Note.elastic_search('the_normal_note', options: { project_ids: [issue.project.id] }).present?).to eq(true)
+        end
+      end
+
+      context 'when changing the title' do
+        let(:update_field) { :title }
+        let(:update_value) { 'abc' }
+
+        it 'does not update issue notes' do
+          expect(issue.elasticsearch_issue_notes_need_updating?).to eq(false)
+          expect(Note.elastic_search('the_normal_note', options: { project_ids: [issue.project.id] }).present?).to eq(false)
+        end
+      end
+    end
+  end
+
   describe 'relative positioning with group boards' do
     let(:group) { create(:group) }
     let!(:board) { create(:board, group: group) }

ee/spec/services/elastic/index_record_service_spec.rb

@@ -327,51 +327,4 @@ describe Elastic::IndexRecordService, :elastic do
 
     expect(Project.elastic_search('project_1').present?).to eq(false)
   end
-
-  context 'when updating an Issue' do
-    context 'when changing the confidential value' do
-      it 'updates issue notes excluding system notes' do
-        project = create(:project, :public)
-        issue = nil
-        Sidekiq::Testing.disable! do
-          issue = create(:issue, project: project, confidential: false)
-          subject.execute(project, true)
-          subject.execute(issue, false)
-          create(:note, note: 'the_normal_note', noteable: issue, project: project)
-          create(:note, note: 'the_system_note', system: true, noteable: issue, project: project)
-        end
-
-        options = { project_ids: [project.id] }
-
-        Sidekiq::Testing.inline! do
-          expect(subject.execute(issue, false, 'changed_fields' => ['confidential'])).to eq(true)
-          Gitlab::Elastic::Helper.refresh_index
-        end
-
-        expect(Note.elastic_search('the_normal_note', options: options).present?).to eq(true)
-        expect(Note.elastic_search('the_system_note', options: options).present?).to eq(false)
-      end
-    end
-
-    context 'when changing the title' do
-      it 'does not update issue notes' do
-        issue = nil
-        Sidekiq::Testing.disable! do
-          issue = create(:issue, confidential: false)
-          subject.execute(issue.project, true)
-          subject.execute(issue, false)
-          create(:note, note: 'the_normal_note', noteable: issue, project: issue.project)
-        end
-
-        options = { project_ids: [issue.project.id] }
-
-        Sidekiq::Testing.inline! do
-          expect(subject.execute(issue, false, 'changed_fields' => ['title'])).to eq(true)
-          Gitlab::Elastic::Helper.refresh_index
-        end
-
-        expect(Note.elastic_search('the_normal_note', options: options).present?).to eq(false)
-      end
-    end
-  end
 end

ee/spec/services/groups/transfer_service_spec.rb

@@ -85,11 +85,11 @@ describe Groups::TransferService, '#execute' do
       project3 = create(:project, :repository, :private, namespace: group)
 
       expect(ElasticIndexerWorker).to receive(:perform_async)
-        .with(:update, "Project", project1.id, project1.es_id, changed_fields: array_including('visibility_level'))
+        .with(:update, "Project", project1.id, project1.es_id)
       expect(ElasticIndexerWorker).to receive(:perform_async)
-        .with(:update, "Project", project2.id, project2.es_id, changed_fields: array_including('visibility_level'))
+        .with(:update, "Project", project2.id, project2.es_id)
       expect(ElasticIndexerWorker).not_to receive(:perform_async)
-        .with(:update, "Project", project3.id, project3.es_id, changed_fields: array_including('visibility_level'))
+        .with(:update, "Project", project3.id, project3.es_id)
 
       transfer_service.execute(new_group)