Skip to content

LG-9229 add transliteration validation to additional fields on state id pg#8099

Merged
svalexander merged 4 commits intomainfrom
shannon/lg-9229-state-id-transliteration
Mar 31, 2023
Merged

LG-9229 add transliteration validation to additional fields on state id pg#8099
svalexander merged 4 commits intomainfrom
shannon/lg-9229-state-id-transliteration

Conversation

@svalexander
Copy link
Contributor

🎫 Ticket

LG-9229

🛠 Summary of changes

Add transliteration validation for address line 1, address line 2 and city.

📜 Testing Plan

Provide a checklist of steps to confirm the changes.

  • Set in_person_capture_secondary_id_enabled to true
  • Walk through ipp flow up to the state id page
  • Enter inputs with non-transliterable characters and make sure error shows after clicking continue; examples:
    * for address lines #1 $treet lañe^ should identify $, ^ as characters that need to be replaced
    * for city line B3st C!ty should identify 3, ! as characters that need to be replaced

👀 Screenshots

If relevant, include a screenshot or screen capture of the changes.

Screen Shot 2023-03-30 at 10 45 21 AM

@svalexander svalexander requested review from a team, NavaTim, allis-green, erogray and rutvigupta-design and removed request for a team, erogray and rutvigupta-design March 30, 2023 15:56
Copy link

@allis-green allis-green left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!


context 'transliteration' do
let(:capture_secondary_id_enabled) { true }
let(:double_address_verification) { true }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would be inclined to test with DAV both on/off until it's enabled in prod.

@NavaTim
Copy link
Contributor

NavaTim commented Mar 30, 2023

You will also need to redact some errors from the log, similar to this:
59c383d#diff-3b900ce50338ce0c9fafd805a3625ef6f1a1591749891fd1c97b65375fd02740

@NavaTim
Copy link
Contributor

NavaTim commented Mar 30, 2023

I removed several feedback items since the messaging makes sense & @allis-green approves. That leaves the log redaction (required) and the additional automated testing (recommended).

@aduth
Copy link
Contributor

aduth commented Mar 30, 2023

You will also need to redact some errors from the log, similar to this: 59c383d#diff-3b900ce50338ce0c9fafd805a3625ef6f1a1591749891fd1c97b65375fd02740

This feels like an easy thing to overlook and would have had bad consequences if not had you caught it. Perhaps separately, can we consider some options to try to catch this sooner, like changing errors to allowlist-only, or enhancing the FakeAnalytics PiiAlerter utility to catch these mock IPP values?

@NavaTim
Copy link
Contributor

NavaTim commented Mar 30, 2023

This feels like an easy thing to overlook and would have had bad consequences if not had you caught it. Perhaps separately, can we consider some options to try to catch this sooner, like changing errors to allowlist-only, or enhancing the FakeAnalytics PiiAlerter utility to catch these mock IPP values?

@aduth It makes sense to add some additional detection. I feel like our team could add a story to check for :nontransliterable_field in the PiiAlerter, but may want to discuss an allowlist w/ other teams before implementing that approach.

@svalexander
Copy link
Contributor Author

You will also need to redact some errors from the log,
@NavaTim want to double check with you how to verify this is working as expected. I tried adding some print statements but cleaned errors is empty before the delete statements

@svalexander svalexander requested a review from NavaTim March 30, 2023 21:26
fill_in t('in_person_proofing.form.state_id.first_name'), with: 'T0mmy "Lee"'
fill_in t('in_person_proofing.form.state_id.last_name'), with: 'Джейкоб'
click_idv_continue
# binding.pry
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this debugging code.

Suggested change
# binding.pry

@@ -23,6 +23,9 @@ def submit(params)
cleaned_errors = errors.deep_dup
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@svalexander I realized I made a mistake when originally working on this, and that's why FakeAnalytics didn't catch the issue here. This change will fix the issue.

Suggested change
cleaned_errors = errors.deep_dup
cleaned_errors = errors.dup

I'd appreciate if you make the same change to address_form.rb as well.

@aduth FYI I think this is part of the issue here, i.e. ActiveModel::Error supports dup for cloning, but not deep_dup. There may also be issues when trying to print out the contents of these error objects directly.

@svalexander svalexander requested a review from NavaTim March 31, 2023 13:20
@svalexander svalexander merged commit 70b427b into main Mar 31, 2023
@svalexander svalexander deleted the shannon/lg-9229-state-id-transliteration branch March 31, 2023 19:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants