Skip to content

LG-10760: refactor validations for DocPiiForm#9469

Merged
amirbey merged 19 commits intomainfrom
amirbey/LG-10760-refactor-doc-pii-form-validations-orig
Nov 1, 2023
Merged

LG-10760: refactor validations for DocPiiForm#9469
amirbey merged 19 commits intomainfrom
amirbey/LG-10760-refactor-doc-pii-form-validations-orig

Conversation

@amirbey
Copy link
Contributor

@amirbey amirbey commented Oct 27, 2023

🎫 Ticket

LG-10760

🛠 Summary of changes

Refactor DocPiiForm to user ActiveModel::Validation helpers that surfaces all validation errors.

📜 Testing Plan

Provide a checklist of steps to confirm the changes.

  1. upload document with missing dob
  2. upload document with dob too young (ie: 2023/10/31)
  3. upload document with missing address
  4. upload document with invalid zipcode (too short or not a string)
  5. upload document with invalid jurisdiction (ie: 'UK')
  6. upload document with invalid state (less than or more than 2 characters)

👀 Screenshots

Screenshot 2023-10-30 at 5 48 32 PM Screenshot 2023-10-30 at 5 48 09 PM Screenshot 2023-10-30 at 5 47 40 PM Screenshot 2023-10-30 at 5 53 57 PM Screenshot 2023-10-30 at 5 53 31 PM

@amirbey amirbey self-assigned this Oct 30, 2023
@amirbey amirbey marked this pull request as ready for review October 30, 2023 22:37
@amirbey amirbey requested review from a team and eileen-nava and removed request for a team October 30, 2023 22:37
end
end

context 'when there is an invalid state' do
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for adding this context block. 👍🏻

@form_response.errors.except(:hints).flat_map do |key, errs|
form_response_errors = @form_response.errors.clone
if form_response_errors.many? { |k, v| %i[name dob dob_min_age state].include?(k) }
form_response_errors = { pii: [I18n.t('doc_auth.errors.general.no_liveness')] }
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice use of .many?

Copy link
Contributor

@dawei-nava dawei-nava Oct 31, 2023

Choose a reason for hiding this comment

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

Another option is to deal this in the doc_pii_form, so it's the form's responsibility to deal with all this? On the otherhand, it is how you present his errors, so fine to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dawei-nava - yes, this consolidation seems to be more about what the FE receives. i did look at perforning this in the DocPIIForm but I figured this way allows us to be more explicit in our analytics and support

expect(doc_auth_log.front_image_submit_count).to eq(count)
end

def pii_like_keypaths
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this helper method is defined and used in three different spec files, I recommend consolidating and defining it in one file that stores helper methods.

[:errors, :zipcode], [:error_details, :zipcode],
[:errors, :jurisdiction], [:error_details, :jurisdiction]
]
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Just for my own note, the pii_like_keypaths looks like following now, this explicitly tells FakeAnalytics that this nested hash keys are allowed entry when invoking track_event and there is no pii info exposed.

[[:pii], [:name, :dob, :dob_min_age, :address1, :state, :zipcode, :jurisdiction], [:errors, :name], [:error_details, :name], [:errors, :dob], [:error_details, :dob], [:errors, :dob_min_age], [:error_details, :dob_min_age], [:errors, :address1], [:error_details, :address1], [:errors, :state], [:error_details, :state], [:errors, :zipcode], [:error_details, :zipcode], [:errors, :jurisdiction], [:error_details, :jurisdiction]]

Copy link
Contributor

@dawei-nava dawei-nava left a comment

Choose a reason for hiding this comment

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

LGTM

@amirbey amirbey merged commit b9db924 into main Nov 1, 2023
@amirbey amirbey deleted the amirbey/LG-10760-refactor-doc-pii-form-validations-orig branch November 1, 2023 16:10
@amirbey amirbey mentioned this pull request Nov 2, 2023
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.

3 participants