Merge pull request #1122 from Shopify/preserve-user-input

Preserve user input when there are validation errors

app/controllers/maintenance_tasks/runs_controller.rb

@@ -6,6 +6,7 @@ module MaintenanceTasks
   #
   # @api private
   class RunsController < ApplicationController
+    helper TasksHelper
     before_action :set_run, except: :create
 
     # Creates a Run for a given Task and redirects to the Task page.
@@ -19,7 +20,9 @@ def create(&block)
       )
       redirect_to(task_path(task))
     rescue ActiveRecord::RecordInvalid => error
-      redirect_to(task_path(error.record.task_name), alert: error.message)
+      flash.now.alert = error.message
+      @task = TaskDataShow.prepare(error.record.task_name, arguments: error.record.arguments)
+      render(template: "maintenance_tasks/tasks/show")
     rescue ActiveRecord::ValueTooLong => error
       task_name = params.fetch(:task_id)
       redirect_to(task_path(task_name), alert: error.message)

app/controllers/maintenance_tasks/tasks_controller.rb

@@ -18,14 +18,7 @@ def index
     # Renders the page responsible for providing Task actions to users.
     # Shows running and completed instances of the Task.
     def show
-      task_name = params.fetch(:id)
-      @task = TaskDataShow.new(task_name)
-      @task.active_runs.load
-      set_refresh if @task.active_runs.any?
-      @runs_page = RunsPage.new(@task.completed_runs, params[:cursor])
-      if @task.active_runs.none? && @runs_page.records.none?
-        Task.named(task_name)
-      end
+      @task = TaskDataShow.prepare(params.fetch(:id), runs_cursor: params[:cursor])
     end
 
     private

app/models/maintenance_tasks/runs_page.rb

@@ -21,6 +21,9 @@ def initialize(runs, cursor)
       @cursor = cursor
     end
 
+    # @return [String, nil] the cursor for the page of Runs.
+    attr_reader :cursor
+
     # Returns the records for a Page, taking into account the cursor if one is
     # present. Limits the number of records to 20.
     #

app/models/maintenance_tasks/task_data_show.rb

@@ -11,17 +11,39 @@ module MaintenanceTasks
   #
   # @api private
   class TaskDataShow
-    # Initializes a Task Data with a name and optionally a related run.
+    # Initializes a Task Data with a name.
     #
     # @param name [String] the name of the Task subclass.
-    def initialize(name)
+    # @param runs_cursor [String, nil] the cursor for the runs page.
+    # @param arguments [Hash, nil] the Task arguments.
+    def initialize(name, runs_cursor: nil, arguments: nil)
       @name = name
+      @arguments = arguments
+      @runs_page = RunsPage.new(completed_runs, runs_cursor)
+    end
+
+    class << self
+      # Prepares a Task Data from a task name.
+      #
+      # @param name [String] the name of the Task subclass.
+      # @param runs_cursor [String, nil] the cursor for the runs page.
+      # @param arguments [Hash, nil] the Task arguments.
+      # @raise [Task::NotFoundError] if the Task doesn't have runs (for the given cursor) and doesn't exist.
+      def prepare(name, runs_cursor: nil, arguments: nil)
+        new(name, runs_cursor:, arguments:)
+          .load_active_runs
+          .ensure_task_exists
+      end
     end
 
     # @return [String] the name of the Task.
     attr_reader :name
     alias_method :to_s, :name
 
+    # @return [RunsPage] the current page of completed runs, based on the cursor
+    #   passed in initialize.
+    attr_reader :runs_page
+
     # The Task's source code.
     #
     # @return [String] the contents of the file which defines the Task.
@@ -38,6 +60,11 @@ def code
       File.read(file)
     end
 
+    # @return [Boolean] whether the task data needs to be refreshed.
+    def refresh?
+      active_runs.any?
+    end
+
     # Returns the set of currently active Run records associated with the Task.
     #
     # @return [ActiveRecord::Relation<MaintenanceTasks::Run>] the relation of
@@ -79,12 +106,34 @@ def parameter_names
       end
     end
 
-    # @return [MaintenanceTasks::Task, nil] an instance of the Task class.
+    # @return [MaintenanceTasks::Task] an instance of the Task class.
     # @return [nil] if the Task file was deleted.
     def new
       return if deleted?
 
-      MaintenanceTasks::Task.named(name).new
+      task = MaintenanceTasks::Task.named(name).new
+      begin
+        task.assign_attributes(@arguments) if @arguments
+      rescue ActiveModel::UnknownAttributeError
+        # nothing to do
+      end
+      task
+    end
+
+    # Preloads the records from the active_runs ActiveRecord::Relation
+    # @return [self]
+    def load_active_runs
+      active_runs.load
+      self
+    end
+
+    # @raise [Task::NotFoundError] if the Task doesn't have Runs (for the given cursor) and doesn't exist.
+    # @return [self]
+    def ensure_task_exists
+      if active_runs.none? && runs_page.records.none?
+        Task.named(name)
+      end
+      self
     end
 
     private

