LG-12994: Fix order of errors displayed when there are multiple errors#10450
Merged
night-jellyfish merged 16 commits intomainfrom Apr 24, 2024
Merged
Conversation
This change:
- moves some test setup to before all / after all
- this is so we can reuse the user and other vars we do not change in
this file and do not care about
- the one drawback is that currently if you ctrl + c twice out of this
test, the `after(:all)` block will not run and you will end up with a
user hanging out in the db that will not be deleted
- we discussed fixing this in this test, but decided it wasn't a
good time. If folks agree we can create a ticket to come back to
this work
- there is no issue if you let the test finish on its own, or if you
only ctrl + c once, letting it clean up and run the `after` block
- pressing it twice skips the `after` block, which is how it ends
up in this broken state
- adds a lot of test fixture files
- we needed a lot of different setup files in order to test the
various combinations
Ideally, instead of having each one of these tests be described a
comment, it would have its own `it` block. Unfortunately right now the
setup would demand running through all the setup steps if we were to do
that, making it very slow.
Instead it is harder to read, but a nice future refactor would let us
pull out the setup in something like a `before(:context)`, allowing us
have proper `it` blocks and change what we need without repetition.
Co-authored-by: Brittany Greaner <brittany.greaner@gsa.gov>
Co-authored-by: Eileen McFarland <eileen.mcfarland@gsa.gov>
Previous to this change we did not have this logic and relied on the `else` case to display the heading for doc auth errors. This meant if we had both a doc auth error and a selfie error, it would catch the selfie error first and display the selfie error `h1` instead. This change adds a new `result_code_invalid?` method to the `image_upload_response_presenter`. Passing this through to the front end allows us to use its logic to determine which `h1` to display in the `document-capture-warning` component when we have a doc auth error. Co-authored-by: Brittany Greaner <brittany.greaner@gsa.gov> Co-authored-by: Eileen McFarland <eileen.mcfarland@gsa.gov>
Previous to this change we did not have this logic and relied on the `else` case to display the body copy for doc auth errors. This meant if we had both a doc auth error and a selfie error, it would catch the selfie error first and display the selfie error body copy instead. This change: - updates the error_generator logic to take doc auth errors into account - previously we were letting the doc auth errors fall all the way through to the `AlertErrorHandler`, that was then handling doc auth, pii, and unknown errors - we introduced a new `DocAuthErrorHandler` that we could call before selfie errors and took much of the logic out of `AlertErrorHandler` - we renamed `AlertErrorHandler` to `UnknownErrorHandler` to better reflect what it does - I also needed to update the default error messages to display what we expect [as represented in the figma](https://www.figma.com/file/YejFLjUrlzZNjjM8McoudH/MVP%3A-Selfie-UX?type=design&node-id=802-5999&mode=design&t=IgxmyFvrKsLZDqZl-0) - it was tricky getting the attention without barcode case right - this resulted in some of the changes in the next commit, about the misc other changes needed for this work. Co-authored-by: Brittany Greaner <brittany.greaner@gsa.gov> Co-authored-by: Eileen McFarland <eileen.mcfarland@gsa.gov> Co-authored-by: Dawei Wang <dawei.wang@gsa.gov>
This change: - updates the mock proofer (and spec) to not delete the attention with barcode message - having the mock proofer delete the attention with barcode error message made it impossible to test - update TrueID specs - these began failing with the change to the default liveness general error - fixes an empty space in one of the yml files - this was a strange one! [See this Slack comment for why](https://gsa-tts.slack.com/archives/C05HSH9RQ57/p1713286668833529).
…splayed when there are multiple errors
charleyf
reviewed
Apr 17, 2024
Contributor
charleyf
left a comment
charleyf
left a comment
There was a problem hiding this comment.
Posting a partial review as I work through understanding these changes. I haven't read through these parts yet:
- specs
- DocAuthErrorHandler
- SelfieErrorHandler
charleyf
reviewed
Apr 19, 2024
Contributor
charleyf
left a comment
charleyf
left a comment
There was a problem hiding this comment.
Some questions about tests.
Contributor
|
Manually testing, need more time. |
dawei-nava
reviewed
Apr 19, 2024
…ess-plus-doc-auth-errors
amirbey
reviewed
Apr 19, 2024
Co-authored-by: Amir Reavis-Bey <amir.reavis-bey@gsa.gov>
amirbey
reviewed
Apr 22, 2024
dawei-nava
reviewed
Apr 23, 2024
dawei-nava
reviewed
Apr 23, 2024
dawei-nava
reviewed
Apr 23, 2024
dawei-nava
reviewed
Apr 23, 2024
charleyf
reviewed
Apr 23, 2024
Contributor
There was a problem hiding this comment.
Went through this PR at a high level again this morning and I think the next steps are:
- Brittany's addressing Dawei and Amir's comments.
- After that around I'm ready to approve
- Note: It doesn't really make sense to test this in dev because part of the problem here is that we only saw the original issue in staging.
- After approval/merge we should manually test as many scenarios as we can (and recruit Kelli's help). I suspect we'll find things to fix which we can address in followup PRs.
Contributor
Author
There was a problem hiding this comment.
That's a good point about dev vs staging. Thanks for reviewing it multiple times, I appreciate it!
Contributor
|
Looks like when testing doc auth error, it does not show the "Try take new picture" subheader?
|
dawei-nava
approved these changes
Apr 24, 2024
Contributor
dawei-nava
left a comment
dawei-nava
left a comment
There was a problem hiding this comment.
Approving.
We do have some issues which are addressed by other tickets or out of scope.
…ess-plus-doc-auth-errors
Contributor
Author
|
@dawei-nava - it looks like it is intentional to not show the subheader if the doc auth status is "Failed". But we are checking on the subheader logic per today's standup in this Slack thread. If the logic used to display the subheader does not match the expectations from the product side, then we will create a new ticket. |
night-jellyfish
deleted the
brittany/lg-12994-fix-errors-displayed-liveness-plus-doc-auth-errors
branch
2 tasks
night-jellyfish
pushed a commit
that referenced
this pull request
May 17, 2024
This reverts a change I made in #10450 that turned out to be the wrong change.
night-jellyfish
pushed a commit
that referenced
this pull request
May 20, 2024
* Add and update specs to reflect doc + add ymls This adds specs for 1. when there are multiple doc auth errors on front and back 2. when there are multiple front doc auth errors 3. when there are multiple back doc auth errors It also updates the previously existing tests to check for the rate limit message, as when I was working on the rate limit message change I found sometimes I had the message showing twice or the wrong message. So I wanted to make sure that the correct message was showing in those tests as well. * Restore previous default error message This reverts a change I made in #10450 that turned out to be the wrong change. * Show subheading whenever IPP and !failedResult Our previous logic for the subheading was too strict and didn't show when selfie had errors. But really we want this h2 to show whever there is a corresponding h2 in the ipp section to balance out the html structure and make it more accessible. * Simplify rate limit message logic After discussing this in the error doc, we found that we: - always want the rate limit message to show - do not know the logic behind showing one message versus the other, and would like to opt for the one that is more clear To that end, I moved the logic for the rate limit message up a level and out of so many conditionals. This ended up being a bigger change than I expected as so many tests relied on the place where it was previously being called, but I think it simplifies the code. * Update test helper for mobile ipp selfie flow Recently this text was changed, and when trying to use the same test helper as in previous specs, it wouldn't work. This change updates the helper to be able to navigate the features enabled. * Rm unusued string from translations Because we decided to consolidate and only use one of the rate limit messages, we no longer use this string. * changelog: User-Facing Improvements, Doc Auth, Fix error messaging for case of multiple errors --------- Co-authored-by: Amir Reavis-Bey <amir.reavis-bey@gsa.gov>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🎫 Ticket
LG-12994
🛠 Summary of changes
When we get multiple errors, we want to display them in the following order:
We never want to display the copy for multiple kinds of errors at the same time.
As an example, this means if we get a doc auth error and a face match error, we want the end user to only see a doc auth error message.
We had expected that the LexisNexis API would not return more than one error, and so for both the
h1and the body copy, we were filtering out other kinds of errors and then letting the doc auth errors be handled by theelsecase.But as we test we are finding that the API is returning multiple errors (like doc auth + face match in the above example). Because our code was handling these in the
elseblock rather than proactively, in the case above, we'd show an h1 for the selfie error and body copy for the doc auth error. It became even more complicated withattentionresults, where the body copy would also be wrong.The change here adds logic to move catching the doc auth errors and return them before we look at selfie errors. The logic for displaying the
h1is in a different place than the logic for the body copy, so this ended up being a rather large change. We also wanted to add much more robust testing so that we can catch any changes in the future.This is likely easiest to review in commits:
Note: inline errors will be handled in LG-12999.
📜 Testing Plan
Here is a Figma file you need for testing.
In each of the below situations, upload a yml file fitting the option, and compare it against the figma linked above on the review issues page.
For this ticket, we are focused on the h1 and body copy.
👀 Screenshots
Doc auth failure + liveness errors
Before:
After:
Doc auth attention + face match errors
Before:
After:
Co-authored-by: Brittany Greaner brittany.greaner@gsa.gov
Co-authored-by: Eileen McFarland eileen.mcfarland@gsa.gov
Co-authored-by: Dawei Wang dawei.wang@gsa.gov