Skip to content

Commit

Permalink
Fixups
Browse files Browse the repository at this point in the history
  • Loading branch information
adrianna-chang-shopify committed May 25, 2021
1 parent 09e854f commit 86967ed
Show file tree
Hide file tree
Showing 12 changed files with 63 additions and 50 deletions.
8 changes: 4 additions & 4 deletions app/controllers/maintenance_tasks/tasks_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down
34 changes: 19 additions & 15 deletions app/models/maintenance_tasks/run.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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) }

Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down
8 changes: 4 additions & 4 deletions app/models/maintenance_tasks/runner.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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!
Expand Down
8 changes: 4 additions & 4 deletions app/views/maintenance_tasks/tasks/show.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,10 @@
<% end %>
<% if @task.parameter_names.any? %>
<div class="block">
<%= 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 %>
</div>
Expand Down
Original file line number Diff line number Diff line change
@@ -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

This file was deleted.

2 changes: 2 additions & 0 deletions test/application_system_test_case.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
12 changes: 8 additions & 4 deletions test/dummy/app/tasks/maintenance/params_task.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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
Expand Down
6 changes: 3 additions & 3 deletions test/dummy/db/schema.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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|
Expand Down
4 changes: 2 additions & 2 deletions test/jobs/maintenance_tasks/task_job_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
9 changes: 6 additions & 3 deletions test/models/maintenance_tasks/run_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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

Expand Down
10 changes: 5 additions & 5 deletions test/system/maintenance_tasks/runs_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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
Expand Down

0 comments on commit 86967ed

Please sign in to comment.