Camunda-modeler: Removing an element template from root element creates error

[MaxTru]

__Describe the Bug__

When removing (not unlinking) an element template from a diagram root element (process or collaboration), an error occurs:

diagram_1.bpmn] Cannot read property 'children' of undefined
    at BpmnOrderingProvider.getOrdering (webpack-internal:///./node_modules/bpmn-js/lib/features/ordering/BpmnOrderingProvider.js:148:34)
    at eval (webpack-internal:///./node_modules/diagram-js/lib/features/ordering/OrderingProvider.js:53:25)
    at invokeFunction (webpack-internal:///./node_modules/diagram-js/lib/core/EventBus.js:517:13)
    at EventBus._invokeListener (webpack-internal:///./node_modules/diagram-js/lib/core/EventBus.js:368:19)
    at EventBus._invokeListeners (webpack-internal:///./node_modules/diagram-js/lib/core/EventBus.js:349:24)
    at EventBus.fire (webpack-internal:///./node_modules/diagram-js/lib/core/EventBus.js:310:24)
    at CommandStack._fire (webpack-internal:///./node_modules/diagram-js/lib/command/CommandStack.js:356:29)
    at CommandStack._internalExecute (webpack-internal:///./node_modules/diagram-js/lib/command/CommandStack.js:398:10)
    at CommandStack.execute (webpack-internal:///./node_modules/diagram-js/lib/command/CommandStack.js:153:8)
    at Modeling.createShape (webpack-internal:///./node_modules/diagram-js/lib/features/modeling/Modeling.js:310:22) [ error ]

__Steps to Reproduce__

1. Set up some template for bpmn:Process:

[
  {
    "name": "KPI Template",
    "id": "kpi",
    "appliesTo": [
      "bpmn:Collaboration","bpmn:Process","bpmn:Task","bpmn:IntermediateCatchEvent"
    ],
    "properties": [
      {
        "label": "KPI Unit",
        "type": "Dropdown",
        "value": "ms",
        "choices": [
          {
            "name": "Milliseconds", "value": "ms"
          },
          {
            "name": "Seconds", "value": "s"
          },
          {
            "name": "Minutes", "value": "m"
          },
          {
            "name": "Hours", "value": "h"
          }
        ],
        "editable": true,
        "binding": {
          "type": "camunda:property",
          "name": "kpiunit"
        }
      },
      {
        "label": "KPI Threshold",
        "type": "String",
        "value": "",
        "editable": true,
        "binding": {
          "type": "camunda:property",
          "name": "kpi"
        }
      }
    ]
  }
]

1. Apply element template to bpmn:Process
2. Remove the element template again (by clicking on Remove)

__Expected Behavior__

Removal of element template from Process creates no error / diagram is still valid.

__Environment__

• OS: Arch Linux
• Camunda Modeler Version: 4.6.0 master branch on 2.March 2021
• Note: this fix is a precondition so to be able to reproduce this error: https://github.com/camunda/camunda-modeler/issues/2125

[MaxTru]

I explored multiple ways to fix this:

A) Using #replaceElement
One approach is to take the same approach as with shapes:

              var newElement = replace.replaceElement(element, {
                type: type,
                eventDefinitionType: eventDefinitionType
              });

However, this does currently not work:

• #replaceElement creates a new shape (e.g., triggering shape.create).
• Root elements (Process and Collaboration) are not treated as regular shapes in many modules we have (example: BpmnOrderingProvider). Hence this does currently not work.

One could argue that #replaceElement should support root elements, but afaik this issue here is the first time that this is needed / requested. Implementing this feature requires various adjustment in core modules. Hence I do not support this solution as of now (this might change, in case we see more requests for #replaceElement to support this rootElements.

B) Using command cavas.updateRoot
Another approach is to user canvas.updateRoot:

  var newRoot = elementFactory.create('root', {
                type: type
              });

              commandStack.execute('canvas.updateRoot', { newRoot: newRoot });

              modeling.moveElements(element.children.slice(), { x: 0, y: 0 }, newRoot);

However, this does also not work (for bpmn:Collaborations) since our existing implementation is based on moving elements from one parent to another, where both parents exist on the canvas (which is not the case). So again, we would require some non-trivial core adjustments.

C) Using modeling.updateProperties
Another approach is to set all properties of the businessObject to undefined. Example:

              var keys = Object.keys(businessObject.$descriptor.propertiesByName);
              var tmp = {};

              for (var i = 0; i < keys.length; i++) {
                if (keys[i] != 'id' && keys[i] != 'bpmn:id') {
                  tmp[keys[i]] = undefined;
                }
              }

              modeling.updateProperties(element, tmp);

This works, but I consider it hacky, since it requires domain knowledge of the BPMN.

---

Since this issue is a real edge-case, I propose to simply do an unlink operation in case remove is done on a Process or Collaboration:

            modeling.updateProperties(element, {
              'camunda:modelerTemplate': null,
              'camunda:modelerTemplateVersion': null
            });

If users report issues regarding this unlink behavior, we could revisit the options again.

@pinussilvestrus @philippfromme @nikku since you guys were active around this, WDYT?

[nikku]

Some general questions I ask myself:

• Is collaboration even considered a target for the template? What would be the use case?
• If a bpmn:Process is a target, can we address that one properly in all cases? Represented by a participant visually, which element would we apply that template to? The participant at the moment I guess? How to apply a template to the process within a bpmn:Participant?
• How does the interaction work if a template on a root element has been chosen an that root element gets replaced (a process transforms into a collaboration)?

Long story short, are we ready to support this end-to-end? If not, it is fine to not tackle any of this right now, as it is clearly not a bug but a _missing feature_ we did not anticipate yet. We should not rush the implementation before we have the full picture how it would work end-to-end.

[nikku]

As an alternative to this _bug fix_ we could also keep applying any templates on root elements disabled, for example.

[philippfromme]

As an alternative to this _bug fix_ we could also keep applying any templates on root elements disabled, for example.

I'd disable templates for root elements for now. Supporting them e2e including removal is not trivial and exceeds the initial scope of fixing a bug.

[pinussilvestrus]

Nice Investigations @MaxTru 👏

I'd second @philippfromme @nikku 's opinion, if it's too much hustle to support it, let's not do it now. Your findings are a good starting point when we tackle it in the future.

[MaxTru]

Thanks for your comments.

I agree, the relation effort <-> value is getting out of hand. One more validation step: I will ask whether the user reporting the initial bug can give some more context regarding the use case and its importance (see https://github.com/camunda/camunda-modeler/issues/2121).

[MaxTru]

@nikku @philippfromme @pinussilvestrus

See the discussion with a heavy user here: https://github.com/camunda/camunda-modeler/issues/2121#issuecomment-790960922

Based on the discussion I would like to re-iterate my proposal:

• Continue to support bpmn:Process and bpmn:Collaboration (as we currently do it)
• Document / be transparent about that this does not work for all cases. In particular, you cannot apply a template to a process, which is referenced by a participant
• Disable remove for root elements (ie. simply dont show the "Remove" button in this case) (also see https://github.com/camunda/camunda-modeler/issues/2128#issuecomment-790736620)
• Document the Improvement needs ((I) support processes in participants, (II) make remove work for root elements) in a seperate issue (to be picked up later)

I believe that this gives a solid 80% solution, which will help our key users.

WDYT?

[nikku]

I'll have a look into the issues you proposed and will be able to add my detailed sentiment later today.

[nikku]

To share my sentiment here: Let's investigate what keeps us from replacing root elements. It is a missing feature that would clearly enable this use-case.

[MaxTru]

See https://github.com/bpmn-io/diagram-js/pull/540#issuecomment-815639997 for an approach how to fix this