LG-7832: Validate State ID and Address are transliterable to a valid USPS submission by NavaTim · Pull Request #7938 · 18F/identity-idp

NavaTim · 2023-03-06T23:50:00Z

🎫 Ticket

LG-7832

🛠 Summary of changes

Changes behind feature flag usps_ipp_transliteration_enabled:
- Validate that we obtain a valid USPS submission when we transliterate the following fields:
  - State ID
    - First name
    - Last name
  - Address
    - Address Line 1
    - Address Line 2
    - City
- Notify the user of the specific invalid characters when the fields fail validation
Support errors with potential PII content in message (e.g. single-character names)
- Errors identified by pii: true
- Skips logging error message (but type will still be logged)
- pii_like_fields will be updated to contain the field name
- Will still serve error to UI in form response
Includes:
- LG-7832: Preserve State ID and Address form data on invalid submissions #7936
- LG-7832: Transliterate name, address, and city for USPS API (w/ FF) #7935
Based on LG-7832: Transliterate fields for the USPS API #7866

📜 Testing Plan

Provide a checklist of steps to confirm the changes.

Complete IPP flow up to state ID / address
Check that state ID and address forms reject non-transliterable characters
Check that state ID and address forms reject transliterable characters that don't meet the USPS API requirements
Check that state ID and address forms accept transliterable characters that have diacritical marks but otherwise meet the USPS API requirements

👀 Screenshots

Desktop (State ID):

Desktop (Address):

Desktop (Verify):

Mobile (State ID):

Mobile (Address):

Mobile (Verify):

changelog: Improvements, In-Person Proofing, USPS API Transliteration

changelog: Bug Fixes, In-Person Proofing, Retain State ID and Address on form re-render after invalid submissions

… tbradley/lg-7832-transliterate-usps-api-frontend

…USPS submission changelog: Improvements, In-Person Proofing, Validate State ID and Address are transliterable to a valid USPS submission

… tbradley/lg-7832-transliterate-usps-api-frontend

…to tbradley/lg-7832-transliterate-usps-api-frontend

…oloader

… tbradley/lg-7832-transliterate-usps-api-frontend

Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>

…to tbradley/lg-7832-transliterate-usps-api-frontend

Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>

…ttps://github.com/18F/identity-idp into tbradley/lg-7832-transliterate-usps-api-integration

Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>

…ley/lg-7832-transliterate-usps-api-integration

…ley/lg-7832-transliterate-usps-api-fix-forms

…ttps://github.com/18F/identity-idp into tbradley/lg-7832-transliterate-usps-api-fix-forms

…to tbradley/lg-7832-transliterate-usps-api-frontend

… tbradley/lg-7832-transliterate-usps-api-frontend

…ley/lg-7832-transliterate-usps-api-frontend

…ccess

app/services/usps_in_person_proofing/transliterable_validator.rb

zachmargolis · 2023-03-07T22:41:50Z

app/services/usps_in_person_proofing/transliterable_validator.rb

+      if (@fields & SUPPORTED_FIELDS).size < @fields.size
+        # Catch incorrect usage of the validator
+        raise StandardError.new(
+          "Unsupported transliteration fields: #{(@fields - SUPPORTED_FIELDS).to_json}",
+        )
+      end


do we need this check ahead of time? If we added .fetch when we look up validations per field, we'd get errors early on that would make it clear which keys are unknown

This is to help catch unsupported usage, so it's better that this error occur as early as possible in the development cycle. If you can point me to a mechanism for disabling this check specifically for prod, then I'd be open to adding that.

zachmargolis · 2023-03-07T22:42:58Z

app/services/form_response.rb

+    pii_keys.each do |key|
+      hash[:pii_like_keypaths] ||= []
+      hash[:pii_like_keypaths].append([:error_details, key]) if error_details.has_key?(key)
+      hash[:errors] = hash[:errors].except(key) if hash[:errors].has_key?(key)
+    end


The goal for pii_like_keypaths was that it's passed in at the analytics.track_event callsite, is the reason we're doing this here because for these 2 forms, the analytics happens "automatically" inside of the FSM?

Yes, the FSM decouples the form from the analytics so we don't directly control how the errors are logged.

Ah I just found how we work around this in other places, we add it to the extra part of the FormResponse:

identity-idp/app/forms/idv/address_form.rb

Line 27 in 683ab69

pii_like_keypaths: [[:errors, :zipcode], [:error_details, :zipcode]],

Can we try that approach instead?

This is a case where we should avoid logging the error message, which could in some cases expose a person's name. The same approach doesn't seem acceptable here.

If the goal is to not log them, can we remove them in the form before instantiating the FormResponse? FormResponse is to capture and transport data specifically to our logs

Like in this spot:

identity-idp/app/services/idv/steps/in_person/address_step.rb

Line 52 in 2469f2a

form.submit(flow_params)

