Use newest records for test webhooks

https://gitlab.com/gitlab-org/gitlab/-/merge_requests/52646 introduced
a change to the order of some test webhook data in order to optimise
the queries.

This change is to align other test webhooks to also product the newest
record in the test payload even though there are no performance gains.

https://gitlab.com/gitlab-org/gitlab/-/issues/290116

app/services/concerns/integrations/project_test_data.rb

@@ -8,6 +8,10 @@ module Integrations
       Gitlab::DataBuilder::Push.build_sample(project, current_user)
     end
 
+    def use_newest_record?
+      Feature.enabled?(:integrations_test_webhook_reorder, project)
+    end
+
     def note_events_data
       note = NotesFinder.new(current_user, project: project, target: project).execute.reorder(nil).last # rubocop: disable CodeReuse/ActiveRecord
 
@@ -33,14 +37,24 @@ module Integrations
     end
 
     def job_events_data
-      build = project.builds.first
+      build = if use_newest_record?
+                Ci::JobsFinder.new(current_user: current_user, project: project).execute.first
+              else
+                project.builds.first
+              end
+
       return { error: s_('TestHooks|Ensure the project has CI jobs.') } unless build.present?
 
       Gitlab::DataBuilder::Build.build(build)
     end
 
     def pipeline_events_data
-      pipeline = project.ci_pipelines.newest_first.first
+      pipeline = if use_newest_record?
+                   Ci::PipelinesFinder.new(project, current_user, order_by: 'id', sort: 'desc').execute.first
+                 else
+                   project.ci_pipelines.newest_first.first
+                 end
+
       return { error: s_('TestHooks|Ensure the project has CI pipelines.') } unless pipeline.present?
 
       Gitlab::DataBuilder::Pipeline.build(pipeline)
@@ -48,6 +62,7 @@ module Integrations
 
     def wiki_page_events_data
       page = project.wiki.list_pages(limit: 1).first
+
       if !project.wiki_enabled? || page.blank?
         return { error: s_('TestHooks|Ensure the wiki is enabled and has pages.') }
       end
@@ -56,14 +71,24 @@ module Integrations
     end
 
     def deployment_events_data
-      deployment = project.deployments.first
+      deployment = if use_newest_record?
+                     DeploymentsFinder.new(project: project, order_by: 'created_at', sort: 'desc').execute.first
+                   else
+                     project.deployments.first
+                   end
+
       return { error: s_('TestHooks|Ensure the project has deployments.') } unless deployment.present?
 
       Gitlab::DataBuilder::Deployment.build(deployment)
     end
 
     def releases_events_data
-      release = project.releases.first
+      release = if use_newest_record?
+                  ReleasesFinder.new(project, current_user, order_by: :created_at, sort: :desc).execute.first
+                else
+                  project.releases.first
+                end
+
       return { error: s_('TestHooks|Ensure the project has releases.') } unless release.present?
 
       release.to_hook_data('create')

app/services/test_hooks/system_service.rb

@@ -6,6 +6,10 @@ module TestHooks
 
     private
 
+    def use_newest_record?
+      Feature.enabled?(:integrations_test_webhook_reorder)
+    end
+
     def data
       strong_memoize(:data) do
         case trigger
@@ -20,7 +24,12 @@ module TestHooks
     end
 
     def merge_requests_events_data
-      merge_request = MergeRequest.of_projects(current_user.projects.select(:id)).first
+      merge_request = if use_newest_record?
+                        MergeRequest.of_projects(current_user.projects.select(:id)).last
+                      else
+                        MergeRequest.of_projects(current_user.projects.select(:id)).first
+                      end
+
       return { error: s_('TestHooks|Ensure one of your projects has merge requests.') } unless merge_request.present?
 
       merge_request.to_hook_data(current_user)

config/feature_flags/development/integrations_test_webhook_reorder.yml

+---
+name: integrations_test_webhook_reorder
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/53433
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/320861
+milestone: '13.9'
+type: development
+group: group::ecosystem
+default_enabled: false

ee/spec/services/ee/integrations/test/project_service_spec.rb

@@ -3,10 +3,9 @@
 require 'spec_helper'
 
 RSpec.describe ::Integrations::Test::ProjectService do
-  let(:user) { double('user') }
-
   describe '#execute' do
     let(:project) { create(:project) }
+    let(:user) { project.owner }
     let(:event) { nil }
     let(:sample_data) { { data: 'sample' } }
     let(:success_result) { { success: true, result: {} } }

spec/services/integrations/test/project_service_spec.rb

