#1759 - Reissue MSFAA - Part 1 - New SIMS API Endpoint #1928

andrewsignori-aot · 2023-05-05T23:52:17Z

Methos for MSFAA creation (used during assessment) and reissue were centralized to execute the below process in the same way.
- Ensure that any pending MSFAA is canceled. A pending MSFAA is a never signed or canceled record.
- Create the new MSFAA number.
- Associate the new MSFAA number with any pending disbursement for the same student and same offering intensity.
Created a new API endpoint under the Ministry application controller to reissue the MSFAA.

E2E Tests for reissuing the MSFAA from SIMS API

ApplicationAESTController(e2e)-reissueMSFAA
√ Should reissue an MSFAA and associate with both disbursements when both disbursements are pending and the current MSFAA is signed but canceled. (449 ms)
√ Should reissue an MSFAA and associate with only the pending disbursement when one disbursement is not pending and the other is pending and the current MSFAA is canceled. (228 ms)
√ Should reissue an MSFAA and associate with only pending disbursements across multiple applications for the same offering intensity and same student when some disbursements are pending, some are not and there is at least on application from a different intensity. (427 ms)
√ Should throw a UnprocessableEntityException when the associated MSFAA is not cancelled. (152 ms)
√ Should throw a UnprocessableEntityException when there is no pending disbursement associated with the application. (141 ms)
√ Should throw a UnprocessableEntityException when the application is not in the expected status. (59 ms)
√ Should throw a NotFoundException when the application does not exist. (11 ms)

ann-aot · 2023-05-09T18:11:05Z

sources/packages/backend/libs/services/src/msfaa-number/msfaa-number-shared.service.ts

