Skip to content
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 #311: Improve detection of failed presence check #315

Merged
merged 9 commits into from
Aug 19, 2022

Conversation

romanstrobl
Copy link
Member

@romanstrobl romanstrobl commented Aug 17, 2022

This pull requests adds an error_origin column which is used for auditing purposes as well as for checking overall OTP verification result during SCA step (failure due to presence check error).

DDL update:

alter table es_document_result add column error_origin VARCHAR(256);
alter table es_document_verification add column error_origin VARCHAR(256);
alter table es_identity_verification add column error_origin VARCHAR(256);
alter table es_onboarding_otp add column error_origin VARCHAR(256);
alter table es_onboarding_process add column error_origin VARCHAR(256);

alter table es_document_result add column reject_origin VARCHAR(256);
alter table es_document_verification add column reject_origin VARCHAR(256);
alter table es_identity_verification add column reject_origin VARCHAR(256);

@romanstrobl romanstrobl added the enhancement New feature or request label Aug 17, 2022
@romanstrobl romanstrobl self-assigned this Aug 17, 2022
Copy link
Contributor

@saalistaja saalistaja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice addition of a new field to track error/rejection origin.

I am just not sure with the shared naming, used for rejectReason too.

Furthermore rhere seems to be left several places in the code with setRejectReason but without setting an origin, is this desired?

@romanstrobl
Copy link
Member Author

Ok, we can extend the logic for the sake of completeness:

  • add reject_origin column
  • always set reject_origin when setting reject_reason
  • always separate handling for rejection and errors

@romanstrobl romanstrobl requested a review from saalistaja August 19, 2022 10:24
@romanstrobl
Copy link
Member Author

@saalistaja Please check the changes now. We will also need to merge the changes with state machine logic, I can try to do it, but I think it will be easier for you?

Copy link
Contributor

@saalistaja saalistaja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last round of review. I am not sure with one condition.

final String rejectReason = idVerification.getRejectReason();

if (StringUtils.isNotBlank(errorDetail) || StringUtils.isNotBlank(rejectReason)) {
if (errorOrigin == ErrorOrigin.PRESENCE_CHECK && (StringUtils.isNotBlank(errorDetail) || StringUtils.isNotBlank(rejectReason))) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we fix this to
(errorOrigin == ErrorOrigin.PRESENCE_CHECK && StringUtils.isNotBlank(errorDetail)) || (rejectOrigin == ErrorOrigin.PRESENCE_CHECK && StringUtils.isNotBlank(rejectReason)) ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, true.

romanstrobl and others added 6 commits August 19, 2022 13:14
- Refactored previous approach to spring state machine
- Mapped current phase, status to new onboarding state enums
- Transitions triggered by simple events from controllers and one technical event for auto-transition on status check
@saalistaja saalistaja self-requested a review August 19, 2022 11:52
Copy link
Contributor

@saalistaja saalistaja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well done!

@romanstrobl romanstrobl merged commit 0af1131 into develop Aug 19, 2022
@romanstrobl romanstrobl deleted the issues/311-failed-presence-check-detection branch August 19, 2022 11:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants