-
Notifications
You must be signed in to change notification settings - Fork 166
LG-12159: Refactor selfie_success to selfie_status and make it return a symbol #9953
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
Changes from all commits
f2f2f44
fd30f4a
2c437fd
9ec4c57
032a7a5
0411d6a
43eff2c
26d53f5
ddd5443
7341f69
594ca5a
9f65bd0
9c79595
d261e54
b7e797a
3dcfc7e
1a6c532
d88acb2
19da555
d964cb9
169c5c6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -60,4 +60,10 @@ def cancel_link_text | |
| def desktop_device? | ||
| !BrowserCache.parse(request.user_agent).mobile? | ||
| end | ||
|
|
||
| def selfie_status_from_response(client_response) | ||
| return client_response.selfie_status if client_response.respond_to?(:selfie_status) | ||
|
|
||
| :not_processed | ||
| end | ||
|
Comment on lines
+64
to
+68
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why was this helper added here? this is a very generic helper, but this is a specific method? What if we made a mixin just for this? |
||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,16 +10,20 @@ | |
| :failed_back_image_fingerprints, | ||
| :captured_at, | ||
| :selfie_check_performed, | ||
| :doc_auth_success, :selfie_success, | ||
| :doc_auth_success, :selfie_status, :selfie_success, | ||
| keyword_init: true, | ||
| allowed_members: [:id, :success, :attention_with_barcode, :failed_front_image_fingerprints, | ||
| :failed_back_image_fingerprints, :captured_at, :selfie_check_performed, | ||
| :doc_auth_success, :selfie_success] | ||
| :doc_auth_success, :selfie_status, :selfie_success] | ||
| ) do | ||
| def self.redis_key_prefix | ||
| 'dcs:result' | ||
| end | ||
|
|
||
| def selfie_status | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. similarly , result[:selfie_status] or result['selfie_status'] may have problem. But not critical.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are there any scenarios in which we read
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @eileen-nava , no just for completeness purpose. |
||
| self[:selfie_status].to_sym | ||
| end | ||
|
|
||
| alias_method :success?, :success | ||
| alias_method :attention_with_barcode?, :attention_with_barcode | ||
| alias_method :pii_from_doc, :pii | ||
|
|
||
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.
Is the
respond_to?check how we are managing the 50-50 state?If this code is running, it will be on a new box, so in that context, the
selfie_statusproperty will exist no matter what. I think we need to check the return values of the properties (because we really want to check the keys of the serialized JSON but that's hidden at this layer) so all we can see is which values arenilThere 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.
No, it is not.
I referred to the existing documentation for renaming a field in a data structure in Redis, which states that in the first deploy, you add the new name and write to the new name everywhere the old name is written. I added
selfie_successback toDocumentCaptureSessionResultin this commit and we will remove it when working on this ticket LG-12200Please let me know if a different approach would be preferable.
Also,
selfie_status_from_responsetakes aResponseobject likeDocAuth::ResponseorMock::ResultResponse. I thought the greatest risk around a 50/50 state error for this PR related to the change of a session field?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.
ah yup that's right, I lost track of which objects were deserialized from redis and which were just in-memory