#4076 - Queue Monitoring - Schedulers Refactor (Fed Restrictions and Receipts) #4135

andrewsignori-aot · 2024-12-18T01:12:54Z

Refactored schedulers disbursement-receipts-file-integration and federal-restrictions-integration.
Adjusted existing E2Es.
As agreed in previous PRs
- moving the audit user to the service instead of the processor.
- removing the friendly start/end logs from the processor since the BaseQueue is already logging similar start/end logs.

E2E `containLogMessages` changes

To allow the below check to happen the method containLogMessages was changed to check if the string "contains" a value instead of "endsWith" it. The endsWith was used to have a more precise check and remove the log initial information (e.g. context and date). Using the "contains" will still give enough assertion precision and allow inspecting errors currently serialized to a JSON.

 // Act/Assert
await expect(processor.processQueue(mockedJob.job)).rejects.toThrow(
  "One or more errors were reported during the process, please see logs for details.",
);
expect(
  mockedJob.containLogMessages([
    `Error downloading file ${expectedFileName}.`,
    "Invalid file footer.",
  ]),
).toBe(true);

lewischen-aot

Looks good to me 👍 Thanks for refactoring this part of the schedulers.

dheepak-aot · 2024-12-18T22:03:40Z

...schedulers/esdc-integration/ecert-integration/disbursement-receipts-integration.scheduler.ts

