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

#1761 - Handling full time BCSL lifetime and federal maximums - DB migrations #1860

Merged
merged 7 commits into from
Apr 11, 2023

Conversation

ann-aot
Copy link
Contributor

@ann-aot ann-aot commented Apr 4, 2023

  • DB migrations for the Story
  • As discussed, not used IF EXISTS in the queries
  • FIX - added numericTransformer to numeric field tuition_remittance_effective_amount
    UPDATE:
    resctrictionCatergory updated to Other for the new restriction BCLM upon confirmation

@ann-aot ann-aot self-assigned this Apr 4, 2023
@ann-aot ann-aot added the DB DB migration involved label Apr 4, 2023
@ann-aot ann-aot marked this pull request as ready for review April 4, 2023 22:09
@andrewsignori-aot andrewsignori-aot changed the title #1761 Handling full time BCSL lifetime and federal maximums - DB migrations #1761 - Handling full time BCSL lifetime and federal maximums - DB migrations Apr 4, 2023
@@ -14,5 +15,6 @@ export function createFakeProgramYear(programYearPrefix?: number): ProgramYear {
programYear.parentFormName = `${programYear.formName}-parent`;
programYear.partnerFormName = `${programYear.formName}-partner`;
programYear.programYearPrefix = programYearPrefix.toString();
programYear.maxLifetimeBCLoanAmount = faker.random.number(100000);
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 we can give an actual number rather giving a random number. As during the lifetime maximums calculations, if this is created, it will be helpful to do the expect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will not always be 50,000 for future program years I guess. but I got the point, I will change

)
VALUES
(
'Provincial',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Along with the development of this ticket, we can add the placeholder for this new restriction code on the student activity also.
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ya, I will add it. Thanks for the point. And I think we need to review the codes there in the UI, a few days back, when I was trying to find the details for a code, I couldn't find it there.

nullable: true,
transformer: numericTransformer,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice catch 😉

restrictionAmountSubtracted?: number;
/**
* References the restriction related to the disbursement
* due to which the amount for subtracted.
Copy link
Collaborator

Choose a reason for hiding this comment

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

The "for" here seems lost, is it supposed to be "due to which the amount was subtracted"?

Copy link
Contributor Author

@ann-aot ann-aot Apr 4, 2023

Choose a reason for hiding this comment

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

updating to
* References the restriction related to the disbursement due to which the amount was subtracted.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks good, just remove the additional white spaces in the middle of the proposed sentence.

Copy link
Collaborator

@andrewsignori-aot andrewsignori-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, just minor comments.

ALTER TABLE
sims.program_years
ADD
COLUMN max_lifetime_bc_loan_amount NUMERIC(8, 2) NOT NULL DEFAULT 0;
Copy link
Collaborator

@dheepak-aot dheepak-aot Apr 4, 2023

Choose a reason for hiding this comment

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

We can default it to 50000 and skip the update query below.

@dheepak-aot
Copy link
Collaborator

dheepak-aot commented Apr 4, 2023

Nice work @ann-aot . few minor comments.

Also there is minor sonar cloud code smell on import.

)
restrictionIdSubtracted?: number;
/**
*Restriction id due to which the award amount was reduced.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add a white space after the "*".

Copy link
Collaborator

@andrewsignori-aot andrewsignori-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, looks good 👍

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 making the changes. LGTM. 👍

Copy link
Collaborator

@andrepestana-aot andrepestana-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!

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

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@github-actions
Copy link

Backend Unit Tests Coverage Report

Totals Coverage
Statements: 17.99% ( 1960 / 10892 )
Methods: 8.12% ( 115 / 1417 )
Lines: 20.71% ( 1716 / 8286 )
Branches: 10.85% ( 129 / 1189 )

@github-actions
Copy link

E2E Workflow Workers Coverage Report

Totals Coverage
Statements: 32.41% ( 176 / 543 )
Methods: 21.25% ( 17 / 80 )
Lines: 39.25% ( 157 / 400 )
Branches: 3.17% ( 2 / 63 )

@github-actions
Copy link

E2E Queue Consumers Coverage Report

Totals Coverage
Statements: 56.72% ( 308 / 543 )
Methods: 46.38% ( 32 / 69 )
Lines: 59.48% ( 276 / 464 )
Branches: 0% ( 0 / 10 )

@github-actions
Copy link

E2E SIMS API Coverage Report

Totals Coverage
Statements: 38.08% ( 2580 / 6775 )
Methods: 29.82% ( 263 / 882 )
Lines: 43.7% ( 2208 / 5053 )
Branches: 12.98% ( 109 / 840 )

@ann-aot ann-aot merged commit ca4daef into main Apr 11, 2023
@ann-aot ann-aot temporarily deployed to DEV April 11, 2023 20:16 — with GitHub Actions Inactive
@ann-aot ann-aot temporarily deployed to DEV April 11, 2023 20:16 — with GitHub Actions Inactive
@ann-aot ann-aot temporarily deployed to DEV April 11, 2023 20:28 — with GitHub Actions Inactive
@ann-aot ann-aot deleted the feature/migrations/sims-#1761 branch April 11, 2023 20:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DB DB migration involved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants