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.

Sign up for GitHub

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

Jump to bottom

Ensure existing values are preserved #86

Merged
merged 3 commits into from
Apr 19, 2024
Merged

Ensure existing values are preserved #86

merged 3 commits into from
Apr 19, 2024

Conversation

nikku
Copy link
Member

@nikku nikku commented Apr 17, 2024 •
edited
Loading

Context

  • With Fix infinite loop #82 we removed the application of filtered element templates, resolving an infinite loop.
  • We broke a core guarantee (valid values are kept) but did not notice due to missing test coverage

This PR

  • Restores the pre Fix infinite loop #82 behavior and adds relevant test coverage
  • It ensures that a template is always conditionally applied
  • It ensures that condition is only applied once per update cycle, from the original template
  • This way we allow the evaluation to stabilize, and stop evaluating recursively (eventually)

Related to camunda/camunda-modeler#4249

@bpmn-io-tasks bpmn-io-tasks bot added the in progress Currently worked on label Apr 17, 2024
@nikku nikku marked this pull request as ready for review April 18, 2024 12:08
@bpmn-io-tasks bpmn-io-tasks bot added needs review Review pending and removed in progress Currently worked on labels Apr 18, 2024
@nikku nikku changed the base branch from main to only-replace-when-necessary April 18, 2024 12:54
@nikku nikku changed the base branch from only-replace-when-necessary to main April 18, 2024 12:59
@nikku nikku requested review from a team, philippfromme and marstamm and removed request for a team April 18, 2024 13:02
@nikku nikku mentioned this pull request Apr 18, 2024
Closed
@nikku
Copy link
Member Author

nikku commented Apr 18, 2024 •
edited
Loading

I'll conduct an E2E test on this feature tomorrow, using camunda/connectors as a foundation.

marstamm
marstamm reviewed Apr 19, 2024
View reviewed changes
Copy link
Collaborator

@marstamm marstamm left a comment

Choose a reason for hiding this comment

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

The logic works well, I would slightly refactor it to make it easier to understand. I added my suggestion in a separate PR: #89

@nikku
Copy link
Member Author

nikku commented Apr 19, 2024

Pending: Integration test against current connector templates. I'm still trying go get these from somewhere.

@nikku
fix(cloud-element-templates): always apply conditional template
722352f 
fix(cloud-element-templates): only apply conditional portion of template

* ensures we properly account for default values
* when we apply a conditional template, ensure we test conditions
  against the original template

----

Documented in https://github.com/bpmn-io/element-templates/blob/main/docs/LIFE_CYCLE.md#upgrade-behavior

Related to camunda/camunda-modeler#4249
@nikku
chore(cloud-element-templates): drop unnecessary check
1a88278 
We apply conditional logic independent of whether old and new template
are the same; any change (upgrade) may trigger it, the only important
aspect to consider is if actual bindings changed (and hence we need
to reapply.
@nikku
test(cloud-element-templates): simplify conditional test case
576aa00 
Do not depend element transformation for dependent dropdowns test.
@nikku
Copy link
Member Author

nikku commented Apr 19, 2024

Thanks @marstamm. Squashed #89 into this PR.

marstamm
marstamm approved these changes Apr 19, 2024
View reviewed changes
Copy link
Collaborator

@marstamm marstamm left a comment

Choose a reason for hiding this comment

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

Great, thank you!
✔️ from my side

@nikku
Copy link
Member Author

nikku commented Apr 19, 2024

During integration testing I found an interesting follow-up issue: camunda/camunda-modeler#4252.

@nikku nikku merged commit 3efbea5 into main Apr 19, 2024
6 checks passed
@nikku nikku deleted the 4249-repro branch April 19, 2024 14:14
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label Apr 19, 2024
@philippfromme philippfromme mentioned this pull request May 8, 2024
Closed
38 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marstamm marstamm marstamm approved these changes

@philippfromme philippfromme Awaiting requested review from philippfromme philippfromme was automatically assigned from bpmn-io/modeling-dev

Assignees

@nikku nikku

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@nikku @marstamm