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

#2303 - Total eligible amount in NOA screen #2365

Merged
merged 17 commits into from
Oct 5, 2023
Merged

#2303 - Total eligible amount in NOA screen #2365

merged 17 commits into from
Oct 5, 2023

Conversation

guru-aot
Copy link
Collaborator

@guru-aot guru-aot commented Sep 29, 2023 •
edited
Loading

Total eligible amount is not showing for NOA (part-time and full-time)

image

image

@guru-aot guru-aot self-assigned this Sep 29, 2023
@guru-aot guru-aot marked this pull request as ready for review October 3, 2023 22:05
dheepak-aot
dheepak-aot reviewed Oct 4, 2023
View reviewed changes
sources/packages/web/src/services/http/dto/Assessment.dto.ts
@@ -58,6 +58,7 @@ export interface AssessmentNOAAPIOutDTO {
offeringStudyEndDate: string;
msfaaNumber: string;
disbursement: unknown;
eligibleAmount: number;
Copy link
Collaborator

Choose a reason for hiding this comment

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

https://github.com/bcgov/SIMS/pull/2365/files#r1345033121

dheepak-aot
dheepak-aot reviewed Oct 4, 2023
View reviewed changes
sources/packages/web/src/services/http/dto/Assessment.dto.ts
@@ -58,6 +58,7 @@ export interface AssessmentNOAAPIOutDTO {
offeringStudyEndDate: string;
msfaaNumber: string;
disbursement: unknown;
eligibleAmount: number;
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 following fields are used in NOA form.io but not saved by the workflow.

Are these fields not relevant to the part time application?

image

image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes if you are talking about the form.io not saving the assessmentData by the PartTime application, yes there are fields that are used only for FullTime application.

Copy link
Collaborator

@dheepak-aot dheepak-aot Oct 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

My question is, does part time noa require Living Allowance, Other allowable cost and Total Assessed Cost ? or they are disregarded part time NOA.

I am doubtful that they may be needed for PT noa as well. But I may be wrong.

But if they are needed, the value is never going to come for PT application unless I am missing something.

@JasonCTang @michesmith can you please confirm on the same?

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 have confirmed with @HRAGANBC regarding this and there will be a future ticket that needs displaying of several other fields in the NOA screen. Will connect the ticket with out ticket for continuation.

dheepak-aot
dheepak-aot reviewed Oct 4, 2023
View reviewed changes
...llers/assessment/_tests_/e2e/assessment.institutions.controller.getAssessmentNOA.e2e-spec.ts Outdated
disbursement.valueAmount;
});
const firstDisbursementScheduleAwards = {};
firstDisbursementSchedule.disbursementValues.forEach(
Copy link
Collaborator

Choose a reason for hiding this comment

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

minor. can use a common method to create disbursement schedule awards.

@dheepak-aot
Copy link
Collaborator

Nice to see NOA details getting better. Please have a look at the comments.

@github-actions
Copy link

github-actions bot commented Oct 4, 2023

E2E SIMS API Coverage Report

Totals Coverage
Statements: 54.94% ( 4031 / 7337 )
Methods: 52.05% ( 496 / 953 )
Lines: 59.76% ( 3272 / 5475 )
Branches: 28.93% ( 263 / 909 )

sh16011993
sh16011993 reviewed Oct 4, 2023
View reviewed changes
sources/packages/backend/libs/test-utils/src/factories/disbursement-schedule.ts Outdated

disbursementSchedule.disbursementValues.forEach((disbursementValue) => {
disbursementScheduleAwards[
`disbursement${disbursementNumber}${disbursementValue.valueCode.toLowerCase()}`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a possibility of having a repeated valueCode creating a key conflict (duplicate key)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No there will not be any

dheepak-aot
dheepak-aot approved these changes Oct 4, 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.

Thanks for making the changes. 👍

Wanted to know the outcome of the comment https://github.com/bcgov/SIMS/pull/2365/files#r1346112384 from @JasonCTang @michesmith

andrewsignori-aot
andrewsignori-aot reviewed Oct 4, 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.

sh16011993
sh16011993 reviewed Oct 4, 2023
View reviewed changes
.../packages/backend/apps/api/src/route-controllers/assessment/assessment.controller.service.ts
.reduce(
(accumulator, disbursementValue) =>
accumulator + disbursementValue.valueAmount,
0,
Copy link
Collaborator

Choose a reason for hiding this comment

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

IMO: Specifying 0 as the initial value explicitly to the reduce is not required here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Creating an accumulator in the start, in case if the array is null, then the value returned will be 0.

sh16011993
sh16011993 reviewed Oct 4, 2023
View reviewed changes
...llers/assessment/_tests_/e2e/assessment.institutions.controller.getAssessmentNOA.e2e-spec.ts Outdated
totalValueAmount +
(schedule.disbursementValues || []).reduce(
(sum, disbursementValue) => sum + disbursementValue.valueAmount,
0,
Copy link
Collaborator

@sh16011993 sh16011993 Oct 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

https://github.com/bcgov/SIMS/pull/2365/files#r1346581248

Same can be done for line 271

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed it

sh16011993
sh16011993 reviewed Oct 4, 2023
View reviewed changes
Copy link
Collaborator

@sh16011993 sh16011993 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 @guru-aot 👍 Added an info and a minor suggestion comment.

@github-actions
Copy link

github-actions bot commented Oct 5, 2023

Backend Unit Tests Coverage Report

Totals Coverage
Statements: 17.42% ( 2206 / 12665 )
Methods: 8.08% ( 129 / 1597 )
Lines: 20.16% ( 1933 / 9587 )
Branches: 9.72% ( 144 / 1481 )

@sonarqubecloud
Copy link

sonarqubecloud bot commented Oct 5, 2023

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

github-actions bot commented Oct 5, 2023

E2E Workflow Workers Coverage Report

Totals Coverage
Statements: 46.73% ( 300 / 642 )
Methods: 40% ( 32 / 80 )
Lines: 51.02% ( 251 / 492 )
Branches: 24.29% ( 17 / 70 )

@github-actions
Copy link

github-actions bot commented Oct 5, 2023

E2E Queue Consumers Coverage Report

Totals Coverage
Statements: 74.93% ( 514 / 686 )
Methods: 67.47% ( 56 / 83 )
Lines: 76.96% ( 451 / 586 )
Branches: 41.18% ( 7 / 17 )

andrewsignori-aot
andrewsignori-aot approved these changes Oct 5, 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, looks good 👍

@guru-aot guru-aot merged commit de1706b into main Oct 5, 2023
@guru-aot guru-aot deleted the 2303_Total_Eligible_Amount branch October 5, 2023 21:51
@guru-aot guru-aot temporarily deployed to DEV October 5, 2023 22:07 — with GitHub Actions Inactive
@guru-aot guru-aot temporarily deployed to DEV October 5, 2023 22:08 — with GitHub Actions Inactive
@guru-aot guru-aot temporarily deployed to DEV October 5, 2023 22:08 — with GitHub Actions Inactive
@guru-aot guru-aot temporarily deployed to DEV October 5, 2023 22:08 — with GitHub Actions Inactive
@guru-aot guru-aot temporarily deployed to DEV October 5, 2023 22:08 — with GitHub Actions Inactive
@guru-aot guru-aot temporarily deployed to DEV October 5, 2023 22:11 — with GitHub Actions Inactive
@guru-aot guru-aot temporarily deployed to DEV October 5, 2023 22:11 — with GitHub Actions Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sh16011993 sh16011993 sh16011993 left review comments

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

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

@ann-aot ann-aot ann-aot approved these changes

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

@JasonCTang JasonCTang Awaiting requested review from JasonCTang

Assignees

@guru-aot guru-aot

Labels
E2E/Unit tests SIMS-Api SIMS-Api
Projects
None yet
Milestone
No milestone
Development

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