Fix StaticAnlysysfeature/sm/32568-adding-variables-to-pipelines-schedules

3 files changed, 79 insertions, 137 deletions

diff --git a/db/migrate/20170620064728_create_ci_pipeline_schedule_variables.rb b/db/migrate/20170620064728_create_ci_pipeline_schedule_variables.rb
index 7d2d313a91a..92833765a82 100644
--- a/db/migrate/20170620064728_create_ci_pipeline_schedule_variables.rb
+++ b/db/migrate/20170620064728_create_ci_pipeline_schedule_variables.rb
@@ -10,7 +10,7 @@ class CreateCiPipelineScheduleVariables < ActiveRecord::Migration
       t.string :encrypted_value_iv
       t.integer :pipeline_schedule_id, null: false
 
-      t.timestamps null: false
+      t.timestamps_with_timezone null: true
     end
 
     add_index :ci_pipeline_schedule_variables,
diff --git a/spec/controllers/projects/pipeline_schedules_controller_spec.rb b/spec/controllers/projects/pipeline_schedules_controller_spec.rb
index 83ce631abd5..110c05ce497 100644
--- a/spec/controllers/projects/pipeline_schedules_controller_spec.rb
+++ b/spec/controllers/projects/pipeline_schedules_controller_spec.rb
@@ -71,7 +71,7 @@ describe Projects::PipelineSchedulesController do
         let(:schedule) { basic_param }
 
         it 'creates a new schedule' do
-          expect { post :create, namespace_id: project.namespace.to_param, project_id: project, schedule: schedule }
+          expect { go }
             .to change { Ci::PipelineSchedule.count }.by(1)
             .and change { Ci::PipelineScheduleVariable.count }.by(0)
 
@@ -82,12 +82,12 @@ describe Projects::PipelineSchedulesController do
       context 'when variables_attributes has one variable' do
         let(:schedule) do
           basic_param.merge({
-            variables_attributes: [ { key: 'AAA', value: 'AAA123' } ]
+            variables_attributes: [{ key: 'AAA', value: 'AAA123' }]
           })
         end
 
         it 'creates a new schedule' do
-          expect { post :create, namespace_id: project.namespace.to_param, project_id: project, schedule: schedule }
+          expect { go }
             .to change { Ci::PipelineSchedule.count }.by(1)
             .and change { Ci::PipelineScheduleVariable.count }.by(1)
 
@@ -98,7 +98,7 @@ describe Projects::PipelineSchedulesController do
 
         context 'when the same key has already been persisted' do
           it 'returns an error that the key of variable is invaild' do
-            post :create, namespace_id: project.namespace.to_param, project_id: project, schedule: schedule
+            go
 
             pipeline_schedule_variable = build(:ci_pipeline_schedule_variable, key: 'AAA', pipeline_schedule: assigns(:schedule))
             expect(pipeline_schedule_variable).to be_invalid
@@ -109,12 +109,12 @@ describe Projects::PipelineSchedulesController do
       context 'when variables_attributes has one variable and key is empty' do
         let(:schedule) do
           basic_param.merge({
-            variables_attributes: [ { key: '', value: 'AAA123' } ]
+            variables_attributes: [{ key: '', value: 'AAA123' }]
           })
         end
 
         it 'returns an error that the key of variable is invaild' do
-          expect { post :create, namespace_id: project.namespace.to_param, project_id: project, schedule: schedule }
+          expect { go }
             .to change { Ci::PipelineSchedule.count }.by(0)
             .and change { Ci::PipelineScheduleVariable.count }.by(0)
 
@@ -125,12 +125,12 @@ describe Projects::PipelineSchedulesController do
       context 'when variables_attributes has two variables and unique' do
         let(:schedule) do
           basic_param.merge({
-            variables_attributes: [ { key: 'AAA', value: 'AAA123' }, { key: 'BBB', value: 'BBB123' } ]
+            variables_attributes: [{ key: 'AAA', value: 'AAA123' }, { key: 'BBB', value: 'BBB123' }]
           })
         end
 
         it 'creates a new schedule' do
-          expect { post :create, namespace_id: project.namespace.to_param, project_id: project, schedule: schedule }
+          expect { go }
             .to change { Ci::PipelineSchedule.count }.by(1)
             .and change { Ci::PipelineScheduleVariable.count }.by(2)
 
@@ -146,39 +146,42 @@ describe Projects::PipelineSchedulesController do
       # context 'when variables_attributes has two variables and duplicted' do
       #   let(:schedule) do
       #     basic_param.merge({