+          cancelledDate: IsNull(),
+        },
+        {
+          cancelledDate: nowISODate,


mm, I was thinking do we need any other indication to know this process happened as part of reissuing the new MSAFAA and was not canceled as part of the file integration? or do we care about that?

File integrations contain new issuing provinces and system cancellation does not.
No requirements so far to make them different other than the above-mentioned.

ann-aot · 2023-05-09T18:14:43Z

sources/packages/backend/libs/services/src/msfaa-number/msfaa-number-shared.service.ts

+   * @returns number to be used for the next MSFAA.
+   */
+  private async consumeNextSequence(
+    offeringIntensity: OfferingIntensity,


pls add it to the comment

ann-aot · 2023-05-09T18:15:14Z

sources/packages/backend/libs/services/src/msfaa-number/msfaa-number-shared.service.ts

+   */
+  async reissueMSFAA(
+    referenceApplicationId: number,
+    auditUserId: number,


pls add it to the comment

ann-aot · 2023-05-09T20:19:48Z

...apps/api/src/route-controllers/application/_tests_/application.aest.reissueMSFAA.e2e-spec.ts

+    const currentMSFAA = createFakeMSFAANumber(
+      { student },
+      {
+        state: MSFAAStates.Signed | MSFAAStates.CancelledOtherProvince,


won't this give the sum of 1 and 4 that is 5

does, it satisfy the condition that we need?

Yes, please review how the check is done.

Sorry but I didn't understand why using the bitwise operations here.

@andrepestana-aot please let me know if need more information and please review the code for the createFakeMSFAANumber method.
#1928 (comment)

ann-aot · 2023-05-09T20:24:21Z

sources/packages/backend/libs/test-utils/src/factories/msfaa-number.ts

-  msfaaNumber.student = relations.student;
-  msfaaNumber.referenceApplication = relations?.referenceApplication;
+
+  if (!options?.state || options.state & MSFAAStates.Pending) {


mm, why we went for bitwise operations

ann-aot · 2023-05-09T20:26:52Z

sources/packages/backend/libs/test-utils/src/factories/msfaa-number.ts

+  Pending = 0,
+  Signed = 1,
+  CancelledSystem = 2,
+  CancelledOtherProvince = 4,


are we not using 3 due to bitwise operations?

So, wouldn't it be good to comment here that a MSFAAStates 3 should be a "Signed" + "CancelledSystem"? That was my understanding at least.

The combination between the enum items is free. It can be as below or others.

Pending

Signed

Signed | CancelledSystem

Signed | CancelledSystem | CancelledOtherProvince

Signed | CancelledOtherProvince

...s/packages/backend/apps/api/src/route-controllers/application/application.aest.controller.ts

ann-aot

Good work @andrewsignori-aot. Added some comments

sonarqubecloud · 2023-05-09T20:50:44Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

ann-aot

LGTM 👍 Thanks for doing the changes

github-actions · 2023-05-09T21:11:04Z

Backend Unit Tests Coverage Report

Totals
Statements:	18.1% ( 2019 / 11155 )
Methods:	8.26% ( 119 / 1441 )
Lines:	20.89% ( 1771 / 8478 )
Branches:	10.44% ( 129 / 1236 )

github-actions · 2023-05-09T21:12:06Z

E2E Workflow Workers Coverage Report

Totals
Statements:	33.02% ( 177 / 536 )
Methods:	22.08% ( 17 / 77 )
Lines:	40.1% ( 158 / 394 )
Branches:	3.08% ( 2 / 65 )

github-actions · 2023-05-09T21:13:00Z

E2E Queue Consumers Coverage Report

Totals
Statements:	68.32% ( 371 / 543 )
Methods:	57.97% ( 40 / 69 )
Lines:	70.47% ( 327 / 464 )
Branches:	40% ( 4 / 10 )

github-actions · 2023-05-09T21:17:34Z

E2E SIMS API Coverage Report

Totals
Statements:	40.47% ( 2781 / 6872 )
Methods:	33.52% ( 299 / 892 )
Lines:	45.74% ( 2345 / 5127 )
Branches:	16.06% ( 137 / 853 )

dheepak-aot · 2023-05-09T21:17:34Z

sources/packages/backend/libs/services/src/msfaa-number/msfaa-number-shared.service.ts

+      const nowISODate = getISODateOnlyString(now);
+      // Cancel any pending MSFAA record that was never reported as signed or cancelled.
+      // This will ensure that the newly created MSFAA will be the only one valid.
+      await entityManager.getRepository(MSFAANumber).update(


I see that is is a very rare edge case scenario where

Student submit an application where MSFAA was valid during submission and before the disbursement was sent it expired and a new application got created with new MSFAA in the mean time.

Hence when reissuing msfaa we are checking for this MSFAA which was never reported or cancelled.

But I have a question here, while re-issuing should we not use this non-reported msfaa number? Is there anything like a policy which stops us doing it?

I was trying to recall the exact reason to have them canceled actually. The goal was to ensure that when a new MSFAA is created there will be only one "not signed".
For instance, the student created an application, generated an MSFAA, and never completed it. After 3 years the same student comes back and a new MSFAA will be generated, in this situation that old record will be "cancelled by the system".
I believe that the idea was just to keep the DB looking better with only one possible not-signed/not-canceled MSFAA.

dheepak-aot · 2023-05-09T21:29:01Z

sources/packages/backend/libs/services/src/msfaa-number/msfaa-number-shared.service.ts

+      // Associate pending disbursements with the new MSFAA.
+      const disbursementScheduleRepo =
+        entityManager.getRepository(DisbursementSchedule);
+      const schedulesToUpdate = await disbursementScheduleRepo.find({


We can directly use update like this right?

await disbursementScheduleRepo.update( { disbursementScheduleStatus: DisbursementScheduleStatus.Pending, studentAssessment: { application: { student: { id: studentId } }, offering: { offeringIntensity: offeringIntensity }, }, }, { msfaaNumber: newMSFAANumber, modifier: auditUser, updatedAt: now, }, );

This was my first attempt. Typeorm does not allow it apparently and, unless I am missing something, this is probably the reason why we do not have any other similar update like this.
Please notice that the main difference is that the "where" clause for this would include relations.

dheepak-aot · 2023-05-09T21:36:34Z

sources/packages/backend/libs/services/src/msfaa-number/msfaa-number-shared.service.ts

+      ApplicationStatus.Cancelled,
+      ApplicationStatus.Overwritten,
+    ];
+    if (nowAllowedApplicationStatuses.includes(application.applicationStatus)) {


Why didn't we include this condition in the query?

Also the Query can be based on status [Assessment,Enrolment,Completed] which will pick just the eligible one and also we can skip line 260-265 based on application being in one of these status.

Related to "Why didn't we include this condition in the query?": the idea was to provide a more a specific error instead of a generic "Application not found" for this situation. I believe that in the past we talked about both approaches being acceptable.

About checking for "Assessment,Enrolment,Completed", the MSFAA as per the assessment workflow right now can be generated while the application is "In Progress", for instance (unless we change it). The idea was only to provide a certain level of validation and ensure that the offering would be present.

dheepak-aot · 2023-05-09T21:58:25Z

...apps/api/src/route-controllers/application/_tests_/application.aest.reissueMSFAA.e2e-spec.ts

+    const [firstSchedule, secondSchedule] =
+      application.currentAssessment.disbursementSchedules;
+    const endpoint = `/aest/application/${application.id}/reissue-msfaa`;
+    const token = await getAESTToken(AESTGroups.BusinessAdministrators);


Just curious to know, couldn't find the AEST group who can re-issue msfaa, is it BusinessAdministrators?

As mentioned today at the daily meeting, there was no requirement for a specific role. It will be added to the ticket and taken care of in the upcoming PR for this ticket 😉

dheepak-aot · 2023-05-09T22:05:32Z

Great work @andrewsignori-aot . Added few comments.

andrepestana-aot

Good job. I'm just not very sure about the bitwise operations idea and left some comments.

andrepestana-aot

Thanks for the explanation.

dheepak-aot

Thanks for the clarifications. LGTM. 👍

guru-aot · 2023-05-10T17:58:49Z

...s/packages/backend/apps/api/src/route-controllers/application/application.aest.controller.ts

+   */
+  @Post(":applicationId/reissue-msfaa")
+  @ApiNotFoundResponse({ description: "Application id not found." })
+  @ApiUnprocessableEntityResponse({


Cool implementation 👍

guru-aot · 2023-05-10T17:59:03Z

sources/packages/backend/libs/services/src/msfaa-number/msfaa-number-shared.service.ts

+          schedule.disbursementScheduleStatus ===
+          DisbursementScheduleStatus.Pending,
+      );
+    if (!pendingDisbursement) {


guru-aot

Nice work @andrewsignori-aot

andrewsignori-aot added 3 commits May 4, 2023 14:36

Created method reissueMSFAA

f5f9f75

Create and reissue MSFAA centralization

ec93b0c

Fix some E2E tests

db9793f

andrewsignori-aot self-assigned this May 6, 2023

andrewsignori-aot added SIMS-Api SIMS-Api Camunda Workers labels May 6, 2023

andrewsignori-aot changed the title ~~#1759 - Reissue MSFAA~~ #1759 - Reissue MSFAA - Part 1 May 8, 2023

andrewsignori-aot added the E2E/Unit tests label May 8, 2023

andrewsignori-aot changed the title ~~#1759 - Reissue MSFAA - Part 1~~ #1759 - Reissue MSFAA - Part 1 - New SIMS API Endpoint May 8, 2023

andrewsignori-aot added 6 commits May 8, 2023 13:54

Added E2E for new API endpoint

7847b25

Fix E2E tests

86341ed

E2E fixes

aaa5464

Fix E2E and Sonarcloud issues.

a02a054

Fix queue consumers E2E tests

5c392b3

Improved test descriptions

569e3df

andrewsignori-aot marked this pull request as ready for review May 8, 2023 23:34

andrewsignori-aot requested review from dheepak-aot, andrepestana-aot, ann-aot, guru-aot, JasonCTang and sh16011993 May 8, 2023 23:34

ann-aot reviewed May 9, 2023

View reviewed changes

dheepak-aot reviewed May 9, 2023

View reviewed changes

...s/packages/backend/apps/api/src/route-controllers/application/application.aest.controller.ts Show resolved Hide resolved

ann-aot reviewed May 9, 2023

View reviewed changes

PR comments

694f0d9

ann-aot approved these changes May 9, 2023

View reviewed changes

dheepak-aot reviewed May 9, 2023

View reviewed changes

andrepestana-aot reviewed May 9, 2023

View reviewed changes

andrepestana-aot approved these changes May 10, 2023

View reviewed changes

dheepak-aot approved these changes May 10, 2023

View reviewed changes

guru-aot reviewed May 10, 2023

View reviewed changes

guru-aot approved these changes May 10, 2023

View reviewed changes

andrewsignori-aot merged commit 674ad2f into main May 10, 2023

andrewsignori-aot deleted the feature/#1759-re-issue-msfaa branch May 10, 2023 19:04

andrewsignori-aot had a problem deploying to DEV May 10, 2023 19:04 — with GitHub Actions Failure

andrewsignori-aot temporarily deployed to DEV May 10, 2023 19:04 — with GitHub Actions Inactive

andrewsignori-aot temporarily deployed to DEV May 10, 2023 19:08 — with GitHub Actions Inactive

andrewsignori-aot temporarily deployed to DEV May 10, 2023 19:19 — with GitHub Actions Inactive

This was referenced Dec 4, 2023

Interface for reissue MSFAA button for Ministry Staff #1782

Closed

Updating NOA to handle new MSFAA being issued. #1781

Closed

#1759 - Reissue MSFAA - Part 1 - New SIMS API Endpoint #1928

#1759 - Reissue MSFAA - Part 1 - New SIMS API Endpoint #1928

Conversation

andrewsignori-aot commented May 5, 2023 • edited Loading

E2E Tests for reissuing the MSFAA from SIMS API

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ann-aot May 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewsignori-aot May 10, 2023 • edited Loading

Choose a reason for hiding this comment

ann-aot left a comment

Choose a reason for hiding this comment

sonarqubecloud bot commented May 9, 2023

ann-aot left a comment

Choose a reason for hiding this comment

github-actions bot commented May 9, 2023

Backend Unit Tests Coverage Report

github-actions bot commented May 9, 2023

E2E Workflow Workers Coverage Report

github-actions bot commented May 9, 2023

E2E Queue Consumers Coverage Report

github-actions bot commented May 9, 2023

E2E SIMS API Coverage Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dheepak-aot May 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dheepak-aot commented May 9, 2023

andrepestana-aot left a comment

Choose a reason for hiding this comment

andrepestana-aot left a comment

Choose a reason for hiding this comment

dheepak-aot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guru-aot left a comment

Choose a reason for hiding this comment

andrewsignori-aot commented May 5, 2023 •

edited

Loading

ann-aot May 9, 2023 •

edited

Loading

andrewsignori-aot May 10, 2023 •

edited

Loading

dheepak-aot May 9, 2023 •

edited

Loading