LG-13006: remove instances of skip_doc_auth

[KeithNava]

🎫 Ticket

Link to the relevant ticket:
LG-13006

🛠 Summary of changes

Removing instances of the old variable skip_doc_auth

📜 Testing Plan

Since this touches a large portion of the application, I would recommend a full regression for in person enrollment flow.

[shanechesnutt-ft]

Everything looks good to me. Nice Job! I just had one quick question.

Does skip_doc_auth need to be removed from the VALID_SESSION_ATTRIBUTES constant in app/services/idv/session.rb?

https://github.com/18F/identity-idp/blob/959fa818c9b989cf6b66b276fb00668bc3beea0b/app/services/idv/session.rb#L28

[KeithNava]

Everything looks good to me. Nice Job! I just had one quick question.

Does skip_doc_auth need to be removed from the VALID_SESSION_ATTRIBUTES constant in app/services/idv/session.rb?

https://github.com/18F/identity-idp/blob/959fa818c9b989cf6b66b276fb00668bc3beea0b/app/services/idv/session.rb#L28

@shanechesnutt-ft Great observation and question! We actually will be removing it after the deployment. It was actually called out specifically in the ticket as the last and final step 👉 https://cm-jira.usa.gov/browse/LG-13006

[eileen-nava]

@KeithNava
(I’m starting this general conversation on a specific file so that replies are threaded.)

When I look at the “Progress” section of the ticket LG-13006, I see checkbox 4

• remove decision points with the old value skip_doc_auth and replace with skip_doc_auth_from_how_to_verify. PR created (23 May), wait for next deploy to merge.

After the “wait for deploy” line, I see checkbox 5

• remove skip_doc_auth from everywhere, except session.rb.

Based on the "Summary of changes" in the PR description, I think this PR is meant to take care of checkbox 5, correct? I don’t see that Micah N’s PR #10690, which I believe took care of checkbox 4, was ever merged. Are you planning on merging that PR before this one? Does it need a review?

Let me know, thanks! I haven’t yet had a chance to review this PR, but I wanted to ask this question upfront because I know sequencing gets tricky for PRs that are affected by 50-50 state.

[KeithNava]

@eileen-nava Thanks. I actually noticed that the logic in PR #10690 was updated via this commit da81567#diff-ffdec6293037a40f4daee9914c58a356de09f502e897c0e73ea4f02c76787c02 so I don't think we would need Micah's PR anymore. I'm tagging @n1zyy to help confirm though.

[n1zyy]

@KeithNava I have to plead some level of ignorance here. 😇

I wasn't intentionally changing anything relating to skip_doc_auth in that PR; I just removed it as appearing to be unused. This comment paints some context for the change—we were passing that value into the constructor but didn't seem to ever be using its value in the show method, so I removed it as dead code. (TBH I don't completely remember why I thought it was unused, but it seems like people agreed with me. I've lost some of this context to time.)

I didn't have Micah's PR in mind when I worked on this. It looks like his change to this file would be unnecessary, but he did also update the HybridHandoffController which I didn't touch, so that likely still needs attention.

[eileen-nava]

I left some comments and will get back to this after retro.

[eileen-nava]

I’m concerned that the changes to this method will cause problems during deployment. When I check the 50/50 state docs, it says in the first deploy to read from both fields with new_name || old_name and then in the second deploy to remove references to the old name.

In one deploy, this change reads from only the new variable name and removes references to the old variable name.

[eileen-nava]

I did some local manual testing of the hybrid handoff page and I think I found an error.

1. I started the app running locally on this branch
2. I navigated to the hybrid handoff page
3. While leaving the hybrid handoff page open in my browser, I killed the app and restarted it running on main
4. The hybrid handoff page reloaded (without me clicking anything) and displayed the “how to verify page” which comes before the hybrid handoff page.

I made a screenrecording and uploaded it to our team google drive here.

[KeithNava]

Thanks for this, we were able to somewhat (not consistently) reproduce this so I went ahead and added back the original check. I also created follow up ticket to remove this secondary check once this is deployed.

[eileen-nava]

@KeithNava I again followed the below steps

