-
Notifications
You must be signed in to change notification settings - Fork 166
LG-3605: Remove doc_success step from doc auth flow #4368
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| class DropDocSuccessViewFromDocAuthLogs < ActiveRecord::Migration[5.2] | ||
| def up | ||
| safety_assured do | ||
| remove_column :doc_auth_logs, :doc_success_view_at | ||
| remove_column :doc_auth_logs, :doc_success_view_count | ||
| end | ||
| end | ||
|
|
||
| def down | ||
| add_column :doc_auth_logs, :doc_success_view_at, :datetime | ||
| add_column :doc_auth_logs, :doc_success_view_count, :integer, default: 0 | ||
| end | ||
| end |
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -41,9 +41,6 @@ def verify_identity_with_doc_auth | |
| click_on 'Continue' | ||
| expect(page).to have_current_path('/verify/doc_auth/verify') | ||
|
|
||
| click_on 'Continue' | ||
| expect(page).to have_current_path('/verify/doc_auth/doc_success') | ||
|
Comment on lines
-44
to
-45
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just ran into a smoke test flake and I'm very glad we're updating this here :]
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. 😄 |
||
|
|
||
| click_on 'Continue' | ||
| expect(page).to have_current_path('/verify/phone') | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_atcorresponds to the CAC flow success step, which is not intended to be removed with these changes†.So it might instead need to stay as...
† 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Restored this for
success_view_atin 0857bcc, so we're only disregarding the now-removeddoc_success_view_atcolumn.Could still do for a second set of eyes (@stevegsa or yourself) to confirm though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 😬
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @stevegsa . If I read correctly, it sounds like we'll want to (or are at least covered in reporting) to remove both the
DocSuccessStepandCac::Successsteps. 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.