-
Notifications
You must be signed in to change notification settings - Fork 14
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
#3260 - CAS Integration 3A - Create new Supplier and Site - CAS API Call #3804
#3260 - CAS Integration 3A - Create new Supplier and Site - CAS API Call #3804
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes related to this PR start at line 50.
The file was renamed.
sources/packages/backend/libs/integrations/src/cas/cas-formaters.ts
Outdated
Show resolved
Hide resolved
sources/packages/backend/libs/integrations/src/cas/cas-formaters.ts
Outdated
Show resolved
Hide resolved
sources/packages/backend/libs/integrations/src/cas/models/cas-service.model.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
sources/packages/backend/apps/queue-consumers/src/services/cas-supplier/cas-supplier.service.ts
Outdated
Show resolved
Hide resolved
const auth = await this.casService.logon(); | ||
summary.info("Logon successful."); | ||
suppliersUpdated = await this.requestCASAndUpdateSuppliers( | ||
suppliersUpdated = await this.processSuppliers( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The silent failing is also happening inside this.processSuppliers
.
Does this method need to fail silently w/o throwing error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The below AC is not implemented yet.
"If some error is present throw in the scheduler to allow it to be retried."
The idea is to inspect the summary (as it is already done in getSuccessMessageWithAttentionCheck
) and throw an error if any error log message is present. Throwing an exception from here or from there would result in the same.
I added it now to the "TODOs" in the PR description.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the idea is that the failure will be caught and then logged for info purpose as summary.error
and then in the next run of the scheduler, the failed cas suppliers will be attempted again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If some error is logged to the summary we will throw to allow the scheduler to retry immediately.
When we have the permanent errors implemented, those should be probably logged as warnings because they will be part of the "business valid scenarios".
@@ -38,10 +50,10 @@ export class CASSupplierIntegrationService { | |||
const summary = new ProcessSummary(); | |||
parentProcessSummary.children(summary); | |||
try { | |||
summary.info("Logging on CAS..."); | |||
summary.info("Logging on CAS."); | |||
const auth = await this.casService.logon(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not related to PR and asking for my understanding, How long is a logon valid?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per the docs, 1 hour (3600 seconds).
...services/cas-supplier/cas-evaluation-result-processor/cas-active-supplier-found-processor.ts
Show resolved
Hide resolved
...s/src/services/cas-supplier/cas-evaluation-result-processor/cas-pre-validations-processor.ts
Show resolved
Hide resolved
{ | ||
supplierNumber: result.response.supplierNumber, | ||
supplierName: result.submittedData.SupplierName, | ||
status: "ACTIVE", | ||
lastUpdated: now, | ||
supplierAddress: { | ||
supplierSiteCode: result.response.supplierSiteCode, | ||
addressLine1: submittedAddress.AddressLine1, | ||
city: submittedAddress.City, | ||
provinceState: submittedAddress.Province, | ||
country: submittedAddress.Country, | ||
postalCode: submittedAddress.PostalCode, | ||
status: "ACTIVE", | ||
lastUpdated: now, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about supplierProtected
and siteProtected
in the update?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean setting them explicitly to null
?
Since the supplier was not found, it will be a new supplier and site. These properties should never exist or be created for a new supplier and site.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These properties should never exist or be created for a new supplier and site. - Ah, is it? I was under the impression that it is set during new supplier and site creation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are legacy properties. As far as I know, they only exist due to some SFAS requirement.
Either way, we do not have any direction about setting them.
{ | ||
id: casSupplier.id, | ||
}, | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The update happening for found and not found is identical. Can we have a common method to be re-used for the update?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My point was conversion of CAS Supplier and CAS Site to SIMS Supplier and SIMS supplier address(and then update). May be something in one of the upcoming CAS tickets.
if (evaluationResult.status !== CASEvaluationStatus.NotFound) { | ||
throw new Error("Incorrect CAS evaluation result processor selected."); | ||
} | ||
summary.info( | ||
`No active CAS supplier found. Reason: ${evaluationResult.reason}.`, | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 for this info in all the processors.
Great Job @andrewsignori-aot. Thanks for trying it make it streamlined for any upcoming CAS updates. |
...services/cas-supplier/cas-evaluation-result-processor/cas-active-supplier-found-processor.ts
Show resolved
Hide resolved
modifier: { id: systemUsersService.systemUser.id }, | ||
}); | ||
// Ensure updatedAt was updated. | ||
expect(updateCASSupplier.updatedAt.getTime()).toBeGreaterThan( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
// Arrange | ||
const referenceDate = new Date(); | ||
const savedCASSupplier = await saveFakeCASSupplier(db); | ||
// Configure CAS mock to return em empty result for the GetSupplier |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: an
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work @andrewsignori-aot 👍 Thank You for the walkthrough of your PR.
Quality Gate passedIssues Measures |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work. Thanks for doing the changes. 👍
Main Changes
CASActiveSupplierNotFoundProcessor
: 🆕CASPreValidationsProcessor
: 🆕Address match
Error handling
API call properly reports errors using HTTP Status 400 and adding meaningful error messages. Please see below samples.
These error messages were used to define the max length validation for the submitted data.
GetSupplier 404
The
GetSupplier
method from CAS API does not return a 404 error when no supplier is found as stated in the documentation.The method seems to be implemented as a search and it will just return an empty array if no items were found.
Secondary PR changes
AddressInfo
itself which containscountry
andselectedCountry
properties.New E2E Tests
Log samples
TODO