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

#1780 - Associate MSFAA with disbursement #1847

Merged
merged 7 commits into from
Mar 30, 2023

Conversation

dheepak-aot
Copy link
Collaborator

@dheepak-aot dheepak-aot commented Mar 28, 2023

Associate MSFAA with disbursement

  • Migration created to remove msfaa_number_id from sims.applications and add to sims.disbursement_schedules

  • Moved the worker associate-msfaa to DisbursementController from ApplicationController in workers as the associate msfaa is related to the disbursement now.

  • E2E test for disbursement controller in worker is adjusted accordingly.

  • Assessment gateway workflow modified to have the service task to associate msfaa for both original assessment and re-assessments.
    image

  • E2E test for assessment-gateway workflow is adjusted accordingly.
    image

  • As DisbursementScheduleService is already available in shared library and it has not been named as *sharedService, the import is using DisbursementScheduleService as DisbursementScheduleSharedService as we have one more DisbursementScheduleService at worker scope. Modifying DisbursementScheduleService to DisbursementScheduleSharedService is not considered to scope of PR or story.

  • The MSFAA number is now at disbursement level, but the NOA still has only one placeholder for MSFAA(not at disbursement schedule level). Hence to get the value appearing in NOA correctly, it uses the msfaa of the first disbursement.
    image

@dheepak-aot dheepak-aot self-assigned this Mar 28, 2023
@dheepak-aot dheepak-aot added SIMS-Api SIMS-Api Camunda Workers E2E/Unit tests Camunda Worflow Involves camunda workflow changes DB DB migration involved labels Mar 28, 2023
@dheepak-aot dheepak-aot marked this pull request as ready for review March 29, 2023 04:58
Copy link
Contributor

@ann-aot ann-aot left a comment

Choose a reason for hiding this comment

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

Added some comments @dheepak-aot . Good work

completedStatus: ApplicationStatus.Completed,
})
.andWhere("assessment.triggerType = :triggerType", {
triggerType: AssessmentTriggerType.OriginalAssessment,
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 that we need to mention/explain/validate this change to @JasonCTang that we are no longer relying on the original assessment offering data to check MSFAA validity.
For instance, with the change, a reassessment of an application that was reusing an existing MSFAA, may en up creating a new MSFAA if the offering start date changes.
Assuming that the application's original assessment had the offering start date of Jan 1st, 2023 and it had an MSFAA valid till Jan 15th, 2023.
If the application is reassessed and the offering change to Jan 31st, 2023, a new MSFAA will be required, right?
I believe that this is the right change, we just need to ensure that it is captured in the ticket and that Jason is aware.

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.

Great work, please take a look at the comments.

Copy link
Contributor

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

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.

Great job! I just left a comment for my knowledge.

if (!firstDisbursement) {
throw new CustomNamedError(
"Disbursement not found.",
DISBURSEMENT_NOT_FOUND,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just for my understanding: if the disbursement is not found here, it will try to associate again in the next scheduled time, right? How many times the queue tries to associate it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Associate disbursement here is the state of workflow which happens after disbursements are saved for an assessment.

image

So the expectation at this state is there must be disbursement(s) available for the assessment to make the association happen.

There must never be a situation when trying to associate msfaa, the disbursements are not found unless there is hidden bug in workflow(for e.g. Save disbursement having an error but not failing and moving to next stage).

@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 1 Code Smell

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% ( 1955 / 10853 )
Methods: 8.16% ( 115 / 1410 )
Lines: 20.71% ( 1711 / 8262 )
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 )

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

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, look good 👍

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

@dheepak-aot dheepak-aot merged commit bdbfc5f into main Mar 30, 2023
@dheepak-aot dheepak-aot temporarily deployed to DEV March 30, 2023 19:41 — with GitHub Actions Inactive
@dheepak-aot dheepak-aot temporarily deployed to DEV March 30, 2023 19:41 — with GitHub Actions Inactive
@dheepak-aot dheepak-aot temporarily deployed to DEV March 30, 2023 19:53 — with GitHub Actions Inactive
guru-aot pushed a commit that referenced this pull request Mar 30, 2023
Associate MSFAA with disbursement
@dheepak-aot dheepak-aot deleted the feature/move-msfaa-disbursement branch April 24, 2023 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Camunda Worflow Involves camunda workflow changes Camunda Workers DB DB migration involved E2E/Unit tests SIMS-Api SIMS-Api
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants