Remove ci_trigger_request_with_variables

9 files changed, 92 insertions, 48 deletions

diff --git a/app/presenters/ci/build_presenter.rb b/app/presenters/ci/build_presenter.rb
index c495c3f39bb..531ccd39cf2 100644
--- a/app/presenters/ci/build_presenter.rb
+++ b/app/presenters/ci/build_presenter.rb
@@ -17,5 +17,16 @@ module Ci
         "Job is redundant and is auto-canceled by Pipeline ##{auto_canceled_by_id}"
       end
     end
+
+    def old_or_new_trigger_variables
+      return @old_or_new_trigger_variables if defined?(@old_or_new_trigger_variables)
+
+      if build.pipeline.variables.any?
+        @old_or_new_trigger_variables =
+          build.pipeline.variables&.map { |v| { v.key => v.value } } &.reduce({}, :merge)
+      else
+        @old_or_new_trigger_variables = build.trigger_request.variables
+      end
+    end
   end
 end
diff --git a/app/views/projects/jobs/_sidebar.html.haml b/app/views/projects/jobs/_sidebar.html.haml
index f5d5bc7eda9..f22de357e5f 100644
--- a/app/views/projects/jobs/_sidebar.html.haml
+++ b/app/views/projects/jobs/_sidebar.html.haml
@@ -46,12 +46,12 @@
             %span.build-light-text Token:
             #{@build.trigger_request.trigger.short_token}
 
-          - if @build.trigger_request.variables
+          - if @build.old_or_new_trigger_variables
             %p
               %button.btn.group.btn-group-justified.reveal-variables Reveal Variables
 
             %dl.js-build-variables.trigger-build-variables.hide
-              - @build.trigger_request.variables.each do |key, value|
+              - @build.old_or_new_trigger_variables.each do |key, value|
                 %dt.js-build-variable.trigger-build-variable= key
                 %dd.js-build-value.trigger-build-value= value
 
diff --git a/spec/factories/ci/builds.rb b/spec/factories/ci/builds.rb
index 25ec63de94a..c2b59239af9 100644
--- a/spec/factories/ci/builds.rb
+++ b/spec/factories/ci/builds.rb
@@ -107,7 +107,7 @@ FactoryGirl.define do
     end
 
     trait :triggered do
-      trigger_request factory: :ci_trigger_request_with_variables
+      trigger_request factory: :ci_trigger_request
     end
 
     after(:build) do |build, evaluator|
diff --git a/spec/factories/ci/pipeline_variable_variables.rb b/spec/factories/ci/pipeline_variables.rb
index 7c1a7faec08..7c1a7faec08 100644
--- a/spec/factories/ci/pipeline_variable_variables.rb
+++ b/spec/factories/ci/pipeline_variables.rb
diff --git a/spec/factories/ci/trigger_requests.rb b/spec/factories/ci/trigger_requests.rb
index 72dad8c418c..40b8848920e 100644
--- a/spec/factories/ci/trigger_requests.rb
+++ b/spec/factories/ci/trigger_requests.rb
@@ -1,13 +1,5 @@
 FactoryGirl.define do
   factory :ci_trigger_request, class: Ci::TriggerRequest do
     trigger factory: :ci_trigger
-
-    # We switched to Ci::PipelineVariable from Ci::TriggerRequest.variables.
-    # Ci::TriggerRequest doesn't save variables anymore, whereas old trigger requests still persist variables.
-    factory :ci_trigger_request_with_variables do
-      after(:create) do |trigger_request, evaluator|
-        trigger_request.update_attribute(:variables, { TRIGGER_KEY_1: 'TRIGGER_VALUE_1', TRIGGER_KEY_2: 'TRIGGER_VALUE_2' } )
-      end
-    end
   end
 end
diff --git a/spec/features/projects/jobs_spec.rb b/spec/features/projects/jobs_spec.rb
index 037ac00d39f..3b5c6966287 100644
--- a/spec/features/projects/jobs_spec.rb
+++ b/spec/features/projects/jobs_spec.rb
@@ -292,26 +292,44 @@ feature 'Jobs' do
     end
 
     feature 'Variables' do
-      let(:trigger_request) { create(:ci_trigger_request_with_variables) }
+      let(:trigger_request) { create(:ci_trigger_request) }
 
       let(:job) do
         create :ci_build, pipeline: pipeline, trigger_request: trigger_request
       end
 
-      before do
-        visit project_job_path(project, job)
+      shared_examples 'expected variables behavior' do
+        it 'shows variable key and value after click', js: true do
+          expect(page).to have_css('.reveal-variables')
+          expect(page).not_to have_css('.js-build-variable')
+          expect(page).not_to have_css('.js-build-value')
+
+          click_button 'Reveal Variables'
+
+          expect(page).not_to have_css('.reveal-variables')
+          expect(page).to have_selector('.js-build-variable', text: 'TRIGGER_KEY_1')
+          expect(page).to have_selector('.js-build-value', text: 'TRIGGER_VALUE_1')
+        end
       end
 
-      it 'shows variable key and value after click', js: true do
-        expect(page).to have_css('.reveal-variables')
-        expect(page).not_to have_css('.js-build-variable')
-        expect(page).not_to have_css('.js-build-value')
+      context 'when variables are stored in trigger_request' do
+        before do
+          trigger_request.update_attribute(:variables, { 'TRIGGER_KEY_1' => 'TRIGGER_VALUE_1' } )
 
-        click_button 'Reveal Variables'
+          visit project_job_path(project, job)
+        end
+
+        it_behaves_like 'expected variables behavior'
+      end
+
+      context 'when variables are stored in pipeline_variables' do
+        before do
+          create(:ci_pipeline_variable, pipeline: pipeline, key: 'TRIGGER_KEY_1', value: 'TRIGGER_VALUE_1')
+
+          visit project_job_path(project, job)
+        end
 
