Implement variables_attributes create/update cases

3 files changed, 31 insertions, 14 deletions

diff --git a/app/controllers/projects/pipeline_schedules_controller.rb b/app/controllers/projects/pipeline_schedules_controller.rb
index fdbf930a5ef..2ee6229cf68 100644
--- a/app/controllers/projects/pipeline_schedules_controller.rb
+++ b/app/controllers/projects/pipeline_schedules_controller.rb
@@ -33,7 +33,8 @@ class Projects::PipelineSchedulesController < Projects::ApplicationController
   end
 
   def update
-    if schedule.update(schedule_params)
+    if Ci::CreatePipelineScheduleService
+        .new(@project, current_user, schedule_params).update(schedule)
       redirect_to namespace_project_pipeline_schedules_path(@project.namespace.becomes(Namespace), @project)
     else
       render :edit
@@ -67,6 +68,6 @@ class Projects::PipelineSchedulesController < Projects::ApplicationController
   def schedule_params
     params.require(:schedule)
       .permit(:description, :cron, :cron_timezone, :ref, :active,
-        variables_attributes: [:key, :value] )
+        variables_attributes: [:id, :key, :value, :_destroy] )
   end
 end
diff --git a/app/models/ci/pipeline_schedule.rb b/app/models/ci/pipeline_schedule.rb
index 61215b1fb46..06b2b981252 100644
--- a/app/models/ci/pipeline_schedule.rb
+++ b/app/models/ci/pipeline_schedule.rb
@@ -15,7 +15,6 @@ module Ci
     validates :cron_timezone, cron_timezone: true, presence: { unless: :importing? }
     validates :ref, presence: { unless: :importing? }
     validates :description, presence: true
-    validates_associated :variables
 
     before_save :set_next_run_at
 
@@ -24,15 +23,6 @@ module Ci
 
     accepts_nested_attributes_for :variables, allow_destroy: true
 
-    before_validation(on: :update) do
-      # TODO: if validation failed, restore the deleted_obj
-      deleted_obj = Ci::PipelineScheduleVariable.where(pipeline_schedule_id: self).destroy_all
-    end
-
-    after_validation(on: :update) do
-      # TODO: if validation failed, restore the deleted_obj
-    end
-
     def owned_by?(current_user)
       owner == current_user
     end
diff --git a/app/services/ci/create_pipeline_schedule_service.rb b/app/services/ci/create_pipeline_schedule_service.rb
index cd40deb6187..4c9cb5c66c6 100644
--- a/app/services/ci/create_pipeline_schedule_service.rb
+++ b/app/services/ci/create_pipeline_schedule_service.rb
@@ -1,13 +1,39 @@
 module Ci
   class CreatePipelineScheduleService < BaseService
     def execute
-      project.pipeline_schedules.create(pipeline_schedule_params)
+      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)
     end
 
     private
 
     def pipeline_schedule_params
-      params.merge(owner: current_user)
+      @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
     end
   end
 end