-
Notifications
You must be signed in to change notification settings - Fork 59
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
Bataller García Eric Create_CancelledWorkflowStepFromInstruction added #3195
base: master
Are you sure you want to change the base?
Conversation
Added the Create_CancelledWorkflowStepFromIntruction function to allow its migration from DRR to CDM
✅ Deploy Preview for finos-cdm ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Business Events - Cancelled workflow stepBackground In CDM, there is the function What is being released? Functions
Review directions In CDM, select the Textual Browser and inspect each of the changes identified above. PR: #3195 |
condition ProposedEventExists: <"The previous step being cancelled must be a proposed step containing an instruction."> | ||
proposedWorkflowStep -> proposedEvent exists | ||
|
||
condition CancelledProposedStep: <"The previous step must be a cancel"> |
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 really the previous step? If we're checking our input, "proposedWorkflowStep", then its the instruction and not the previous step, is that correct?
Would the comments thus be better if they said something like "Check the instruction has a proposed event..." and "... is set as a Cancel..."?
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.
Hi Chris, sorry for the late response. Your description change looks fine to me, it makes this clearer. I will recontribute the changes.
set cancelledWorkflowStep -> previousWorkflowStep: proposedWorkflowStep -> previousWorkflowStep as-key | ||
|
||
set cancelledWorkflowStep -> businessEvent: <"Assign the business event corresponding to the workflow step."> | ||
Create_BusinessEvent( |
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.
If we're cancelling a WorkflowStep, and the WorkflowStep has a proposedEvent in it, as confirmed in an earlier condition, why are we now creating a BusinessEvent? Isn't a BusinessEvent only created as a result of an accepted WorkflowStep?
Here you're checking for the existence of a proposedEvent which implies that the workflow has not completed successfully and there should be no BusinessEvent at this point?
If there is a businessEvent on the WorkflowStep already then I can see the logic in including it on the cancelledWorkflowStep, but I can't see why we would want to create a new BusinessEvent for a cancelled WorkflowStep.
Can you give a bit more detail please in case 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.
Hi, the proposal is fulfilling the requirement to represent that the previous step was accepted by mistake. We use it as previous state to propose a new instruction, in which the cancellation flag is set and it's executed through the proposed function.
Requirement in DRR was to be able to represent an ERROR on previously submitted states.
Updated description of steps for Create_CancelledWorkflowStepFromInstruction
@chrisisla @ebataller can you please confirm if this PR is ready for release process or is it still under review |
Hi @PayalKhanna , |
No description provided.