-
Notifications
You must be signed in to change notification settings - Fork 14
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
#4079 - Queue Monitoring - Schedulers Refactor (Part 6) #4177
#4079 - Queue Monitoring - Schedulers Refactor (Part 6) #4177
Conversation
@@ -86,7 +90,7 @@ export class ATBCIntegrationProcessingService { | |||
this.logger.log( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we remove this logger?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed.
processSummary.info( | ||
`Total notifications processed ${processNotificationResponse.notificationsProcessed}.`, | ||
); | ||
processSummary.info( | ||
`Total notifications successfully processed ${processNotificationResponse.notificationsSuccessfullyProcessed}.`, | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor. As this is a result, it can be part of return. As there is no E2E tests for this scheduler, there won't be further impacts in changing the return.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added the result to the summary and result also.
if ( | ||
processNotificationResponse.notificationsProcessed !== | ||
processNotificationResponse.notificationsSuccessfullyProcessed | ||
) { | ||
processSummary.warn( | ||
"Not all pending notifications were successfully processed.", | ||
); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
"Found 0 application changes.", | ||
`Application changes report with file name ${expectedFileName} has been uploaded successfully.`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
// Mock the current date. | ||
const now = new Date(); | ||
MockDate.set(now); | ||
expectedFileName = `DPBC.EDU.APPCHANGES.${formatFileNameDate(now)}.csv`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great Job. Only 2 minor comments.
Recording our conversation regarding moving the code to add the message to queue inside the transaction here. workflow-enqueuer.service.ts
It is outside the scope of the PR.
expect( | ||
mockedJob.containLogMessages([ | ||
"Retrieving all application changes which were not reported already.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very minor: maybe it could be rephrased to: "Retrieving all application changes that have not yet been reported."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated as suggested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Just minor comments.
Quality Gate passedIssues Measures |
Refactored the below schedulers and associated E2E tests.