From 86967edf80870f9ca94134b78721785b2c987740 Mon Sep 17 00:00:00 2001 From: Adrianna Chang Date: Thu, 20 May 2021 12:42:07 -0400 Subject: [PATCH] Fixups --- .../maintenance_tasks/tasks_controller.rb | 8 ++--- app/models/maintenance_tasks/run.rb | 34 +++++++++++-------- app/models/maintenance_tasks/runner.rb | 8 ++--- .../maintenance_tasks/tasks/show.html.erb | 8 ++--- ...add_arguments_to_maintenance_tasks_runs.rb | 6 ++++ ...dd_parameters_to_maintenance_tasks_runs.rb | 6 ---- test/application_system_test_case.rb | 2 ++ .../app/tasks/maintenance/params_task.rb | 12 ++++--- test/dummy/db/schema.rb | 6 ++-- test/jobs/maintenance_tasks/task_job_test.rb | 4 +-- test/models/maintenance_tasks/run_test.rb | 9 +++-- test/system/maintenance_tasks/runs_test.rb | 10 +++--- 12 files changed, 63 insertions(+), 50 deletions(-) create mode 100644 db/migrate/20210517131953_add_arguments_to_maintenance_tasks_runs.rb delete mode 100644 db/migrate/20210517131953_add_parameters_to_maintenance_tasks_runs.rb diff --git a/app/controllers/maintenance_tasks/tasks_controller.rb b/app/controllers/maintenance_tasks/tasks_controller.rb index 2ebc02b77..ab148a131 100644 --- a/app/controllers/maintenance_tasks/tasks_controller.rb +++ b/app/controllers/maintenance_tasks/tasks_controller.rb @@ -28,7 +28,7 @@ def run task = Runner.run( name: params.fetch(:id), csv_file: params[:csv_file], - params: task_params, + arguments: task_arguments, ) redirect_to(task_path(task)) rescue ActiveRecord::RecordInvalid => error @@ -39,10 +39,10 @@ def run private - def task_params - return {} unless params[:task_params].present? + def task_arguments + return {} unless params[:task_arguments].present? task_attributes = Task.named(params[:id]).attribute_names - params.require(:task_params).permit(*task_attributes).to_h + params.require(:task_arguments).permit(*task_attributes).to_h end def set_refresh diff --git a/app/models/maintenance_tasks/run.rb b/app/models/maintenance_tasks/run.rb index 0e1e5b360..a233281aa 100644 --- a/app/models/maintenance_tasks/run.rb +++ b/app/models/maintenance_tasks/run.rb @@ -34,12 +34,12 @@ class Run < ApplicationRecord Task.available_tasks.map(&:to_s) } } validate :csv_attachment_presence, on: :create - validate :valid_task_parameters?, on: :create + validate :valid_task_arguments?, on: :create attr_readonly :task_name serialize :backtrace - serialize :parameters, Hash + serialize :arguments, Hash scope :active, -> { where(status: ACTIVE_STATUSES) } @@ -208,14 +208,17 @@ def csv_attachment_presence nil end - # Performs validation on the parameters to use for the Task. Attributes are - # assigned to a Task instance based on the parameters passed to the Run's + # Performs validation on the arguments to use for the Task. Attributes are + # assigned to a Task instance based on the arguments passed to the Run's # constructor. If the Task is invalid, the errors are added to the Run. - def valid_task_parameters? + def valid_task_arguments? + set_task_attributes if task.invalid? - error_messages = task.errors.full_messages + error_messages = task.errors.messages.map do |attribute, message| + "#{attribute.inspect} #{message}" + end errors.add( - :parameters, + :arguments, "are invalid: #{error_messages.join("; ")}" ) end @@ -234,17 +237,18 @@ def csv_file super end - # Returns a Task instance for this Run. Assigns any attributes to the Task - # based on the Run's parameters. + # Returns a Task instance for this Run. # # @return [Task] a Task instance. def task - @task ||= begin - task = Task.named(task_name).new - if task.attribute_names.any? && parameters.present? - task.assign_attributes(parameters) - end - task + @task ||= Task.named(task_name).new + end + + private + + def set_task_attributes + if task.attribute_names.any? && arguments.present? + task.assign_attributes(arguments) end end end diff --git a/app/models/maintenance_tasks/runner.rb b/app/models/maintenance_tasks/runner.rb index 94e2444e2..d3040706c 100644 --- a/app/models/maintenance_tasks/runner.rb +++ b/app/models/maintenance_tasks/runner.rb @@ -37,17 +37,17 @@ def initialize(run) # for the Task to iterate over when running, in the form of an attachable # (see https://edgeapi.rubyonrails.org/classes/ActiveStorage/Attached/One.html#method-i-attach). # Value is nil if the Task does not use CSV iteration. - # @param params [Hash] the parameters to make accessible to the Task and to - # persist to the Run. + # @param arguments [Hash] the arguments to persist to the Run and to make + # accessible to the Task. # # @return [Task] the Task that was run. # # @raise [EnqueuingError] if an error occurs while enqueuing the Run. # @raise [ActiveRecord::RecordInvalid] if validation errors occur while # creating the Run. - def run(name:, csv_file: nil, params: {}) + def run(name:, csv_file: nil, arguments: {}) run = Run.active.find_by(task_name: name) || - Run.new(task_name: name, parameters: params) + Run.new(task_name: name, arguments: arguments) run.csv_file.attach(csv_file) if csv_file run.enqueued! diff --git a/app/views/maintenance_tasks/tasks/show.html.erb b/app/views/maintenance_tasks/tasks/show.html.erb index d37e24960..8f99dd7d6 100644 --- a/app/views/maintenance_tasks/tasks/show.html.erb +++ b/app/views/maintenance_tasks/tasks/show.html.erb @@ -18,10 +18,10 @@ <% end %> <% if @task.parameter_names.any? %>
- <%= form.fields_for :task_params do |ff| %> - <% @task.parameter_names.each do |param| %> - <%= ff.label param, "#{param}: ", class: "label" %> - <%= ff.text_area param %> + <%= form.fields_for :task_arguments do |ff| %> + <% @task.parameter_names.each do |parameter| %> + <%= ff.label parameter, "#{parameter}: ", class: "label" %> + <%= ff.text_area parameter %> <% end %> <% end %>
diff --git a/db/migrate/20210517131953_add_arguments_to_maintenance_tasks_runs.rb b/db/migrate/20210517131953_add_arguments_to_maintenance_tasks_runs.rb new file mode 100644 index 000000000..fa5e89515 --- /dev/null +++ b/db/migrate/20210517131953_add_arguments_to_maintenance_tasks_runs.rb @@ -0,0 +1,6 @@ +# frozen_string_literal: true +class AddArgumentsToMaintenanceTasksRuns < ActiveRecord::Migration[6.0] + def change + add_column(:maintenance_tasks_runs, :arguments, :text) + end +end diff --git a/db/migrate/20210517131953_add_parameters_to_maintenance_tasks_runs.rb b/db/migrate/20210517131953_add_parameters_to_maintenance_tasks_runs.rb deleted file mode 100644 index 2b3ab962f..000000000 --- a/db/migrate/20210517131953_add_parameters_to_maintenance_tasks_runs.rb +++ /dev/null @@ -1,6 +0,0 @@ -# frozen_string_literal: true -class AddParametersToMaintenanceTasksRuns < ActiveRecord::Migration[6.0] - def change - add_column(:maintenance_tasks_runs, :parameters, :text) - end -end diff --git a/test/application_system_test_case.rb b/test/application_system_test_case.rb index 4240335f9..433c486ca 100644 --- a/test/application_system_test_case.rb +++ b/test/application_system_test_case.rb @@ -20,11 +20,13 @@ class ApplicationSystemTestCase < ActionDispatch::SystemTestCase setup do travel_to Time.zone.local(2020, 1, 9, 9, 41, 44) Maintenance::UpdatePostsTask.fast_task = false + Maintenance::ParamsTask.fast_task = false end teardown do assert_empty page.driver.browser.manage.logs.get(:browser) Maintenance::UpdatePostsTask.fast_task = true + Maintenance::ParamsTask.fast_task = true FileUtils.rm_rf("test/dummy/tmp/downloads") end end diff --git a/test/dummy/app/tasks/maintenance/params_task.rb b/test/dummy/app/tasks/maintenance/params_task.rb index 06a2bd45f..828a52df0 100644 --- a/test/dummy/app/tasks/maintenance/params_task.rb +++ b/test/dummy/app/tasks/maintenance/params_task.rb @@ -3,9 +3,13 @@ module Maintenance class ParamsTask < MaintenanceTasks::Task attribute :post_ids, :string - validates :post_ids, presence: true - validates_format_of :post_ids, with: /\A(\s?\d+(,\s?\d+\s?)*)\z/, - allow_blank: true + validates :post_ids, + presence: true, + format: { with: /\A(\s?\d+(,\s?\d+\s?)*)\z/, allow_blank: true } + + class << self + attr_accessor :fast_task + end def collection Post.where(id: post_ids_array) @@ -16,7 +20,7 @@ def count end def process(post) - sleep(1) + sleep(1) unless self.class.fast_task post.update!(content: "New content added on #{Time.now.utc}") end diff --git a/test/dummy/db/schema.rb b/test/dummy/db/schema.rb index 9d576ade3..25c89e43a 100644 --- a/test/dummy/db/schema.rb +++ b/test/dummy/db/schema.rb @@ -48,15 +48,15 @@ t.integer "tick_count", default: 0, null: false t.integer "tick_total" t.string "job_id" - t.bigint "cursor" + t.integer "cursor" t.string "status", default: "enqueued", null: false t.string "error_class" t.string "error_message" t.text "backtrace" t.datetime "created_at", precision: 6, null: false t.datetime "updated_at", precision: 6, null: false - t.text "parameters" - t.index ["task_name", "created_at"], name: "index_maintenance_tasks_runs_on_task_name_and_created_at", order: { created_at: :desc } + t.text "arguments" + t.index ["task_name", "created_at"], name: "index_maintenance_tasks_runs_on_task_name_and_created_at" end create_table "posts", force: :cascade do |t| diff --git a/test/jobs/maintenance_tasks/task_job_test.rb b/test/jobs/maintenance_tasks/task_job_test.rb index 03950ef2a..d53fa2335 100644 --- a/test/jobs/maintenance_tasks/task_job_test.rb +++ b/test/jobs/maintenance_tasks/task_job_test.rb @@ -356,13 +356,13 @@ class << self assert_predicate run.reload, :succeeded? end - test ".perform_now makes Task parameters available" do + test ".perform_now makes arguments supplied for Task parameters available" do post = Post.last Maintenance::ParamsTask.any_instance.expects(:process).once.with(post) run = Run.create!( task_name: "Maintenance::ParamsTask", - parameters: { post_ids: post.id.to_s } + arguments: { post_ids: post.id.to_s } ) TaskJob.perform_now(run) diff --git a/test/models/maintenance_tasks/run_test.rb b/test/models/maintenance_tasks/run_test.rb index dbbc7710f..3bd18acbb 100644 --- a/test/models/maintenance_tasks/run_test.rb +++ b/test/models/maintenance_tasks/run_test.rb @@ -23,7 +23,7 @@ class RunTest < ActiveSupport::TestCase test "invalid if associated Task has parameters and they are invalid" do run = Run.new( task_name: "Maintenance::ParamsTask", - parameters: { post_ids: "xyz" } + arguments: { post_ids: "xyz" } ) refute run.valid? end @@ -289,11 +289,14 @@ class RunTest < ActiveSupport::TestCase assert run.task.is_a?(Maintenance::UpdatePostsTask) end - test "#task assigns parameter attributes if the Task supports parameters" do + test "#valid_task_arguments? instantiates Task and assigns arguments if Task has parameters" do run = Run.new( task_name: "Maintenance::ParamsTask", - parameters: { post_ids: "1,2,3" } + arguments: { post_ids: "1,2,3" } ) + run.valid_task_arguments? + + assert_predicate run, :valid? assert_equal "1,2,3", run.task.post_ids end diff --git a/test/system/maintenance_tasks/runs_test.rb b/test/system/maintenance_tasks/runs_test.rb index 56e55d524..34caac16f 100644 --- a/test/system/maintenance_tasks/runs_test.rb +++ b/test/system/maintenance_tasks/runs_test.rb @@ -29,12 +29,12 @@ class RunsTest < ApplicationSystemTestCase assert_no_button "Run" end - test "run a Task with parameters" do + test "run a Task that accepts parameters" do visit maintenance_tasks_path click_on("Maintenance::ParamsTask") post_id = Post.first.id - fill_in("[task_params][post_ids]", with: post_id.to_s) + fill_in("[task_arguments][post_ids]", with: post_id.to_s) perform_enqueued_jobs do click_on "Run" @@ -45,14 +45,14 @@ class RunsTest < ApplicationSystemTestCase assert_text "Processed 1 out of 1 item (100%)." end - test "errors for Task with invalid parameters shown" do + test "errors for Task with invalid arguments shown" do visit maintenance_tasks_path click_on("Maintenance::ParamsTask") - fill_in("[task_params][post_ids]", with: "xyz") + fill_in("[task_arguments][post_ids]", with: "xyz") click_on "Run" - alert_text = "Validation failed: Parameters are invalid: Post ids is "\ + alert_text = "Validation failed: Arguments are invalid: :post_ids is "\ "invalid" assert_text alert_text end