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

#3254 - CAS AP Invoice - Invoices and Batches - Scheduler #4297

Merged
merged 10 commits into from
Jan 30, 2025

Conversation

andrewsignori-aot
Copy link
Collaborator

@andrewsignori-aot andrewsignori-aot commented Jan 28, 2025

  • Created the new scheduler default setup including DB migrations.
    • Added information in the wiki with the agreed time with the business.
    • Created only the first E2E to allow some testing. Adding further E2E is planned but it will also demand adding more factories hence this was a point to stop for now.
  • Created DB seeding for the new CAS distribution accounts to allow easy execution assertions on expected distribution accounts. It may be shared with API when API tests are created.
    • Note: DB seeding was never enabled for queue-consumers and few files were modified to allow it following the same already in place for the API.
    • Minor refactors were executed in the DB seeding to allow the system user to be retrieved and the default logger to be used instead of console.
  • The sequence-control service was changed to allow outside control of the sequence increment.

Pending Receipts Query

  • Query to retrieve the pending receipts was kept as "query builder" to enforce the inner joins (even if some of them would work as left joins enforced by the where condition).
  • Query to retrieve the pending receipts currently does not have a stop date to look for pending disbursements and is just retrieving anything that is pending.

Saving Operations

The method to save the invoices is using a single transaction and chunk inserts. The 150-chunk size was determined by local tests using 50 to 500-chunk sizes. The tests were realized with 1000 invoice records and each one had 4 invoice details where it took around 1 second to complete the DB persistence operation. In terms of SQL parameters, Postgres can accept a maximum of 65K parameters in a single command.

The save operation in chunks was not logging any additional select and was grouping the insert operations as below for invoices and similar for invoice details.

INSERT INTO
    "sims"."cas_invoices"("created_at", "updated_at", "invoice_number", "invoice_status", "invoice_status_updated_on", "errors", "creator", "modifier", "cas_invoice_batch_id", "disbursement_receipt_id", "cas_supplier_id")
VALUES 
    (DEFAULT, DEFAULT, $1, $2, $3, DEFAULT, $4, DEFAULT, $5, $6, $7),
    (DEFAULT, DEFAULT, $8, $9, $10, DEFAULT, $11, DEFAULT, $12, $13, $14),
    (DEFAULT, DEFAULT, $15, $16, $17, DEFAULT, $18, DEFAULT, $19, $20, $21),
    (DEFAULT, DEFAULT, $22, $23, $24, DEFAULT, $25, DEFAULT, $26, $27, $28),
    (DEFAULT, DEFAULT, $29, $30, $31, DEFAULT, $32, DEFAULT, $33, $34, $35),
    (DEFAULT, DEFAULT, $36, $37, $38, DEFAULT, $39, DEFAULT, $40, $41, $42)
    (...)

Migration revert

image

@andrewsignori-aot andrewsignori-aot added Queue Consumers DB DB migration involved labels Jan 28, 2025
@andrewsignori-aot andrewsignori-aot self-assigned this Jan 28, 2025
@andrewsignori-aot andrewsignori-aot marked this pull request as ready for review January 29, 2025 18:03
@dheepak-aot dheepak-aot self-requested a review January 29, 2025 19:56
@@ -20,6 +20,7 @@ RUN npm ci
# Copying sources.
COPY ./apps/queue-consumers ./apps/queue-consumers
COPY ./apps/db-migrations ./apps/db-migrations
COPY ./apps/test-db-seeding ./apps/test-db-seeding
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

.where("disbursementReceiptValue.grantType = :grantType", {
grantType: BC_TOTAL_GRANT_AWARD_CODE,
})
.andWhere("disbursementReceiptValue.grantAmount > 0")
Copy link
Collaborator

Choose a reason for hiding this comment

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

What about considering using the these values from disbursement_receipts table instead of going into disbursement_receipt_values ?
image

Do we think using the disbursementValue.valueType = BCSG will be more accurate?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If BCSG was not available I would be more open to using the total_disbursed_grant_amount, since we have BCSG it does seem more accurate, IMO.
BCSG represents the exact SUM of provincial grants generated by SIMS and the receipt sent back.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see that we can avoid joining the disbursement_receipt_values if we can use total_disbursed_grant_amount here. But not a blocker. Let me know if I am missing something.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I do understand it, but to me was clear that the check must happen using the BCSG and that would be my recommendation also. I will ask the business if they can ensure the goal will be achieved in the same way I will change.

.andWhere("disbursementValue.valueType = :valueType", {
valueType: DisbursementValueType.BCGrant,
})
.andWhere("disbursementValue.effectiveAmount > 0")
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

const invoiceDetails: CASInvoiceDetail[] = [];
for (const disbursedAward of disbursedAwards) {
// Find the distribution accounts for the award.
const accounts = distributionAccounts.filter(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Will it not be useful to throw error if the distribution account size is 0?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It may, but I will raise the question and leave it to a second PR.
It may be useful if it is 0 records, or if there is only one, or if it is not a pair of debit and credit operations or they may want the ability to not have any and avoid including it in the invoice.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It will be taken care on an upcoming PR as per business agreement. Just some basic validation.

sequenceName: CAS_INVOICE_SEQUENCE_NAME,
},
});
expect(currentInvoiceSequenceNumber.sequenceNumber).toEqual("2");
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

});
});

