From f9fbdadb410361c6890b5a3f62bd5e47aa9d6fdb Mon Sep 17 00:00:00 2001 From: Jeremy Friesen Date: Thu, 10 Sep 2020 10:56:36 -0400 Subject: [PATCH] WIP Re-rendering Work params for failed submit Prior to this commit, when I would submit an invalid work create form, the re-rendered form would have blank input values. With this change all non-file attributes are re-loaded with the submitted values. Please note, this needs a bit more time to put full polish on, but I want to make sure its a path worth pursuing. If this is a reasonable path, I'm happy to add the additional specs and inline comments to get this working. I'm presently dissatisfied with the changes I needed to make to the case statement, and would appreciate guidance on how to correct that. Related to #3978 --- .../hyrax/works_controller_behavior.rb | 3 + .../forms/failed_submission_form_wrapper.rb | 110 ++++++++++++++++++ app/helpers/hyrax/embargo_helper.rb | 2 +- app/helpers/hyrax/lease_helper.rb | 2 +- .../failed_submission_form_wrapper_spec.rb | 49 ++++++++ 5 files changed, 164 insertions(+), 2 deletions(-) create mode 100644 app/forms/hyrax/forms/failed_submission_form_wrapper.rb create mode 100644 spec/forms/hyrax/forms/failed_submission_form_wrapper_spec.rb diff --git a/app/controllers/concerns/hyrax/works_controller_behavior.rb b/app/controllers/concerns/hyrax/works_controller_behavior.rb index c5735eefd2..f94b5d2570 100644 --- a/app/controllers/concerns/hyrax/works_controller_behavior.rb +++ b/app/controllers/concerns/hyrax/works_controller_behavior.rb @@ -53,12 +53,15 @@ def new end def create + cached_params = params[hash_key_for_curation_concern].deep_dup if create_work after_create_response else respond_to do |wants| wants.html do build_form + wrapper = Hyrax::Forms::FailedSubmissionFormWrapper.new(form: @form, input_params: cached_params, permitted_params: @form.class.permitted_params) + @form = wrapper render 'new', status: :unprocessable_entity end wants.json { render_json_response(response_type: :unprocessable_entity, options: { errors: curation_concern.errors }) } diff --git a/app/forms/hyrax/forms/failed_submission_form_wrapper.rb b/app/forms/hyrax/forms/failed_submission_form_wrapper.rb new file mode 100644 index 0000000000..aa66681458 --- /dev/null +++ b/app/forms/hyrax/forms/failed_submission_form_wrapper.rb @@ -0,0 +1,110 @@ +# frozen_string_literal: true +module Hyrax + module Forms + # This object is responsible for exposing the user submitted + # params from a failed POST/PUT/PATCH. + # + # @see https://github.com/samvera/hyrax/issues/3978 + class FailedSubmissionFormWrapper < SimpleDelegator + # @param form + # @param input_params [ActionController::Parameters] + # + # @param permitted_params [Hash] this likely comes from a class + # method on the given form. It's used for enforcing + # strong parameters. It's more of a schema for what the + # input parameters should be. Why not rely on the + # form.class.permitted_params? This is a violation of + # the Law of Demeter, and due to object delegation, the + # form.class may be a delegate class, and not the one + # that has permitted_params. + # + # @see Hyrax::WorkForm.build_permitted_params + def initialize(form:, input_params:, permitted_params:) + @form = form + @input_params = input_params + @permitted_params = permitted_params + @exposed_params = {} + super(@form) + build_exposed_params! + end + + # Yes, I don't want to delegate class to the form, but based on + # past experiences, there are times where SimpleForm requests + # class.model_name and that had better align with the underlying + # form. + # + # Upon testing, when I don't have this method, the Javascript + # for "Requirements" on the new form will not properly + # acknowledge that we have re-filled the HTML form with the + # submitted non-file fields. + def class + @form.class + end + + def model_name + @form.model_name + end + + def to_model + self + end + + def inspect + "Hyrax::Forms::FailedSubmissionFormWrapper(#{@form})" + end + + def [](key) + if @exposed_params.key?(key) + @exposed_params.fetch(key) + else + super + end + end + + private + + def method_missing(method_name, *args, &block) + if @exposed_params.key?(method_name) + @exposed_params.fetch(method_name) + else + super + end + end + + def respond_to_missing?(method_name, *args) + return true if @exposed_params.key?(method_name) + super + end + + def build_exposed_params! + @permitted_params.each do |permitted_param| + case permitted_param + when ::Symbol + @exposed_params[permitted_param] = @input_params.fetch(permitted_param) if @input_params.key?(permitted_param) + when ::Hash + permitted_param.each do |key, value_schema| + build_exposed_param_hash_element!(key: key, value_schema: value_schema) + end + end + end + end + + def build_exposed_param_hash_element!(key:, value_schema:) + return if @input_params.key?(key) + # I don't have a non-Array example of what this value_schema could be. + return unless value_schema.is_a?(::Array) + if value_schema.empty? + @exposed_params[key] = ::Array.wrap(@input_params.fetch(key)) + elsif value_schema.is_a?(::Array) + # We're expecting nested attributes which will have the form: + # { "0" => , "1" => } + hash = {} + @input_params.fetch(key).each_pair do |nested_key, nested_value| + hash[nested_key] = nested_value.slice(*value_schema) + end + @exposed_params[key] = hash + end + end + end + end +end diff --git a/app/helpers/hyrax/embargo_helper.rb b/app/helpers/hyrax/embargo_helper.rb index f4d06daa88..a8a88041b6 100644 --- a/app/helpers/hyrax/embargo_helper.rb +++ b/app/helpers/hyrax/embargo_helper.rb @@ -24,7 +24,7 @@ def embargo_enforced?(resource) case resource when Hydra::AccessControls::Embargoable !resource.embargo_release_date.nil? - when HydraEditor::Form + when HydraEditor::Form, Hyrax::Forms::FailedSubmissionFormWrapper embargo_enforced?(resource.model) when Valkyrie::ChangeSet Hyrax::EmbargoManager.new(resource: resource.model).enforced? diff --git a/app/helpers/hyrax/lease_helper.rb b/app/helpers/hyrax/lease_helper.rb index ebb6f6abb0..93da200fbb 100644 --- a/app/helpers/hyrax/lease_helper.rb +++ b/app/helpers/hyrax/lease_helper.rb @@ -24,7 +24,7 @@ def lease_enforced?(resource) case resource when Hydra::AccessControls::Embargoable !resource.lease_expiration_date.nil? - when HydraEditor::Form + when HydraEditor::Form, Hyrax::Forms::FailedSubmissionFormWrapper lease_enforced?(resource.model) when Valkyrie::ChangeSet Hyrax::LeaseManager.new(resource: resource.model).enforced? diff --git a/spec/forms/hyrax/forms/failed_submission_form_wrapper_spec.rb b/spec/forms/hyrax/forms/failed_submission_form_wrapper_spec.rb new file mode 100644 index 0000000000..8e8b34b77d --- /dev/null +++ b/spec/forms/hyrax/forms/failed_submission_form_wrapper_spec.rb @@ -0,0 +1,49 @@ +# frozen_string_literal: true +RSpec.describe Hyrax::Forms::FailedSubmissionFormWrapper do + let(:form) { instance_double(Hyrax::Forms::WorkForm, **form_attributes) } + let(:form_attributes) { { title: ["Form's Title"], description: ["Form's description"] } } + # I would love to use Hyrax::Forms::WorkForm, but I encountered an + # exception when I do Hyrax::Forms::WorkForm.permitted_params. In + # this spec, things fail, however in + # ./spec/forms/hyrax/forms/work_form_spec.rb it works. + let(:permitted_params) do + [ + { title: [] }, + :representative_id, + { description: [] }, + { based_near_attributes: [:id, :_destroy], member_of_collections_attributes: [:id, :_destroy], work_members_attributes: [:id, :_destroy] }, + { permissions_attributes: [:type, :name, :access, :id, :_destroy] } + ] + end + let(:input_params) do + ActionController::Parameters.new( + title: ["Title"], + representative_id: "123", + obviously_missing_attribute: "One", + member_of_collections_attributes: { "0" => { id: "1" }, "2" => { id: "2", _destroy: "1" } } + ) + end + + let(:wrapper) { described_class.new(form: form, input_params: input_params, permitted_params: permitted_params) } + + describe "input param key is part of permitted params" do + it "exposes the given input params" do + expect(wrapper.representative_id).to eq(input_params.fetch(:representative_id)) + expect(wrapper[:representative_id]).to eq(input_params.fetch(:representative_id)) + + expect(wrapper.title).to eq(input_params.fetch(:title)) + expect(wrapper[:title]).to eq(input_params.fetch(:title)) + + expect(wrapper.member_of_collections_attributes).to eq(input_params.fetch(:member_of_collections_attributes)) + expect(wrapper[:member_of_collections_attributes]).to eq(input_params.fetch(:member_of_collections_attributes)) + + expect { wrapper.obviously_missing_attribute }.to raise_error(NoMethodError) + end + + it "delegates to the underlying form when an input param is not given" do + expect(wrapper.description).to eq(form.description) + allow(form).to receive(:[]).with(:description).and_return(form_attributes.fetch(:description)) + expect(wrapper[:description]).to eq(form.description) + end + end +end