-      #       variables_attributes: [ { key: 'AAA', value: 'AAA123' }, { key: 'AAA', value: 'BBB123' } ]
+      #       variables_attributes: [{ key: 'AAA', value: 'AAA123' }, { key: 'AAA', value: 'BBB123' }]
       #     })
       #   end
 
       #   it 'returns an error that the keys of variable are duplicated' do
-      #     expect { post :create, namespace_id: project.namespace.to_param, project_id: project, schedule: schedule }
+      #     expect { go }
       #       .to change { Ci::PipelineSchedule.count }.by(0)
       #       .and change { Ci::PipelineScheduleVariable.count }.by(0)
 
       #     expect(assigns(:schedule).errors['variables.key']).not_to be_empty
       #   end
       # end
+
+      def go
+        post :create, namespace_id: project.namespace.to_param, project_id: project, schedule: schedule
+      end
     end
 
     describe 'security' do
-      let(:action) do
-        proc do |user|
-          post :create, namespace_id: project.namespace.to_param, 
-            project_id: project,
-            schedule: { description: 'aaaaaaaa', cron: '0 4 * * *',
-              cron_timezone: 'UTC', ref: 'master', active: '1' }
-        end
+      it { expect { go }.to be_allowed_for(:admin) }
+      it { expect { go }.to be_allowed_for(:owner).of(project) }
+      it { expect { go }.to be_allowed_for(:master).of(project) }
+      it { expect { go }.to be_allowed_for(:developer).of(project) }
+      it { expect { go }.to be_denied_for(:reporter).of(project) }
+      it { expect { go }.to be_denied_for(:guest).of(project) }
+      it { expect { go }.to be_denied_for(:user) }
+      it { expect { go }.to be_denied_for(:external) }
+      it { expect { go }.to be_denied_for(:visitor) }
+
+      def go
+        post :create, namespace_id: project.namespace.to_param,
+                      project_id: project,
+                      schedule: { description: 'aaaaaaaa', cron: '0 4 * * *',
+                                  cron_timezone: 'UTC', ref: 'master',
+                                  active: '1' }
       end
-
-      specify { expect(action).to be_allowed_for(:admin) }
-      specify { expect(action).to be_allowed_for(:owner).of(project) }
-      specify { expect(action).to be_allowed_for(:master).of(project) }
-      specify { expect(action).to be_allowed_for(:developer).of(project) }
-      specify { expect(action).to be_denied_for(:reporter).of(project) }
-      specify { expect(action).to be_denied_for(:guest).of(project) }
-      specify { expect(action).to be_denied_for(:user) }
-      specify { expect(action).to be_denied_for(:external) }
-      specify { expect(action).to be_denied_for(:visitor) }
     end
   end
 
@@ -202,8 +205,7 @@ describe Projects::PipelineSchedulesController do
           let(:schedule) { basic_param }
 
           it 'updates only scheduled pipeline attributes' do
-            put :update, namespace_id: project.namespace.to_param,
-              project_id: project, id: pipeline_schedule, schedule: schedule
+            go
 
             pipeline_schedule.reload
 
@@ -220,15 +222,12 @@ describe Projects::PipelineSchedulesController do
         context 'when params include one variable' do
           let(:schedule) do
             basic_param.merge({
-              variables_attributes: [ { key: 'AAA', value: 'AAA123' } ]
+              variables_attributes: [{ key: 'AAA', value: 'AAA123' }]
             })
           end
 
           it 'inserts new variable to the pipeline schedule' do
-            expect do
-              put :update, namespace_id: project.namespace.to_param,
-                project_id: project, id: pipeline_schedule, schedule: schedule
-            end.to change { Ci::PipelineScheduleVariable.count }.by(1)
+            expect { go }.to change { Ci::PipelineScheduleVariable.count }.by(1)
 
             pipeline_schedule.reload
 
@@ -241,15 +240,12 @@ describe Projects::PipelineSchedulesController do
         context 'when params include two unique variables' do
           let(:schedule) do
             basic_param.merge({
-              variables_attributes: [ { key: 'AAA', value: 'AAA123' }, { key: 'BBB', value: 'BBB123' } ]
+              variables_attributes: [{ key: 'AAA', value: 'AAA123' }, { key: 'BBB', value: 'BBB123' }]
             })
           end
 
           it 'inserts two new variables to the pipeline schedule' do
-            expect do
-              put :update, namespace_id: project.namespace.to_param,
-                project_id: project, id: pipeline_schedule, schedule: schedule
-            end.to change { Ci::PipelineScheduleVariable.count }.by(2)
+            expect { go }.to change { Ci::PipelineScheduleVariable.count }.by(2)
 
             pipeline_schedule.reload
 
@@ -265,7 +261,7 @@ describe Projects::PipelineSchedulesController do
         # context 'when params include two duplicated variables' do
         #   let(:schedule) do
         #     basic_param.merge({
-        #       variables_attributes: [ { key: 'AAA', value: 'AAA123' }, { key: 'AAA', value: 'BBB123' } ]
+        #       variables_attributes: [{ key: 'AAA', value: 'AAA123' }, { key: 'AAA', value: 'BBB123' }]
         #     })
         #   end
 
@@ -292,8 +288,7 @@ describe Projects::PipelineSchedulesController do
           let(:schedule) { basic_param }
 
           it 'updates only scheduled pipeline attributes' do
-            put :update, namespace_id: project.namespace.to_param,
-              project_id: project, id: pipeline_schedule, schedule: schedule
+            go
 
             pipeline_schedule.reload
 
@@ -317,10 +312,7 @@ describe Projects::PipelineSchedulesController do
             end
 
             it 'adds the new variable' do
-              expect do
-                put :update, namespace_id: project.namespace.to_param,
-                  project_id: project, id: pipeline_schedule, schedule: schedule
-              end.to change { Ci::PipelineScheduleVariable.count }.by(1)
+              expect { go }.to change { Ci::PipelineScheduleVariable.count }.by(1)
 
               expect(pipeline_schedule.variables.last.key).to eq('AAA')
             end
@@ -334,10 +326,7 @@ describe Projects::PipelineSchedulesController do
             end
 
             it 'updates the variable' do
-              expect do
-                put :update, namespace_id: project.namespace.to_param,
-                  project_id: project, id: pipeline_schedule, schedule: schedule
-              end.not_to change { Ci::PipelineScheduleVariable.count }
+              expect { go }.not_to change { Ci::PipelineScheduleVariable.count }
 
               pipeline_schedule_variable.reload
 
@@ -353,14 +342,17 @@ describe Projects::PipelineSchedulesController do
             end
 
             it 'delete the existsed variable' do
-              expect do
-                put :update, namespace_id: project.namespace.to_param,
-                  project_id: project, id: pipeline_schedule, schedule: schedule
-              end.to change { Ci::PipelineScheduleVariable.count }.by(-1)
+              expect { go }.to change { Ci::PipelineScheduleVariable.count }.by(-1)
             end
           end
         end
       end
+
+      def go
+        put :update, namespace_id: project.namespace.to_param,
+                     project_id: project, id: pipeline_schedule,
+                     schedule: schedule
+      end
     end
 
     describe 'security' do
@@ -373,39 +365,15 @@ describe Projects::PipelineSchedulesController do
         end
 
         context 'when the developer updates' do
-          let(:action) do
-            proc do |user|
-              put :update, namespace_id: project.namespace.to_param,
-                project_id: project, id: pipeline_schedule,
-                schedule: { description: 'updated_desc' }
-            end
-          end
-
-          specify { expect(action).to be_allowed_for(developer_1) }
+          it { expect { go }.to be_allowed_for(developer_1) }
         end
 
         context 'when another developer updates' do
-          let(:action) do
-            proc do |user|
-              put :update, namespace_id: project.namespace.to_param,
-                project_id: project, id: pipeline_schedule,
-                schedule: { description: 'updated_desc' }
-            end
-          end
-
-          specify { expect(action).to be_denied_for(:developer).of(project) }
+          it { expect { go }.to be_denied_for(:developer).of(project) }
         end
 
         context 'when a master updates' do
-          let(:action) do
-            proc do |user|
-              put :update, namespace_id: project.namespace.to_param,
-                project_id: project, id: pipeline_schedule,
-                schedule: { description: 'updated_desc' }
-            end
-          end
-
-          specify { expect(action).to be_allowed_for(:master).of(project) }
+          it { expect { go }.to be_allowed_for(:master).of(project) }
         end
       end
 
@@ -418,42 +386,24 @@ describe Projects::PipelineSchedulesController do
         end
 
         context 'when the master updates' do
-          let(:action) do
-            proc do |user|
-              put :update, namespace_id: project.namespace.to_param,
-                project_id: project, id: pipeline_schedule,
-                schedule: { description: 'updated_desc' }
-            end
-          end
-
-          specify { expect(action).to be_allowed_for(master_1) }
+          it { expect { go }.to be_allowed_for(master_1) }
         end
 
         context 'when other masters updates' do