@@ -3,6 +3,8 @@
 require 'spec_helper'
 
 RSpec.describe Integrations::Test::ProjectService do
+  include AfterNextHelpers
+
   describe '#execute' do
     let_it_be(:project) { create(:project) }
     let(:integration) { create(:slack_service, project: project) }
@@ -72,9 +74,7 @@ RSpec.describe Integrations::Test::ProjectService do
           create(:note, project: project)
 
           allow(Gitlab::DataBuilder::Note).to receive(:build).and_return(sample_data)
-          allow_next_instance_of(NotesFinder) do |finder|
-            allow(finder).to receive(:execute).and_return(Note.all)
-          end
+          allow_next(NotesFinder).to receive(:execute).and_return(Note.all)
 
           expect(integration).to receive(:test).with(sample_data).and_return(success_result)
           expect(subject).to eq(success_result)
@@ -92,9 +92,7 @@ RSpec.describe Integrations::Test::ProjectService do
         it 'executes integration' do
           allow(project).to receive(:issues).and_return([issue])
           allow(issue).to receive(:to_hook_data).and_return(sample_data)
-          allow_next_instance_of(IssuesFinder) do |finder|
-            allow(finder).to receive(:execute).and_return([issue])
-          end
+          allow_next(IssuesFinder).to receive(:execute).and_return([issue])
 
           expect(integration).to receive(:test).with(sample_data).and_return(success_result)
           expect(subject).to eq(success_result)
@@ -124,9 +122,7 @@ RSpec.describe Integrations::Test::ProjectService do
 
         it 'executes integration' do
           allow(merge_request).to receive(:to_hook_data).and_return(sample_data)
-          allow_next_instance_of(MergeRequestsFinder) do |finder|
-            allow(finder).to receive(:execute).and_return([merge_request])
-          end
+          allow_next(MergeRequestsFinder).to receive(:execute).and_return([merge_request])
 
           expect(integration).to receive(:test).with(sample_data).and_return(success_result)
           expect(subject).to include(success_result)
@@ -135,6 +131,7 @@ RSpec.describe Integrations::Test::ProjectService do
 
       context 'deployment' do
         let_it_be(:project) { create(:project, :test_repo) }
+        let(:deployment) { build(:deployment) }
         let(:event) { 'deployment' }
 
         it 'returns error message if not enough data' do
@@ -143,16 +140,32 @@ RSpec.describe Integrations::Test::ProjectService do
         end
 
         it 'executes integration' do
-          create(:deployment, project: project)
+          allow(Gitlab::DataBuilder::Deployment).to receive(:build).and_return(sample_data)
+          allow_next(DeploymentsFinder).to receive(:execute).and_return([deployment])
+
+          expect(integration).to receive(:test).with(sample_data).and_return(success_result)
+          expect(subject).to eq(success_result)
+        end
+
+        context 'when the reorder feature flag is disabled' do
+          before do
+            stub_feature_flags(integrations_test_webhook_reorder: false)
+          end
+
+          it 'executes the old query' do
             allow(Gitlab::DataBuilder::Deployment).to receive(:build).and_return(sample_data)
 
+            expect(DeploymentsFinder).not_to receive(:new)
+            expect(project).to receive(:deployments).and_return([deployment])
             expect(integration).to receive(:test).with(sample_data).and_return(success_result)
             expect(subject).to eq(success_result)
           end
         end
+      end
 
       context 'pipeline' do
         let(:event) { 'pipeline' }
+        let(:pipeline) { build(:ci_pipeline) }
 
         it 'returns error message if not enough data' do
           expect(integration).not_to receive(:test)
@@ -160,13 +173,28 @@ RSpec.describe Integrations::Test::ProjectService do
         end
 
         it 'executes integration' do
+          allow(Gitlab::DataBuilder::Pipeline).to receive(:build).and_return(sample_data)
+          allow_next(Ci::PipelinesFinder).to receive(:execute).and_return([pipeline])
+
+          expect(integration).to receive(:test).with(sample_data).and_return(success_result)
+          expect(subject).to eq(success_result)
+        end
+
+        context 'when the reorder feature flag is disabled' do
+          before do
+            stub_feature_flags(integrations_test_webhook_reorder: false)
+          end
+
+          it 'executes the old query' do
             create(:ci_empty_pipeline, project: project)
             allow(Gitlab::DataBuilder::Pipeline).to receive(:build).and_return(sample_data)
 
