Skip to content

CL-8938 - fix idv_document_upload_submitted event#7918

Merged
n1zyy merged 4 commits intomainfrom
mattw/LG-8938_bug
Mar 3, 2023
Merged

CL-8938 - fix idv_document_upload_submitted event#7918
n1zyy merged 4 commits intomainfrom
mattw/LG-8938_bug

Conversation

@n1zyy
Copy link
Contributor

@n1zyy n1zyy commented Mar 2, 2023

🎫 Ticket

Found while working on LG-8938.

🛠 Summary of changes

Currently, these events are only logging events that are the results of a form validation failures, but not to vendor errors.

There are actually multiple forms so we missed some events.

n1zyy added 2 commits March 2, 2023 16:54
Vendor responses were not being captured due to where we were calling the tracker.
changelog: Internal, Attempts API, Bugfix for doc auth events
@n1zyy n1zyy requested a review from a team March 2, 2023 22:06
)

# This is the last upload which triggers the rate limit, apparently.
# I do find this moderately confusing.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Someone want to check my logic here? I wasn't expecting both events initially, but that seems to be what happens with analytics events.

Want to make sure I'm enshrining a new bug in tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think there's the attempt, and then that triggers the limit? I think that's fine

document_issued: nil,
document_number: nil,
document_state: nil,
failure_reason: { front: ['The selection was not a valid file.'] },
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Opinion question: I think the academically-correct thing to do here would be to re-use the front: [I18n.t('doc_auth.errors.not_a_file')] that's set up above, maybe moving it to a little let(:error_msg) assignment.

I have a slight preference for using the actual English string here, though, to better highlight the unusual (compared to other events) behavior where we are sending the actual translated string across. I think this makes it clearer.

Anyone feel strongly one way or the other?

Copy link
Contributor

Choose a reason for hiding this comment

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

i think it's fine to hardcode the string like this, makes it easiest to change if something else changes

Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines +303 to +316
success: response.success?,
document_state: pii_from_doc[:state],
document_number: pii_from_doc[:state_id_number],
document_issued: pii_from_doc[:state_id_issued],
document_expiration: pii_from_doc[:state_id_expiration],
document_front_image_filename: stored_image_result&.front_filename,
document_back_image_filename: stored_image_result&.back_filename,
document_image_encryption_key: stored_image_result&.encryption_key,
first_name: pii_from_doc[:first_name],
last_name: pii_from_doc[:last_name],
date_of_birth: pii_from_doc[:dob],
address: pii_from_doc[:address1],
failure_reason: response.errors&.except(:hints)&.presence,
)
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto here (also the last closing paren needs to move)

Suggested change
success: response.success?,
document_state: pii_from_doc[:state],
document_number: pii_from_doc[:state_id_number],
document_issued: pii_from_doc[:state_id_issued],
document_expiration: pii_from_doc[:state_id_expiration],
document_front_image_filename: stored_image_result&.front_filename,
document_back_image_filename: stored_image_result&.back_filename,
document_image_encryption_key: stored_image_result&.encryption_key,
first_name: pii_from_doc[:first_name],
last_name: pii_from_doc[:last_name],
date_of_birth: pii_from_doc[:dob],
address: pii_from_doc[:address1],
failure_reason: response.errors&.except(:hints)&.presence,
)
success: response.success?,
document_state: pii_from_doc[:state],
document_number: pii_from_doc[:state_id_number],
document_issued: pii_from_doc[:state_id_issued],
document_expiration: pii_from_doc[:state_id_expiration],
document_front_image_filename: stored_image_result&.front_filename,
document_back_image_filename: stored_image_result&.back_filename,
document_image_encryption_key: stored_image_result&.encryption_key,
first_name: pii_from_doc[:first_name],
last_name: pii_from_doc[:last_name],
date_of_birth: pii_from_doc[:dob],
address: pii_from_doc[:address1],
failure_reason: response.errors&.except(:hints)&.presence,
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm actually not sure how this happened—I don't do that either! 😆 I think my editor tried to get fancy when I let it automatically "refactor" this block into a method (really just a cut-and-paste).

n1zyy and others added 2 commits March 2, 2023 17:32
Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>
@n1zyy n1zyy merged commit aa5d45b into main Mar 3, 2023
@n1zyy n1zyy deleted the mattw/LG-8938_bug branch March 3, 2023 17:27
@jmdembe jmdembe mentioned this pull request Mar 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants