#4354 - Change Request Revamp - 1st effort - Warping up workflow during "in-progress" application #4491
Conversation
andrewsignori-aot
added
Camunda Workers
Camunda Worflow
andrewsignori-aot
changed the title
andrewsignori-aot
changed the title
andrewsignori-aot
changed the title
| * @param workflowData workflow data. | ||
| * @param options customize the payload. | ||
| * - `workflowData` workflow data. | ||
| * - `wrapUpType` wrap-up type, default {@link WorkflowWrapUpType.CompleteWrapUp} if not provided. |
...kers/src/controllers/assessment/_tests_/e2e/assessment.controller-workflowWrapUp.e2e-spec.ts
Show resolved
Hide resolved
| const studentAssessment = await studentAssessmentRepo.findOne({ | ||
| select: { studentAssessmentStatus: true, workflowData: true as unknown }, | ||
| select: { studentAssessmentStatus: true }, |
There was a problem hiding this comment.
Not part of the PR. But selecting id here would make the query(sql) better as we have observed.
There was a problem hiding this comment.
Just to ensure that we are on the same page, related to SQL/DB, the query is better without the ID since it is not required.
From Typeorm's perspective, it may fail for child objects when the parent does not have a single property as part of the select, and that leads us to include the ID in the parent object sometimes.
There was a problem hiding this comment.
I meant, from what I remembered in the past if we do not select the id, typeorm was creating a SQL select with id and also the selected properties(from find/findone) and was pulling the selected properties from that query output.
I am not sure if that was the right understanding, but it looks like typeorm always uses id irrespective if it being used in select or not.
e.g.
await db.studentAssessment.findOne({
select: { id: true, studentAssessmentStatus: true },
where: { id: application.currentAssessment.id },
});results in
select
"StudentAssessment"."student_assessment_status" as "StudentAssessment_student_assessment_status",
"StudentAssessment"."id" as "StudentAssessment_id"
from
"sims"."student_assessments" "StudentAssessment"
where
(("StudentAssessment"."id" = $1))
limit 1
| if (workflowIsCancelled) { | ||
| if (!options?.workflowData) { | ||
| // No update is executed because the data was already present or the workflow was cancelled. | ||
| return false; |
There was a problem hiding this comment.
May or may not be part of the PR. But this logic should be part of the controller rather than service.
There was a problem hiding this comment.
It may, but the controller is already executing too much and I was not focusing on going further in any refactoring.
The controller logic should be segregated already in at least two other methods but I have no intention to address this as part of this ticket goal 😉
| @@ -261,52 +267,67 @@ export class AssessmentController { | |||
| jobLogger.error(message); | |||
| return job.error(ASSESSMENT_NOT_FOUND, message); | |||
| } | |||
| jobLogger.log( | |||
| `Workflow wrap-up type requested: ${job.customHeaders.wrapUpType}.`, | |||
There was a problem hiding this comment.
If the backward-compatibility is also the intention this will print null if previous version of workflow is used.
There was a problem hiding this comment.
Yes, it will. The backward compatibility will be achieved where it matters. The workflow needs to be backward-compatible enough to allow any in-flight assessment to be finished. Any new assessment executed once this is deployed will start using the header independently if it is a change request or not.
I am not sure if it i clear but one point is the backward compatibility mentioned in the ticket that will allow assessments to be executed.
The backward compatibility mentioned in the PR description is to allow the in-flight workflows to be finalized when a worker is changed, in this case, to receive a new header.
There was a problem hiding this comment.
I get the intention more precisely when the in-flight instances are mentioned. I am good here.
| ); | ||
| if (jobActionAcknowledgement) { | ||
| // If there is a job result to be returned, the worker should finalize its processing. | ||
| return jobActionAcknowledgement; |
| * Executes all wrap-up operations, setting the assessment status to completed and | ||
| * executing other processes related to sequential processing. | ||
| */ | ||
| CompleteWrapUp = "Complete wrap-up", |
There was a problem hiding this comment.
Minor, it could be just Complete as it belongs to wrap up type.
There was a problem hiding this comment.
Changed.
| <bpmn:extensionElements> | ||
| <zeebe:taskDefinition type="workflow-wrap-up" /> | ||
| <zeebe:taskHeaders> | ||
| <zeebe:header key="wrapUpType" value="Assessment status only" /> |
There was a problem hiding this comment.
It could be a variable initialized at start of workflow and used across many places.
There was a problem hiding this comment.
I do not see it as a benefit or a better approach. The headers are meant to pass parameters to influence the worker process behavior, which is exactly what we are executing here.
There was a problem hiding this comment.
But what is mentioned above is not about usage of headers. It is about using a value from variable instead of repeating the value. Let me know if I am missing something here.
There was a problem hiding this comment.
I got it now that it is related to a broader conversation about the enum usage in the workflow and a possible way to have an enum-like structure type in BPMN. I will take a look outside this PR effort. Thanks for the chat.
|
Great way to wrap up the workflow ends. Added few comments. Please have a look. |
|
E2E Workflow Workers Coverage Report
|
Backend Unit Tests Coverage Report
|
E2E Queue Consumers Coverage Report
|
E2E SIMS API Coverage Report
|
andrewsignori-aot
deleted the
feature/#4431-set-assessment-status-as-completed
branch
assessment-gateway"Applications In Progress" lane, as shown in items 1 to 3 in the below image (image 4 is working as before).job.complete()was returned from inside a transaction block with the intention of finishing the worker task but was actually finishing only the transaction block, leaving the rest of the worker processing to still be executed.The workflow task now accepts a header to define its behavior, as shown below.