Remove CreateTriggerRequestService and forbit to save variables on Ci::TriggerRequest

app/models/ci/trigger_request.rb

@@ -6,6 +6,10 @@ module Ci
     belongs_to :pipeline, foreign_key: :commit_id
     has_many :builds
 
+    # Ws swtiched to Ci::PipelineVariable from Ci::TriggerRequest.variables.
+    # Ci::TriggerRequest doesn't save variables anymore.
+    validates :variables, absence: true
+
     serialize :variables # rubocop:disable Cop/ActiveRecordSerialize
 
     def user_variables

app/services/ci/create_trigger_request_service.rb

-# This class is deprecated because we're closing Ci::TriggerRequest.
-# New class is PipelineTriggerService (app/services/ci/pipeline_trigger_service.rb)
-# which is integrated with Ci::PipelineVariable instaed of Ci::TriggerRequest.
-# We remove this class after we removed v1 and v3 API. This class is still being
-# referred by such legacy code.
-module Ci
-  module CreateTriggerRequestService
-    Result = Struct.new(:trigger_request, :pipeline)
-
-    def self.execute(project, trigger, ref, variables = nil)
-      trigger_request = trigger.trigger_requests.create(variables: variables)
-
-      pipeline = Ci::CreatePipelineService.new(project, trigger.owner, ref: ref)
-        .execute(:trigger, ignore_skip_ci: true, trigger_request: trigger_request)
-
-      Result.new(trigger_request, pipeline)
-    end
-  end
-end

lib/api/triggers.rb

@@ -15,16 +15,16 @@ module API
         optional :variables, type: Hash, desc: 'The list of variables to be injected into build'
       end
       post ":id/(ref/:ref/)trigger/pipeline", requirements: { ref: /.+/ } do
+        authenticate!
+        authorize! :admin_build, user_project
+
         # validate variables
         params[:variables] = params[:variables].to_h
         unless params[:variables].all? { |key, value| key.is_a?(String) && value.is_a?(String) }
           render_api_error!('variables needs to be a map of key-valued strings', 400)
         end
 
-        project = find_project(params[:id])
-        not_found! unless project
-
-        result = Ci::PipelineTriggerService.new(project, nil, params).execute
+        result = Ci::PipelineTriggerService.new(user_project, nil, params).execute
         not_found! unless result
 
         if result[:http_status]

lib/api/v3/triggers.rb

@@ -16,25 +16,32 @@ module API
           optional :variables, type: Hash, desc: 'The list of variables to be injected into build'
         end
         post ":id/(ref/:ref/)trigger/builds", requirements: { ref: /.+/ } do
-          project = find_project(params[:id])
-          trigger = Ci::Trigger.find_by_token(params[:token].to_s)
-          not_found! unless project && trigger
-          unauthorized! unless trigger.project == project
+          authenticate!
+          authorize! :admin_build, user_project
 
           # validate variables
-          variables = params[:variables].to_h
-          unless variables.all? { |key, value| key.is_a?(String) && value.is_a?(String) }
+          params[:variables] = params[:variables].to_h
+          unless params[:variables].all? { |key, value| key.is_a?(String) && value.is_a?(String) }
             render_api_error!('variables needs to be a map of key-valued strings', 400)
           end
 
-          # create request and trigger builds
-          result = Ci::CreateTriggerRequestService.execute(project, trigger, params[:ref].to_s, variables)
-          pipeline = result.pipeline
+          result = Ci::PipelineTriggerService.new(user_project, nil, params).execute
+          not_found! unless result
 
-          if pipeline.persisted?
-            present result.trigger_request, with: ::API::V3::Entities::TriggerRequest
+          if result[:http_status]
+            render_api_error!(result[:message], result[:http_status])
           else
