Skip to content

LG-7096 add Event: IDV Doc auth upload submitted#6820

Merged
olatifflexion merged 2 commits intomainfrom
olatif/LG-7096-idv-doc-upload
Aug 25, 2022
Merged

LG-7096 add Event: IDV Doc auth upload submitted#6820
olatifflexion merged 2 commits intomainfrom
olatif/LG-7096-idv-doc-upload

Conversation

@olatifflexion
Copy link
Contributor

changelog: Internal, Attempts API, Track additional events

changelog: Internal, Attempts API, Track additional events
Copy link
Contributor

@n1zyy n1zyy left a comment

Choose a reason for hiding this comment

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

I'm not too familiar with the proofing process so I might let someone more familiar leave an actual approval.

I left a couple small comments musing about things, which shouldn't be construed as required changes. The code looks excellent.


expect(@irs_attempts_api_tracker).to receive(:track_event).with(
:idv_document_upload_submitted,
success: true,
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 I'm questioning existing logic, not your code, but -- it's valid to have this many fields blank?

@n1zyy n1zyy requested a review from a team August 23, 2022 15:11
@n1zyy
Copy link
Contributor

n1zyy commented Aug 23, 2022

(Adding @18F/identity-agnes as a requested reviewer)

service_provider: current_sp,
analytics: analytics,
uuid_prefix: current_sp&.app_id,
irs_attempts_api_tracker: irs_attempts_api_tracker,
Copy link
Contributor

Choose a reason for hiding this comment

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

There are currently two implementations for document capture image submission. This is the "synchronous" implementation. The asynchronous implementation isn't yet shipped, and is triggered through Api::Verify::DocumentCaptureController. Will we want to log the event there as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not Sure :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe we will want to track the asynchronous implementation as well. imo. if we plan to have it turned on for IRS.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mdiarra3 should we imp async event in this PR and Jira ticket? Or should we create a new ticket for this? What would you suggest?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd do both in the same PR if it's easy

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not an easy :D, it is enqueuing(DocumentProofingJob) background job and there we need to create irs_attempts_api_tracker instance like (analytics = build_analytics(dcs))

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, seems like a good candidate for a follow-up then

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @aduth can you help me with how i can test this asynchronous scenario in my local?

Copy link
Contributor

Choose a reason for hiding this comment

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

Circling back to this -- I'd cast another vote for filing a followup story for later. That lets us get this one closed out, and we can come back to the trickier async event separately. The fact that the async version hasn't shipped feels like further reason to separate it out.

Copy link
Contributor

Choose a reason for hiding this comment

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

@olatifflexion You can add the following to config/application.yml:

doc_auth_enable_presigned_s3_urls: true

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

@olatifflexion olatifflexion merged commit ba899ad into main Aug 25, 2022
@olatifflexion olatifflexion deleted the olatif/LG-7096-idv-doc-upload branch August 25, 2022 14:14
@aduth aduth mentioned this pull request Aug 30, 2022
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.

5 participants