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: pasting compensation activity #2220

Merged
merged 2 commits into from
Aug 28, 2024
Merged

Conversation

abdul99ahad
Copy link
Contributor

@abdul99ahad abdul99ahad commented Aug 16, 2024

Issue:

When copying and pasting a compensation activity the isForCompensation marker isn't being removed on paste. Since that marker is being added and removed automatically the expectation would be that it's removed on paste and only added once the activity is connected to a compensation boundary event.

294470078-d1970923-9086-47b4-bb51-7ccc813f7be7

Fix:

The compensation activity can only exist when connected with compensation boundary activity. When compensation activity is being pasted, it results in simple activity.

Aug-16-2024 10-34-49

Aug-16-2024 11-22-10

Closes #2070

@abdul99ahad abdul99ahad self-assigned this Aug 16, 2024
@bpmn-io-tasks bpmn-io-tasks bot added the needs review Review pending label Aug 16, 2024
@abdul99ahad abdul99ahad marked this pull request as draft August 16, 2024 08:42
@bpmn-io-tasks bpmn-io-tasks bot added in progress Currently worked on and removed needs review Review pending labels Aug 16, 2024
@abdul99ahad abdul99ahad force-pushed the 2070-pasting-compensation-activity branch from a4bf3f4 to 4bf491a Compare August 16, 2024 09:05
@abdul99ahad abdul99ahad marked this pull request as ready for review August 16, 2024 09:17
@bpmn-io-tasks bpmn-io-tasks bot added needs review Review pending and removed in progress Currently worked on labels Aug 16, 2024
Copy link
Contributor

@philippfromme philippfromme left a comment

Choose a reason for hiding this comment

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

It took me a while to understand why this fix is working. It turns out it only works because of CompensateBoundaryEventBehavior which adds isForCompensation when connecting a task to a compensation boundary event and the fact that during paste connections are created after shapes. So with your change in BpmnReplace isForCompensation is first removed and then added again only if CompensateBoundaryEventBehavior is present. This is not the right fix. Instead, as we discussed, we want to check after replace whether we need to remove isForCompensation . Try removing the change in BpmnReplace and instead check in BpmnRules not only whether the element is a compensation activity but also whether it is connected to a compensation boundary event. 😉

test/spec/features/replace/BpmnReplaceSpec.js Outdated Show resolved Hide resolved
@bpmn-io-tasks bpmn-io-tasks bot added in progress Currently worked on and removed needs review Review pending labels Aug 16, 2024
@abdul99ahad abdul99ahad force-pushed the 2070-pasting-compensation-activity branch from 4bf491a to 8511c2a Compare August 20, 2024 19:30
@bpmn-io-tasks bpmn-io-tasks bot removed the in progress Currently worked on label Aug 20, 2024
@abdul99ahad abdul99ahad reopened this Aug 20, 2024
@bpmn-io-tasks bpmn-io-tasks bot added the needs review Review pending label Aug 20, 2024
@abdul99ahad abdul99ahad force-pushed the 2070-pasting-compensation-activity branch from 9cb1bbd to e6c8ff5 Compare August 20, 2024 19:55
@abdul99ahad abdul99ahad force-pushed the 2070-pasting-compensation-activity branch from 6133944 to 9069333 Compare August 21, 2024 12:03
Copy link
Member

@nikku nikku left a comment

Choose a reason for hiding this comment

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

I could not break it, and the implementation looks solid / simple enough. Good job.


function isForCompensation(element) {
const bo = getBusinessObject(element);
return bo && typeof bo.get === 'function' && bo.get('isForCompensation');
Copy link
Member

Choose a reason for hiding this comment

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

Not sure what we want to guard with `bo.get === 'function'.

If there is anything specific, let's create a test case. Otherwise, let's assume that the BO is always a moddle element (flowElement) and has a .get method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue was caused by some label elements that were breaking this function. I have updated the PR so that now only Task elements are checked for the compensation property.

Copy link
Member

Choose a reason for hiding this comment

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

Cf. 61d9c02. The test case was broken. For bpmn-js, a business object must always exist.

Also, you don't have to rely on get, but check the property directly. This is what I changed.

@nikku nikku self-requested a review August 28, 2024 10:04
Copy link
Member

@nikku nikku left a comment

Choose a reason for hiding this comment

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

Please clarify #2220 (comment) before we can get this merged.

@abdul99ahad abdul99ahad force-pushed the 2070-pasting-compensation-activity branch from 4009909 to 702904e Compare August 28, 2024 11:15
@nikku nikku force-pushed the 2070-pasting-compensation-activity branch from 702904e to 61d9c02 Compare August 28, 2024 15:08
@nikku
Copy link
Member

nikku commented Aug 28, 2024

Squashed into single semantic commit, will go ahead and update CHANGELOG.

@nikku nikku force-pushed the 2070-pasting-compensation-activity branch from 61d9c02 to 523cfb7 Compare August 28, 2024 15:10
@nikku
Copy link
Member

nikku commented Aug 28, 2024

Waiting for CI to pass so we can merge this.

@nikku nikku merged commit 5016fe0 into develop Aug 28, 2024
11 checks passed
@nikku nikku deleted the 2070-pasting-compensation-activity branch August 28, 2024 15:15
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label Aug 28, 2024
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.

Pasting Compensation Activity Doesn't Remove isForCompensation Marker
4 participants