feat: change task type when element template is applied #648

marstamm · 2022-03-31T09:21:07Z

Try it out with

npx @bpmn-io/sr bpmn-io/bpmn-js-properties-panel#572-element-template-change-task-type -c 'npm run start:cloud-templates'

This PR allows you to define a elementType that will be enforced when a template is applied

We use the bpmn MetaModel to define what can be changed
Only same base types can be used for replacing (Task, Event, Gateway). Other Combinations result in invalid templates
Works for cloud and platform

closes #572
closes #642

pinussilvestrus · 2022-03-31T09:22:06Z

/cc @nikku

pinussilvestrus

From quick testing I can say it works 👏

Implementation wise we should consider some things before we merge it. 👍

package.json

src/provider/cloud-element-templates/ReplaceBehavior.js

src/provider/cloud-element-templates/Validator.js

src/provider/element-templates/Validator.js

test/spec/provider/cloud-element-templates/cmd/ChangeElementTemplateHandler.spec.js

test/spec/provider/element-templates/cmd/ChangeElementTemplateHandler.spec.js

test/spec/provider/cloud-element-templates/Validator.spec.js

test/spec/provider/element-templates/cmd/ChangeElementTemplateHandler.spec.js

test/spec/provider/cloud-element-templates/cmd/ChangeElementTemplateHandler.spec.js

nikku · 2022-03-31T15:09:41Z

@marstamm I could only have a quick look on this today. If a detailed look is needed from my end I can see what I can do tomorrow or on Monday.

test/spec/provider/cloud-element-templates/cmd/ChangeElementTemplateHandler.spec.js

marstamm · 2022-03-31T15:12:27Z

@nikku , I will tackle the "return new element" topic tomorrow, so nothing urgent. @pinussilvestrus already gave some great suggestions on the code, but would be great to get some feedback if it is the correct behavior for connectors as well.

test/spec/provider/cloud-element-templates/cmd/ChangeElementTemplateHandler.spec.js

test/spec/provider/cloud-element-templates/fixtures/complex.json

test/spec/provider/element-templates/cmd/ChangeElementTemplateHandler.spec.js

nikku · 2022-03-31T15:18:27Z

Added a few more comments, especially around the change command.

TLDR: Let us work with the newly introduced ElementTemplate#applyTemplate API for "applying a template" exclusively; let us encapsulate the "element can change" logic within that API call.

nikku · 2022-03-31T15:18:46Z

Will test drive this against connectors as soon as I can.

marstamm · 2022-04-01T13:03:23Z

I added the requested changes in separate commits to help with reviewing the changes. I will squash them before merging. Ready for another round of reviews 🏇

(Please wait for Nico to confirm the behavior in Connectors land before approving)

src/provider/cloud-element-templates/Validator.js

src/provider/element-templates/Validator.js

test/spec/provider/element-templates/cmd/ChangeElementTemplateHandler.spec.js

package.json

marstamm · 2022-04-04T11:24:42Z

Added last review hints and squashed all commits

nikku · 2022-04-04T12:04:15Z

From what I found during initial testing, this works like a charm!

We are aware of bpmn-io/bpmn-js-connectors-extension#32 as a limitation, but I guess our users will verify how crucial of a limitation that is, post April 12th 😉.

nikku · 2022-04-04T12:06:09Z

I found one bug, not related to this PR, but to the overall "check if properties are still valid during replace" interaction:

Given I have a template that defines a task header <a>
If I change to another template that does not define task headers
Then the task header <a> binding is not cleaned up

I believe this is due to our C7 legacy of "only touching what has been defined"; it does not seem to be the appropriate behavior for C8 anymore.

Created a bug for this (#653).

package.json

nikku · 2022-04-04T12:27:42Z

test/spec/provider/cloud-element-templates/Validator.spec.js

+        templates.addAll(templateDescriptor);
+
+        // then
+        expect(errors(templates)).to.contain('template(id: <example.com.TaskToGateway>, name: <Element Type (Task -> Gateway)>): can not morph <bpmn:ServiceTask> into <bpmn:ExclusiveGateway>');


What is the reason for this limitations?

We want to prevent the changing to broken types, like Task -> Process or Task -> SequenceFlow. As the simplest approach, we decided to only allow Task -> Task, Gateway -> Gateway and Event -> Event

cf. camunda/element-templates-json-schema#48 (comment)

Well, one way to approach this would also be "allow users to do what they want" (and see how the editing experience blows up in the process) :).

But I see how we rather want to be safe.

