Skip to content
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

lg-14790 handle network failures for socure #11430

Merged
merged 18 commits into from
Nov 7, 2024

Conversation

AShukla-GSA
Copy link
Member

🎫 Ticket

Link to the relevant ticket:
LG-14790

🛠 Summary of changes

Implemented handle_connection_error for socure requests

Copy link
Contributor

@theabrad theabrad left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@amirbey amirbey left a comment

Choose a reason for hiding this comment

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

  1. can we add to DocAuth::Socure::Request
def handle_connection_error
  raise NotImplementedError
end
  1. DocmentRequest.fetch is expecting a hash, so it seems like DocumentRequest#handle_connection_error should return a hash
  2. similar to LN request, it looks like DocAuth::Socure::Request#handle_http_response should return handle_connection_error
  3. can we add testing for the Socure standard and hybrid document capture controllers
Screenshot 2024-10-31 at 2 21 47 PM

@amirbey
Copy link
Contributor

amirbey commented Nov 1, 2024

the caller of DocvResultRequest#fetch is currently receiving a hash, while it is expecting to receive a DocvResultResponse. it looks like we need to override Socure::Request#handle_invalid_response by creating DocvResultResponse#handle_invalid_response that returns a call to handle_connection_error(DocvResultResponse).

I noticed that the http response body (received from Socure) can be blank dependent upon the type of error (ie: invalid API key -- see screenshot). Otherwise, the API doc shows the body for other types of errors (400, 401) which can help us to determine how to process the http response when creating the DocvResultResponse.

Screenshot 2024-11-01 at 4 46 45 PM

@amirbey
Copy link
Contributor

amirbey commented Nov 1, 2024

It'd be helpful to add some deeper testing (particularly in our controllers) to catch and protect against these types of scenarios 🤔

@AShukla-GSA AShukla-GSA requested a review from amirbey November 6, 2024 20:36
Copy link
Contributor

@amirbey amirbey left a comment

Choose a reason for hiding this comment

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

LGTM ... thanks @AShukla-GSA 👍🏿

@AShukla-GSA AShukla-GSA merged commit 4e8a421 into main Nov 7, 2024
2 checks passed
@AShukla-GSA AShukla-GSA deleted the lg-14790-handle-network-failures-for-socure branch November 7, 2024 15:48
AShukla-GSA added a commit that referenced this pull request Nov 7, 2024
AShukla-GSA added a commit that referenced this pull request Nov 7, 2024
@AShukla-GSA AShukla-GSA restored the lg-14790-handle-network-failures-for-socure branch November 7, 2024 19:20
@aduth aduth mentioned this pull request Nov 12, 2024
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.

3 participants