-
Notifications
You must be signed in to change notification settings - Fork 2
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
feat: add rules for ad-hoc subprocess completion configuration #196
feat: add rules for ad-hoc subprocess completion configuration #196
Conversation
…ainingInstances only supported from camunda 8.8
4811713
to
ff4e5e8
Compare
Thanks for the contribution. So if I understand it correctly, in 8.7 we add ad-hoc subprocess support, but the completion condition and "cancel remaining instances" are not available, right? |
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.
Check out the comments for potential improvement. Then, we can merge the PR. Great quality of the contribution 🚀
data: { | ||
node, | ||
parentNode: null | ||
if (!greaterOrEqual(version, COMPLETION_ATTRIBUTES_SUPPORT_VERSION)) { |
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.
Let's use the hasProperties
or hasProperty
helper for the checks. This decouples the error message from the rule, and also attaches the metadata (error type) consistently.
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.
Example:
errors = hasProperties(subscription, { |
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.
Another one with allowedVersion:
allowedVersion: '8.0' |
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.
Updated it to use the hasProperties
check - as far as I could see the helper still needs the version check around the call as the version passed as argument is only used to generate the error message. Still needed to keep that check on the cancelRemainingInstances
property being defined to achieve the desired check.
]; | ||
|
||
RuleTester.verify('called-element', rule, { | ||
RuleTester.verify('ad-hoc-sub-process', rule, { |
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.
Scout rule - I like it :)
Thanks for the feedback!
Exactly. Technically the process could be deployed on 8.7 including these attributes as they are defined in the spec, but the engine would not make use of them. Would you have concerns raising them as an error although the process can be deployed? |
I think it's the right approach. This way, we can clearly tell the user that certain features are not supported (cf. no surprises). |
I've pushed small improvements via 7ba0097, and this is now ready to be merged. Thank you for contributing this! |
The
completionCondition
expression andcancelRemainingInstances
attribute will only be supported in Camunda 8.8+. This PR adds support to raise an error if one of these properties is set on a lower version.Proposed Changes
cancelRemainingInstances
attribute is set totrue
: raise an error if the version is < 8.8<bpmn:completionCondition />
element exists: raise an error if the version is < 8.8An example process XML containing both properties:
Relates to camunda/camunda-modeler#4850.
Checklist
To ensure you provided everything we need to look at your PR:
@bpmn-io/sr
toolCloses {LINK_TO_ISSUE}
orRelated to {LINK_TO_ISSUE}