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(cloud-templates): keep exisiting configuration after apply #661

Merged
merged 3 commits into from
May 5, 2022

Conversation

marstamm
Copy link
Contributor

@marstamm marstamm commented Apr 8, 2022

Recording 2022-04-12 at 12 02 33

related to #638

@nikku
Copy link
Member

nikku commented Apr 11, 2022

My proposal is that we start to test element template logic against the new APIs (ElementTemplate#applyTemplate) rather than internally. That will help us long term.

@marstamm
Copy link
Contributor Author

We are already testing against this API, just with some utility around it to ensure the templates are added beforehand:

return getBpmnJS().invoke(function(elementTemplates, elementRegistry) {
if (isString(element)) {
element = elementRegistry.get(element);
}
expect(element).to.exist;
elementTemplates.set(templates);
return elementTemplates.applyTemplate(element, newTemplate);
});

@nikku
Copy link
Member

nikku commented Apr 11, 2022

Sorry, my bad. What I meant is "move the test to that place" (or consider not to do it).

@marstamm
Copy link
Contributor Author

I like having it as a separate file from ElementTemplates.spec, as we are testing the behavior of the ChangeElementTemplateHandler and is easier to find in the IDE this way.

But this is just a slight preference, no strong feelings either way

@nikku
Copy link
Member

nikku commented Apr 11, 2022

#661 (comment)

Great, in this case let us keep it.

I'll just make sure we have some basic coverage in the element template spec, too.

@marstamm marstamm force-pushed the template-keep-existing-config branch 2 times, most recently from 844b83d to 7d92c81 Compare April 12, 2022 10:14
@marstamm
Copy link
Contributor Author

@nikku, how do we want to continue with this PR? From my side, the implementation for keeping configuration C8 is ready for review. Do you want to add the test cases to this PR or is this a separate issue/PR?

@marstamm
Copy link
Contributor Author

To be confirmed: this PR solved https://camunda.slack.com/archives/C02JLRNQQ05/p1649846975972899

(double taskDefinitions after applying template to configured service task). I will add an explicit test case for this

@marstamm marstamm force-pushed the template-keep-existing-config branch from 57ca456 to dab3f94 Compare April 27, 2022 13:00
@marstamm marstamm marked this pull request as ready for review April 27, 2022 13:28
@bpmn-io-tasks bpmn-io-tasks bot added needs review Review pending and removed in progress Currently worked on labels Apr 27, 2022
@marstamm
Copy link
Contributor Author

Added integration tests. This is ready for review now.

@marstamm marstamm requested review from nikku, a team and barmac and removed request for a team April 27, 2022 13:28
@nikku
Copy link
Member

nikku commented May 3, 2022

My appologies for not looking into this earlier. I have this PR on my list for tomorrow 🙏

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.

Works as it should 👍

Found one unrelated visual glitch: #675.

@fake-join fake-join bot merged commit b0deec0 into master May 5, 2022
@fake-join fake-join bot deleted the template-keep-existing-config branch May 5, 2022 12:59
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label May 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