LG-10759 Fix State Address Validation

[svalexander]

🎫 Ticket

LG-10759

🛠 Summary of changes

This fixes a bug that allowed a user to submit the state id form with blank spaces at Address Line 1 and City. If the user did this and clicked true for same_address_as_id then on submission the next page is not rendered. This pr adds those fields to the state id validator to make sure the user input is validated.

📜 Testing Plan

Provide a checklist of steps to confirm the changes.

• Set in_person_capture_secondary_id_enabled to true
• Walk through ipp flow and arrive at state id form page
• Fill out form but on address line 1 and city input fields type in blank spaces
• Check yes for 'same_address_as_id`
• Submit and verify that you do not progress to the next page and that there are error messages presented
• Change same_address_as_id to no and re-submit
• Verify that you do not progress to the next page and that there are error messages presented

👀 Screenshots

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

[gina-yamada]

Tested both same as address and different and working as expected.
Both an empty value and a value of only spaces display a validation message for both address 1 and city on the page. In both cases, a user can not continue until fields are entered.

LGTM

[aduth]

To avoid the lint, could we use .presence on the config value, since it'd be true if true, and nil if false, and I think that's the behavior you're after?

[1] pry(main)> false.presence
=> nil
[2] pry(main)> true.presence
=> true

Suggested change

	# rubocop:disable Style/MultilineIfModifier
	validates :identity_doc_address1,
	:identity_doc_city,
	presence: true if IdentityConfig.store.in_person_capture_secondary_id_enabled
	# rubocop:enable Style/MultilineIfModifier
	validates :identity_doc_address1,
	:identity_doc_city,
	presence: IdentityConfig.store.in_person_capture_secondary_id_enabled.presence

[svalexander]

@aduth @NavaTim I wanted to only validate those fields if the user has encountered dav in order to fix the pii detected error I'm seeing for some of the other specs. However the form is still logging pii in specs when dav is not enabled for the user. Part of what I thought was the issue was the in_person_helper.rb file was not checking capture_secondary_id_enabled?. I tried updating the helper to check that field but that's not resolving the issue.

[NavaTim]

@aduth @NavaTim I wanted to only validate those fields if the user has encountered dav in order to fix the pii detected error I'm seeing for some of the other specs. However the form is still logging pii in specs when dav is not enabled for the user. Part of what I thought was the issue was the in_person_helper.rb file was not checking capture_secondary_id_enabled?. I tried updating the helper to check that field but that's not resolving the issue.

First, update the conditional validation and check if the issue is still happening. The conditional you currently have is evaluated with the class definition, so the validator may not actually be added/removed between tests.

If that doesn't fix it, then here's some more context.

The address field name is making it into the error details, which makes it into the log entry via the Idv::StateIdForm class in the FormResponse object.

The safest way to prevent the issue is to scrub the error entirely, similar to how the transliteration errors were removed (though that removes potentially valuable info from logs). An alternative would be manually adding [:error_details, :identity_doc_address1] and [:error_details, :identity_doc_city] to the allowlisted keypaths.

[NavaTim]

The implementation looks good, but we should simplify the tests before proceeding.

[NavaTim]

double_address_verification and capture_secondary_id_enabled appear to be synonymous with regard to the test helpers. Do we really need an additional parameter here?

[svalexander]

good point, i did think about removing double_address_verification from the places where I checked capture_secondary_id_enabled but had decided against it since other tests are still using dav and wanted to minimize any confusion. But it is a bit redundant. I'll remove dav from the tests I updated.

[NavaTim]

I would prefer some additional test changes, but approving since the issue is likely to become moot very soon.

[NavaTim]

The usage of both double_address_verification and capture_secondary_id_enabled here seems likely to cause confusion. I think we should only use one or the other for the test methods in general unless a specific test needs to differentiate.

My preferred approach in this case is reverting to double_address_verification for most usages & mapping to capture_secondary_id_enabled where needed.

[svalexander]

i'm going to go ahead and merge since we'll be removing dav though I understand it might be confusing to see the different params for state_id_form vs address_form