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

#1715 COE and Tuition Remittance - e-Cert Adjustments #1840

Merged
merged 6 commits into from
Mar 30, 2023
Merged

#1715 COE and Tuition Remittance - e-Cert Adjustments #1840

merged 6 commits into from
Mar 30, 2023

Conversation

ann-aot
Copy link
Contributor

@ann-aot ann-aot commented Mar 27, 2023 •
edited
Loading

  • Added a new field in tuition_remittance_effective_amount in DisbursementSchedule
  • Added a function to reuse the logic implemented on #1714 to determine the maximum award using the effective_value calculated for the e-Cert and used as Step 3 in prepareDisbursementsForECertGeneration
  • Replaced the schoolAmount used during the e-Cert generation to reflect the value calculated for the column tuition_remittance_effective_amount, it is using the COE requested value
  • Bug fix
    • while canceling the application, the logic to delete the previous assessment was only checking for the completed state, it was not considering edit application scenario

@ann-aot ann-aot self-assigned this Mar 27, 2023
@ann-aot ann-aot added User Story Institution Institution Features SIMS-Api SIMS-Api DB DB migration involved labels Mar 27, 2023
@ann-aot ann-aot marked this pull request as ready for review March 29, 2023 01:22
andrewsignori-aot
andrewsignori-aot reviewed Mar 29, 2023
View reviewed changes
...nd/apps/queue-consumers/src/processors/assessment/cancel-application-assessment.processor.ts
@@ -55,10 +55,12 @@ export class CancelApplicationAssessmentProcessor {
}

if (
assessment.application.applicationStatus !== ApplicationStatus.Cancelled
![ApplicationStatus.Cancelled, ApplicationStatus.Overwritten].includes(
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 doing the fix 😉

andrewsignori-aot
andrewsignori-aot reviewed Mar 29, 2023
View reviewed changes
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, please take a look at the comments.

dheepak-aot
dheepak-aot reviewed Mar 29, 2023
View reviewed changes
.../db-migrations/src/sql/DisbursementSchedules/Add-tuition-remittance-effective-amount-col.sql
-- Add tuition_remittance_effective_amount to disbursement_schedules table.
ALTER TABLE
sims.disbursement_schedules
ADD
Copy link
Collaborator

Choose a reason for hiding this comment

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

May be not a part of the PR or may be. The effective_amount and the awards(which are money values) are NUMERIC(8 , 2) but the tuition_remittance and tuition_remittance_effective_value are INT. Don't we eventually have one data type for the the amounts? @ann-aot @andrewsignori-aot @guru-aot @andrepestana-aot @sh16011993

image

Copy link
Collaborator

Choose a reason for hiding this comment

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

We had some back and forth about the money values actually having decimals or not. I would say that we can ensure that every money amount on DB has the ability to receive the decimals.
We also have different NUMERIC precisions (e.g. NUMERIC(8, 2) and NUMERIC(7, 2)).
I would say that we should normalize them all as NUMERIC(8, 2) which should allow 999,999.99.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, I can create a ticket to have this tracked.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ticket created https://app.zenhub.com/workspaces/student-information-management-system-5fce9df5aa1b45000e937014/issues/gh/bcgov/sims/1859

dheepak-aot
dheepak-aot reviewed Mar 29, 2023
View reviewed changes
...nd/apps/queue-consumers/src/processors/assessment/cancel-application-assessment.processor.ts
@@ -55,10 +55,12 @@ export class CancelApplicationAssessmentProcessor {
}

if (
assessment.application.applicationStatus !== ApplicationStatus.Cancelled
![ApplicationStatus.Cancelled, ApplicationStatus.Overwritten].includes(
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

dheepak-aot
dheepak-aot reviewed Mar 29, 2023
View reviewed changes
...ges/backend/libs/integrations/src/esdc-integration/e-cert-integration/e-cert-file-handler.ts Outdated
@@ -281,6 +281,8 @@ export class ECertFileHandler extends ESDCFileHandler {
gender: application.student.gender,
maritalStatus: application.relationshipStatus,
studentNumber: application.studentNumber,
tuitionRemittanceEffectiveAmount:
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 e-cert file handler must not be modified(schoolAmount). Only the assignment must change.

guru-aot
guru-aot reviewed Mar 29, 2023
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.

Good work @ann-aot . Other than the comment on assignment for schoolAmount, I do not have any.

@dheepak-aot
Copy link
Collaborator

Nice work @ann-aot 👍. Please have a look at the comments.

andrewsignori-aot
andrewsignori-aot approved these changes Mar 30, 2023
View reviewed changes
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, it looks good 👍

@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: 18.01% ( 1956 / 10859 )
Methods: 8.15% ( 115 / 1411 )
Lines: 20.71% ( 1712 / 8267 )
Branches: 10.92% ( 129 / 1181 )

@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 )

guru-aot
guru-aot approved these changes Mar 30, 2023
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, thanks for doing the changes @ann-aot

@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.04% ( 2560 / 6730 )
Methods: 29.79% ( 261 / 876 )
Lines: 43.65% ( 2191 / 5020 )
Branches: 12.95% ( 108 / 834 )

dheepak-aot
dheepak-aot approved these changes Mar 30, 2023
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.

Good job @ann-aot 👍

@ann-aot ann-aot merged commit 2dcbe73 into main Mar 30, 2023
@ann-aot ann-aot temporarily deployed to DEV March 30, 2023 21:00 — with GitHub Actions Inactive
@ann-aot ann-aot temporarily deployed to DEV March 30, 2023 21:00 — with GitHub Actions Inactive
@ann-aot ann-aot temporarily deployed to DEV March 30, 2023 21:17 — with GitHub Actions Inactive
guru-aot pushed a commit that referenced this pull request Mar 30, 2023
@ann-aot @guru-aot
#1715 COE and Tuition Remittance - e-Cert Adjustments (#1840)
626e9b9 
#1715 COE and Tuition Remittance - e-Cert Adjustments (#1840)
@ann-aot ann-aot deleted the feature/sims-#1715 branch April 11, 2023 20:28
@hellolynn-tbtb hellolynn-tbtb mentioned this pull request Apr 26, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

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

@andrepestana-aot andrepestana-aot andrepestana-aot approved these changes

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

@andrewsignori-aot andrewsignori-aot andrewsignori-aot approved these changes

@sh16011993 sh16011993 Awaiting requested review from sh16011993

@JasonCTang JasonCTang Awaiting requested review from JasonCTang

Assignees

@ann-aot ann-aot

Labels
DB DB migration involved Institution Institution Features SIMS-Api SIMS-Api User Story
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ann-aot @dheepak-aot @andrewsignori-aot @guru-aot @andrepestana-aot