search

bpmn-io / bpmn-js

A BPMN 2.0 rendering toolkit and web modeler.
https://bpmn.io/toolkit/bpmn-js/
Other
8.67k stars 1.34k forks source link

Plain StartEvent is created instead of Message StartEvent (non-interrupting) #1462

Open 0x00000001A opened 3 years ago

0x00000001A commented 3 years ago

Describe the Bug

Hi. I’m trying to add Message Start Event (non-interrupting) together with SubTask to the diagram. The problem is the “MessageStartEvent (non-interruptig)” visually looks like “StartEvent”, but actually, it’s not. I’ve compared the XML in both cases and they are the same. And after I’ve reimported the XML, everything is ok, I mean, MessageStartEvent gets the correct shape.

On Create Drag Start On Create Drag End

I've tried to find a way how to solve it (and you may find more info about the bug there) (note, there was a typo, I've specified what the 8.1.0 version is installed, but actually, it was 8.5.0) https://forum.bpmn.io/t/add-message-start-event-non-interrupting-inside-subtask-by-default/6302

But looks like, the only way to fix this issue - install bpmn-js@8.2.0. In my case, I have the bpmn-js@8.5.0 installed and I've tried to upgrade to the bpmn-js@8.6.1 - bug still exists.

Steps to Reproduce

  1. Codesandbox with bpmn-js@8.2.0
  2. Codesandbox with bpmn-js@8.6.1
  3. Codesandbox with bpmn-js@8.5.0

Expected Behavior

Message StartEvent (non-interrupting) is created, instead of StartEvent.

Environment

barmac commented 3 years ago

Thank you for your bug report. I was able to reproduce the issue and can confirm that the bug was introduced in v8.5.0.

I suspect that it's caused by the behaviour implemented in this pull request. It seems that the canReplace rule for some reason decides that the message start event should be replaced.

0x00000001A commented 3 years ago

@barmac I've figured out where is the problem. The problem exists in the "BpmnRules.js", in the "canReplace" method, as you suggested. The problem is that conditions in the "canReplace" method check for "isEventSubProcess", "is bpmn:SubProcess" wrong element, it checks target instead of element parent in these cases. So, in this case, the "elements" array contains "SubProcess" (parent is the root process), and "StartEvent (non-interrupting)", and the last one has a different target from the first one because the "StartEvent" target is the "SubProcess". So, the conditions there are not correct. Anyway, it may look easy to fix, i.e. we have to take actual element parent OR target that was passed in, for use in conditions, instead of using the passed in the target only.

I've fixed it by myself, but there appeared another issue - tests (especially BpmnReplacePreviewSpec.js). I can't figure out how to fix them. Yes, I'm not good at testing, but I've tried to do my best to fix them (to use element parent instead of root process), but they are wont to work if drag target/element target is not root process element. Mb you can give me some suggestions, what I can do with this problem.

You may check my fix in the commit that you may see above.

barmac commented 3 years ago

Amazing! Thanks for the fix proposal. I had a look into the commit you shared and it seems that this is exactly what we need.

Would you mind creating a pull request? The test could be based on the code existing in the ReplaceElementBehaviourSpec.

It's also OK if the tests are incomplete. I am happy to support you. Feel free to tag me in the PR to get it done faster.