fixfix/sm/34805-get-back-validates-uniqueness

5 files changed, 56 insertions, 21 deletions

diff --git a/app/controllers/projects/pipeline_schedules_controller.rb b/app/controllers/projects/pipeline_schedules_controller.rb
index ec7c645df5a..a74c042039a 100644
--- a/app/controllers/projects/pipeline_schedules_controller.rb
+++ b/app/controllers/projects/pipeline_schedules_controller.rb
@@ -33,7 +33,11 @@ class Projects::PipelineSchedulesController < Projects::ApplicationController
   end
 
   def update
-    if schedule.update(schedule_params)
+    Ci::UpdatePipelineScheduleService
+      .new(@project, current_user, schedule_params)
+      .execute(schedule)
+
+    if schedule.changed?
       redirect_to project_pipeline_schedules_path(@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 cd40deb6187..cbb18c9a579 100644
--- a/app/services/ci/create_pipeline_schedule_service.rb
+++ b/app/services/ci/create_pipeline_schedule_service.rb
@@ -1,13 +1,27 @@
 module Ci
   class CreatePipelineScheduleService < BaseService
     def execute
-      project.pipeline_schedules.create(pipeline_schedule_params)
-    end
+      pipeline_schedule = Ci::PipelineSchedule.new(
+        description: params[:description],
+        ref: params[:ref],
+        cron: params[:cron],
+        cron_timezone: params[:cron_timezone],
+        project: project,
+        owner: current_user
+      )
+
+      Ci::PipelineSchedule.transaction do
+        pipeline_schedule.save!
 
-    private
+        if params[:variables_attributes].is_a?(Array)
+          pipeline_schedule.variables.create!(params[:variables_attributes])
+        end
+      end
 
-    def pipeline_schedule_params
-      params.merge(owner: current_user)
+      rescue Exception => e
+        pipeline_schedule.errors[:base] << "Failed to persist the pipeline schedule: #{e}"
+      ensure
+        return pipeline_schedule
     end
   end
 end
diff --git a/app/services/ci/update_pipeline_schedule_service.rb b/app/services/ci/update_pipeline_schedule_service.rb
new file mode 100644
index 00000000000..f71ea5cbee2
--- /dev/null
+++ b/app/services/ci/update_pipeline_schedule_service.rb
@@ -0,0 +1,18 @@
+module Ci
+  class UpdatePipelineScheduleService < BaseService
+    def execute(pipeline_schedule)
+      Ci::PipelineSchedule.transaction do
+        pipeline_schedule.update!(params.delete(:variables_attributes))
+
+        if params[:variables_attributes].is_a?(Array)
+          pipeline_schedule.variables.update!(params[:variables_attributes])
+        end
+      end
+
+      rescue Exception => e
+        pipeline_schedule.errors[:base] << "Failed to update the pipeline schedule: #{e}"
+      ensure
+        return pipeline_schedule
+    end
+  end
+end
diff --git a/app/validators/variable_duplicates_validator.rb b/app/validators/variable_duplicates_validator.rb
index b9b9596565c..edeac5456f9 100644
--- a/app/validators/variable_duplicates_validator.rb
+++ b/app/validators/variable_duplicates_validator.rb
@@ -1,14 +1,13 @@
-# VariableDuplicatesValidator
-#
-# This validtor is designed for especially the following condition
-# - Use `accepts_nested_attributes_for :xxx` in a parent model
-# - Use `validates :xxx, uniqueness: { scope: :xxx_id }` in a child model
-class VariableDuplicatesValidator < ActiveModel::EachValidator
-  def validate_each(record, attribute, value)
-    puts "#{self.class.name} - #{__callee__}: value: #{value.map{ |v| [v.id, v.key, v.value, v.marked_for_destruction?]}}"
-    duplicates = value.reject(&:marked_for_destruction?).group_by(&:key).select { |_, v| v.many? }.map(&:first)
-    if duplicates.any?
-      record.errors.add(attribute, "Duplicate variables: #{duplicates.join(", ")}")
-    end
-  end
-end
+# # VariableDuplicatesValidator
+# #
+# # This validtor is designed for especially the following condition
+# # - Use `accepts_nested_attributes_for :xxx` in a parent model
+# # - Use `validates :xxx, uniqueness: { scope: :xxx_id }` in a child model
+# class VariableDuplicatesValidator < ActiveModel::EachValidator
+#   def validate_each(record, attribute, value)
+#     duplicates = value.reject(&:marked_for_destruction?).group_by(&:key).select { |_, v| v.many? }.map(&:first)
+#     if duplicates.any?
+#       record.errors.add(attribute, "Duplicate variables: #{duplicates.join(", ")}")
+#     end
+#   end
+# end
diff --git a/spec/controllers/projects/pipeline_schedules_controller_spec.rb b/spec/controllers/projects/pipeline_schedules_controller_spec.rb
index 5a244fd5eb2..f07adb6bc54 100644
--- a/spec/controllers/projects/pipeline_schedules_controller_spec.rb
+++ b/spec/controllers/projects/pipeline_schedules_controller_spec.rb
@@ -108,7 +108,7 @@ describe Projects::PipelineSchedulesController do
             .to change { Ci::PipelineSchedule.count }.by(0)
             .and change { Ci::PipelineScheduleVariable.count }.by(0)
 
-          expect(assigns(:schedule).errors['variables']).not_to be_empty
+          expect(assigns(:schedule).errors).not_to be_empty
         end
       end
     end