+    processSummary: ProcessSummary,
+  ): Promise<string> {
+    await this.disbursementReceiptProcessingService.process(processSummary);
+    return "Completed disbursement receipts integration.";


...esdc-integration/disbursement-receipt-integration/disbursement-receipt.processing.service.ts

dheepak-aot · 2024-12-18T22:22:05Z

...esdc-integration/disbursement-receipt-integration/disbursement-receipt.processing.service.ts

-        this.logger.error(error);
+        this.logger.error(logMessage, error);
+        processSummary.error(logMessage, error);
+        return;


Adding the return here will modify the existing behavior. i.e. if there is an error processing one or more detail record, then we skip the those one and process others. With return it will abort at first failure.

Is there a reason for making this change?

Yes, it is changing the behavior and keeping the file on error. I will remove to short the conversation and move on with the PR but I do not recall why we chose this direction. Do you remember? If not, I will start a separate conversation with biz.

I believe we chose this direction as each receipt record can be considered independently and processed ignoring one or more records with data errors or other errors. But I would say we should check the direction with business.

As we talked, I will check with the business and consider if we should generate the report or not.
For this ticket and this PR, I will keeping the existing behavior as suggested.

...esdc-integration/disbursement-receipt-integration/disbursement-receipt.processing.service.ts

dheepak-aot · 2024-12-18T22:32:33Z

...esdc-integration/disbursement-receipt-integration/disbursement-receipt.processing.service.ts

+      const errorMessage =
+        "Error while generating provincial daily disbursement CSV report file.";
+      this.logger.error(errorMessage, error);
+      processSummary.error(errorMessage, error);


Should the retry happen when the report generation alone fail? - let me know if I am missing something here.

Is there a reason not to retry? The import should be able to be executed twice and the report generation is part of the process, hence if it fails it would not need manual intervention.
pleas let me know if we need to discuss this further.

As we discussed, let's keep the retry to allow the report to be sent and the job auto-recover avoiding the manual intervention.

dheepak-aot · 2024-12-18T22:37:51Z

...tions/src/esdc-integration/fed-restriction-integration/fed-restriction.processing.service.ts

-    let downloadResult: FedRestrictionFileRecord[];
+    processSummary: ProcessSummary,
+  ): Promise<void> {
+    processSummary.info(`Processing file ${remoteFilePath}.`);
    this.logger.log(`Starting download of file ${remoteFilePath}.`);


#4135 (comment) also at line 109

To be clear, I did not remove them all because they were serving different purposes and I did not consider it as part of the immediate refactor. I can explain further if needed. I will remove this also.

dheepak-aot · 2024-12-18T22:43:50Z

...tions/src/esdc-integration/fed-restriction-integration/fed-restriction.processing.service.ts

@@ -139,7 +132,7 @@ export class FedRestrictionProcessingService {
        const invalidDataMessage = restriction.getInvalidDataMessage();
        if (invalidDataMessage) {
          const errorMessage = `Found record with invalid data at line number ${restriction.lineNumber}: ${invalidDataMessage}`;
-          result.errorsSummary.push(errorMessage);
+          processSummary.error(errorMessage);


shouldn't we use processSummary.warn as we are not expecting retry for invalid data in the file.?

This will trigger a different conversation. Yes, we can change to warn to avoid the retry, but it also should alert the Ministry in some way. I will change to a warn to move on and discuss further with biz.

dheepak-aot · 2024-12-18T22:51:47Z

sources/packages/backend/apps/queue-consumers/test/helpers/mock-utils/job-mock-utils.ts

   * @returns true if the log message was found. otherwise false.
   */
  containLogMessage(logMessage: string): boolean {
-    return this.logMessages.some((message) => message.endsWith(logMessage));
+    return this.logMessages.some((message) => message.includes(logMessage));


dheepak-aot · 2024-12-18T22:53:42Z

Good Job @andrewsignori-aot. Added some comments. Few of them were clarifications too.

sonarqubecloud · 2024-12-18T23:31:12Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

github-actions · 2024-12-18T23:32:01Z

Backend Unit Tests Coverage Report

Totals
Statements:	22.08% ( 3783 / 17133 )
Methods:	10.07% ( 219 / 2175 )
Lines:	25.47% ( 3278 / 12868 )
Branches:	13.68% ( 286 / 2090 )

github-actions · 2024-12-18T23:32:20Z

E2E Workflow Workers Coverage Report

Totals
Statements:	65.43% ( 583 / 891 )
Methods:	59.26% ( 64 / 108 )
Lines:	68.54% ( 464 / 677 )
Branches:	51.89% ( 55 / 106 )

github-actions · 2024-12-18T23:34:46Z

E2E Queue Consumers Coverage Report

Totals
Statements:	81.73% ( 1302 / 1593 )
Methods:	67.51% ( 133 / 197 )
Lines:	84.9% ( 1091 / 1285 )
Branches:	70.27% ( 78 / 111 )

github-actions · 2024-12-18T23:37:18Z

E2E SIMS API Coverage Report

Totals
Statements:	67.4% ( 5922 / 8787 )
Methods:	65.09% ( 729 / 1120 )
Lines:	71.33% ( 4647 / 6515 )
Branches:	47.4% ( 546 / 1152 )

dheepak-aot

Thanks for making the changes. Looks good 👍

andrewsignori-aot added 2 commits December 17, 2024 14:31

Federal restrictions refactor

63d6e09

Fed restrictions and receipt refactors

757bb07

andrewsignori-aot added the Queue Consumers label Dec 18, 2024

andrewsignori-aot self-assigned this Dec 18, 2024

andrewsignori-aot added 3 commits December 18, 2024 11:22

Merge branch 'main' into feature/#4076-schedulers-refactor-part-3

0afa735

Refactor for audit user and friendly processor messages.

33bfe16

Pre-review

e5bb792

andrewsignori-aot marked this pull request as ready for review December 18, 2024 20:09

andrewsignori-aot changed the title ~~#4076 - Queue Monitoring - Schedulers Refactor (Fed Restrictions and more)~~ #4076 - Queue Monitoring - Schedulers Refactor (Fed Restrictions and Receipts) Dec 18, 2024

lewischen-aot self-requested a review December 18, 2024 21:21

lewischen-aot approved these changes Dec 18, 2024

View reviewed changes

dheepak-aot reviewed Dec 18, 2024

View reviewed changes

dheepak-aot added the E2E/Unit tests label Dec 18, 2024

dheepak-aot reviewed Dec 18, 2024

View reviewed changes

...esdc-integration/disbursement-receipt-integration/disbursement-receipt.processing.service.ts Outdated Show resolved Hide resolved

dheepak-aot reviewed Dec 18, 2024

View reviewed changes

...esdc-integration/disbursement-receipt-integration/disbursement-receipt.processing.service.ts Outdated Show resolved Hide resolved

dheepak-aot reviewed Dec 18, 2024

View reviewed changes

PR review comments

77ba677

dheepak-aot approved these changes Dec 19, 2024

View reviewed changes

andrewsignori-aot added this pull request to the merge queue Dec 19, 2024

Merged via the queue into main with commit 77f6d41 Dec 19, 2024
21 checks passed

andrewsignori-aot temporarily deployed to DEV December 19, 2024 21:10 — with GitHub Actions Inactive

andrewsignori-aot temporarily deployed to DEV December 19, 2024 21:11 — with GitHub Actions Inactive

andrewsignori-aot temporarily deployed to DEV December 19, 2024 21:14 — with GitHub Actions Inactive

andrewsignori-aot deleted the feature/#4076-schedulers-refactor-part-3 branch December 19, 2024 21:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#4076 - Queue Monitoring - Schedulers Refactor (Fed Restrictions and Receipts) #4135

#4076 - Queue Monitoring - Schedulers Refactor (Fed Restrictions and Receipts) #4135

andrewsignori-aot commented Dec 18, 2024 •

edited

Loading

lewischen-aot left a comment

dheepak-aot Dec 18, 2024

dheepak-aot Dec 18, 2024 •

edited

Loading

andrewsignori-aot Dec 18, 2024

dheepak-aot Dec 19, 2024

andrewsignori-aot Dec 19, 2024

dheepak-aot Dec 18, 2024

andrewsignori-aot Dec 18, 2024

andrewsignori-aot Dec 19, 2024

dheepak-aot Dec 18, 2024 •

edited

Loading

andrewsignori-aot Dec 18, 2024

dheepak-aot Dec 18, 2024

andrewsignori-aot Dec 18, 2024

dheepak-aot Dec 18, 2024

dheepak-aot commented Dec 18, 2024

sonarqubecloud bot commented Dec 18, 2024

github-actions bot commented Dec 18, 2024

github-actions bot commented Dec 18, 2024

github-actions bot commented Dec 18, 2024

github-actions bot commented Dec 18, 2024

dheepak-aot left a comment

#4076 - Queue Monitoring - Schedulers Refactor (Fed Restrictions and Receipts) #4135

#4076 - Queue Monitoring - Schedulers Refactor (Fed Restrictions and Receipts) #4135

Conversation

andrewsignori-aot commented Dec 18, 2024 • edited Loading

E2E containLogMessages changes

lewischen-aot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dheepak-aot Dec 18, 2024 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dheepak-aot Dec 18, 2024 • 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

Choose a reason for hiding this comment

dheepak-aot commented Dec 18, 2024

sonarqubecloud bot commented Dec 18, 2024

Quality Gate passed

github-actions bot commented Dec 18, 2024

Backend Unit Tests Coverage Report

github-actions bot commented Dec 18, 2024

E2E Workflow Workers Coverage Report

github-actions bot commented Dec 18, 2024

E2E Queue Consumers Coverage Report

github-actions bot commented Dec 18, 2024

E2E SIMS API Coverage Report

dheepak-aot left a comment

Choose a reason for hiding this comment

andrewsignori-aot commented Dec 18, 2024 •

edited

Loading

E2E `containLogMessages` changes

dheepak-aot Dec 18, 2024 •

edited

Loading

dheepak-aot Dec 18, 2024 •

edited

Loading