Revert extra validation for duplication between same keys on a submit

3 files changed, 34 insertions, 59 deletions

diff --git a/app/controllers/projects/pipeline_schedules_controller.rb b/app/controllers/projects/pipeline_schedules_controller.rb
index 86c9628d917..9bcdfc2b510 100644
--- a/app/controllers/projects/pipeline_schedules_controller.rb
+++ b/app/controllers/projects/pipeline_schedules_controller.rb
@@ -33,8 +33,7 @@ class Projects::PipelineSchedulesController < Projects::ApplicationController
   end
 
   def update
-    if Ci::CreatePipelineScheduleService
-        .new(@project, current_user, schedule_params).update(schedule)
+    if schedule.update(schedule_params)
       redirect_to namespace_project_pipeline_schedules_path(@project.namespace.becomes(Namespace), @project)
     else
       render :edit
diff --git a/app/services/ci/create_pipeline_schedule_service.rb b/app/services/ci/create_pipeline_schedule_service.rb
index 4c9cb5c66c6..cd40deb6187 100644
--- a/app/services/ci/create_pipeline_schedule_service.rb
+++ b/app/services/ci/create_pipeline_schedule_service.rb
@@ -1,39 +1,13 @@
 module Ci
   class CreatePipelineScheduleService < BaseService
     def execute
-      pipeline_schedule = project.pipeline_schedules.build(pipeline_schedule_params)
-
-      if variable_keys_duplicated?
-        pipeline_schedule.errors.add('variables.key', "keys are duplicated")
-
-        return pipeline_schedule
-      end
-
-      pipeline_schedule.save
-      pipeline_schedule
-    end
-
-    def update(pipeline_schedule)
-      if variable_keys_duplicated?
-        pipeline_schedule.errors.add('variables.key', "keys are duplicated")
-
-        return false
-      end
-
-      pipeline_schedule.update(pipeline_schedule_params)
+      project.pipeline_schedules.create(pipeline_schedule_params)
     end
 
     private
 
     def pipeline_schedule_params
-      @pipeline_schedule_params ||= params.merge(owner: current_user)
-    end
-
-    def variable_keys_duplicated?
-      attributes = pipeline_schedule_params['variables_attributes']
-      return false unless attributes.is_a?(Array)
-
-      attributes.map { |v| v['key'] }.uniq.length != attributes.length
+      params.merge(owner: current_user)
     end
   end
 end
diff --git a/spec/controllers/projects/pipeline_schedules_controller_spec.rb b/spec/controllers/projects/pipeline_schedules_controller_spec.rb
index 5f6b3c0a187..1567bde2a62 100644
--- a/spec/controllers/projects/pipeline_schedules_controller_spec.rb
+++ b/spec/controllers/projects/pipeline_schedules_controller_spec.rb
@@ -142,21 +142,22 @@ describe Projects::PipelineSchedulesController do
         end
       end
 
-      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' } ]
-          })
-        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 }
-            .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
+      # This test no longer passes, since we removed a custom validation
+      # 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' } ]
+      #     })
+      #   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 }
+      #       .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
     end
 
     describe 'security' do
@@ -260,20 +261,21 @@ describe Projects::PipelineSchedulesController do
           end
         end
 
-        context 'when params include two duplicated variables' do
-          let(:schedule) do
-            basic_param.merge({
-              variables_attributes: [ { key: 'AAA', value: 'AAA123' }, { key: 'AAA', value: 'BBB123' } ]
-            })
-          end
-
-          it 'returns an error that variables are duplciated' do
-            put :update, namespace_id: project.namespace.to_param,
-              project_id: project, id: pipeline_schedule, schedule: schedule
-
-            expect(assigns(:schedule).errors['variables.key']).not_to be_empty
-          end
-        end
+        # This test no longer passes, since we removed a custom validation
+        # context 'when params include two duplicated variables' do
+        #   let(:schedule) do
+        #     basic_param.merge({
+        #       variables_attributes: [ { key: 'AAA', value: 'AAA123' }, { key: 'AAA', value: 'BBB123' } ]
+        #     })
+        #   end
+
+        #   it 'returns an error that variables are duplciated' do
+        #     put :update, namespace_id: project.namespace.to_param,
+        #       project_id: project, id: pipeline_schedule, schedule: schedule
+
+        #     expect(assigns(:schedule).errors['variables.key']).not_to be_empty
+        #   end
+        # end
       end
 
       context 'when a pipeline schedule has one variable' do