-        expect(page).not_to have_css('.reveal-variables')
-        expect(page).to have_selector('.js-build-variable', text: 'TRIGGER_KEY_1')
-        expect(page).to have_selector('.js-build-value', text: 'TRIGGER_VALUE_1')
+        it_behaves_like 'expected variables behavior'
       end
     end
 
diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb
index 3fe3ec17d36..08d22f166e4 100644
--- a/spec/models/ci/build_spec.rb
+++ b/spec/models/ci/build_spec.rb
@@ -1492,10 +1492,12 @@ describe Ci::Build do
 
     context 'when build is for triggers' do
       let(:trigger) { create(:ci_trigger, project: project) }
-      let(:trigger_request) { create(:ci_trigger_request_with_variables, pipeline: pipeline, trigger: trigger) }
+      let(:trigger_request) { create(:ci_trigger_request, pipeline: pipeline, trigger: trigger) }
+
       let(:user_trigger_variable) do
-        { key: :TRIGGER_KEY_1, value: 'TRIGGER_VALUE_1', public: false }
+        { key: 'TRIGGER_KEY_1', value: 'TRIGGER_VALUE_1', public: false }
       end
+
       let(:predefined_trigger_variable) do
         { key: 'CI_PIPELINE_TRIGGERED', value: 'true', public: true }
       end
@@ -1504,8 +1506,26 @@ describe Ci::Build do
         build.trigger_request = trigger_request
       end
 
-      it { is_expected.to include(user_trigger_variable) }
-      it { is_expected.to include(predefined_trigger_variable) }
+      shared_examples 'returns variables for triggers' do
+        it { is_expected.to include(user_trigger_variable) }
+        it { is_expected.to include(predefined_trigger_variable) }
+      end
+
+      context 'when variables are stored in trigger_request' do
+        before do
+          trigger_request.update_attribute(:variables, { 'TRIGGER_KEY_1' => 'TRIGGER_VALUE_1' } )
+        end
+
+        it_behaves_like 'returns variables for triggers'
+      end
+
+      context 'when variables are stored in pipeline_variables' do
+        before do
+          create(:ci_pipeline_variable, pipeline: pipeline, key: 'TRIGGER_KEY_1', value: 'TRIGGER_VALUE_1')
+        end
+
+        it_behaves_like 'returns variables for triggers'
+      end
     end
 
     context 'when pipeline has a variable' do
diff --git a/spec/requests/api/runner_spec.rb b/spec/requests/api/runner_spec.rb
index 993164aa8fe..6fede7c9f75 100644
--- a/spec/requests/api/runner_spec.rb
+++ b/spec/requests/api/runner_spec.rb
@@ -557,17 +557,36 @@ describe API::Runner do
                { 'key' => 'TRIGGER_KEY_1', 'value' => 'TRIGGER_VALUE_1', 'public' => false }]
             end
 
+            let(:trigger) { create(:ci_trigger, project: project) }
+            let!(:trigger_request) { create(:ci_trigger_request, pipeline: pipeline, builds: [job], trigger: trigger) }
+
             before do
-              trigger = create(:ci_trigger, project: project)
-              create(:ci_trigger_request_with_variables, pipeline: pipeline, builds: [job], trigger: trigger)
               project.variables << Ci::Variable.new(key: 'SECRET_KEY', value: 'secret_value')
             end
 
-            it 'returns variables for triggers' do
-              request_job
+            shared_examples 'expected variables behavior' do
+              it 'returns variables for triggers' do
+                request_job
 
-              expect(response).to have_http_status(201)
-              expect(json_response['variables']).to include(*expected_variables)
+                expect(response).to have_http_status(201)
+                expect(json_response['variables']).to include(*expected_variables)
+              end
+            end
+
+            context 'when variables are stored in trigger_request' do
+              before do
+                trigger_request.update_attribute(:variables, { TRIGGER_KEY_1: 'TRIGGER_VALUE_1' } )
+              end
+
+              it_behaves_like 'expected variables behavior'
+            end
+
+            context 'when variables are stored in pipeline_variables' do
+              before do
+                create(:ci_pipeline_variable, pipeline: pipeline, key: :TRIGGER_KEY_1, value: 'TRIGGER_VALUE_1')
+              end
+
+              it_behaves_like 'expected variables behavior'
             end
           end
 
diff --git a/spec/views/projects/jobs/show.html.haml_spec.rb b/spec/views/projects/jobs/show.html.haml_spec.rb
index 117f48450e2..d4279626e75 100644
--- a/spec/views/projects/jobs/show.html.haml_spec.rb
+++ b/spec/views/projects/jobs/show.html.haml_spec.rb
@@ -195,20 +195,4 @@ describe 'projects/jobs/show' do
         text: /\A\n#{Regexp.escape(commit_title)}\n\Z/)
     end
   end
-
-  describe 'shows trigger variables in sidebar' do
-    let(:trigger_request) { create(:ci_trigger_request_with_variables, pipeline: pipeline) }
-
-    before do
-      build.trigger_request = trigger_request
-      render
-    end
-
-    it 'shows trigger variables in separate lines' do
-      expect(rendered).to have_css('.js-build-variable', visible: false, text: 'TRIGGER_KEY_1')
-      expect(rendered).to have_css('.js-build-variable', visible: false, text: 'TRIGGER_KEY_2')
-      expect(rendered).to have_css('.js-build-value', visible: false, text: 'TRIGGER_VALUE_1')
-      expect(rendered).to have_css('.js-build-value', visible: false, text: 'TRIGGER_VALUE_2')
-    end
-  end
 end