we could still submit the form, but then post-process that FormResponse to omit errors we don't want to log

e.g. create a version of the Errors object with the transliteration errors filtered out when creating FormResponse in these classes?

yup exactly that

Ok, that seems doable. Thanks!

Updated to remove the errors potentially containing PII from the returned FormResponse. It looks like the errors are still displaying as expected, and they are not being logged.

app/services/usps_in_person_proofing/transliterable_validator_helper.rb

…d string

zachmargolis · 2023-03-09T16:20:38Z

app/validators/idv/form_state_id_validator.rb

+                     message: (proc do |invalid_chars|
+                                 I18n.t(
+                                   'in_person_proofing.form.state_id.errors.unsupported_chars',
+                                   char_list: invalid_chars.join(', '),
+                                 )
+                               end)


I think if we switch this to "stabby lambda" syntax we may not need the extra wrapping parens anymore, so I just went and pushed that in 5b20738

- Real ActiveModel::Errors instance - Model fields all defined in one spot - spy on real validator.transliterator instead of stubbing

zachmargolis

LGTM

a few notes:

I went ahead and pushed some syntactic/stubbing cleanups myself.
I wish we had unit tests for the FormAddressValidator/FormStateIdValidator (or at least the forms they're included in) so we don't have to go all the way to feature specs, but that's a bigger change than this PR

zachmargolis · 2023-03-09T16:22:17Z

config/locales/in_person_proofing/en.yml

+          unsupported_chars: 'Our system cannot read the following characters:
+            %{char_list}. Please try again using substitutes for those
+            characters.'


I feel like the copy is a little misleading, clearly the system can read the characters if we can render them back... but I'm guessing this copy already went through UX, and this is not the hill I die on

rutvigupta-design · 2023-03-10T16:59:06Z

This looks good to me!

…USPS submission (#7938) * LG-7832: Validate State ID and Address are transliterable to a valid USPS submission changelog: Improvements, In-Person Proofing, Validate State ID and Address are transliterable to a valid USPS submission Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com> * LG-7832: Make test updates based on PR feedback * Update spec/services/usps_in_person_proofing/enrollment_helper_spec.rb Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com> * Update app/services/usps_in_person_proofing/enrollment_helper.rb Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com> * LG-7832: Remove incorrect usage of .class * LG-7832: Feature should only be enabled by default in dev environments * Update app/services/idv/steps/in_person/address_step.rb Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com> * Update app/services/idv/steps/in_person/state_id_step.rb Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com> * LG-7832: Use case no longer requires support for HashWithIndifferentAccess * LG-7832: Clean up translation usage * LG-7832: Update test * LG-7832: Update test * LG-7832: Simplify unsupported character extraction from transliterated string * LG-7832: Clarify transliteration result documentation * LG-7832: Refactor helper logic into validator classes * Switch to ->() lambda syntax to remove extra parens * Change one more proc syntax to "stabby lambda" * un-stub the TransliterableValidator spec as much as possible - Real ActiveModel::Errors instance - Model fields all defined in one spot - spy on real validator.transliterator instead of stubbing --------- Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com> Co-authored-by: Zach Margolis <zachary.margolis@gsa.gov>

NavaTim added 4 commits March 6, 2023 15:53

LG-7832: Transliterate name, address, and city for USPS API (w/ FF)

306b9ad

changelog: Improvements, In-Person Proofing, USPS API Transliteration

LG-7832: Preserve State ID and Address form data on invalid submissions

bbb6bd8

changelog: Bug Fixes, In-Person Proofing, Retain State ID and Address on form re-render after invalid submissions

Merge branch 'tbradley/lg-7832-transliterate-usps-api-fix-forms' into…

7e43196

… tbradley/lg-7832-transliterate-usps-api-frontend

LG-7832: Validate State ID and Address are transliterable to a valid …

d2f5e4c

…USPS submission changelog: Improvements, In-Person Proofing, Validate State ID and Address are transliterable to a valid USPS submission

NavaTim requested a review from zachmargolis March 6, 2023 23:50

NavaTim added 2 commits March 6, 2023 17:00

LG-7832: Fix address test

7dd52e4

Merge branch 'tbradley/lg-7832-transliterate-usps-api-fix-forms' into…

e3c5080

… tbradley/lg-7832-transliterate-usps-api-frontend

NavaTim mentioned this pull request Mar 7, 2023

LG-7832: Transliterate fields for the USPS API #7866

Closed

5 tasks

NavaTim and others added 22 commits March 6, 2023 17:15

LG-7832: Remove bad auto-merged configs

0f5103b

Merge branch 'tbradley/lg-7832-transliterate-usps-api-integration' in…

73e3fa6

…to tbradley/lg-7832-transliterate-usps-api-frontend

LG-7832: Move TransliterationResult to location expected by Rails aut…

3b1bb95

…oloader

LG-7832: Fix state ID step test

3d683fb

Merge branch 'tbradley/lg-7832-transliterate-usps-api-fix-forms' into…

acf503a

… tbradley/lg-7832-transliterate-usps-api-frontend

Update Makefile

ce32726

Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>

Merge branch 'tbradley/lg-7832-transliterate-usps-api-integration' in…

06ea563

…to tbradley/lg-7832-transliterate-usps-api-frontend

Update app/services/usps_in_person_proofing/transliterator.rb

923e77b

Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>

Update app/services/usps_in_person_proofing/transliterator.rb

36c5cab

Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>

LG-7832: Make test updates based on PR feedback

05b579c

Merge branch 'tbradley/lg-7832-transliterate-usps-api-integration' of h…

2b42b45

…ttps://github.com/18F/identity-idp into tbradley/lg-7832-transliterate-usps-api-integration

LG-7832: Lint fix

e15cb28

Update spec/services/usps_in_person_proofing/enrollment_helper_spec.rb

63df395

Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>

Update app/services/usps_in_person_proofing/enrollment_helper.rb

cad7a75

Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>

LG-7832: Remove incorrect usage of .class

8bbf6bd

LG-7832: Feature should only be enabled by default in dev environments

8bda24e

Update app/services/idv/steps/in_person/address_step.rb

0a85a50

Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>

Update app/services/idv/steps/in_person/state_id_step.rb

d76ccba

Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>

Merge branch 'main' of https://github.com/18F/identity-idp into tbrad…

fc48a3e

…ley/lg-7832-transliterate-usps-api-integration

Merge branch 'main' of https://github.com/18F/identity-idp into tbrad…

969c2f4

…ley/lg-7832-transliterate-usps-api-integration

Merge branch 'main' of https://github.com/18F/identity-idp into tbrad…

0fc657c

…ley/lg-7832-transliterate-usps-api-fix-forms

Merge branch 'tbradley/lg-7832-transliterate-usps-api-fix-forms' of h…

22ba566

…ttps://github.com/18F/identity-idp into tbradley/lg-7832-transliterate-usps-api-fix-forms

NavaTim added 7 commits March 7, 2023 13:40

Merge branch 'tbradley/lg-7832-transliterate-usps-api-integration' in…

959b264

…to tbradley/lg-7832-transliterate-usps-api-frontend

Merge branch 'tbradley/lg-7832-transliterate-usps-api-fix-forms' into…

94c54b7

… tbradley/lg-7832-transliterate-usps-api-frontend

Merge branch 'main' of https://github.com/18F/identity-idp into tbrad…

d2b2390

…ley/lg-7832-transliterate-usps-api-frontend

Merge branch 'main' of https://github.com/18F/identity-idp into tbrad…

eb4fee3

…ley/lg-7832-transliterate-usps-api-frontend

LG-7832: Use case no longer requires support for HashWithIndifferentA…

f8daae1

…ccess

LG-7832: Clean up translation usage

2d9d5d6

LG-7832: Update test

966e7ac

NavaTim marked this pull request as ready for review March 7, 2023 22:07

LG-7832: Update test

66cbf42

zachmargolis reviewed Mar 7, 2023

View reviewed changes

NavaTim added 2 commits March 7, 2023 18:27

LG-7832: Simplify unsupported character extraction from transliterate…

79e09fc

…d string

LG-7832: Clarify transliteration result documentation

b772dc5

NavaTim requested a review from zachmargolis March 8, 2023 01:57

NavaTim and others added 2 commits March 8, 2023 17:14

LG-7832: Refactor helper logic into validator classes

7b49ac3

Switch to ->() lambda syntax to remove extra parens

5b20738

zachmargolis reviewed Mar 9, 2023

View reviewed changes

zachmargolis added 2 commits March 9, 2023 09:13

Change one more proc syntax to "stabby lambda"

0841878

un-stub the TransliterableValidator spec as much as possible

9a8522e

- Real ActiveModel::Errors instance - Model fields all defined in one spot - spy on real validator.transliterator instead of stubbing

zachmargolis approved these changes Mar 9, 2023

View reviewed changes

NavaTim requested review from a team and erogray and removed request for a team March 10, 2023 16:53

NavaTim merged commit 59c383d into main Mar 10, 2023

NavaTim deleted the tbradley/lg-7832-transliterate-usps-api-frontend branch March 10, 2023 17:11

zachmargolis mentioned this pull request Mar 13, 2023

Deploy RC 261 to Prod #7978

Merged

zachmargolis mentioned this pull request Mar 15, 2023

Deploy RC 262 to Prod #7992

Merged

Conversation

NavaTim commented Mar 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎫 Ticket

🛠 Summary of changes

📜 Testing Plan

👀 Screenshots

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zachmargolis Mar 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zachmargolis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rutvigupta-design commented Mar 10, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

NavaTim commented Mar 6, 2023 •

edited

Loading

zachmargolis Mar 7, 2023 •

edited

Loading