Do Not Copy Message Flows Without Participant#1904
Do Not Copy Message Flows Without Participant#1904philippfromme wants to merge 2 commits intodevelopfrom
Conversation
philippfromme
force-pushed
the
message-flow-fix
branch
from
3588b52 to
d84c638
Compare
| return true; | ||
| } | ||
|
|
||
| if (is(element, 'bpmn:Lane') && !includes(elements, element.parent)) { |
There was a problem hiding this comment.
We're getting rid of null-safety here. Is this intentional?
There was a problem hiding this comment.
Let me have a look.
| isMessageFlowTarget(target) && | ||
| !isSameOrganization(source, target) | ||
| ); | ||
| return isMessageFlowSource(source) |
There was a problem hiding this comment.
I'm hesitant to merge this, as this is completely unclear what these additions ensure.
There was a problem hiding this comment.
I'll have to check whether this check is strictly necessary. The point is to make sure source and target are in different participants.
| return every(elements, function(element) { | ||
| if (isConnection(element)) { | ||
| return canConnect(element.source, element.target, element); | ||
| return (canConnect(element.source, element.target, element) || {}).type === element.type; |
There was a problem hiding this comment.
Let's elaborate what this does.
My guess is it ensures that connections can only be created if they would not change their type?
There was a problem hiding this comment.
Yes, the check was useless before because we want to create a number of shapes and connections that are predefined and must not change their type during creation.
| @@ -58,7 +58,8 @@ describe('features/modeling/rules - BpmnRules', function() { | |||
| sequenceFlow = elementFactory.createConnection({ | |||
There was a problem hiding this comment.
As we're fixing something I'd expect a test case that verifies the new behavior.
nikku
left a comment
nikku
left a comment
There was a problem hiding this comment.
As indicated in my comments it is fairly hard to understand changes in the BpmnRules. Maybe certain repetitive patterns can be simplified, and moved to a separate function with a human readable name?
This applies to some repetitive patterns, including, but not limited to d84c638#diff-6031dc6e8e3d4e38aef3c1dda35e6ff836a6b6be1263157300284d0191a6d5d4R1165.
bpmn-io-tasks
bot
added
in progress
|
Looks like this PR is stalled. @philippfromme Do you still plan to follow up? If not, let's mark the issue as |
|
It's still on my radar. I just don't have capacity to work on it right now. Since we already have this pull request in progress I'd like to finish it. |
|
Let's move it to draft till you come back to the topic. |
barmac
force-pushed
the
develop
branch
6 times, most recently
from
1be3741 to
0b4e7a3
Compare
philippfromme
force-pushed
the
develop
branch
from
d604e3b to
ec9aa3b
Compare
3 participants
Closes #1902