LG-12794 remove effective user #10446
Conversation
changelog: Internal, IdV, Remove effective user
| @@ -73,7 +72,7 @@ def analytics | |||
| end | |||
|
|
|||
| def analytics_user | |||
| effective_user || AnonymousUser.new | |||
| current_user || AnonymousUser.new | |||
There was a problem hiding this comment.
We need a solution here to make sure that in the hybrid flow we are not logging anonymous-id. I just did a quick test and we do in fact log anonymous-id if the hybrid flow is opened in a browser without a user logged in.
A few options:
- We can update this code to recognize we are in a hybrid flow session and select the correct user
- We can add an override to the hybrid flow methods to make sure
analytics_userreturns the user we are operating on
| hybrid_user || AnonymousUser.new | ||
| end | ||
|
|
||
| def hybrid_user |
There was a problem hiding this comment.
can we update the naming to something more reflective that this can be a current user or a hybrid user?
| @@ -283,26 +283,6 @@ | |||
| end | |||
| end | |||
|
|
|||
| context 'with hybrid user' do | |||
There was a problem hiding this comment.
i think we had this spec b/c a hybrid user can end up in the in person flow. What was the error that was observed before removing this spec?
There was a problem hiding this comment.
The error I was getting was enrollment on line 297 would end up being nil
There was a problem hiding this comment.
@night-jellyfish - I'm hoping you might be able to id if we should still include a test for the hybrid flow?
There was a problem hiding this comment.
That's a great question @svalexander ! The TLDR is that yes, I do believe we want to keep this test.
I had to do a lot of digging because I wasn't sure initially, and this part of the code has changed a lot in other work. In our workflow we don't end up going to the "try in person" option very often.
After that research I still wasn't sure, and I ended up testing it out locally. I found that yes - we do have the ability to search for post offices on the mobile side when in the hybrid flow, if ipp is enabled and we have an attention result. I believe that's what this is testing.
Unfortunately I'm not sure what to do about the user being nil in this code change. I think that would take me a good deal more research.
There was a problem hiding this comment.
@night-jellyfish OK it is returning nil because I removed effective_user. We removed it because hybrid flow was only pertaining to uploading a photo and viewing if that was successful. If people also can look up USPS locations on the hybrid flow I might have to add something to check for doc_capture_user_id.
There was a problem hiding this comment.
If the user is in the in-person flow, they need to be fully authenticated, right? effective_user was meant to support photo upload via phones only. Any other idv pages should be looking at current_user (unless i am missing something major about how IPP works)
…ntendlog and usps locations
| @@ -59,7 +59,7 @@ def proofer | |||
|
|
|||
| def add_proofing_component | |||
| ProofingComponent. | |||
| create_or_find_by(user: effective_user). | |||
| create_or_find_by(user: current_or_hybrid_user). | |||
There was a problem hiding this comment.
This should probably just be current_user, right? Can someone from @18F/identity-joy confirm?
spec/features/idv/in_person_spec.rb
Outdated
| @@ -216,54 +216,6 @@ | |||
| end | |||
| end | |||
|
|
|||
| context 'with hybrid document capture' do | |||
There was a problem hiding this comment.
I'm not sure I understand this test removal? Do we have coverage elsewhere for in-person starting in the hybrid flow?
There was a problem hiding this comment.
I can add it back in.
🎫 Ticket
LG-12794
🛠 Summary of changes
Removed EffectiveUserConcern and all references to effective_user
📜 Testing Plan
Provide a checklist of steps to confirm the changes.