fix: do not remove documentation and execution listeners when template is removed #120
Conversation
| * @param {ModdleElement} sourceBusinessObject | ||
| * @param {ModdleElement} targetBusinessObject | ||
| */ | ||
| _copyNonTemplateProperties(sourceBusinessObject, targetBusinessObject) { |
There was a problem hiding this comment.
Great solution, in the appropriate place, I think.
[Q]: This handles both C7 and C8. Is the behavior applicable for C7? If so, the standard way would be to split both implementations, and and move the cloud one to cloud-element-templates.
There was a problem hiding this comment.
As C7 and C8 templates work slightly differently I'd be fine to only implement this change for C8.
There was a problem hiding this comment.
cloud-element-templates actually use the same remove handler.
Should we introduce a place for common code?
There was a problem hiding this comment.
Yes, element-templates is not the place for zeebe properties.
There was a problem hiding this comment.
I extracted some common code in a7b14c4
| } | ||
|
|
||
| if (extensionElements && extensionElements.values) { | ||
| const exts = copy.copyProperty(extensionElements, targetBusinessObject, 'extensionElements'); |
There was a problem hiding this comment.
[Q]: We copy all extension elements here, why don't we copy only the individual values that we want to keep?
[Q2]: Does it make sense to statically configure what we want to keep (in a variable)?
There was a problem hiding this comment.
Haven't found a way to copy execution listeners with ModdleCopy#copyProperty directly.
Moreover, with isAny checking for either C7 or C8 type, there is one implementation that works for both and doesn't need to be aware of engine version.
Is there a smarter way to copy only execution listeners?
| * @param {ModdleElement} sourceBusinessObject | ||
| * @param {ModdleElement} targetBusinessObject | ||
| */ | ||
| _copyNonTemplateProperties(sourceBusinessObject, targetBusinessObject) { |
There was a problem hiding this comment.
As C7 and C8 templates work slightly differently I'd be fine to only implement this change for C8.
|
@nikku I moved the 2 command handlers shared between C7 and C8 to a common folder. Is there any reason we shouldn't support C7? |
Camunda 7 works slightly differently, and we don't aim to touch (and potentially break) existing Camunda 7 behaviors. |
|
Will have another look onto this PR shortly. |
There was a problem hiding this comment.
I like the idea to move shared things outside of element-templates / cloud-element-templates.
However the handler now still mixes both implementations, which I'd like to avoid. Thinking in terms of modularization I'd like to remove the element-templates folder, some day in the future, and all element template related code is gone. Same thing the other way around.
|
I'll give this PR another look and sketch the relevant changes :) |
I'll move the PR to draft until the change is implemented or we reconsider. |
bpmn-io-tasks
bot
added
in progress
Closes #114
Proposed Changes
When removing template from element, documentation and execution listeners (and potentially more non-template properties in the future) are kept.
Screen.Recording.2024-08-28.at.12.39.23.mov
Checklist
To ensure you provided everything we need to look at your PR:
@bpmn-io/srtoolCloses {LINK_TO_ISSUE}orRelated to {LINK_TO_ISSUE}