If we got that route though, why do we choose bpmn:Task and not bpmn:Activity as the base for all "task nodes".

why do we choose bpmn:Task and not bpmn:Activity

Good point, I see no reason for it. I'll change it 👍

src/provider/element-templates/Validator.js

nikku

Code-wise I have two concerns (but just comments at this point):

Is the direct bpmn-moddle dependency really required? What do we achieve with it (in terms of features / safeguarding our users) that we don't achive by other means?

What is, in two simple lines our rules for "appliesTo" / "elementType" validation?

I'm missing a bunch of comments (examples) in the test suite / source code which makes our logic there hard to follow.

marstamm · 2022-04-04T12:35:44Z

What is, in two simple lines our rules for "appliesTo" / "elementType" validation?

We validate that:

The Element Template also applies to the elementType Target
The appliesTo and elementType have compatible classes (eg. no Task -> SequenceFlow allowed)

What do we achieve with it (in terms of features / safeguarding our users) that we don't achive by other means?

We need it to check the above Assumptions. E.g.,

appliesTo: ['bpmn:Task']
elementType: {
  value: 'bpmn:ServiceTask'
}

is valid, but requires the meta-model to validate that the Service Task is a subClass of Task.

nikku · 2022-04-04T12:39:24Z

Cf. #648 (comment).

Maybe we can re-use the existing moddle instance?

src/provider/cloud-element-templates/ReplaceBehavior.js

closes #642

marstamm · 2022-04-04T14:26:12Z

src/provider/element-templates/util/validate.js