1. I started the app running locally on this branch
2. I navigated to the hybrid handoff page
3. While leaving the hybrid handoff page open in my browser, I killed the app and restarted it running on main
4. The hybrid handoff page reloaded (without me clicking anything) and displayed the “how to verify page” which comes before the hybrid handoff page.

Again, I made a screenrecording and uploaded it to our team google drive as LG-13006Sep2550:50StateBug.mov.

Please let me know if you'd like to pair on this. If you want, we could try these steps on your local setup and see if you encounter the same bug.

[KeithNava]

Looks like this is happening because the hybrid_handoff controller (https://github.com/18F/identity-idp/blob/main/app/controllers/idv/hybrid_handoff_controller.rb#L51) is expecting skip_doc_auth to be set which it wouldn't be when coming from this branch since it's being removed.

I'm not exactly sure how to get around this given our strategy around removing all instances of skip_doc_auth in this PR - Unless we preemptively change the logic to expect either skip_doc_auth or skip_doc_auth_from_how_to_verify in this logic. 🤔

[eileen-nava]

Since you added idv_session.skip_doc_auth_from_how_to_verify == false || idv_session.skip_doc_auth == false to line 51 of the hybrid_handoff_controller, I wouldn't expect there to be a problem here. The code is following the instruction in 50/50 state documentation about reading from both the old and new field. 🤔

[eileen-nava]

Hi, as I noted in my comment at the top of this thread:

When I check the 50/50 state docs, it says in the first deploy to read from both fields with new_name || old_name and then in the second deploy to remove references to the old name.
In one deploy, this change reads from only the new variable name and removes references to the old variable name.

This pattern persisted in the how_to_verify_controller. I modified the how_to_verify_controller to read from both fields with new_name || old_name. After that change, I no longer see an error when I follow the below steps.

1. I started the app running locally on this branch
2. I navigated to the hybrid handoff page
3. While leaving the hybrid handoff page open in my browser, I killed the app and restarted it running on main. I was able to navigate forwarding in the IPP flow. Success!

I took a screenrecording. Let me know if it would be helpful to see.

Below is a git diff of the change.

diff --git a/app/controllers/idv/how_to_verify_controller.rb b/app/controllers/idv/how_to_verify_controller.rb
index 233d6165d..0cd089dda 100644
--- a/app/controllers/idv/how_to_verify_controller.rb
+++ b/app/controllers/idv/how_to_verify_controller.rb
@@ -34,12 +34,12 @@ module Idv
       if result.success?
         if how_to_verify_form_params['selection'] == Idv::HowToVerifyForm::REMOTE
           idv_session.opted_in_to_in_person_proofing = false
-          idv_session.skip_doc_auth_from_how_to_verify = false
+          idv_session.skip_doc_auth_from_how_to_verify = false || idv_session.skip_doc_auth = false
           redirect_to idv_hybrid_handoff_url
         else
           idv_session.opted_in_to_in_person_proofing = true
           idv_session.flow_path = 'standard'
-          idv_session.skip_doc_auth_from_how_to_verify = true
+          idv_session.skip_doc_auth_from_how_to_verify = true || idv_session.skip_doc_auth = true
           redirect_to idv_document_capture_url
         end
       else
@@ -63,7 +63,7 @@ module Idv
           idv_session.service_provider&.in_person_proofing_enabled
         end,
         undo_step: ->(idv_session:, user:) {
-                     idv_session.skip_doc_auth_from_how_to_verify = nil
+                     idv_session.skip_doc_auth_from_how_to_verify = nil || idv_session.skip_doc_auth = nil
                      idv_session.opted_in_to_in_person_proofing = nil
                    },
       )

[eileen-nava]

I understand removing idv_session.skip_doc_auth = false. I'm not sure why idv_session.skip_doc_auth_from_handoff = false was added here.

[shanechesnutt-ft]

Did some regression testing and things look good to me! 👍🏻

[gina-yamada]

@KeithNava Your pipeline is failing. It looks like lint and specs need to be fixed. Also, you mentioned that you had some follow up work that could not be complete in the PR. Can you link the tickets in here? Thank you

I'd prefer to review this after specs are passing in-case more code changes need to be made. Please reach out in the Joy Channel via Slack when the pipeline is passing so we can give you a review.

