-
Notifications
You must be signed in to change notification settings - Fork 166
LG-11012: redo document capture bug fixes #9256
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
14504d5
a57035b
4cad8df
9fc9da6
b7b48e6
a6ca7e6
1e550eb
7cfd1bc
540e87e
7c31b32
4545aea
e2740b6
e031646
6ed1278
8e0e912
d74080d
2b2166c
b69c16b
62dd008
7c25830
f7da966
0fd5164
55f6482
a1df755
862cc5d
e3a9312
42b9eda
ca3f12e
3ec054a
7266674
4f0ba64
2293105
bf1f93a
34385da
9b0447e
637ab57
246b239
37e0b83
33356f3
4668cc1
e3ee502
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 |
|---|---|---|
|
|
@@ -8,9 +8,10 @@ | |
| :attention_with_barcode, | ||
| :failed_front_image_fingerprints, | ||
| :failed_back_image_fingerprints, | ||
| :captured_at, | ||
|
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. Is there any issue with existing document_capture_sessions in redis not having this attribute?
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. great catch ... added dig(:captured_at) to check if it exists before evaluating 👍🏿 |
||
| keyword_init: true, | ||
| allowed_members: [:id, :success, :attention_with_barcode, :failed_front_image_fingerprints, | ||
| :failed_back_image_fingerprints], | ||
| :failed_back_image_fingerprints, :captured_at], | ||
| ) do | ||
| def self.redis_key_prefix | ||
| 'dcs:result' | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
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 like this definition, very practical. "Have we done this already?" I think it will be resilient as we enable the back button, too.
Would it make sense to define it as
DocumentCaptureSession#redo?(captured_at:)to avoid repeating the definition in DocumentCaptureController ?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.
yes i wanted to do that. however, this line is the blocker ... do you knowi is this load_async still necessary? 🤔
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.
Don't know off-hand. @aduth might know if it's part of the doc auth async code. Happy to dig in with you if that would be helpful.
Uh oh!
There was an error while loading. Please reload this page.
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.
Yes, I think the
load_doc_auth_async_resultis related to the now-defunct (as of #8377) async document capture code. So it would probably be safe to remove all of that.Uh oh!
There was an error while loading. Please reload this page.
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.
thanks @aduth ... after briefly testing out the removal of
load_doc_auth_async_result.. i think it'd be best to remove it in a follow up PR