Skip to content

LG-6399 Update react app to use new API requests#6536

Merged
aduth merged 14 commits intomainfrom
lg-6399-nprimak-use-new-api
Jul 7, 2022
Merged

LG-6399 Update react app to use new API requests#6536
aduth merged 14 commits intomainfrom
lg-6399-nprimak-use-new-api

Conversation

@nprimak
Copy link
Contributor

@nprimak nprimak commented Jun 28, 2022

LG-6399

Why
Finishing up the api work that @peggles2 started with this PR . Hooking up api to react app for async document capture.

Comment on lines 77 to 79
Copy link
Contributor

Choose a reason for hiding this comment

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

TIL that we had previously been creating a distinct DocumentCaptureSession record for the document capture async verification:

create_document_capture_session(verify_document_capture_session_uuid_key)

This is not the same record as the one we populate into the document capture page and attach to all its API requests, which is the one created at the first welcome step:

create_document_capture_session(document_capture_session_uuid_key)

document_capture_session_uuid: flow_session[:document_capture_session_uuid],

const formData = {
document_capture_session_uuid: appRoot.getAttribute('data-document-capture-session-uuid'),
locale: document.documentElement.lang,
};

I'm assuming this was intentional, though I'm not really familiar with the reasoning. And if we do want to continue doing creating distinct records, I've a few thoughts:

  1. Was it not an issue / inconsistency that the hybrid mobile would be using the welcome step session?
  2. Maybe we should create the distinct record when the document capture step is visited, so that we can initialize the view with that one instead of the one created at the welcome step?

cc @zachmargolis @stevegsa for thoughts / insights (via git blame'd #4313, #4665)

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 the idea was we'd create a separate DocumentCaptureSession recod for each step, since each one would have a separate result_id and we'd minimize the risk of overriding one step with another... I have no memory of how it affects the hybrid flow....

If I was starting from scratch, maybe I'd have one table with like a separate column for resolution_result_uuid, address_result_uuid and document_result_uuid or something but 🤷

Copy link
Contributor

@aduth aduth Jul 5, 2022

Choose a reason for hiding this comment

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

@zachmargolis Thanks. Curious, though: In that framing, what's the purpose of the session created during the welcome step?

I'm starting to dig into restoring the behavior of a distinct "verify document" session, but the more I implement of it, the more it feels we can / should just repurpose the initial session for this, since (a) the welcome session wouldn't otherwise have a "result" that I'm aware of, and (b) to support hybrid, I expect we would need to create the verify result session fairly early anyways (I was looking at "agreement" step) so that it can be referenced in both document_capture and link_sent flows.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm reaching the limits of my memory and my ability to unpack what this was supposed to do....I think the one created at the welcome step was probably meant to be shared across desktop + mobile in the hybrid flow

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, it seems that it's used today for hybrid flow. I guess what I'm proposing is to use that everywhere in place of the distinct "verify document action" session which was used for desktop. That's essentially what this pull request is doing in its current form.

@aduth aduth marked this pull request as ready for review June 29, 2022 16:39
Copy link
Contributor

@solipet solipet left a comment

Choose a reason for hiding this comment

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

LGTM. Was there resolution on the DocumentCaptureSession ?

@aduth aduth force-pushed the lg-6399-nprimak-use-new-api branch from 6883563 to e598f7b Compare July 6, 2022 12:51
nprimak and others added 14 commits July 6, 2022 09:14
Simplification enabled by new API
Previous implementation would create a new DocumentCaptureSession and assign into flow_session . We reuse the one created at the start of the flow.

Alternatively, consider continuing to use "verify_document_capture_session_uuid_key", but create it when the user visits document capture so it's used consistently there.
changelog: Upcoming Features, Identity Verification, Use document capture API for async submission
Match client behavior and updated controller logic of 5ef750833
Checked that it's not referenced anywhere. We could shim this in, but since this is directly tied to FSM "actions" machinery, doesn't seem reasonable.
Previously this had been read from flow_session, now passed as a request parameter
@aduth aduth force-pushed the lg-6399-nprimak-use-new-api branch from e598f7b to 25db0f0 Compare July 6, 2022 13:30
@aduth aduth merged commit b1299df into main Jul 7, 2022
@aduth aduth deleted the lg-6399-nprimak-use-new-api branch July 7, 2022 12:41
@jmdembe jmdembe mentioned this pull request Jul 12, 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.

4 participants