app/views/maintenance_tasks/tasks/show.html.erb

@@ -38,7 +38,7 @@
   <pre><code><%= highlight_code(code) %></code></pre>
 <% end %>
 
-<%= tag.div(data: { refresh: (defined?(@refresh) && @refresh) || "" }) do %>
+<%= tag.div(data: { refresh: @task.refresh? || "" }) do %>
   <% if @task.active_runs.any? %>
     <hr/>
 
@@ -47,13 +47,13 @@
     <%= render partial: "maintenance_tasks/runs/run", collection: @task.active_runs %>
   <% end %>
 
-  <% if @runs_page.records.present? %>
+  <% if @task.runs_page.records.present? %>
     <hr/>
 
     <h4 class="title is-4">Previous Runs</h4>
 
-    <%= render partial: "maintenance_tasks/runs/run", collection: @runs_page.records %>
+    <%= render partial: "maintenance_tasks/runs/run", collection: @task.runs_page.records %>
 
-    <%= link_to "Next page", task_path(@task, cursor: @runs_page.next_cursor) unless @runs_page.last? %>
+    <%= link_to "Next page", task_path(@task, cursor: @task.runs_page.next_cursor) unless @task.runs_page.last? %>
   <% end %>
 <% end %>

config/routes.rb

@@ -9,6 +9,7 @@
         put "resume"
       end
     end
+    get :runs, to: redirect("tasks/%{task_id}")
   end
 
   root to: "tasks#index"

test/models/maintenance_tasks/task_data_show_test.rb

@@ -4,6 +4,23 @@
 
 module MaintenanceTasks
   class TaskDataShowTest < ActiveSupport::TestCase
+    test ".prepare returns a TaskDataShow with active_runs loaded" do
+      task_data = TaskDataShow.prepare("Maintenance::UpdatePostsTask")
+      assert_predicate task_data.active_runs, :loaded?
+    end
+
+    test ".prepare raises if the task doesn't exist and doesn't have runs" do
+      assert_raises Task::NotFoundError do
+        TaskDataShow.prepare("Maintenance::DoesNotExist")
+      end
+    end
+
+    test ".prepare doesn't raise if the task was deleted (doesn't exist but has runs)" do
+      assert_nothing_raised do
+        TaskDataShow.prepare("Maintenance::DeletedTask")
+      end
+    end
+
     test "#code returns the code source of the Task" do
       task_data = TaskDataShow.new("Maintenance::UpdatePostsTask")
 
@@ -74,6 +91,20 @@ class TaskDataShowTest < ActiveSupport::TestCase
       refute_predicate TaskDataShow.new("Maintenance::DoesNotExist"), :csv_task?
     end
 
+    test "#refresh? returns true if there are active runs" do
+      assert_predicate TaskDataShow.new("Maintenance::UpdatePostsTask"), :refresh?
+    end
+
+    test "#refresh? returns false if there are no active runs" do
+      refute_predicate TaskDataShow.new("Maintenance::DoesNotExist"), :refresh?
+    end
+
+    test "#runs_page returns a RunsPage with the cursor set" do
+      runs_page = TaskDataShow.new("MaintenanceTasks::UpdatePostsTask", runs_cursor: 42).runs_page
+      assert_kind_of RunsPage, runs_page
+      assert_equal 42, runs_page.cursor
+    end
+
     test "#parameter_names returns list of parameter names for Tasks supporting parameters" do
       assert_equal(
         [
@@ -111,5 +142,15 @@ class TaskDataShowTest < ActiveSupport::TestCase
     test "#new returns nil for a deleted Task" do
       assert_nil TaskDataShow.new("Maintenance::DoesNotExist").new
     end
+
+    test "#new returns a Task prefilled with arguments" do
+      task_data = TaskDataShow.prepare("Maintenance::ParamsTask", arguments: { content: "super content" })
+      assert_equal "super content", task_data.new.content
+    end
+
+    test "#new ignores unknown arguments" do
+      task_data = TaskDataShow.prepare("Maintenance::ParamsTask", arguments: { unknown: nil })
+      assert_nothing_raised { task_data.new }
+    end
   end
 end

test/system/maintenance_tasks/runs_test.rb

@@ -99,9 +99,11 @@ class RunsTest < ApplicationSystemTestCase
 
       click_on("Maintenance::ParamsTask")
       fill_in("task[post_ids]", with: "xyz")
+      fill_in("task[content]", with: "super content")
       click_on "Run"
 
       assert_text "Validation failed: Arguments are invalid: :post_ids is invalid"
+      assert_field "task[content]", with: "super content"
     end
 
     test "download the CSV attached to a run for a CSV Task" do