Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix TypeUtil#isDifferentType type util #1787

Merged
merged 1 commit into from
Dec 5, 2022
Merged

Conversation

nikku
Copy link
Member

@nikku nikku commented Dec 5, 2022

Closes the TypeUtil#isDifferentType issue reported through camunda/camunda-bpmn-js#213 (comment).

The underlying issue is that TypeUtil#isDifferentType would not detect that the transaction entry matches a transaction element. This is due to triggerByEvent=false on the bpmn:Transaction, and triggeredByEvent=undefined on the menu entry. This leads to false === undefined which is false when it should be true.

The reason this did not surface yet is that we had distinct entries for transactions and event-subprocesses so that no filtering was ever needed.


We rely on the utility to properly work in connectors land, working around #1786.

@nikku nikku requested review from smbea and barmac December 5, 2022 13:19
@bpmn-io-tasks bpmn-io-tasks bot added the needs review Review pending label Dec 5, 2022
@nikku nikku changed the title Fix popup menu type util Fix TypeUtil#isDifferentType type util Dec 5, 2022
The current ReplaceOptions.TRANSACTION / ReplaceOptions.EVENT_SUB_PROCESS
mask a bug that prevents us from properly detecting equality of popup
menu entries.

This commit fixes the bug and removes the workaround in the replace options.
@nikku nikku force-pushed the fix-popup-menu-type-util branch from cc24ea1 to 0f977c0 Compare December 5, 2022 13:19
Comment on lines -34 to +36
businessObject.triggeredByEvent === target.triggeredByEvent

// coherse to <false>
!!target.triggeredByEvent === !!businessObject.triggeredByEvent
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not fully understanding what this achieves. Could you clarify a bit more?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the issue with additional context.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the additional context

@nikku nikku merged commit 4d594d5 into develop Dec 5, 2022
@nikku nikku deleted the fix-popup-menu-type-util branch December 5, 2022 16:37
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label Dec 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants