Merge branch '288370-add-check-for-pipeline-touch' into 'master'

Add checking if pipeline needs to be touched

See merge request gitlab-org/gitlab!48629

ee/app/models/ee/ci/pipeline.rb

@@ -80,6 +80,10 @@ module EE
         end
       end
 
+      def needs_touch?
+        updated_at < 5.minutes.ago
+      end
+
       def triggers_subscriptions?
         # Currently we trigger subscriptions only for tags.
         tag? && project_has_subscriptions?

ee/app/models/vulnerabilities/feedback.rb

@@ -88,7 +88,10 @@ module Vulnerabilities
     end
 
     def touch_pipeline
-      pipeline&.touch
+      pipeline&.touch if pipeline&.needs_touch?
+    rescue ActiveRecord::StaleObjectError
+      # Often the pipeline has already been updated by creating vulnerability feedback
+      # in batches. In this case, we can ignore the exception as it's already been touched.
     end
 
     def finding

ee/spec/models/ci/pipeline_spec.rb

@@ -606,4 +606,24 @@ RSpec.describe Ci::Pipeline do
       it { is_expected.to be_falsey }
     end
   end
+
+  describe '#needs_touch?' do
+    subject { pipeline.needs_touch? }
+
+    context 'when pipeline was updated less than 5 minutes ago' do
+      before do
+        pipeline.updated_at = 4.minutes.ago
+      end
+
+      it { is_expected.to eq(false) }
+    end
+
+    context 'when pipeline was updated more than 5 minutes ago' do
+      before do
+        pipeline.updated_at = 6.minutes.ago
+      end
+
+      it { is_expected.to eq(true) }
+    end
+  end
 end

ee/spec/models/vulnerabilities/feedback_spec.rb

@@ -79,6 +79,85 @@ RSpec.describe Vulnerabilities::Feedback do
     end
   end
 
+  describe 'callbacks' do
+    let_it_be(:project) { create(:project) }
+    let_it_be_with_refind(:pipeline) { create(:ci_pipeline, project: project) }
+
+    shared_examples 'touches the pipeline' do
+      context 'when feedback is for dismissal' do
+        let_it_be_with_refind(:feedback) { create(:vulnerability_feedback, :dismissal, project: project) }
+
+        context 'when pipeline is not assigned to feedback' do
+          it 'does not touch the pipeline' do
+            expect(pipeline).not_to receive(:touch)
+            subject
+          end
+        end
+
+        context 'when pipeline is assigned to feedback' do
+          before do
+            feedback.update(pipeline: pipeline)
+          end
+
+          context 'when pipeline was updated less than 5 minutes ago' do
+            before do
+              pipeline.touch(time: 3.minutes.ago)
+            end
+
+            it 'touches the pipeline' do
+              expect(pipeline).not_to receive(:touch)
+              subject
+            end
+          end
+
+          context 'when pipeline was updated more than 5 minutes ago' do
+            before do
+              pipeline.touch(time: 6.minutes.ago)
+            end
+
+            it 'touches the pipeline' do
+              expect(pipeline).to receive(:touch)
+              subject
+            end
+
+            context 'when pipeline touch raises ActiveRecord::StaleObjectError' do
+              before do
+                allow(pipeline).to receive(:touch).and_raise(ActiveRecord::StaleObjectError)
+              end
+
+              it 'does not raise an error' do
+                expect {subject}.not_to raise_error
+              end
+            end
+          end
+        end
+      end
+
+      context 'when feedback is not for dismissal' do
+        let_it_be_with_refind(:feedback) { create(:vulnerability_feedback, :issue) }
+
+        context 'when pipeline is not assigned to feedback' do
+          it 'does not touch the pipeline' do
+            expect(pipeline).not_to receive(:touch)
+            subject
+          end
+        end
+      end
+    end
+
+    context 'after_save :touch_pipeline' do
+      subject { feedback.update!(vulnerability_data: { category: 'dependency_scanning' }) }
+
+      it_behaves_like 'touches the pipeline'
+    end
+
+    context 'after_destroy :touch_pipeline' do
+      subject { feedback.destroy! }
+
+      it_behaves_like 'touches the pipeline'
+    end
+  end
+
   describe '.with_category' do
     it 'filters by category' do
       described_class.categories.each do |category, _|

ee/spec/services/vulnerability_feedback/create_service_spec.rb

@@ -6,7 +6,7 @@ RSpec.describe VulnerabilityFeedback::CreateService, '#execute' do
   let(:group)   { create(:group) }
   let(:project) { create(:project, :public, :repository, namespace: group) }
   let(:user)    { create(:user) }
-  let(:pipeline) { create(:ci_pipeline, project: project) }
+  let(:pipeline) { create(:ci_pipeline, project: project, created_at: 6.minutes.ago, updated_at: 6.minutes.ago) }
   let(:dismiss_vulnerability) { true }
 
   before do
@@ -71,9 +71,19 @@ RSpec.describe VulnerabilityFeedback::CreateService, '#execute' do
         expect(feedback.merge_request).to be_nil
       end
 
+      context 'when pipeline was updated more than 5 minutes ago' do
         it 'touches pipeline related to feedback' do
           expect { result }.to change { pipeline.reload.updated_at }
         end
+      end
+
+      context 'when pipeline was updated less than 5 minutes ago' do
+        let(:pipeline) { create(:ci_pipeline, project: project, created_at: 4.minutes.ago, updated_at: 4.minutes.ago) }
+
+        it 'does not touch pipeline related to feedback' do
+          expect { result }.not_to change { pipeline.reload.updated_at }
+        end
+      end
 
       context 'when feedback params has a comment' do
         it 'sets the comment attributes' do