LG-3605: Remove doc_success step from doc auth flow

[aduth]

Why: As a user, I don't want the Social Security number/personal information success step to look like the end of the flow, so that I am not frustrated when I realize I am not done. Instead, I want to see feedback that my personal information was verified and go directly to the next step.

Before	After

This removes the doc_success step of the doc_auth flow, including associated funnel reporting database columns (cc @stevegsa).

The acceptance criteria "Use success alert component in login.gov design system" is not reflected here, due to the fact that IDP flash uses custom alert styling. I've created LG-3711 as a follow-on task.

[aduth]

The codeclimate/total-coverage build failure seems expected and acceptable, since we're largely removing code here.

Or, as @mitchellhenke put it so well last time we encountered similarly:

We reduced the numerator and denominator, so the average is going to go down

[mitchellhenke]

😁

looks good to me!

[mitchellhenke]

Should https://github.com/18F/identity-idp/blob/master/app/services/idv/steps/doc_success_step.rb be deleted?

[aduth]

Should https://github.com/18F/identity-idp/blob/master/app/services/idv/steps/doc_success_step.rb be deleted?

Yeah, it should. Good call 👍

[zachmargolis]

Do we know if any of our reporting consumers rely on this column? Or rely on a specified # of columns?

If so, should we put a 0, -1 or "N/A" there?

[aduth]

Do we know if any of our reporting consumers rely on this column? Or rely on a specified # of columns?

If so, should we put a 0, -1 or "N/A" there?

I'm not totally familiar with how these are used, so I'd defer to others for specifically how they're used. I wasn't able to find many resources on how these reports are used. Do you know if any exist?

Though, as I look at it again, this may need to stay regardless, because success_view_at corresponds to the CAC flow success step, which is not intended to be removed with these changes†.

So it might instead need to stay as...

Suggested change

	count(COALESCE(doc_success_view_at,success_view_at)) as doc_success,
	count(success_view_at) as doc_success,

---

† It seems to serve a very similar purpose to the doc auth success step though, so if we're removing one, we should probably remove the other as well. Not something I'd expect to be covered by the current incarnation of the ticket.

[aduth]

Restored this for success_view_at in 0857bcc, so we're only disregarding the now-removed doc_success_view_at column.

Could still do for a second set of eyes (@stevegsa or yourself) to confirm though.

[zachmargolis]

Unfortunately, I don't have a good sense of how these are used, Chris and Silke had workflows for these but they've left the project so I don't know who looks at them now 😬

[stevegsa]

The metrics will still work for the other steps after doc_success so it's ok to take it out in both instances (cac and doc_auth). I don't think there are any queries where we specifically check doc_success other than just reporting that step in the funnel. After this change we can just remove that step in the funnel. Then the people that make it to phone screen should still be the same but it would necessarily imply that they succeeded the verify step.

[aduth]

Thanks @stevegsa . If I read correctly, it sounds like we'll want to (or are at least covered in reporting) to remove both the DocSuccessStep and Cac::Success steps. Since this ticket only accounted for the first of these, I'll plan to keep everything as-is for the CAC success step, and open a ticket to explore its removal in the future (UX feedback, etc).

Let me know if I've misunderstood or if there are any concerns to do this separately.

[anniehirshman-gsa]

Screenshot looks good! With the caveat that the margin below the success alert should match other success alerts in the flow (it looks like it does from the image, just double-checking). Thanks!

[aduth]

Screenshot looks good! With the caveat that the margin below the success alert should match other success alerts in the flow (it looks like it does from the image, just double-checking). Thanks!

There is a bit of inconsistency here, due to the fact that the alerts we use elsewhere in the IDP are custom (not USWDS). I didn't use the same approach here as with the alert added in #4165 (LG-3267) since there appear to be multiple paths which can lead to this phone screen, some of which need to show their own alerts or error texts (e.g. invalid phone number, phone confirmation failure, etc).

Ideally we update all alerts to use USWDS and eliminate this custom variant, which I hoped to capture in LG-3711.

As a short-term solution, I could imagine one of:

• Use the built-in flash, but customize it in a way which overrides it to use USWDS as a temporary opt-in (flash[:success_uswds] 🤷 )
• Just migrate alerts across the application to use USWDS alerts
• Live with the inconsistency for now, and deal with it as part of LG-3711
• Some other option, like query parameters in the URL when navigating from doc auth verify step to the phone entry

I'll plan to think on this a little more.

[zachmargolis]

Just ran into a smoke test flake and I'm very glad we're updating this here :]

[aduth]

Just ran into a smoke test flake and I'm very glad we're updating this here :]

I don't know that I'd have a ton of confidence that this won't just flake on the next screen, but at least one fewer assertion that can go awry. 😄

[aduth]

As a short-term solution, I could imagine one of:

• Use the built-in flash, but customize it in a way which overrides it to use USWDS as a temporary opt-in (flash[:success_uswds] 🤷 )
• Just migrate alerts across the application to use USWDS alerts
• Live with the inconsistency for now, and deal with it as part of LG-3711
• Some other option, like query parameters in the URL when navigating from doc auth verify step to the phone entry

I created #4375 to correspond to the second option.

The third option is the next best bet, if we'd rather not deal with potential fallout from updating flash alerts across the application.

The other two seem to be hacking around the problem of something we'll eventually want to address anyways (i.e. increase technical debt).

[aduth]

Also, for what it's worth, the notice we're showing here is the same as currently shown in further steps. Which is technically not USWDS (yet, 'til #4375), but at least consistently inconsistent 🤷

[anniehirshman-gsa]

Thanks, this all sounds good! I dug in a bit today, and if the bottom margin is 3 rem here and you're using the existing component from elsewhere, I think that is the most consistent. So this is good to go on my end.

(There may actually be an inconsistency on other success alerts I need to clean up elsewhere, but that's for another day, another PR.)

[aduth]

(There may actually be an inconsistency on other success alerts I need to clean up elsewhere, but that's for another day, another PR.)

Yeah, unfortunately there's inconsistency any way we look at it, since all doc auth flow steps except document capture and the hybrid flow confirmation screen use the custom non-USWDS alerts. The changes here fall in line with those other steps. #4375 can push us closer to having consistent USWDS alerts. It sounds to me like we're fine to leave that as a separate task from this one, but I wanted to make sure to at least get that one prepared to avoid contributing to the technical debt.

[solipet]

lgtm