-export default function validate(descriptors) {
-  return new Validator().addAll(descriptors).getErrors();
+export default function validate(descriptors, moddle) {
+  return new Validator(moddle).addAll(descriptors).getErrors();


Not really happy with this one, but we can't move the validation to ElementTemplates because that would cause a circular dependency in the TemplateFactory

I thing we're fine with this.

Moddle is the meta-model we rely on. If we start to validate type hierarchies (which you decided to do) there is no other way to add this.

nikku

Added 7eb303b on top. Looks good to me.

marstamm requested review from pinussilvestrus and a team March 31, 2022 09:21

marstamm self-assigned this Mar 31, 2022

marstamm requested review from MaxTru and removed request for a team March 31, 2022 09:21

bpmn-io-tasks bot added the needs review Review pending label Mar 31, 2022

pinussilvestrus suggested changes Mar 31, 2022

View reviewed changes

bpmn-io-tasks bot added in progress Currently worked on and removed needs review Review pending labels Mar 31, 2022

pinussilvestrus reviewed Mar 31, 2022

View reviewed changes

test/spec/provider/element-templates/cmd/ChangeElementTemplateHandler.spec.js Outdated Show resolved Hide resolved

pinussilvestrus reviewed Mar 31, 2022

View reviewed changes

test/spec/provider/cloud-element-templates/cmd/ChangeElementTemplateHandler.spec.js Show resolved Hide resolved

marstamm mentioned this pull request Mar 31, 2022

Do not carry over element templates during replacement #642

Closed

nikku reviewed Mar 31, 2022

View reviewed changes

test/spec/provider/cloud-element-templates/cmd/ChangeElementTemplateHandler.spec.js Outdated Show resolved Hide resolved

nikku reviewed Mar 31, 2022

View reviewed changes

test/spec/provider/cloud-element-templates/cmd/ChangeElementTemplateHandler.spec.js Outdated Show resolved Hide resolved

nikku reviewed Mar 31, 2022

View reviewed changes

test/spec/provider/cloud-element-templates/fixtures/complex.json Show resolved Hide resolved

nikku reviewed Mar 31, 2022

View reviewed changes

test/spec/provider/element-templates/cmd/ChangeElementTemplateHandler.spec.js Outdated Show resolved Hide resolved

marstamm mentioned this pull request Apr 1, 2022

fix: required value for element type property camunda/element-templates-json-schema#53

Merged

marstamm requested a review from pinussilvestrus April 1, 2022 13:03

pinussilvestrus reviewed Apr 4, 2022

View reviewed changes

src/provider/cloud-element-templates/Validator.js Outdated Show resolved Hide resolved

src/provider/element-templates/Validator.js Outdated Show resolved Hide resolved

test/spec/provider/element-templates/cmd/ChangeElementTemplateHandler.spec.js Show resolved Hide resolved

pinussilvestrus reviewed Apr 4, 2022

View reviewed changes

package.json Outdated Show resolved Hide resolved

pinussilvestrus mentioned this pull request Apr 4, 2022

Integrate template icons camunda/camunda-bpmn-js#108

Merged

marstamm force-pushed the 572-element-template-change-task-type branch from 5434384 to 1577ab4 Compare April 4, 2022 10:19

nikku mentioned this pull request Apr 4, 2022

Headers / inputs are not cleaned when switching between element templates #653

Closed

nikku reviewed Apr 4, 2022

View reviewed changes

package.json Outdated Show resolved Hide resolved

nikku reviewed Apr 4, 2022

View reviewed changes

src/provider/element-templates/Validator.js Outdated Show resolved Hide resolved

nikku reviewed Apr 4, 2022

View reviewed changes

src/provider/cloud-element-templates/ReplaceBehavior.js Show resolved Hide resolved

marstamm added 6 commits April 4, 2022 16:22

deps: bump to @bpmn-io/[email protected]

984566a

feat(element-templates): verify elementType property

16ce442

feat(element-templates): change element to specified type

b3b1ab9

feat(element-templates): unlink element template on replace

e421019

closes #642

chore(cloud-element-templates): use elementType during crate

9022464

chore: add elementType to example JSON

ba57360

marstamm force-pushed the 572-element-template-change-task-type branch from 1577ab4 to 628a98a Compare April 4, 2022 14:24

marstamm commented Apr 4, 2022

View reviewed changes

chore(element-templates): use existing moddle instance

b378bd2

marstamm force-pushed the 572-element-template-change-task-type branch from 628a98a to b378bd2 Compare April 4, 2022 14:29

nikku added 2 commits April 4, 2022 20:32

chore(element-templates): validate with out instantiation

7eb303b

test: improve formatting

6b981bd

nikku approved these changes Apr 4, 2022

View reviewed changes

fake-join bot merged commit eaa7436 into master Apr 4, 2022

fake-join bot deleted the 572-element-template-change-task-type branch April 4, 2022 18:37

bpmn-io-tasks bot removed the in progress Currently worked on label Apr 4, 2022

nikku mentioned this pull request Apr 4, 2022

chore(CHANGELOG): update to v1.0.0-alpha.11 #655

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: change task type when element template is applied #648

feat: change task type when element template is applied #648

marstamm commented Mar 31, 2022 •

edited by nikku

Loading

pinussilvestrus commented Mar 31, 2022

pinussilvestrus left a comment

nikku commented Mar 31, 2022 •

edited

Loading

marstamm commented Mar 31, 2022

nikku commented Mar 31, 2022

nikku commented Mar 31, 2022

marstamm commented Apr 1, 2022 •

edited

Loading

marstamm commented Apr 4, 2022

nikku commented Apr 4, 2022

nikku commented Apr 4, 2022 •

edited

Loading

nikku Apr 4, 2022

marstamm Apr 4, 2022

nikku Apr 4, 2022

marstamm Apr 4, 2022

nikku left a comment

marstamm commented Apr 4, 2022

nikku commented Apr 4, 2022

marstamm Apr 4, 2022

nikku Apr 4, 2022

nikku left a comment

feat: change task type when element template is applied #648

feat: change task type when element template is applied #648

Conversation

marstamm commented Mar 31, 2022 • edited by nikku Loading

pinussilvestrus commented Mar 31, 2022

pinussilvestrus left a comment

Choose a reason for hiding this comment

nikku commented Mar 31, 2022 • edited Loading

marstamm commented Mar 31, 2022

nikku commented Mar 31, 2022

nikku commented Mar 31, 2022

marstamm commented Apr 1, 2022 • edited Loading

marstamm commented Apr 4, 2022

nikku commented Apr 4, 2022

nikku commented Apr 4, 2022 • edited Loading

nikku Apr 4, 2022

Choose a reason for hiding this comment

marstamm Apr 4, 2022

Choose a reason for hiding this comment

nikku Apr 4, 2022

Choose a reason for hiding this comment

marstamm Apr 4, 2022

Choose a reason for hiding this comment

nikku left a comment

Choose a reason for hiding this comment

marstamm commented Apr 4, 2022

nikku commented Apr 4, 2022

marstamm Apr 4, 2022

Choose a reason for hiding this comment

nikku Apr 4, 2022

Choose a reason for hiding this comment

nikku left a comment

Choose a reason for hiding this comment

marstamm commented Mar 31, 2022 •

edited by nikku

Loading

nikku commented Mar 31, 2022 •

edited

Loading

marstamm commented Apr 1, 2022 •

edited

Loading

nikku commented Apr 4, 2022 •

edited

Loading