LG-4001: Fix async verify step alert flashes not being shown

[aduth]

Related (optionally blocked by): #4553

Why: Redirects from JavaScript-based form-steps-wait behavior will cause flash messages to be dropped. To make "sticky", use existing flow_session error message for error messages, and statically render success on following phone step view.

Why: As a user, I expect that I am informed when the server encounters an unhandled error, so that I can retry my request later, and am not left confused by an infinitely-spinning spinner.

[aduth]

This is closer to being ready. One new challenge I'm encountering is that we can't use the flow_session workaround for the phone async button, since it's technically outside the flow, and as such flow_session is not available.

identity-idp/app/controllers/idv/phone_controller.rb

Line 20 in e82740a

flash[:info] = I18n.t('idv.failure.timeout')

[aduth]

Quick update: To better support the phone validation errors, I'm looking to try experimenting with an implementation like one considered earlier on, which is to try to parse the redirected response HTML to discover if any alerts are included in the markup and to show those in the current form (building on renderError).

[aduth]

Quick update: To better support the phone validation errors, I'm looking to try experimenting with an implementation like one considered earlier on, which is to try to parse the redirected response HTML to discover if any alerts are included in the markup and to show those in the current form (building on renderError).

Implemented in ec5d505.

Still need to test in the phone step. Here's how it looks in the preceding verify step:

[zachmargolis]

Visually, I was thinking we'd put the error message at the top of the page like our other flashes? But functionality-wise this LGTM

[zachmargolis]

What about the null-safe navigator? Or is that different because it returns undefined instead of null?

Suggested change

	return textContent ? textContent.trim() : null;
	return textContent?.trim();

[aduth]

What about the null-safe navigator? Or is that different because it returns undefined instead of null?

Yeah, your suggestion can work, but as you mention, it returns undefined. Purely for the sake of keeping the return type to string or null, the ternary is used.

It could be simplified even further if we'd not mind a return type of string?=:

return dom.querySelector(selector)?.textContent?.trim();

[zachmargolis]

It could be simplified even further if we'd not mind a return type of string?=:

That works for me!

[aduth]

Visually, I was thinking we'd put the error message at the top of the page like our other flashes? But functionality-wise this LGTM

Yeah, I'd had the same expectation. It was made difficult by the fact that button_to creates the form specific for that button, and since ideally FormStepsWait doesn't know anything outside the form its bound to, placing the alert elsewhere on the page isn't quite so straight-forward. In addition to that, being adjacent to the button has some advantages as far as being colocated to the button which triggered the action.

That being said, maybe we could create an option for the target element container for notices, so we could do something like:

<div id="form-wait-steps-alert"></div>
<!-- ...Page contents... -->
<%= button_to(form: { data: { error_alert_target: 'form-wait-steps-alert' } }) %>

[zachmargolis]

That being said, maybe we could create an option for the target element container for notices...

I like that option! Could add in a follow-up PR if you wanted to merge this one sooner

[aduth]

Added alert target support in a88ea88.

---

Still need to test in the phone step.

Couple issues here:

1. Whoops, I'd left the polling request as method: 'HEAD', so it would never be able to parse a notice from the response in asynchronous polling 😬 Fixed in 9657fe3.
2. More tricky: Phone verification doesn't have a dedicated "wait" step, at least by URL, so this logic block will never be hit: https://github.com/18F/identity-idp/blob/a88ea881c4c73693e749f0ef3a6c1328579a7c13/app/javascript/packs/form-steps-wait.jsx#L89

It's kinda difficult to have generic logic know when a step is "done":

1. Changes URL? Works for phone verification, but since /verify has separate /verify_wait step, not enough.
   a. Maybe differentiate by _wait suffixing convention as considered "in the same step"
2. Alert type in response (success vs. error)? Seems maybe delicate to rely so heavily on a detail of the view of a next step
3. Change status code of failure responses.
   b. This is probably the "best" option, but more work than 1a

[zachmargolis]

2. More tricky: Phone verification doesn't have a dedicated "wait" step, at least by URL, so this logic block will never be hit:

I could be misunderstanding, but what if we detect isWaitStep by to scraping the body of the response for a meta[http-equiv="refresh"] tag? (And I think that meta tags are simple enough we could regex for it instead of having to parse the whole response into the DOM if we wanted, or I guess we're already doing that for the alert tags so 🤷 )

I was looking into ways to maybe add an HTTP header we could key off of....but unfortunately because we use content_for in these templates, we don't set the values until we start to render, and we can't set HTTP headers after we start rendering

[aduth]

I could be misunderstanding, but what if we detect isWaitStep by to scraping the body of the response for a meta[http-equiv="refresh"] tag? (And I think that meta tags are simple enough we could regex for it instead of having to parse the whole response into the DOM if we wanted, or I guess we're already doing that for the alert tags so 🤷 )

Yeah, that could work as well.

I'd started chipping away at this a bit earlier, where I was originally thinking something along the lines of (pseudo-code):

isRedirectedToNextStep = (redirectPath) => 
  redirectPath !== location.pathname && 
  redirectPath !== location.pathname + '_wait'

[aduth]

I could be misunderstanding, but what if we detect isWaitStep by to scraping the body of the response for a meta[http-equiv="refresh"] tag?

After some tinkering, ultimately I think this will be the most reliable check. And as you mention, since we've got the precedent for scraping the response, it's not a far stretch to implement. Updated in 4aaa63d.

Only thing remaining here is that I'd like to have some more JS test coverage in rspec feature tests for the flow.

[zachmargolis]

LGTM

[zachmargolis]

this is always success because took this out of verify_wait_step_show right?

[aduth]

this is always success because took this out of verify_wait_step_show right?

Right, instead of assigning by flash, we statically show the alert on the following step. This makes an assumption that the only case for reaching this view is via successful completion of the verify step.

[zachmargolis]

maybe we could just delete all rows here instead of stubbing?

[aduth]

maybe we could just delete all rows here instead of stubbing?

Seems like it'd be an improvement, but in trying this out, this triggers a different behavior, because it seems to kill off the session value and cause a none result instead of a timed_out result.

identity-idp/app/services/idv/phone_step.rb

Lines 22 to 25 in e1be6c2

	dcs_uuid = idv_session.idv_phone_step_document_capture_session_uuid
	dcs = DocumentCaptureSession.find_by(uuid: dcs_uuid)
	return ProofingSessionAsyncResult.none if dcs_uuid.nil?
	return timed_out if dcs.nil?

Another option (probably more ideal anyways) would be to remove the Redis entries loaded here:

identity-idp/app/services/idv/phone_step.rb

Lines 27 to 28 in e1be6c2

	proofing_job_result = dcs.load_proofing_result
	return timed_out if proofing_job_result.nil?

But that doesn't appear to be directly exposed.

This code was also partly inherited from the verify step specs, where the mock is restored to simulate an eventual success. Not sure we care to have something similar here, though it'd be made difficult if we outright deleted those entries.

[zachmargolis]

Ok good point, likely not worth it, probably fine to continue stubbing