-
Notifications
You must be signed in to change notification settings - Fork 1
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
Fix #484: Final document verification #489
Conversation
f724afb
to
edf2393
Compare
...arding-common/src/main/java/com/wultra/app/onboardingserver/common/service/AuditService.java
Show resolved
Hide resolved
...g/src/main/java/com/wultra/app/onboardingserver/docverify/zenid/config/ZenidConfigProps.java
Show resolved
Hide resolved
final OwnerId ownerId) { | ||
documentVerifications.forEach(docVerification -> | ||
auditService.audit(docVerification, "Document accepted at final verification for user: {}", identityVerification.getUserId())); | ||
identityVerificationService.moveToPhaseAndStatus(identityVerification, DOCUMENT_VERIFICATION_FINAL, ACCEPTED, ownerId); |
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.
Not introduced as a separate state of state machine configuration yet. Added just for auditing reasons. For sync processing, this step makes sense to be done all at once. Should be added to the diagram.
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.
Ok.
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 assume this may change with async processing, right?
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.
This pull request should work as expected for common scenarios. There are some corner cases, I raised issues for them and left one for review. Let's proceed with testing and handle any additional changes based on test results as another pull request.
...arding-common/src/main/java/com/wultra/app/onboardingserver/common/service/AuditService.java
Show resolved
Hide resolved
...g/src/main/java/com/wultra/app/onboardingserver/docverify/zenid/config/ZenidConfigProps.java
Show resolved
Hide resolved
final OwnerId ownerId) { | ||
documentVerifications.forEach(docVerification -> | ||
auditService.audit(docVerification, "Document accepted at final verification for user: {}", identityVerification.getUserId())); | ||
identityVerificationService.moveToPhaseAndStatus(identityVerification, DOCUMENT_VERIFICATION_FINAL, ACCEPTED, ownerId); |
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.
Ok.
final OwnerId ownerId) { | ||
documentVerifications.forEach(docVerification -> | ||
auditService.audit(docVerification, "Document accepted at final verification for user: {}", identityVerification.getUserId())); | ||
identityVerificationService.moveToPhaseAndStatus(identityVerification, DOCUMENT_VERIFICATION_FINAL, ACCEPTED, ownerId); |
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 assume this may change with async processing, right?
@@ -125,9 +125,11 @@ public void processClientEvaluation(final IdentityVerificationEntity identityVer | |||
|
|||
private static String getVerificationId(final IdentityVerificationEntity identityVerification) { | |||
return identityVerification.getDocumentVerifications().stream() | |||
.filter(DocumentVerificationEntity::isUsedForVerification) | |||
.filter(it -> it.getStatus() == DocumentStatus.ACCEPTED) | |||
.findAny() |
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.
This check is still suboptimal. We rather want to executed findAll()
, put all results into a Set
and make sure this set has only one item which is the ID of verification we want to use. In case of multiple IDs found, this is a severe error in business logic and we want to abort the processing. I will raise an issue for future improvement request.
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.
...g/src/main/java/com/wultra/app/onboardingserver/docverify/zenid/config/ZenidConfigProps.java
Show resolved
Hide resolved
reject(identityVerification, result, documentVerifications, ownerId); | ||
break; | ||
case IN_PROGRESS: | ||
throw new DocumentVerificationException("Only sync mode is supported, " + ownerId); |
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.
See: #495
.../java/com/wultra/app/onboardingserver/impl/service/document/DocumentVerificationService.java
Outdated
Show resolved
Hide resolved
.map(DocumentVerificationEntity::getUploadId) | ||
.collect(toList()); | ||
|
||
final DocumentsVerificationResult result = documentVerificationProvider.verifyDocuments(ownerId, uploadIds); |
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.
In case of an error during verifyDocuments
call the identity verification may get stuck in this phase/status. Let's raise a ticket for handling this separately.
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.
Raised as: #496
No description provided.