LG-7832: Transliterate fields for the USPS API#7866
Conversation
…ion of transliterable field element
…egrating with state ID page
…ley/lg-7832-transliterate-usps-api
|
app/controllers/idv/in_person/validate_transliterable_controller.rb
Outdated
Show resolved
Hide resolved
| # @option fields [String,#to_s] :address2 | ||
| # @option fields [String,#to_s] :city | ||
| # @return [Hash,nil] Field names mapped to error messages, or nil if there were no issues | ||
| def validate(fields) |
There was a problem hiding this comment.
Why not use kwargs here? and build a hash from that?
| def validate(fields) | |
| def validate(first_name: nil, last_name: nil, address1: nil, address2: nil, city: nil) | |
| fields = { first_name:, last_name:, address1:, address2: city: }.select(&:present?) |
There was a problem hiding this comment.
I'll take a look at the impact implementing this change would have on the controller. I was trying here to keep logic about which fields get validated relatively isolated to this class, but your suggested usage could be easier to work with.
|
|
||
| private | ||
|
|
||
| ADDITIONAL_UNSUPPORTED_CHARS = ActiveSupport::HashWithIndifferentAccess.new( |
There was a problem hiding this comment.
if we construct the hash using keywords like above, we don't need this to have indifferent access, it could just be a plan hash?
|
I haven't had a chance to dive deep into this, but am I correct that this validates the characters prior to submission from the frontend using fetch? Would it be simpler to validate this from the backend and use FSM obfuscates the form, but the general idea being... form.errors.add(:address1, t('in_person_proofing.form.state_id.errors.transliteration')) if has_unsupported_character? |
Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov>
…ley/lg-7832-transliterate-usps-api
@aduth It's looking like the state ID and address forms may not be setup to work with this approach out-of-the box, i.e. I'm not seeing errors populate on the form with the approach you suggested. If you think it's worth pursuing that approach further, then let me know and maybe we can discuss it tomorrow. You can check out the changes I made to test this approach in this commit. |
Yeah unfortunately it looks like FSM doesn't make it very easy to maintain values through an invalid submission to the next render. I was able to get a minimal working prototype going with some small revisions, though (diff below). Having to touch FSM to get this to work is unfortunate. Luckily, I suspect that if FSM were refactored away, the form bits could still carry over and be useful and easier to implement. I guess my overall concern is that I suspect the conventional Rails implementation might be more maintainable and have fewer moving parts than asynchronous frontend validation. Alternatively, is there a way we could implement the validation on the frontend without calling out to an API? diff --git a/app/forms/idv/state_id_form.rb b/app/forms/idv/state_id_form.rb
index 5b2a1a1b7..c7635693b 100644
--- a/app/forms/idv/state_id_form.rb
+++ b/app/forms/idv/state_id_form.rb
@@ -9,2 +9,4 @@ module Idv
+ validate :validate_supported_characters
+
def self.model_name
@@ -28,2 +30,6 @@ module Idv
+ def validate_supported_characters
+ errors.add(:first_name, I18n.t('in_person_proofing.form.state_id.errors.transliteration'))
+ end
+
def consume_params(params)
diff --git a/app/services/flow/base_flow.rb b/app/services/flow/base_flow.rb
index 6f5779794..85d0c7266 100644
--- a/app/services/flow/base_flow.rb
+++ b/app/services/flow/base_flow.rb
@@ -40,3 +40,3 @@ module Flow
@flow_session[:error_message] = nil
- handler = step_handler(step)
+ handler = step_handler_instance(step)
return failure("Unhandled step #{step}") unless handler
@@ -46,6 +46,5 @@ module Flow
def extra_view_variables(step)
- handler = step_handler(step)
+ handler = step_handler_instance(step)
return failure("Unhandled step #{step}") unless handler
- obj = handler.new(self)
- obj.extra_view_variables
+ handler.extra_view_variables
end
@@ -62,6 +61,10 @@ module Flow
+ def step_handler_instance(step)
+ @step_handler_instances ||= {}
+ @step_handler_instances[step] ||= step_handler(step)&.new(self)
+ end
+
def wrap_send(handler)
- obj = handler.new(self)
- value = obj.base_call
- form_response(obj, value)
+ value = handler.base_call
+ form_response(handler, value)
end
diff --git a/app/services/idv/steps/in_person/state_id_step.rb b/app/services/idv/steps/in_person/state_id_step.rb
index 3afeb3f0d..ff82e91ba 100644
--- a/app/services/idv/steps/in_person/state_id_step.rb
+++ b/app/services/idv/steps/in_person/state_id_step.rb
@@ -31,2 +31,3 @@ module Idv
{
+ form:,
pii: flow_session[:pii_from_user],
@@ -40,4 +41,4 @@ module Idv
def form_submit
- Idv::StateIdForm.new(current_user).submit(
- permit(
+ form.submit(
+ flow_params.permit(
*Idv::StateIdForm::ATTRIBUTES,
@@ -51,2 +52,10 @@ module Idv
end
+
+ def flow_params
+ params.require(:state_id)
+ end
+
+ def form
+ @form ||= Idv::StateIdForm.new(current_user)
+ end
end
diff --git a/app/views/idv/in_person/state_id.html.erb b/app/views/idv/in_person/state_id.html.erb
index 3ba30bd91..1d05c544b 100644
--- a/app/views/idv/in_person/state_id.html.erb
+++ b/app/views/idv/in_person/state_id.html.erb
@@ -27,3 +27,3 @@
-<%= simple_form_for :doc_auth,
+<%= simple_form_for form,
url: url_for, |
@aduth Thanks for the additional clarification. I mentioned in our discussion today a concern with the stateful Ruby class instances being reused, but after looking into the actual usage that appears not to be the case. The flow is fully re-initialized on each request in module Flow
module FlowStateMachine
extend ActiveSupport::Concern
included do
before_action :initialize_flow_state_machine # this line
before_action :ensure_correct_step, only: :show
end
# ...This is good since it means that there's no risk of the state being inappropriately preserved between requests. I'll test to make sure that the errors appear in the expected manner using this approach. If so, then I'll check in with design since it would effectively remove one of the error states we discussed, then likely proceed with implementation. |
…ley/lg-7832-transliterate-usps-api
…hub.com/18F/identity-idp into tbradley/lg-7832-transliterate-usps-api
|
@zachmargolis @svalexander I resolved some of the comments here because I am switching to the implementation suggested by @aduth. I'll let you know when the updated implementation is ready for review. |
…ley/lg-7832-transliterate-usps-api
… validation into validator modules for forms
…ley/lg-7832-transliterate-usps-api
app/services/form_response.rb
Outdated
| @success | ||
| end | ||
|
|
||
| def analytics_hash |
There was a problem hiding this comment.
I think adding another method to the form response object specifically for this is not a good approach. I see that it's skipping certain errors, but I would recommend finding another way if possible
see notes in our docs about creating separate properties for sensitive values, because the entire point of the to_h method on this class is for logging things to our events log
There was a problem hiding this comment.
Based on how the form is behaving when I attempt to set the sensitive properties, I think the current flow/form system simply does not support form errors that could contain PII.
i.e. returning the errors via FormResponse is inherently connected to logging those same errors. Those need to be decoupled.
I can attempt to make the approach more lightweight (e.g. by using a pii_ prefix convention in the error type), but it looks like something similar to this will need to change in order to support this scenario.
There was a problem hiding this comment.
@zachmargolis The guidance to add properties during #submit simply did not work when I tried it, so I pushed up code with a slightly different approach. If you think there's a problem with the new approach, then I'd like to discuss alternative approaches on Monday.
If you think the new approach is acceptable, then I can add a description of the new option to the guidance.
The new option automatically redacts the message if you mark an error with pii: true, but it keeps the error type (which is ultimately what the error_details code is getting, though that's a little obscured). It also adds to :pii_like_keypaths so the error type doesn't trigger the PII check.
…diacritical marks for USPS proofing
| end | ||
| end | ||
|
|
||
| context 'transliteration' |
There was a problem hiding this comment.
this doesn't throw an error, but it also doesn't get printed when running tests; we should follow convention and make it a block that wraps the new test below
context 'transliteration' do
...
endThere was a problem hiding this comment.
I intended to use a block here. Thanks for pointing out the issue.
|
Per request from @zachmargolis, this has been split into the following pull requests. |
5 participants
🎫 Ticket
🛠 Summary of changes
📜 Testing Plan
Provide a checklist of steps to confirm the changes.
👀 Screenshots