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(Condition): use update logic for applying conditions #35

Merged
merged 2 commits into from
Dec 4, 2023

Conversation

marstamm
Copy link
Collaborator

@marstamm marstamm commented Dec 1, 2023

fixes #32

  • We re-use the upgrade logic for applying conditions
  • Unused properties are removed for property and Message bindings

@marstamm marstamm requested a review from a team December 1, 2023 13:25
@marstamm marstamm self-assigned this Dec 1, 2023
@marstamm marstamm requested review from philippfromme and barmac and removed request for a team December 1, 2023 13:25
@bpmn-io-tasks bpmn-io-tasks bot added the needs review Review pending label Dec 1, 2023
this.preExecute([ 'propertiesPanel.zeebe.changeTemplate' ], HIGH_PRIORITY, this._applyCondtions , true, this);
}

_applyCondtions(context) {
Copy link
Member

Choose a reason for hiding this comment

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

Hmm this looks like a typo. There is _applyConditions already.

- _applyCondtions
+ _applyConditions

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Whoops, let's get this fixed 😄

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Renamed to _handleTemplateUpgrade since it only applies for the change template handler

Comment on lines +39 to +42
// Event does not have an event definition
if (!bo) {
return;
}
Copy link
Member

Choose a reason for hiding this comment

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

Makes sense.

Copy link
Member

@barmac barmac left a comment

Choose a reason for hiding this comment

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

Let's fix the _applyConditions confusion before we merge this :)

@bpmn-io-tasks bpmn-io-tasks bot added in progress Currently worked on and removed needs review Review pending labels Dec 1, 2023
@barmac barmac self-requested a review December 4, 2023 10:34
@bpmn-io-tasks bpmn-io-tasks bot added needs review Review pending and removed in progress Currently worked on labels Dec 4, 2023
@fake-join fake-join bot merged commit 499f65f into main Dec 4, 2023
8 checks passed
@fake-join fake-join bot deleted the fix-conditon-updates branch December 4, 2023 11:05
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label Dec 4, 2023
@@ -78,8 +80,8 @@ describe('provider/cloud-element-templates - ElementTemplatesConditionChecker',
// then
expect(businessObject.get('customProperty')).to.exist;

expect(businessObject.get('noDefaultProperty')).to.exist;
Copy link
Member

Choose a reason for hiding this comment

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

Let's clearly document this change in behavior. I believe we agreed this is a bug fix?

@@ -372,8 +374,7 @@ describe('cloud-element-templates/cmd - ChangeElementTemplateHandler', function(

const taskDefinition = findExtension(task, 'zeebe:TaskDefinition');

expect(taskDefinition).to.exist;
Copy link
Member

Choose a reason for hiding this comment

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

Let's clearly document this change in behavior. I believe we agreed this is a bug fix?

@nikku
Copy link
Member

nikku commented Dec 5, 2023

This seems to ship to other hidden bug fixes ⬆️. Let's ensure they are marked as such in the CHANGELOG.

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.

Property value is lost after template version upgrade if condition was changed
3 participants