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

Adds an error catch for null responses #501

Merged
merged 6 commits into from
Oct 13, 2021
Merged

Adds an error catch for null responses #501

merged 6 commits into from
Oct 13, 2021

Conversation

lomky
Copy link
Contributor

@lomky lomky commented Sep 22, 2021

Checks if the API has responded with what we consider a null response, and logs that case clearly.

===

Resolves #480

  • Relevant documentation (e.g. READMEs, Technical Foundation) updated
  • Issue AC are copied to this PR & are met
  • Manual Browser Testing performed (with modheader, either locally or on BrowserStack)
  • Changes are tested on Staging post-merge into main

@lomky lomky requested a review from rocketnova September 22, 2021 14:59
@lomky lomky self-assigned this Sep 22, 2021
utils/queryApiGateway.ts Show resolved Hide resolved
Comment on lines 125 to 131
// This is the response for a bad UniqueNumber, or a UniqueNumber without a claim
const nullResponseShort = {
claimDetails: null,
hasCertificationWeeksAvailable: false,
hasPendingWeeks: false,
pendingDetermination: null,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

If you include this, you are in effect making it so that this is unreachable:

if (apiData?.uniqueNumber !== uniqueNumber) {
const mismatchError = new Error(
`Mismatched API response and Header unique number (${apiData.uniqueNumber || 'null'} and ${uniqueNumber})`,
)
const logger: Logger = Logger.getInstance()
logger.log('error', mismatchError, 'Unexpected API gateway response')
throw mismatchError
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Therefore, I think you should either not include the nullResponseShort or handle the mismatchError first before you handle the new null response error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh interesting - I was considering mismatched UniqueNumber when the API gave us a null to be incidentally catching the null response case, and that this is a more accurate error for that situation. That is still reachable if we get back the different unique number, or get back some claim data but a null unique number.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh interesting. I see what you mean. This sounds good to me!

Comment on lines 152 to 153
assert.notDeepStrictEqual(response, nullResponseShort, 'Response is null')
assert.notDeepStrictEqual(response, nullResponseLong, 'Response is null')
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 neat

Comment on lines 135 to 149
const nullResponseLong = {
claimDetails: {
programType: '',
benefitYearStartDate: null,
benefitYearEndDate: null,
claimBalance: null,
weeklyBenefitAmount: null,
lastPaymentIssued: null,
lastPaymentAmount: null,
monetaryStatus: '',
},
hasCertificationWeeksAvailable: false,
hasPendingWeeks: false,
pendingDetermination: [],
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like hard coding these "null" responses is not the right way to approach this, but I think that #150 is actually the right way, so this seems like a good enough approach until we get #150 done.

Copy link
Contributor

@rocketnova rocketnova left a comment

Choose a reason for hiding this comment

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

One minor nit. LGTM!

utils/queryApiGateway.ts Outdated Show resolved Hide resolved
@lomky lomky merged commit 97c30f3 into main Oct 13, 2021
@lomky lomky deleted the kat/nulls_catch branch October 13, 2021 14:57
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.

Properly log & alert on null response with a Unique Number
2 participants