it("Should create a new CAS invoice batch for a full-time receipt when there is a e-Cert receipt with no invoice associated.", async () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

E2E looks great. Unless there are more E2E tests going to come here in upcoming PRs, I would ask to have an E2E with no pending invoices to process.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There will be more E2Es. The next PR will be for UI, but UI and schedulers must receive more E2Es.

operationCode: "CR" | "DR";
}

export const CAS_DISTRIBUTION_ACCOUNTS_INITIAL_DATE: CASDistributionAccountBaseData[] =
Copy link
Collaborator

Choose a reason for hiding this comment

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

It is great idea to have it data seeded.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The idea is to share them with the API E2E tests also in upcoming PRs in this sprint.

sequenceRecord.sequenceNumber = nextSequenceNumber.toString();
const result = await process(nextSequenceNumber, entityManager);
if (typeof result === "number") {
// If the external process returned a number, it will be used as the new sequence number.
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

Copy link

Backend Unit Tests Coverage Report

Totals Coverage
Statements: 22.44% ( 3887 / 17320 )
Methods: 10.32% ( 226 / 2189 )
Lines: 25.8% ( 3353 / 12998 )
Branches: 14.44% ( 308 / 2133 )

Copy link

E2E Workflow Workers Coverage Report

Totals Coverage
Statements: 65.59% ( 589 / 898 )
Methods: 59.63% ( 65 / 109 )
Lines: 68.72% ( 468 / 681 )
Branches: 51.85% ( 56 / 108 )

Copy link

E2E Queue Consumers Coverage Report

Totals Coverage
Statements: 86.76% ( 1356 / 1563 )
Methods: 83.8% ( 150 / 179 )
Lines: 89.06% ( 1123 / 1261 )
Branches: 67.48% ( 83 / 123 )

Copy link

E2E SIMS API Coverage Report

Totals Coverage
Statements: 68.11% ( 6019 / 8837 )
Methods: 65.84% ( 742 / 1127 )
Lines: 71.94% ( 4712 / 6550 )
Branches: 48.71% ( 565 / 1160 )

await this.sequenceControlService.consumeNextSequenceWithExistingEntityManager(
"CAS_INVOICE",
entityManager,
async (nextSequenceNumber: number) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can be explicit with the return type here.

async (nextSequenceNumber: number): Promise<number>

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.

Thanks for doing the changes @andrewsignori-aot. Looks good. One last minor comment.

As discussed, if there is any change based on response from business, we can have that in upcoming PR.

@@ -33,6 +38,7 @@ import { CreateRestrictions } from "./db-seeding/restriction";
CreateAESTUsers,
CreateStudentUsers,
CreateRestrictions,
CreateCASDistributionAccounts,
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

@@ -31,7 +31,7 @@
"test:e2e:workers:local": "cross-env ENVIRONMENT=test jest --collect-coverage --verbose --config ./apps/workers/test/jest-e2e.json --forceExit",
"test:e2e:workflow": "npm run deploy:camunda:definitions && cross-env ENVIRONMENT=test jest --verbose --config ./workflow/test/jest-e2e.json --forceExit",
"test:e2e:workflow:local": "cross-env ENVIRONMENT=test TZ=UTC jest --verbose --config ./workflow/test/jest-e2e.json --forceExit",
"test:e2e:queue-consumers": "npm run migration:run && cross-env ENVIRONMENT=test jest --collect-coverage --verbose --config ./apps/queue-consumers/test/jest-e2e.json --runInBand --forceExit",
"test:e2e:queue-consumers": "npm run db:seed:test filter=CreateCASDistributionAccounts && cross-env ENVIRONMENT=test jest --collect-coverage --verbose --config ./apps/queue-consumers/test/jest-e2e.json --runInBand --forceExit",
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

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 @andrewsignori-aot

@andrewsignori-aot andrewsignori-aot added this pull request to the merge queue Jan 30, 2025
Merged via the queue into main with commit 44ffff8 Jan 30, 2025
21 checks passed
github-merge-queue bot pushed a commit that referenced this pull request Feb 6, 2025
Created new E2E tests for the new endpoints and extended scheduler E2E
tests previously added (only one).

## API Endpoints (CASInvoiceBatchAESTController)
- getCASInvoiceBatchReport
- Should generate an invoice batch report with part-time and full-time
invoices when the batch exists.
- Should throw a HttpStatus Not Found (404) when the requested invoice
batch does not exist.
- Should throw a HttpStatus Forbidden (403) error when an unauthorized
Ministry user tries to get the invoice batch report.
- getInvoiceBatches
- Should be able to get invoice batches for the first page in a
paginated result with a limit of two per page in descending order when
there are three invoice batches available.
- Should be able to get only pending invoice batches when the filter for
status is applied.
- Should throw a HttpStatus Bad Request (400) error when the filter for
status is invalid.
- Should throw a HttpStatus Bad Request (400) error when the sortField
is invalid.
- Should throw a HttpStatus Forbidden (403) error when an unauthorized
Ministry user tries to get the invoice batches.

## Scheduler
- Should create a new CAS invoice and avoid making a second invoice when
a receipt already has an invoice associated with it.
- Should interrupt the process when an invoice is trying to be generated
but there are no distribution accounts available to create the invoice
details ([addressing previous PR
comment](#4297 (comment))).
- Should finalize the process nicely when there is no pending receipt to
process ([addressing previous PR
comment](#4297 (comment))).
@andrewsignori-aot andrewsignori-aot deleted the feature/#3254-cas-invoices-scheduler branch February 7, 2025 22:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DB DB migration involved Queue Consumers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants