- Notifications
You must be signed in to change notification settings - Fork1.4k
Do Not Copy Message Flows Without Participant#1904
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:develop
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| returntrue; | ||
| } | ||
| if(is(element,'bpmn:Lane')&&!includes(elements,element.parent)){ |
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.
We're getting rid of null-safety here. Is this intentional?
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 me have a look.
| isMessageFlowTarget(target)&& | ||
| !isSameOrganization(source,target) | ||
| ); | ||
| returnisMessageFlowSource(source) |
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.
I'm hesitant to merge this, as this is completely unclear what these additions ensure.
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.
I'll have to check whether this check is strictly necessary. The point is to make sure source and target are in different participants.
| returnevery(elements,function(element){ | ||
| if(isConnection(element)){ | ||
| returncanConnect(element.source,element.target,element); | ||
| return(canConnect(element.source,element.target,element)||{}).type===element.type; |
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
| // given | ||
| vartask1=elementFactory.createShape({type:'bpmn:Task'}), | ||
| task2=elementFactory.createShape({type:'bpmn:Task'}), | ||
| sequenceFlow=elementFactory.createConnection({ |
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.
As we're fixing something I'd expect a test case that verifies the new behavior.
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.
As indicated in my comments it is fairly hard to understand changes in theBpmnRules. 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 tod84c638#diff-6031dc6e8e3d4e38aef3c1dda35e6ff836a6b6be1263157300284d0191a6d5d4R1165.
Looks like this PR is stalled.@philippfromme Do you still plan to follow up? If not, let's mark the issue as |
philippfromme commentedMay 26, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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. |
1be3741 to0b4e7a3Compare
Closes#1902