+            expect(Ci::PipelinesFinder).not_to receive(:new)
             expect(integration).to receive(:test).with(sample_data).and_return(success_result)
             expect(subject).to eq(success_result)
           end
         end
+      end
 
       context 'wiki_page' do
         let_it_be(:project) { create(:project, :wiki_repo) }

spec/services/test_hooks/project_service_spec.rb

@@ -3,6 +3,8 @@
 require 'spec_helper'
 
 RSpec.describe TestHooks::ProjectService do
+  include AfterNextHelpers
+
   let(:current_user) { create(:user) }
 
   describe '#execute' do
@@ -64,9 +66,7 @@ RSpec.describe TestHooks::ProjectService do
         create(:note, project: project)
 
         allow(Gitlab::DataBuilder::Note).to receive(:build).and_return(sample_data)
-        allow_next_instance_of(NotesFinder) do |finder|
-          allow(finder).to receive(:execute).and_return(Note.all)
-        end
+        allow_next(NotesFinder).to receive(:execute).and_return(Note.all)
 
         expect(hook).to receive(:execute).with(sample_data, trigger_key).and_return(success_result)
         expect(service.execute).to include(success_result)
@@ -83,9 +83,7 @@ RSpec.describe TestHooks::ProjectService do
 
       it 'executes hook' do
         allow(issue).to receive(:to_hook_data).and_return(sample_data)
-        allow_next_instance_of(IssuesFinder) do |finder|
-          allow(finder).to receive(:execute).and_return([issue])
-        end
+        allow_next(IssuesFinder).to receive(:execute).and_return([issue])
 
         expect(hook).to receive(:execute).with(sample_data, trigger_key).and_return(success_result)
         expect(service.execute).to include(success_result)
@@ -118,9 +116,7 @@ RSpec.describe TestHooks::ProjectService do
 
       it 'executes hook' do
         allow(merge_request).to receive(:to_hook_data).and_return(sample_data)
-        allow_next_instance_of(MergeRequestsFinder) do |finder|
-          allow(finder).to receive(:execute).and_return([merge_request])
-        end
+        allow_next(MergeRequestsFinder).to receive(:execute).and_return([merge_request])
 
         expect(hook).to receive(:execute).with(sample_data, trigger_key).and_return(success_result)
         expect(service.execute).to include(success_result)
@@ -130,6 +126,7 @@ RSpec.describe TestHooks::ProjectService do
     context 'job_events' do
       let(:trigger) { 'job_events' }
       let(:trigger_key) { :job_hooks }
+      let(:ci_job) { build(:ci_build) }
 
       it 'returns error message if not enough data' do
         expect(hook).not_to receive(:execute)
@@ -137,17 +134,33 @@ RSpec.describe TestHooks::ProjectService do
       end
 
       it 'executes hook' do
-        create(:ci_build, project: project)
         allow(Gitlab::DataBuilder::Build).to receive(:build).and_return(sample_data)
+        allow_next(Ci::JobsFinder).to receive(:execute).and_return([ci_job])
 
         expect(hook).to receive(:execute).with(sample_data, trigger_key).and_return(success_result)
         expect(service.execute).to include(success_result)
       end
+
+      context 'when the reorder feature flag is disabled' do
+        before do
+          stub_feature_flags(integrations_test_webhook_reorder: false)
+        end
+
+        it 'executes the old query' do
+          allow(Gitlab::DataBuilder::Build).to receive(:build).and_return(sample_data)
+
+          expect(Ci::JobsFinder).not_to receive(:new)
+          expect(project).to receive(:builds).and_return([ci_job])
+          expect(hook).to receive(:execute).with(sample_data, trigger_key).and_return(success_result)
+          expect(service.execute).to include(success_result)
+        end
+      end
     end
 
     context 'pipeline_events' do
       let(:trigger) { 'pipeline_events' }
       let(:trigger_key) { :pipeline_hooks }
+      let(:pipeline) { build(:ci_empty_pipeline) }
 
       it 'returns error message if not enough data' do
         expect(hook).not_to receive(:execute)
@@ -155,13 +168,28 @@ RSpec.describe TestHooks::ProjectService do
       end
 
       it 'executes hook' do
+        allow(Gitlab::DataBuilder::Pipeline).to receive(:build).and_return(sample_data)
+        allow_next(Ci::PipelinesFinder).to receive(:execute).and_return([pipeline])
+
+        expect(hook).to receive(:execute).with(sample_data, trigger_key).and_return(success_result)
+        expect(service.execute).to include(success_result)
+      end
+
+      context 'when the reorder feature flag is disabled' do
+        before do
+          stub_feature_flags(integrations_test_webhook_reorder: false)
+        end
+
+        it 'executes the old query' do
           create(:ci_empty_pipeline, project: project)
           allow(Gitlab::DataBuilder::Pipeline).to receive(:build).and_return(sample_data)
 
