feat: add completionCondition + cancelRemainingInstances properties to ad-hoc subprocess #1114
Conversation
maff
force-pushed
the
feature/4850-add-ad-hoc-completion-condition-handling
branch
2 times, most recently
from
574938c to
e4f2342
Compare
| * @param {string} newValue | ||
| * @param {BpmnFactory} bpmnFactory | ||
| */ | ||
| function updateFormalExpression(element, propertyName, newValue, bpmnFactory) { |
There was a problem hiding this comment.
There is a certain amount of duplication across other properties which handle expressions - this one was mostly adapted from the MultiInstanceProps. Do we have a preference on duplication vs. extracting into reusable utils/helpers?
There was a problem hiding this comment.
Given that this is the third place where we can update formal expression (multiinstance, sequence flow condition, ad-hoc subprocess), having a util in https://github.com/bpmn-io/bpmn-js-properties-panel/tree/main/src/utils is a good idea.
There was a problem hiding this comment.
Refactored into a utility in 68d86a8 and adapted the 2 new AdHoc props implementations to use that one. I would create a follow-up PR to adapt the other code parts as well to not expand the scope of this PR.
There was a problem hiding this comment.
Refactored into a utility in 68d86a8 and adapted the 2 new AdHoc props implementations to use that one. I would create a follow-up PR to adapt the other code parts as well to not expand the scope of this PR.
As long as you clearly separate refactoring and your improvement in separate commits we're fine. We don't have limits in terms of size of a PR, what we ask for is that the commits are semantically structured. Your changes will also not be squashed, so whatever work you structure will be "followable" later on in the commit history.
There was a problem hiding this comment.
Fully agree on the semantically structured commits. I'm open to both - the follow-up PR would make it easier to follow the refactoring-only change on unrelated components with a dedicated PR description, but on the other hand changing other components here makes it easier to verify that the abstraction of the helper works properly for multiple use cases 😉
I now updated the MultiInstanceProps to use the new helper in 533f220 but kept the sequence flow as it was as its logic seems a bit different than the other 2.
maff
force-pushed
the
feature/4850-add-ad-hoc-completion-condition-handling
branch
3 times, most recently
from
f88e1fe to
3986b64
Compare
Only applied to the new ad-hoc subprocess completions now, but we can adapt other parts as well in a follow-up.
maff
force-pushed
the
feature/4850-add-ad-hoc-completion-condition-handling
branch
from
fef86fa to
68d86a8
Compare
bpmn-io-tasks
bot
added
needs review
…nt for more flexibility
|
Is it the logical order in which I configure an ad-hoc subprocess? Initially, I thought that I'd first define which activities will be activated when element is reached, and only then I'd define the completion condition. However, the condition can be independent from the actions that the ad-hoc subprocess executor can take. I think the order is correct, but still want to flag it for discussion. |
💡 That's a very good point. I think they can independent, but it may be more logical to move the completion condition below the active elements when thinking at the flow of the process (first activate, then complete). I'll try to update it accordingly. |
|
Updated to reorder the groups in 1ab3114:
|
…t is unchecked as the default value is true
Proposed Changes
Adds support to configure
completionCondition(expression) andcancelRemainingInstances(boolean) for ad-hoc subprocesses.BPMN implementation
Zeebe specific implementation
Related to camunda/camunda-modeler#4850.
Checklist
To ensure you provided everything we need to look at your PR:
@bpmn-io/srtoolCloses {LINK_TO_ISSUE}orRelated to {LINK_TO_ISSUE}