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-11718: make sure err http status is handled. #9957

Merged
merged 7 commits into from
Jan 30, 2024

Conversation

dawei-nava
Copy link
Contributor

@dawei-nava dawei-nava commented Jan 22, 2024

🎫 Ticket

Link to the relevant ticket:
LG-11718

🛠 Summary of changes

Review and update tests to make sure http error from TrueID is handled and bubbles up as needed.

@dawei-nava dawei-nava force-pushed the dwang/LG-11718-err-http-status branch from 6788758 to a290133 Compare January 25, 2024 14:35
extra: { vendor: 'TrueID' },
extra: {
vendor: 'TrueID',
selfie_live: false,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

with some defaults for http error.

let(:image_source) { DocAuth::ImageSources::ACUANT_SDK }
it 'is a network error with 5xx status' do
stub_request(:post, full_url).to_return(body: '{}', status: 500)
response = subject.fetch
expect(response.network_error?).to eq(true)
end
it 'is not a network error with 440, 438, 439' do
stub_request(:post, full_url).to_return(body: '{}', status: 443)
it 'is a network error with non 5xx error' do
Copy link
Contributor Author

@dawei-nava dawei-nava Jan 25, 2024

Choose a reason for hiding this comment

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

Phasing out Acuant, there 438, 439 etc have special meanings.

@dawei-nava dawei-nava marked this pull request as ready for review January 25, 2024 15:08
Copy link
Contributor

@eileen-nava eileen-nava left a comment

Choose a reason for hiding this comment

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

Approved.

expected_message = [
subject.class.name,
'Unexpected HTTP response',
status,
].join(' ')
expect(NewRelic::Agent).to receive(:notice_error) do |arg|
Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't used do blocks like this in testing very much. It like it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@eileen-nava , it has issue with multiple parameters when using .with(xxxx) matcher.

@@ -186,6 +186,8 @@
complete_doc_auth_steps_before_document_capture_step
mock_doc_auth_trueid_http_non2xx_status(438)
attach_and_submit_images
# verify it's a network error
expect(page).to have_content(I18n.t('doc_auth.errors.general.network_error'))
Copy link
Contributor

Choose a reason for hiding this comment

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

👍🏻

before do
allow(subject).to receive(:post_images_to_client).and_return(failed_response)
subject.instance_variable_set(:@doc_auth_client, doc_auth_client)
allow(doc_auth_client).to receive(:post_images) { failed_response }
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch here! 💪🏻


describe 'when selfie failure response returned' do
before do
stub_request(:post, image_upload_url).to_return(
Copy link
Contributor

Choose a reason for hiding this comment

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

This is non-blocking feedback. I peeked at the LexisNexisFixtures.true_id_response_failure_with_liveness file, and most of it was created four years ago. Are we sure that file is still up to date?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@eileen-nava , the portrait match result part are the same.
The main differences are what they called disposition based description of some data fields, which I dont think we are using for any business purpose.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it, thanks.

let(:path) { "/restws/identity/v3/accounts/#{account_id}/workflows/#{workflow}/conversations" }
let(:full_url) { base_url + path }
let(:applicant) { { uuid: SecureRandom.uuid, uuid_prefix: '123' } }
let(:non_cropping_non_liveness_flow) { 'test_workflow' }
Copy link
Contributor

Choose a reason for hiding this comment

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

👍🏻

let(:base_url) { 'https://lexis.nexis.example.com' }
let(:workflow) { subject.send(:workflow) }
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 curious why you made this change? Not against it, just curious about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@eileen-nava , to verify workflows being used (which is determined by a private method)


it 'logs analytics event' do
form.submit
expect(fake_analytics).to have_logged_event(
Copy link
Contributor

@amirbey amirbey Jan 29, 2024

Choose a reason for hiding this comment

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

nit: alot of the values here are nil due to the test case ... is it possible we could use something like hash_including instead and only check relevant values?

@amirbey amirbey self-requested a review January 29, 2024 19:43
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 ... just had 1 comment

@dawei-nava dawei-nava merged commit d84cb7a into main Jan 30, 2024
2 checks passed
@dawei-nava dawei-nava deleted the dwang/LG-11718-err-http-status branch January 30, 2024 14:14
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