-
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
#2180 - Queued Assessments - Assessment Workflow Enqueuer Scheduler - Start Assessment (Part 1) #2285
#2180 - Queued Assessments - Assessment Workflow Enqueuer Scheduler - Start Assessment (Part 1) #2285
Conversation
sources/packages/backend/apps/queue-consumers/src/services/application/application.service.ts
Show resolved
Hide resolved
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.
Good job. Left some questions.
.../apps/db-migrations/src/sql/StudentAssessments/Rollback-update-student-assessment-status.sql
Show resolved
Hide resolved
...ces/packages/backend/apps/queue-consumers/src/services/workflow/workflow-enqueuer.service.ts
Outdated
Show resolved
Hide resolved
...ces/packages/backend/apps/queue-consumers/src/services/workflow/workflow-enqueuer.service.ts
Outdated
Show resolved
Hide resolved
sources/packages/backend/apps/api/src/services/student-appeal/student-appeal.service.ts
Show resolved
Hide resolved
...d/apps/api/src/services/student-scholastic-standings/student-scholastic-standings.service.ts
Show resolved
Hide resolved
...packages/backend/apps/db-migrations/src/sql/Queue/Add-assessment-workflow-enqueuer-queue.sql
Show resolved
Hide resolved
sources/packages/backend/apps/queue-consumers/src/services/application/application.service.ts
Show resolved
Hide resolved
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.
Good work @andrewsignori-aot 👍 Thanks for the explanation.
AND sims.applications.application_status NOT IN ( |
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.
👍
...s/backend/apps/db-migrations/src/sql/StudentAssessments/Update-student-assessment-status.sql
Outdated
Show resolved
Hide resolved
.../apps/db-migrations/src/sql/StudentAssessments/Rollback-update-student-assessment-status.sql
Show resolved
Hide resolved
|
||
@Process() | ||
async startAssessment(job: Job<StartAssessmentQueueInDTO>) { | ||
let workflowName = job.data.workflowName; |
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.
❤️
) | ||
.getQuery(); | ||
return ( | ||
this.applicationRepo |
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.
Just wondering, this query could have come from student assessments instead of applications right? so that it will just return the assesments(assessment id) which needs to be 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.
True. It was originally designed to allow the later update of the application + assessment together but it did not work as expected and I ended up doing the individual updates in a transaction.
Right now the only benefit is that it makes it pretty easy to have the assessments grouped to allow the processing and also pick the next assessment to be processed if multiple exists under the same application.
currentProcessingAssessment: { id: nextAssessment.id }, | ||
}); | ||
if (!applicationUpdateResult.affected) { | ||
throw new Error("Application update did not affected any records."); |
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.
The error message could be more specific e.g. currentProcessingAssessment id was already set or incorrect application id. something like that.
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.
That log itself may not provide the entire picture, but please check below the logs around it. As shown below there is some additional info that would make it easier to have it traced.
About the suggestions:
- "currentProcessingAssessment id was already set" would not be accurate.
- "incorrect application id" can be checked using the additional logs also.
Queueing next pending assessment for application id 123.
Found 2 pending assessment(s). Queueing assessment 999.
Associating application currentProcessingAssessment as assessment id 999.
(...)
Application update did not affected any records.
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.
By the way, planning to do some logging improvement in the next PR.
}); | ||
if (!assessmentUpdateResults.affected) { | ||
throw new Error( | ||
"Student assessment update did not affected any records.", |
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 with creating the queue to process assessments. Great to see the depth of analysis and considerations involved in the effort. Thanks for explaining the issue with migrations. Please have a look at the comments. |
Kudos, SonarCloud Quality Gate passed!
|
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! Thanks for the explanation.
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.
Thanks for making the changes 👍
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.
LGTM, nice work @andrewsignori-aot
startAssessment
removed since it is no longer needed.hasIncompleteAssessment
andassertAllAssessmentsCompleted
removed since there is no purpose anymore.StartApplicationAssessment
queue can now be started without providing theworkflowName
.assessment_workflow_id
, andassessment_data
. The rollback is less meaningful and it was created to try to bring the column back to its original state where the column was set with its default valueSubmitted
.TODO:
ProcessSummaryResult
from integrations tolib
to allow its use outside integrations.