-          let(:action) do
-            proc do |user|
-              put :update, namespace_id: project.namespace.to_param,
-                project_id: project, id: pipeline_schedule,
-                schedule: { description: 'updated_desc' }
-            end
-          end
-
-          specify { expect(action).to be_allowed_for(:master).of(project) }
+          it { expect { go }.to be_allowed_for(:master).of(project) }
         end
 
         context 'when a developer updates' do
-          let(:action) do
-            proc do |user|
-              put :update, namespace_id: project.namespace.to_param,
-                project_id: project, id: pipeline_schedule,
-                schedule: { description: 'updated_desc' }
-            end
-          end
-
-          specify { expect(action).to be_denied_for(:developer).of(project) }
+          it { expect { go }.to be_denied_for(:developer).of(project) }
         end
       end
     end
+
+    def go
+      put :update, namespace_id: project.namespace.to_param,
+                   project_id: project, id: pipeline_schedule,
+                   schedule: { description: 'updated_desc' }
+    end
   end
 
   describe 'GET edit' do
diff --git a/spec/support/matchers/access_matchers_for_controller.rb b/spec/support/matchers/access_matchers_for_controller.rb
index 68734b4977d..fb43f51c70c 100644
--- a/spec/support/matchers/access_matchers_for_controller.rb
+++ b/spec/support/matchers/access_matchers_for_controller.rb
@@ -1,4 +1,3 @@
-
 # AccessMatchersForController
 #
 # For testing authorize_xxx in controller. 
@@ -6,8 +5,8 @@ module AccessMatchersForController
   extend RSpec::Matchers::DSL
   include Warden::Test::Helpers
 
-  EXPECTED_STATUS_CODE_ALLOWED = [200, 302].freeze
-  EXPECTED_STATUS_CODE_DENIED = [404].freeze
+  EXPECTED_STATUS_CODE_ALLOWED = [200, 201, 302].freeze
+  EXPECTED_STATUS_CODE_DENIED = [401, 404].freeze
 
   def emulate_user(role, membership = nil)
     case role
@@ -20,21 +19,15 @@ module AccessMatchersForController
     when :external
       user = create(:user, external: true)
       sign_in(user)
-    when :visitor # rubocop:disable Lint/EmptyWhen
-      # no-op
+    when :visitor
+      user = nil
     when User
       user = role
       sign_in(user)
     when *Gitlab::Access.sym_options_with_owner.keys # owner, master, developer, reporter, guest
       raise ArgumentError, "cannot emulate #{role} without membership parent" unless membership
 
-      if role == :owner && membership.owner
-        user = membership.owner
-      else
-        user = create(:user)
-        membership.public_send(:"add_#{role}", user)
-      end
-
+      user = create_user_by_membership(role, membership)
       sign_in(user)
     else
       raise ArgumentError, "cannot emulate user #{role}"
@@ -43,20 +36,24 @@ module AccessMatchersForController
     user
   end
 
+  def create_user_by_membership(role, membership)
+    if role == :owner && membership.owner
+      user = membership.owner
+    else
+      user = create(:user)
+      membership.public_send(:"add_#{role}", user)
+    end
+    user
+  end
+
   def description_for(role, type, expected, result)
-    "be #{type} for #{role}." \
-    " Expected: #{expected.join(',')} Got: #{result}"
+    "be #{type} for #{role}. Expected: #{expected.join(',')} Got: #{result}"
   end
 
   matcher :be_allowed_for do |role|
     match do |action|
-      user = emulate_user(role, @membership)
-      # begin
-        action.call(user)
-      # rescue
-      #   # Ignore internal exceptions which will be caused in the controller
-      #   # In such cases, response.status will be 200.
-      # end
+      emulate_user(role, @membership)
+      action.call
 
       EXPECTED_STATUS_CODE_ALLOWED.include?(response.status)
     end
@@ -71,13 +68,8 @@ module AccessMatchersForController
 
   matcher :be_denied_for do |role|
     match do |action|
-      user = emulate_user(role, @membership)
-      # begin
-        action.call(user)
-      # rescue
-      #   # Ignore internal exceptions which will be caused in the controller
-      #   # In such cases, response.status will be 200.
-      # end
+      emulate_user(role, @membership)
+      action.call
 
       EXPECTED_STATUS_CODE_DENIED.include?(response.status)
     end