LG-12979: Create new idv_level value of 'in_person' on Profile #10371
Conversation
added multiple newline to separate cases, updated context for consistent communication around tmx Co-authored-by: Matt Wagner <mattwagner@navapbc.com>
app/services/idv/profile_maker.rb
Outdated
| @@ -47,7 +47,9 @@ def save_profile( | |||
| private | |||
|
|
|||
| def set_idv_level(in_person_verification_needed:, selfie_check_performed:) | |||
| if in_person_verification_needed | |||
| if in_person_verification_needed && IdentityConfig.store.in_person_proofing_enforce_tmx | |||
There was a problem hiding this comment.
Slight refactor to help clarify what's going on with this check (feel free to ignore)
if in_person_verification_needed
if IdentityConfig.store.in_person_proofing_enforce_tmx
:in_person
else
:legacy_in_person
elsif ...There was a problem hiding this comment.
I originally had it coded that way but refactored it. I believe elsif is more performant over a nested if. I am happy to change it if nested is preferred.
There was a problem hiding this comment.
I don't feel strongly about any of this, but...
I had actually initially gotten hung up on line 52, because I think the && !IdentityConfig.store.in_person_proofing_enforce_tmx bit is logically unnecessary—if it weren't false, it would have matched on line 50. But I think having it anyway makes it a tiny bit clearer what is going on (Jack's suggestion does the same), so I didn't comment.
I'm academically curious to experiment with how nested conditionals compare to inlined ones, but I wouldn't worry too much about that level of optimization here.
tl;dr, I don't have a strong opinion and will support whatever you think is most readable here.
There was a problem hiding this comment.
Okay, for readability I am going with Jack's suggestion as that is how I originally wrote it as well. See commit c08407d
There was a problem hiding this comment.
It's an edge case, but I think we should probably also check that FeatureManagement.proofing_device_profiling_decisioning_enabled? is returning true before marking as non-legacy.
There was a problem hiding this comment.
@matthinz Just curious but what does that value represent?
There was a problem hiding this comment.
@matthinz - Jack and I talked through your suggestion. We agree with it! I talked Jack into implement it in a different PR as we thought we had better inspect all instances of IdentityConfig.store.in_person_proofing_enforce_tmx to evaluate if FeatureManagement.proofing_device_profiling_decisioning_enabled? should also be considered. That task seemed out of scope for this ticket and we didn't want it to get overlooked here.
Thank you for reviewing this!
Manual testing
|
…79-create-new-idv-level-value-of-in-person
gina-yamada
requested review from
JackRyan1989,
n1zyy,
KeithNava and
racingspider
| @@ -48,7 +48,11 @@ def save_profile( | |||
|
|
|||
| def set_idv_level(in_person_verification_needed:, selfie_check_performed:) | |||
| if in_person_verification_needed | |||
| :legacy_in_person | |||
| if IdentityConfig.store.in_person_proofing_enforce_tmx | |||
There was a problem hiding this comment.
Noting here that I will add a check for FeatureManagement.proofing_device_profiling_decisioning_enabled? in a separate pull request.
gina-yamada
deleted the
yamada/LG-12979-create-new-idv-level-value-of-in-person
branch
🎫 Ticket
LG-12979 Create new idv_level value of 'in_person'
🛠 Summary of changes
📜 Testing Plan
Testing locally...
in_person_proofing_enforce_tmxto false in config.application.ymlrails ce = InPersonEnrollment.last. (Note the timestamp)p = e.profileto confirm the value of idv_level is "legacy_in_person"in_person_proofing_enforce_tmxto true in config.application.ymlrails ce2 = InPersonEnrollment.last(Confirm is was the enrollment you just created by checking timestamps.)p2 = e2.profileto confirm the value of idv_level is "in_person"