+          expect(Ci::PipelinesFinder).not_to receive(:new)
           expect(hook).to receive(:execute).with(sample_data, trigger_key).and_return(success_result)
           expect(service.execute).to include(success_result)
         end
       end
+    end
 
     context 'wiki_page_events' do
       let_it_be(:project) { create(:project, :wiki_repo) }
@@ -192,6 +220,7 @@ RSpec.describe TestHooks::ProjectService do
     context 'releases_events' do
       let(:trigger) { 'releases_events' }
       let(:trigger_key) { :release_hooks }
+      let(:release) { build(:release) }
 
       it 'returns error message if not enough data' do
         expect(hook).not_to receive(:execute)
@@ -199,12 +228,27 @@ RSpec.describe TestHooks::ProjectService do
       end
 
       it 'executes hook' do
-        allow(project).to receive(:releases).and_return([Release.new])
-        allow_any_instance_of(Release).to receive(:to_hook_data).and_return(sample_data)
+        allow(release).to receive(:to_hook_data).and_return(sample_data)
+        allow_next(ReleasesFinder).to receive(:execute).and_return([release])
 
         expect(hook).to receive(:execute).with(sample_data, trigger_key).and_return(success_result)
         expect(service.execute).to include(success_result)
       end
+
+      context 'when the reorder feature flag is disabled' do
+        before do
+          stub_feature_flags(integrations_test_webhook_reorder: false)
+        end
+
+        it 'executes the old query' do
+          allow(release).to receive(:to_hook_data).and_return(sample_data)
+
+          expect(ReleasesFinder).not_to receive(:new)
+          expect(project).to receive(:releases).and_return([release])
+          expect(hook).to receive(:execute).with(sample_data, trigger_key).and_return(success_result)
+          expect(service.execute).to include(success_result)
+        end
+      end
     end
   end
 end

spec/services/test_hooks/system_service_spec.rb

@@ -3,12 +3,12 @@
 require 'spec_helper'
 
 RSpec.describe TestHooks::SystemService do
-  let(:current_user) { create(:user) }
+  include AfterNextHelpers
 
   describe '#execute' do
-    let(:project) { create(:project, :repository) }
+    let_it_be(:project) { create(:project, :repository) }
     let(:hook)    { create(:system_hook) }
-    let(:service) { described_class.new(hook, current_user, trigger) }
+    let(:service) { described_class.new(hook, project.owner, trigger) }
     let(:success_result) { { status: :success, http_status: 200, message: 'ok' } }
 
     before do
@@ -63,6 +63,9 @@ RSpec.describe TestHooks::SystemService do
 
     context 'merge_requests_events' do
       let(:trigger) { 'merge_requests_events' }
+      let(:trigger_key) { :merge_request_hooks }
+      let(:merge_requests) { build_list(:merge_request, 2) }
+      let(:sample_data) { { data: 'sample' } }
 
       it 'returns error message if the user does not have any repository with a merge request' do
         expect(hook).not_to receive(:execute)
@@ -70,15 +73,24 @@ RSpec.describe TestHooks::SystemService do
       end
 
       it 'executes hook' do
-        trigger_key = :merge_request_hooks
-        sample_data = { data: 'sample' }
-        create(:project_member, user: current_user, project: project)
-        create(:merge_request, source_project: project)
-        allow_any_instance_of(MergeRequest).to receive(:to_hook_data).and_return(sample_data)
+        expect(MergeRequest).to receive(:of_projects).and_return(merge_requests)
+        expect(merge_requests.last).to receive(:to_hook_data).and_return(sample_data)
+        expect(hook).to receive(:execute).with(sample_data, trigger_key).and_return(success_result)
+        expect(service.execute).to include(success_result)
+      end
 
+      context 'when the reorder feature flag is disabled' do
+        before do
+          stub_feature_flags(integrations_test_webhook_reorder: false)
+        end
+
+        it 'executes the old query' do
+          expect(MergeRequest).to receive(:of_projects).and_return(merge_requests)
+          expect(merge_requests.first).to receive(:to_hook_data).and_return(sample_data)
           expect(hook).to receive(:execute).with(sample_data, trigger_key).and_return(success_result)
           expect(service.execute).to include(success_result)
         end
       end
     end
+  end
 end