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

#4155 - Disbursement Receipt Process - Expectations on Fail #4232

Merged

Conversation

guru-aot
Copy link
Collaborator

@guru-aot guru-aot commented Jan 14, 2025

Rollback change migration screenshot
REPORT change to generate the zero record CSV.
image
CRON change for queue-configuration.
image

E2E test cases added and updated the existing e2e.

@guru-aot guru-aot self-assigned this Jan 14, 2025
@guru-aot guru-aot added the DB DB migration involved label Jan 14, 2025
@guru-aot guru-aot marked this pull request as ready for review January 14, 2025 23:11
@@ -0,0 +1,11 @@
update
Copy link
Collaborator

@sh16011993 sh16011993 Jan 15, 2025

Choose a reason for hiding this comment

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

Should we do these keywords in uppercase? For other places and files too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Are we following this in our PRs?

Copy link
Collaborator

Choose a reason for hiding this comment

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

From my previous understanding, I don't think that we are using lowercase for keywords anywhere.

@@ -62,6 +62,13 @@ export class DisbursementReceiptProcessingService {
this.esdcConfig.ftpResponseFolder,
fileSearch,
);
// Throw an error if there are no files to be processed.
if (filePaths.length === 0) {
processSummary.error(
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

SELECT
1
FROM
disbursement_receipt_dataset
Copy link
Collaborator

@sh16011993 sh16011993 Jan 15, 2025

Choose a reason for hiding this comment

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

Shouldn't the FROM be disbursement_receipts.

Currently, disbursement_receipt_dataset also works since the where conditions are the same but in future if the where conditions inside the inner select statement changes, the inner query might not return any data and therefore, if that happens, we will have to add the empty record line as expected by the business but as per the current logic it won't get added.

Just a suggestion to change it to disbursement_receipts.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It should be from the disbursement_receipt_dataset as we are trying to provide a default record when the dataset is empty.

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 Right now the where conditions in the view selected at the top via disbursement_receipt_dataset and the SELECT immediately below it are the same. In future, it might very well happen that the SELECT has a different WHERE condition which results in removing everything from the above selected view. If this happens, we will have to add the empty record line as expected by the business but as per the current logic it won't get added.

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 . Left a few comments.

expect(fileContent).toContain(
"Full Time BC Student Loan,Full Time BC Student Grant,Full Time BC Total,Part Time BC Student Grant,Part Time BC Total,BC Total,Total Records,File Date,Batch Run Date,Sequence Number",
);
expect(fileContent).toContain("0,0,0,0,0,0,0,2024-01-31,,3228");
Copy link
Collaborator

Choose a reason for hiding this comment

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

The date and sequence number can be used a constant so that it can be used across many tests in the file.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please use the constant that is already defined.

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 believe it is from existing code.

Comment on lines +780 to +794
const createdNotification = await db.notification.findOne({
select: {
id: true,
dateSent: true,
messagePayload: true,
notificationMessage: { id: true, templateId: true },
},
relations: { notificationMessage: true },
where: {
dateSent: IsNull(),
notificationMessage: {
id: NotificationMessageType.MinistryNotificationProvincialDailyDisbursementReceipt,
},
},
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

Minor and not blocker. I see this code repeating a lot. We can create a common function and re-use.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

will do as part of another effort

Comment on lines 105 to 106
UNION
ALL
Copy link
Collaborator

Choose a reason for hiding this comment

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

While setting a default record on result set being empty UNION vs UNION ALL does not make any difference and UNION is more accurate here. But I won't consider it as blocker.

@dheepak-aot
Copy link
Collaborator

Great work @guru-aot. Please have a look at the comments.

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 doing the changes. Looks good 👍

Copy link

Backend Unit Tests Coverage Report

Totals Coverage
Statements: 22.52% ( 3864 / 17158 )
Methods: 10.33% ( 224 / 2169 )
Lines: 25.93% ( 3336 / 12865 )
Branches: 14.31% ( 304 / 2124 )

Copy link

E2E Workflow Workers Coverage Report

Totals Coverage
Statements: 65.59% ( 589 / 898 )
Methods: 59.63% ( 65 / 109 )
Lines: 68.72% ( 468 / 681 )
Branches: 51.85% ( 56 / 108 )

Copy link

E2E Queue Consumers Coverage Report

Totals Coverage
Statements: 86.14% ( 1249 / 1450 )
Methods: 82.42% ( 136 / 165 )
Lines: 88.51% ( 1032 / 1166 )
Branches: 68.07% ( 81 / 119 )

Copy link

E2E SIMS API Coverage Report

Totals Coverage
Statements: 67.77% ( 5994 / 8844 )
Methods: 65.57% ( 739 / 1127 )
Lines: 71.63% ( 4696 / 6556 )
Branches: 48.15% ( 559 / 1161 )

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.

Great work @guru-aot 👍

@guru-aot guru-aot added this pull request to the merge queue Jan 16, 2025
Merged via the queue into main with commit ce07ef1 Jan 16, 2025
21 checks passed
@guru-aot guru-aot deleted the feature/#4155-Disbursement_Receipt_Process_Fail_Expectations branch January 16, 2025 17:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants