-
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
#1834 - Service task identifier for subprocesses #1842
#1834 - Service task identifier for subprocesses #1842
Conversation
The e2e test cases for assessment gatewat for program year 2021-2022 is removed. Is that because those years are no longer used and its just a duplication? |
...workflow/test/2021-2022/assessment-gateway/assessment-gateway.originalAssessment.e2e-spec.ts
Outdated
Show resolved
Hide resolved
...workflow/test/2023-2024/assessment-gateway/assessment-gateway.originalAssessment.e2e-spec.ts
Show resolved
Hide resolved
WorkflowSubprocesses.PartnerIncomeVerification, | ||
WorkflowSubprocesses.RetrieveSupportingInfoParent1, | ||
WorkflowSubprocesses.Parent1IncomeVerification, | ||
WorkflowSubprocesses.RetrieveSupportingInfoParent2, | ||
WorkflowSubprocesses.Parent2IncomeVerification, | ||
); |
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.
👍
sources/packages/backend/workflow/test/test-utils/constants/system-configurations-constants.ts
Show resolved
Hide resolved
...orkflow/test/test-utils/mock/assessment-gateway/create-supporting-users-parents-task-mock.ts
Outdated
Show resolved
Hide resolved
* @returns mock for the 'Load consolidated data' completed subprocess. | ||
*/ | ||
export function createLoadAssessmentConsolidatedDataMock(options: { | ||
assessmentConsolidatedData: AssessmentConsolidatedData; |
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.
comment missing
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.
Done.
const assessmentConsolidatedData: AssessmentConsolidatedData = { | ||
assessmentTriggerType: AssessmentTriggerType.OriginalAssessment, | ||
...createFakeConsolidatedFulltimeData(PROGRAM_YEAR), | ||
...createParentsData({ numberOfParents: 2 }), |
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.
I believe that there is confusion here. The service task above has specific mocked data to return its data.
The assumption that parent data is returned by the workflow in the picture is not correct.
All the data (I mean, all the data) to process an assessment (and reassessments) is returned in the consolidated data.
Keep in mind that the reassessment does not go down this path and if this was the workflow responsible for loading the parent's data the reassessment would never have supporting users' data.
Please let me know If it is clear or if I am missing something.
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 work @andrewsignori-aot 👍 . Added some minor comments. I believe I got the idea, let me know if you planning to do a walkthrough, I would also like to join, to cover up in case I miss something.
Kudos, SonarCloud Quality Gate passed!
|
serviceTasks.forEach((serviceTask) => { | ||
expect(workflowResultVariables[serviceTask]).toBeUndefined; | ||
expectedTasks.forEach((expectedTask) => { | ||
expect(workflowResultVariables[expectedTask]).toBeUndefined(); |
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.
👍
* @param assessmentConsolidatedData assessment consolidated data. | ||
* @returns mock for the 'Load consolidated data' completed subprocess. | ||
*/ | ||
export function createLoadAssessmentConsolidatedDataMock(options: { |
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.
is options not optional ?. May be assessmentConsolidatedData not one of the options?
Great work and thanks for the walkthrough. In github actions I saw these logs which I haven't seen before. Still have doubts on about #1842 (comment) and we can talk to get my understanding better. |
*/ | ||
export function createCheckSupportingUserResponseTaskMock(options: { | ||
totalIncome: number; | ||
subprocesses?: WorkflowSubprocesses; |
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.
My be we could name it subprocess as we not take an array.
* Camunda variable name like service_task_id. | ||
*/ | ||
function getNormalizedServiceTaskId(serviceTaskId: string) { | ||
return serviceTaskId.replace( |
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.
I would assume that if we will be changing the way that we are using the IDs we would be changing everywhere, not limited to only tasks, and also renaming the subprocesses calls.
We can do it but I do not believe that it would be the scope of this PR.
|
||
/** | ||
* Creates the mock for the 'Load consolidated data' completed subprocess. | ||
* @param assessmentConsolidatedData assessment consolidated data. |
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.
@param options
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! I left some comments
*/ | ||
export function createMockedWorkerResult( | ||
serviceTaskId: WorkflowServiceTasks, | ||
options: { |
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.
Shouldn't options be optional?
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.
For this options
is not optional because ideally one of the three parameters should be provided.
if (options?.subprocesses?.length) { | ||
fullServiceTaskId = [...options.subprocesses, fullServiceTaskId].join( | ||
MOCKS_SEPARATOR, | ||
); | ||
} | ||
const mockedWorkerResult: Record<string, unknown> = {}; | ||
if (options.jobCompleteMock) { |
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.
Either options is optional and we do the check with "?" or we don't need to check for null.
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.
Is this a comment for line 164?
For line 164 options will not be null but options.jobCompleteMock
can be null.
The comment would make sense for line 158 where we can have it as if (options.subprocesses?.length)
. Make sense?
totalIncome: options.totalIncome, | ||
}, | ||
subprocesses: [options?.subprocesses], |
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.
LGTM, nice work @andrewsignori-aot
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 work @andrewsignori-aot . Thanks for doing the changes and working on organizing the structure 👍
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 the walk through and nice discussions. LGTM. 👍
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 the explanation about how it works.
Test cases enabled
To provide complete support for mocking the workers, and allowing the workflow to be tested including the subprocesses, some scenarios from parents were used. As part of this PR the below scenarios were enabled:
Single Mocked Worker
Created a single mocked worker method to return the job completed object and also publish messages needed to unblock the workflow.
While creating the mocked result for a worker, it is possible to specify the object to be returned to Camunda and also configure 1 to n messages that must be published to unblock the workflow. As shown below, the
cra-integration-income-verification
needs to be completed using 3 mocked objects created using 2 helpers.Create income request
must have some data returned;Create income request
is completed, the workflow must receive a message, also mocked and shown on the below message.incomeVerificationCompleted as true
to proceed.Validation/Asserts
Examples of variables available on the workflow to perform the validations related to the flow path
expectToPassThroughServiceTasks
andexpectNotToPassThroughServiceTasks
.The value that indicates if the service task or subprocess was changed from
true
to thestring
above makes it easier to identify which validation failed, as shown below.Camunda bpmn configurations for subprocess identification
Sample mocked start event data
Scenario:
Variable names were adapted trying to follow Camunda recommendations for variables names: https://docs.camunda.io/docs/components/concepts/variables/#variable-names