Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow leaving id_number and id_type fields empty in id_info for JT6. #60

Merged
merged 11 commits into from
Aug 31, 2023

Conversation

nii-mants3
Copy link
Contributor

@nii-mants3 nii-mants3 commented Aug 30, 2023

This PR introduces changes to allow country and id_type fields to be empty in id_info for JT6

@nii-mants3 nii-mants3 marked this pull request as ready for review August 30, 2023 13:08
@@ -93,6 +93,7 @@ def id_info=(id_info)

if updated_id_info[:entered] && updated_id_info[:entered] == 'true'
%i[country id_type id_number].each do |key|
next if @partner_params[:job_type].to_i == JobType::DOCUMENT_VERIFICATION
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to do this for all fields or just id_type? Why not do this above the loop rather than inside of it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah makes sense 👍

@@ -91,7 +91,7 @@ def id_info=(id_info)
# if it's a boolean
updated_id_info[:entered] = id_info[:entered].to_s if !updated_id_info[:entered].nil? == updated_id_info[:entered]

if updated_id_info[:entered] && updated_id_info[:entered] == 'true'
if updated_id_info[:entered] && updated_id_info[:entered] == 'true' && @partner_params[:job_type].to_i != JobType::DOCUMENT_VERIFICATION
Copy link
Contributor

Choose a reason for hiding this comment

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

You should check for equality instead. != JobType::DOCUMENT_VERIFICATION will be more fragile than checking against known job types. [country id_type id_number] are required for JobType::BIOMETRIC_KYC, JobType::DOCUMENT_VERIFICATION needs [country]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure i get the equality part. I'm not checking the type here. JobType::DOCUMENT_VERIFICATION resolves to 6. See https://github.com/smileidentity/smile-identity-core-ruby/blob/main/lib/smile-identity-core/constants/job_type.rb#L17

nii-mants3 and others added 2 commits August 30, 2023 17:28
Co-authored-by: Michael <michael.l.dangelo@gmail.com>
@nii-mants3 nii-mants3 changed the title Allow leaving country and id_type fields empty in id_info for JT6. Allow leaving id_number and id_type fields empty in id_info for JT6. Aug 30, 2023
Copy link
Contributor

@mldangelo mldangelo left a comment

Choose a reason for hiding this comment

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

This looks good to me. Going to defer to @beaglebets for final approval. Thank you @nii-mants3

@@ -91,8 +91,13 @@ def id_info=(id_info)
# if it's a boolean
updated_id_info[:entered] = id_info[:entered].to_s if !updated_id_info[:entered].nil? == updated_id_info[:entered]

if updated_id_info[:entered] && updated_id_info[:entered] == 'true'
%i[country id_type id_number].each do |key|
if updated_id_info[:entered] == '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 don't think this condition (entered == 'true') is necessary for job type 6, but I'll defer to @beaglebets. If it's not necessary, consider moving the JobType::DOCUMENT_VERIFICATION check above entered.

Copy link
Contributor

@beaglebets beaglebets left a comment

Choose a reason for hiding this comment

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

There are some odd things happening here. entered=true is only applicable to biometric kyc. I think you should heed Michael's advice and do things specific to the job types they are applicable to.

Copy link
Contributor

@mldangelo mldangelo left a comment

Choose a reason for hiding this comment

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

thank you

@nii-mants3 nii-mants3 merged commit 11e2622 into main Aug 31, 2023
6 checks passed
@nii-mants3 nii-mants3 deleted the allow-doc-nil-idinfo branch August 31, 2023 14:27
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