Conversation
…anding. changelog: Internal, Doc Auth, Refactor doc auth error generation.
| unknown_fail_count = scan_for_unknown_alerts(response_info) | ||
|
|
||
| # check whether ID type supported | ||
| error = IdTypeErrorHandler.new.handle(response_info) |
There was a problem hiding this comment.
Is there a reason to keep redefining error? If not, I think it'd be clearer to name this id_error and the next image_error and so on. I think it would improve readability.
There was a problem hiding this comment.
@night-jellyfish , changed. If you think from top down, then there should be no confusion, if you think from bottom up, yes, name them differently will makes it clearer.
| end | ||
| end | ||
| def is_selfie_failure(error) | ||
| error == Errors::SELFIE_FAILURE |
There was a problem hiding this comment.
I think I may not be following, but does this mean is_selfie_failure would return false if SELFIE_POOR_QUALITY or SELFIE_NOT_LIVE?
There was a problem hiding this comment.
@night-jellyfish , renamed the function, basically this is a case that we present a general selfie error.
| end | ||
| end | ||
| end | ||
| def present_generic_selfie_error?(error) |
There was a problem hiding this comment.
| def present_generic_selfie_error?(error) | |
| def is_generic_selfie_error?(error) |
| selfie_error = selfie_error_handler.handle(response_info) | ||
|
|
||
| # if selfie itself is ok, but we have selfie related error | ||
| if selfie_error_handler.present_generic_selfie_error?(selfie_error) |
There was a problem hiding this comment.
| if selfie_error_handler.present_generic_selfie_error?(selfie_error) | |
| if selfie_error_handler.is_generic_selfie_error?(selfie_error) |
i found it a challenge to know what is #present_generic_selfie_error? without looking at the method defintion
| alert_error = AlertErrorHandler.new(config: config, liveness_enabled: liveness_enabled). | ||
| handle(known_alert_error_count, response_info, selfie_error) |
There was a problem hiding this comment.
| alert_error = AlertErrorHandler.new(config: config, liveness_enabled: liveness_enabled). | |
| handle(known_alert_error_count, response_info, selfie_error) | |
| alert_error_handler = AlertErrorHandler.new(config: config, liveness_enabled: liveness_enabled) | |
| alert_error = alert_error_handler.handle(known_alert_error_count, response_info, selfie_error) |
i think this would be more debuggable 🤔
| unknown_fail_count = scan_for_unknown_alerts(response_info) | ||
|
|
||
| # check whether ID type supported | ||
| id_type_error = IdTypeErrorHandler.new.handle(response_info) |
There was a problem hiding this comment.
| id_type_error = IdTypeErrorHandler.new.handle(response_info) | |
| id_type_error_handler = IdTypeErrorHandler.new | |
| id_type_error = id_type_error_handler.handle(response_info) |
i think this is more debuggable 🤔 ... kind of like how you implemented # check selfie error
amirbey
left a comment
amirbey
left a comment
There was a problem hiding this comment.
Big thanks for taking this on Dawei 🙏🏿
I left a few minor suggestions
3 participants
🎫 Ticket
Link to the relevant ticket:
LG-12494
🛠 Summary of changes
Refactoring
ErrorGenerator. By creating POROs with single responsibilities. At top level,ErrorGeneratoris responsible mostly to decide what error to bubble up and what errors to consolidate.📜 Testing Plan
Provide a checklist of steps to confirm the changes.
Existing test cases and manual testing for failure scenarios.