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.

Sign up for GitHub

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

Jump to bottom

#3729 - CAS Integration 3B - Create New Site (Supplier found, Multiple Sites / No matching address) #3884

Merged
merged 6 commits into from
Nov 12, 2024
Merged

#3729 - CAS Integration 3B - Create New Site (Supplier found, Multiple Sites / No matching address) #3884

merged 6 commits into from
Nov 12, 2024

Conversation

lewischen-aot
Copy link
Collaborator

@lewischen-aot lewischen-aot commented Nov 4, 2024 •
edited
Loading

  • If a Get Supplier Response is Successful and returns Active Supplier and NO active Sites, Create New Site
  • If a Get Supplier Response is Successful and returns Active Supplier and ONE OR MANY active Site, Compare the Response AddressLine1 and Postal Code to SIMS address
    • The address match logic was already added during the implementation of #3260.
  • Created the new API createExistingSupplierSite call on CAS service.
    • Fields to include:
      • SupplierNumber,
      • AddressLine1: Use first addressline from SIMS. Truncate if ours is longer than supported. Convert or omit special characters (like is done with other integrations).
      • City: Use as-is,
      • Province: Use abbreviated versions AB,BC,MB,NB,NL,NS,NT,NU,ON,PE,QC,YT,
      • Country,
      • PostalCode: ALL CAPS, no space,
      • EmailAddress: Use as-is
  • Created Unit test
    • "Should invoke CAS API createSiteForExistingSupplier with formatted payload when all data was provided as expected."
  • Created E2E test
    • "Should create a new site and update the student CAS supplier when an active CAS supplier exists with no match addresses."
    • "Should create a new site and update the student CAS supplier when an inactive CAS supplier exists with matching addresses."

@lewischen-aot lewischen-aot self-assigned this Nov 4, 2024
@lewischen-aot lewischen-aot marked this pull request as ready for review November 6, 2024 16:57
@dheepak-aot dheepak-aot self-requested a review November 6, 2024 18:02
@dheepak-aot
Copy link
Collaborator

Nice work. Please take a look at the comments.

dheepak-aot
dheepak-aot reviewed Nov 8, 2024
View reviewed changes
...end/libs/integrations/src/cas/_tests_/unit/cas.service.createSiteForExistingSupplier.spec.ts Outdated
jest.resetAllMocks();
});

it("Should invoke CAS API createSiteForExistingSupplier with formatted payload when all data was provided as expected.", async () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for adding the unit test ❤️

Please use user friendly description. createSiteForExistingSupplier -> create site for existing supplier

@guru-aot guru-aot self-requested a review November 8, 2024 18:34
@dheepak-aot
Copy link
Collaborator

Thanks for making the changes. Added more comments. Please have a look.

I tested the create site CAS API as well.

image

guru-aot
guru-aot reviewed Nov 8, 2024
View reviewed changes
...services/cas-supplier/cas-evaluation-result-processor/cas-active-supplier-found-processor.ts
let result: CreateExistingSupplierSiteResponse;
try {
const address = studentSupplier.address;
result = await this.casService.createSiteForExistingSupplier({
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

guru-aot
guru-aot reviewed Nov 8, 2024
View reviewed changes
Copy link
Collaborator

@guru-aot guru-aot left a comment

Choose a reason for hiding this comment

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

Nice work @lewischen-aot , please work on the comments given.

dheepak-aot
dheepak-aot reviewed Nov 9, 2024
View reviewed changes
sources/packages/backend/libs/integrations/src/cas/models/cas-service.model.ts
* Data used during the creation of a supplier and site on CAS for existing suppliers.
*/
export class CreateExistingSupplierAndSiteSubmittedData {
SupplierName?: string;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe supplier name is no more required here.

dheepak-aot
dheepak-aot reviewed Nov 9, 2024
View reviewed changes
...processors/schedulers/cas-integration/_tests_/cas-supplier-integration.scheduler.e2e-spec.ts Outdated
Promise.resolve(
createFakeCASSupplierResponse({
initialValues: {
status: "INACTIVE", // The status is set to "INACTIVE" to mismatch the address.
Copy link
Collaborator

Choose a reason for hiding this comment

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

The inactive here is the supplier status and not the site status. the factory needs following update to send the site status.

image

image

dheepak-aot
dheepak-aot approved these changes Nov 9, 2024
View reviewed changes
Copy link
Collaborator

@dheepak-aot dheepak-aot 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 comment and a E2E test needs to be updated. Approving as main code doesn't have any blockers.

Please take care of the 2 last comments.

guru-aot
guru-aot approved these changes Nov 12, 2024
View reviewed changes
Copy link
Collaborator

@guru-aot guru-aot left a comment

Choose a reason for hiding this comment

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

LGTM, nice work @lewischen-aot. Approving ignoring the duplication

@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
6.6% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

@github-actions GitHub Actions
Copy link

Backend Unit Tests Coverage Report

Totals Coverage
Statements: 22.3% ( 3709 / 16630 )
Methods: 10.38% ( 214 / 2061 )
Lines: 25.65% ( 3217 / 12544 )
Branches: 13.73% ( 278 / 2025 )

@github-actions GitHub Actions
Copy link

E2E Workflow Workers Coverage Report

Totals Coverage
Statements: 65.43% ( 583 / 891 )
Methods: 59.26% ( 64 / 108 )
Lines: 68.54% ( 464 / 677 )
Branches: 51.89% ( 55 / 106 )

@github-actions GitHub Actions
Copy link

E2E Queue Consumers Coverage Report

Totals Coverage
Statements: 86.92% ( 1243 / 1430 )
Methods: 88.57% ( 124 / 140 )
Lines: 87.77% ( 1055 / 1202 )
Branches: 72.73% ( 64 / 88 )

@github-actions GitHub Actions
Copy link

E2E SIMS API Coverage Report

Totals Coverage
Statements: 66.45% ( 5642 / 8490 )
Methods: 63.9% ( 692 / 1083 )
Lines: 70.48% ( 4445 / 6307 )
Branches: 45.91% ( 505 / 1100 )

@lewischen-aot lewischen-aot added this pull request to the merge queue Nov 12, 2024
Merged via the queue into main with commit f06eee1 Nov 12, 2024
19 of 20 checks passed
@lewischen-aot lewischen-aot deleted the feature/#3729-cas-integration-3b branch November 12, 2024 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrewsignori-aot andrewsignori-aot andrewsignori-aot left review comments

@dheepak-aot dheepak-aot dheepak-aot approved these changes

@guru-aot guru-aot guru-aot approved these changes

Assignees

@lewischen-aot lewischen-aot

Labels
Business Items under Business Consideration E2E/Unit tests Integration Queue Consumers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lewischen-aot @dheepak-aot @andrewsignori-aot @guru-aot