18F / identity-idp

Login.gov Core App: Identity Provider (IdP)
https://secure.login.gov/
Other
524 stars 112 forks source link

Fix 404 Error on PO Search Page for IPP Flow #11287

Closed WilliamBirdsall closed 3 weeks ago

WilliamBirdsall commented 1 month ago

🎫 Ticket

Link to the relevant ticket: LG-14545

πŸ›  Summary of changes

On the PO search page for the IPP flow, the extendSession callback was being called without a sessionURL being provided. This is due to the IPP flow being almost entirely in Ruby and not having a current session on the React side of things.

This change conditionally adds the extendSession callback only if the flow is not Opt In IPP.

This issue is also not happening in the Help Center so there is no separate ticket for that work as per the AC.

πŸ“œ Testing Plan

WilliamBirdsall commented 1 month ago

Notes:

WilliamBirdsall commented 1 month ago

Dropping a note here that folks from both Ada and TImnit have given this a once over. Thanks everyone who helped out!

aduth commented 1 month ago

Hm, I actually think there's a bigger issue at play here, which is only being surfaced because there's a multi-step process. extendSession previously didn't require a sessionsURL parameter, until #10894. The way extendSession is being used here will never work. I think we should either remove it altogether, or fix it so that sessionsURL is provided as expected.

WilliamBirdsall commented 1 month ago

Hm, I actually think there's a bigger issue at play here, which is only being surfaced because there's a multi-step process. extendSession previously didn't require a sessionsURL parameter, until #10894. The way extendSession is being used here will never work. I think we should either remove it altogether, or fix it so that sessionsURL is provided as expected.

Yeah noticing that this is the case...

How should we move forward with this? Should a new ticket be written? Who would be the owner of that ticket?

Depending on how long a fix for the larger issue may take should we still merge this in?

aduth commented 1 month ago

I think we should fix the underlying issue as part of this ticket.

WilliamBirdsall commented 4 weeks ago

I understand wanting to fix the underlying issue. Since I’ve only worked on IPP and am not familiar with the Document Capture flow code-wise, I am concerned I don’t have enough context to effectively determine which fix is better and how to best implement it. Since it is in their domain, I think someone from Timnit might be better suited. I’d be happy to do code review or pair with the dev who works on the underlying issue!

aduth commented 4 weeks ago

If we wanted a short-term solution, what I'd suggest is:

The behavior from LG-3813 is relevant for both IPP and the new multi-step selfie flow added by @AShukla-GSA in #11285, since the idea with this code is that it might take a user longer than 15 minutes to complete the multi-step process, and renewing the session avoids them being signed-out.

WilliamBirdsall commented 4 weeks ago

Sounds good!

aduth commented 3 weeks ago

@WilliamBirdsall Can you link to the ticket if we're planning to reintroduce the session extending behavior?

WilliamBirdsall commented 3 weeks ago

@WilliamBirdsall Can you link to the ticket if we're planning to reintroduce the session extending behavior?

For sure. Writing that up today!

WilliamBirdsall commented 3 weeks ago

Going to link the new ticket here once its ready, but going to merge this in for now.