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

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

Merged
merged 1 commit into from
Oct 9, 2024
Merged

Conversation

WilliamBirdsall
Copy link
Contributor

@WilliamBirdsall WilliamBirdsall commented Sep 26, 2024

🎫 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

  • Step 1: Login or create a new account via the Sinatra app and begin IPP flow
  • Step 2: Confirm that there is no 404 error in the network tab/console on the PO Search page

@WilliamBirdsall WilliamBirdsall marked this pull request as draft September 26, 2024 17:09
@WilliamBirdsall
Copy link
Contributor Author

WilliamBirdsall commented Sep 26, 2024

Notes:

  • There may be a better way to do this instead of the throwaway noop function. The DocumentCapture component already has a noop as default for onStepChange so there may be a way to use this...
  • Must ensure that extendSession still works for other flows (including hybrid). Is optedInToInPersonProofing true during hybrid? If so the conditional needs to be reworked.

@WilliamBirdsall WilliamBirdsall force-pushed the will/LG-14545 branch 3 times, most recently from 5d5bf65 to 850f308 Compare October 3, 2024 13:59
@@ -205,7 +205,7 @@ const App = composeComponents(
[
DocumentCapture,
{
onStepChange: extendSession,
onStepChange: optedInToInPersonProofing === 'true' ? () => null : extendSession,
Copy link
Contributor

Choose a reason for hiding this comment

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

That looks exactly right to me, after poking around a bit.

@WilliamBirdsall WilliamBirdsall marked this pull request as ready for review October 4, 2024 13:43
@WilliamBirdsall
Copy link
Contributor Author

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

@aduth
Copy link
Member

aduth commented Oct 4, 2024

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
Copy link
Contributor Author

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
Copy link
Member

aduth commented Oct 4, 2024

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

@WilliamBirdsall
Copy link
Contributor Author

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
Copy link
Member

aduth commented Oct 7, 2024

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
Copy link
Contributor Author

Sounds good!

@aduth
Copy link
Member

aduth commented Oct 8, 2024

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

@WilliamBirdsall
Copy link
Contributor Author

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

For sure. Writing that up today!

@WilliamBirdsall
Copy link
Contributor Author

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

@WilliamBirdsall WilliamBirdsall merged commit 6c2bf3c into main Oct 9, 2024
2 checks passed
@WilliamBirdsall WilliamBirdsall deleted the will/LG-14545 branch October 9, 2024 17:04
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.

6 participants