-            render_validation_error!(pipeline)
+            pipeline = result[:pipeline]
+            trigger_request = pipeline.trigger_request
+
+            # Ws swtiched to Ci::PipelineVariable from Ci::TriggerRequest.variables.
+            # Ci::TriggerRequest doesn't save variables anymore.
+            # Although, to prevent braking compatibility, copying variables and present it as Ci::TriggerRequest.
+            pipeline.variables.each do |variable|
+              trigger_request.variables << { key: variable.key, value: variable.value }
+            end
+
+            present trigger_request, with: ::API::V3::Entities::TriggerRequest
           end
         end
 

spec/factories/ci/trigger_requests.rb

@@ -2,13 +2,14 @@ FactoryGirl.define do
   factory :ci_trigger_request, class: Ci::TriggerRequest do
     trigger factory: :ci_trigger
 
-    factory :ci_trigger_request_with_variables do
-      variables do
-        {
-          TRIGGER_KEY_1: 'TRIGGER_VALUE_1',
-          TRIGGER_KEY_2: 'TRIGGER_VALUE_2'
-        }
-      end
-    end
+    # TODO: 
+    # factory :ci_trigger_request_with_variables do
+    #   variables do
+    #     {
+    #       TRIGGER_KEY_1: 'TRIGGER_VALUE_1',
+    #       TRIGGER_KEY_2: 'TRIGGER_VALUE_2'
+    #     }
+    #   end
+    # end
   end
 end

spec/models/ci/trigger_request_spec.rb

+require 'spec_helper'
+
+describe Ci::TriggerRequest do
+  describe 'validation' do
+    it 'be invalid if saving a variable' do
+      trigger = build(:ci_trigger_request, variables: { TRIGGER_KEY_1: 'TRIGGER_VALUE_1' } )
+
+      expect(trigger.valid?).to be_falsey
+    end
+
+    it 'be valid if not saving a variable' do
+      trigger = build(:ci_trigger_request)
+
+      expect(trigger.valid?).to be_truthy
+    end
+  end
+end

spec/services/ci/create_trigger_request_service_spec.rb

-require 'spec_helper'
-
-describe Ci::CreateTriggerRequestService do
-  let(:service) { described_class }
-  let(:project) { create(:project, :repository) }
-  let(:trigger) { create(:ci_trigger, project: project, owner: owner) }
-  let(:owner) { create(:user) }
-
-  before do
-    stub_ci_pipeline_to_return_yaml_file
-
-    project.add_developer(owner)
-  end
-
-  describe '#execute' do
-    context 'valid params' do
-      subject { service.execute(project, trigger, 'master') }
-
-      context 'without owner' do
-        it { expect(subject.trigger_request).to be_kind_of(Ci::TriggerRequest) }
-        it { expect(subject.trigger_request.builds.first).to be_kind_of(Ci::Build) }
-        it { expect(subject.pipeline).to be_kind_of(Ci::Pipeline) }
-        it { expect(subject.pipeline).to be_trigger }
-      end
-
-      context 'with owner' do
-        it { expect(subject.trigger_request).to be_kind_of(Ci::TriggerRequest) }
-        it { expect(subject.trigger_request.builds.first).to be_kind_of(Ci::Build) }
-        it { expect(subject.trigger_request.builds.first.user).to eq(owner) }
-        it { expect(subject.pipeline).to be_kind_of(Ci::Pipeline) }
-        it { expect(subject.pipeline).to be_trigger }
-        it { expect(subject.pipeline.user).to eq(owner) }
-      end
-    end
-
-    context 'no commit for ref' do
-      subject { service.execute(project, trigger, 'other-branch') }
-
-      it { expect(subject.pipeline).not_to be_persisted }
-    end
-
-    context 'no builds created' do
-      subject { service.execute(project, trigger, 'master') }
-
-      before do
-        stub_ci_pipeline_yaml_file('script: { only: [develop], script: hello World }')
-      end
-
-      it { expect(subject.pipeline).not_to be_persisted }
-    end
-  end
-end