[gina-yamada]

Nice work here Keith! I can see where this gets complicated fast.

✅ opted_in_to_in_person_proofing is set as I'd expect when I move forward and backward in the flow
✅ Not seeing errors in the console as I step through this
✅ I tested a handful of cases changing config vars in_person_proofing_enabled and in_person_proofing_opt_in_enabled because they work with the code in React- I am being directed where I'd expect in the flow with those changed
✅ created an ID-IPP enrollment locally
✅ Other than the one change above, it looks like all instances of skip_doc_auth have been replaced with skip_doc_auth_from_how_to_verify
🔴 One instance in comment above needs to be changed
🔴 Have you done testing around selfie_check_required? I think this is an area when should confirm we have tested manually and/or have good test coverage around. There is conditional logic in React around this- that is why I mentioned it. I attempted to require it but it is near the end of the day. I would consider have Team Ada have a review before merging as they work in doc auth a lot

Approving but would love to see one review from Ada because of the doc auth changes if possible

[eileen-nava]

Thanks for updating the how_to_verify_controller. Since we talked about automated testing for the 50/50 state in our 1:1 and you said you didn't have any examples, I'm including links to examples below:

• Example of automated 50/50 state testing in how_to_verify feature spec
• State id 50/50 spec

It's up to you if you want to add them. I realize this PR has already been very thoroughly reviewed and manually tested.

One addendum to Gina's comment - if you do reach out to another team, Timnit is focused on document capture and would have the most context on that. That being said, I'm sure Ada could offer helpful perspective, too.

Thanks for all the work you've done on this. 🙏🏻

[n1zyy]

I'm confused by this and the previous couple of instances. It looks like variable assignments joined with an OR? I have a sneaking suspicion that what is happening here is not what is intended.

[matthinz]

There is not an emoji response for "I agree, I am also puzzled by this" but pretend I added it to the comment above

[KeithNava]

Yep yep - it is strange. The short "explanation" is that when I originally removed the idv_session.skip_doc_auth variable completely we saw a bug during 50/50 state testing (going from this branch to main), where this variable was expected to be set. So we made the awkward but temporary change here, following the recommended new_var || old_var pattern, deploy, then remove old_var and that seemed to work 🤷

The longer details can be found in this discussion #11043 (comment)

In isolation, I am not the most comfortable with this change but given that the very next PR will remove idv_session.skip_doc_auth 🤷

[eileen-nava]

(I already chatted with Keith about this on slack, but I’m also posting here to keep Matt squared and co in the loop.)

Due to the difficulty with 50/50 state issues encountered on this PR, I think it would be a good idea to write automated tests to catch potential 50/50 state bugs.

Also, I’m definitely open to the idea that there’s a better approach than what I shared. I understand why it seems strange, and since you mention not being comfortable with it, I’d be interested to see what other solutions you’d propose.

Please let me know if it would be helpful to pair or what other support the team can offer.

[jmax-gsa]

+1 on 50/50 test. Happy (really; it's the kind of think I enjoy) to pair and help if you'd like.

[jmax-gsa]

And while I appreciate the value of following patterns, the practice exists to serve us, not the other way around, and in this specific case, I don't think clarity resulted. I'd prefer less puzzling code this time.

[n1zyy]

I took a look at this with my @18F/identity-ada hat on. Overall it looks good to me, but there's one section in the controller I left a question on. Other than that, 🤜🤛 looks good to me!

[n1zyy]

Just confirming my understanding: the idea here is backwards compatibility with people who might already have skip_doc_auth on their session?

[KeithNava]

Yep that's exactly it! The follow up ticket would remove skip_doc_auth entirely.

[jmax-gsa]

This looks correct as step 1, with step 2 (as noted in the conversation) being removing support for the A/B state eventually.

[jmax-gsa]

+1 on 50/50 test. Happy (really; it's the kind of think I enjoy) to pair and help if you'd like.

[jmax-gsa]

And while I appreciate the value of following patterns, the practice exists to serve us, not the other way around, and in this specific case, I don't think clarity resulted. I'd prefer less puzzling code this time.