diff --git a/app/controllers/projects/pipeline_schedules_controller.rb b/app/controllers/projects/pipeline_schedules_controller.rb index f0ac0e7098c..fdbf930a5ef 100644 --- a/app/controllers/projects/pipeline_schedules_controller.rb +++ b/app/controllers/projects/pipeline_schedules_controller.rb @@ -1,11 +1,11 @@ class Projects::PipelineSchedulesController < Projects::ApplicationController - before_action :schedule, except: [:index, :new, :create] - before_action :authorize_read_pipeline_schedule! before_action :authorize_create_pipeline_schedule!, only: [:new, :create] - before_action :authorize_update_pipeline_schedule!, except: [:index, :new, :create] + before_action :authorize_update_pipeline_schedule!, only: [:edit, :take_ownership, :update] before_action :authorize_admin_pipeline_schedule!, only: [:destroy] + before_action :schedule, only: [:edit, :update, :destroy, :take_ownership] + def index @scope = params[:scope] @all_schedules = PipelineSchedulesFinder.new(@project).execute @@ -67,10 +67,6 @@ class Projects::PipelineSchedulesController < Projects::ApplicationController def schedule_params params.require(:schedule) .permit(:description, :cron, :cron_timezone, :ref, :active, - variables_attributes: [:id, :key, :value, :_destroy] ) - end - - def authorize_update_pipeline_schedule! - return access_denied! unless can?(current_user, :update_pipeline_schedule, schedule) + variables_attributes: [:key, :value] ) end end diff --git a/app/models/ci/pipeline_schedule.rb b/app/models/ci/pipeline_schedule.rb index ad9f8b89924..61215b1fb46 100644 --- a/app/models/ci/pipeline_schedule.rb +++ b/app/models/ci/pipeline_schedule.rb @@ -9,17 +9,13 @@ module Ci belongs_to :owner, class_name: 'User' has_one :last_pipeline, -> { order(id: :desc) }, class_name: 'Ci::Pipeline' has_many :pipelines - has_many :variables, class_name: 'Ci::PipelineScheduleVariable' + has_many :variables, :dependent => :destroy, class_name: 'Ci::PipelineScheduleVariable' validates :cron, unless: :importing?, cron: true, presence: { unless: :importing? } validates :cron_timezone, cron_timezone: true, presence: { unless: :importing? } validates :ref, presence: { unless: :importing? } validates :description, presence: true - validates :variables, uniqueness_of_in_memory: { - :collection => :variables, - :attrs => [:pipeline_schedule_id, :key], - :message => ['variables.key', 'keys are duplicated'] - } + validates_associated :variables before_save :set_next_run_at @@ -28,6 +24,15 @@ 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 @@ -64,9 +69,5 @@ module Ci Gitlab::Ci::CronParser.new(worker_cron, worker_time_zone) .next_time_from(next_run_at) end - - def job_variables - variables&.map(&:to_runner_variable) || [] - end end end diff --git a/app/models/ci/pipeline_schedule_variable.rb b/app/models/ci/pipeline_schedule_variable.rb index 2d681446d00..ee5b8733fac 100644 --- a/app/models/ci/pipeline_schedule_variable.rb +++ b/app/models/ci/pipeline_schedule_variable.rb @@ -4,6 +4,7 @@ module Ci include HasVariable belongs_to :pipeline_schedule + validates :key, uniqueness: { scope: :pipeline_schedule_id } end end diff --git a/app/views/projects/pipeline_schedules/_form.html.haml b/app/views/projects/pipeline_schedules/_form.html.haml index 0e600c59ef6..3857a59a043 100644 --- a/app/views/projects/pipeline_schedules/_form.html.haml +++ b/app/views/projects/pipeline_schedules/_form.html.haml @@ -22,6 +22,38 @@ = f.label :ref, _('Target Branch'), class: 'label-light' = dropdown_tag(_("Select target branch"), options: { toggle_class: 'btn js-target-branch-dropdown', dropdown_class: 'git-revision-dropdown', title: _("Select target branch"), filter: true, placeholder: s_("OfSearchInADropdown|Filter"), data: { data: @project.repository.branch_names, default_branch: @project.default_branch } } ) = f.text_field :ref, value: @schedule.ref, id: 'schedule_ref', class: 'hidden', name: 'schedule[ref]', required: true + -# TODO: Test code + = @schedule.variables.inspect + - if @schedule.variables.present? + - @schedule.variables.each_with_index do |variable, i| + .form-group + .col-md-9 + %label.label-light Key + %input.form-control{:name => "schedule[variables_attributes][#{i}][key]", :type => "text", :value => variable.key}/ + %p.gl-field-error.hide This field is required. + %label.label-light Value + %input.form-control{:name => "schedule[variables_attributes][#{i}][value]", :type => "text", :value => variable.value}/ + %p.gl-field-error.hide This field is required. + - if @schedule.variables.count == 1 + - (1..1).each do |i| + .form-group + .col-md-9 + %label.label-light Key + %input.form-control{:name => "schedule[variables_attributes][#{i}][key]", :type => "text"}/ + %p.gl-field-error.hide This field is required. + %label.label-light Value + %input.form-control{:name => "schedule[variables_attributes][#{i}][value]", :type => "text"}/ + %p.gl-field-error.hide This field is required. + - else + - (0..0).each do |i| + .form-group + .col-md-9 + %label.label-light Key + %input.form-control{:name => "schedule[variables_attributes][#{i}][key]", :type => "text"}/ + %p.gl-field-error.hide This field is required. + %label.label-light Value + %input.form-control{:name => "schedule[variables_attributes][#{i}][value]", :type => "text"}/ + %p.gl-field-error.hide This field is required. .form-group .col-md-9 %label.label-light