LG-12994: fix messages when there are multiple doc auth errors #10635
Conversation
This file contains 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
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.
This reverts a change I made in #10450 that turned out to be the wrong change.
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.
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.
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.
Because we decided to consolidate and only use one of the rate limit messages, we no longer use this string.
…r case of multiple errors
night-jellyfish
force-pushed
the
brittany/lg-12994-multiple-doc-auth-errors
branch
from
11fb950
to
7ef0b47
Compare
amirbey
reviewed
May 17, 2024
spec/fixtures/ial2_test_credential_multiple_doc_auth_failures_back_side_only.yml
Show resolved
Hide resolved
amirbey
reviewed
May 17, 2024
spec/fixtures/ial2_test_credential_multiple_doc_auth_failures_front_side_only.yml
Show resolved
Hide resolved
…back_side_only.yml Co-authored-by: Amir Reavis-Bey <[email protected]>
…front_side_only.yml Co-authored-by: Amir Reavis-Bey <[email protected]>
charleyf
approved these changes
May 20, 2024
There was a problem hiding this comment.
I've read through the commits, code, and specs.
- The FE/BE code changes look correct to me based on the requirements.
- The spec changes also look correct, though I wasn't able to fully work through all of them. The code changes are simple and supported by the spec changes.
- The scope of doing the manual testing seemed too big to do every option. I tried a few scenarios that would have failed in
mainand they were correct.
| isFailedDocType={isFailedDocType} | ||
| isFailedSelfie={isFailedSelfie} | ||
| isFailedSelfieLivenessOrQuality={isFailedSelfieLivenessOrQuality} | ||
| altIsFailedSelfieDontIncludeAttempts | ||
| altFailedDocTypeMsg={isFailedDocType ? t('doc_auth.errors.doc.wrong_id_type_html') : null} | ||
| hasDismissed={hasDismissed} | ||
| /> | ||
| <p> |
There was a problem hiding this comment.
Always show on the re-upload page 👍
| !isFailedDocType && | ||
| !isFailedSelfieLivenessOrQuality && | ||
| !isFailedSelfie; | ||
| function getSubheading({ nonIppOrFailedResult, t }) { |
There was a problem hiding this comment.
It's nice how much simpler this is.
| /> | ||
| </p> | ||
| )} | ||
| <p> |
There was a problem hiding this comment.
Always show on the interstitial page before going back to the reupload page 👍
| def self.general_error(liveness_enabled) | ||
| liveness_enabled ? Errors::GENERAL_ERROR_LIVENESS : Errors::GENERAL_ERROR | ||
| def self.general_error(_liveness_enabled) | ||
| Errors::GENERAL_ERROR |
There was a problem hiding this comment.
Simplify, only use the GENERAL_ERROR now 👍
aduth
reviewed
May 20, 2024
app/javascript/packages/document-capture/components/unknown-error.tsx
Outdated
Show resolved
Hide resolved
night-jellyfish
added a commit
that referenced
this pull request
May 20, 2024
[skip changelog] A change requested in [10635](#10635 (comment)). We are not changing the general error message when liveness is enabled, so we do not need to pass it in as a parameter.
night-jellyfish
added a commit
that referenced
this pull request
May 22, 2024
A change requested in [10635](#10635 (comment)). We are not changing the general error message when liveness is enabled, so we do not need to pass it in as a parameter. [skip changelog]
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
Adding tests and updating code to match the expectations in our error documentation for conditions where we have multiple failures:
and IPP is enabled.
This is likely easiest to review in commits:
(Each commit has a description with more information about the change)
📜 Testing Plan
👀 Screenshots
Multiple errors for front and back:
Before:
After: