Validate linked forms in Web Modeler

wollefitz commented 1 year ago

What should we do?

When a user links a form (via formId) to a user task or start event, no lint error should be thrown when checking against a Zeebe version lower than 8.4 in Web Modeler.

Why should we do it?

For Web Modeler, we'll implement a mechanism where linked forms will automatically be converted to embedded forms when deploying to a cluster version lower than 8.4 (see camunda/web-modeler#6603 and camunda/web-modeler#6723). Displaying a lint error to the user in this case would be confusing as it could create the impression that deploying a BPMN diagram with linked forms to Zeebe < 8.4 would not work.

In Desktop Modeler, the lint rule for checking linked forms (implemented in https://github.com/camunda/bpmnlint-plugin-camunda-compat/issues/144) should still throw an error though as no automatic conversion mechanism exists.

This means the following combinations should throw lint errors (:x:) or pass the linter (✅) in Web Modeler:

	Zeebe >= 8.3	Zeebe < 8.3
Linked Start Form	:white_check_mark:	:x:
Embedded Start Form	:white_check_mark:	:x:
Linked User Task Form	:white_check_mark:	:white_check_mark:
Embedded User Task Form	:white_check_mark:	:white_check_mark:

Acceptance Criteria

[x] https://github.com/camunda/web-modeler/blob/15a2ea4e49f8bee6db4c235df6d3deb062fcb16b/webapp/src/App/Pages/Diagram/Publication/publicationPropertiesGroupExtension/FormGroup/StartEventFormLintRule.js is moved to the core
[x] no linting error for Web Modeler is raised for Web Modeler when a form is linked to a user task or start event and checking against Zeebe < 8.4
[x] a linting error for Web Modeler is raised when linking a form to a start event and checking against Zeebe < 8.3 (same as for embedded forms)
[x] a linting error is raised when both a formKey and a formId are defined on a start event or user task - the form types are mutually exclusive

#

Definition of Ready - Checklist

[x] the issue has a meaningful title, description and testable acceptance criteria
[x] edge cases and limitations are clear
[x] unclear requirements have been broken down into separate issues (always make progress)

marcellobarile commented 11 months ago

One thought: could we have the lint rule without having the form group in the properties panel in the core library? Right now, we have a custom lint rule in our codebase for start events, see https://github.com/camunda/web-modeler/blob/main/webapp/src/App/Pages/Diagram/Publication/publicationPropertiesGroupExtension/FormGroup/StartEventFormLintRule.js and I'm updating it contextually with https://github.com/camunda/web-modeler/issues/6615

marcellobarile commented 11 months ago

edit: I wasn't aware that such a lint rule was added to the core library; but somehow it seems to not work in the web-modeler 🥲 , we still have our custom rule in place though, that's why the lint error is shown anyway. We will have a follow-up issue to see what's wrong. @philippfromme eventually we can discuss this as well in our next meeting.

wollefitz commented 11 months ago

I had a quick chat with Marcello and it turns out the root cause for this confusion is that we currently have the situation that there is a lint rule for start forms implemented in the core via https://github.com/camunda/bpmnlint-plugin-camunda-compat/issues/106 and an additional rule in Web Modeler that checks that the form JSON of an embedded form is valid (see https://github.com/camunda/web-modeler/blob/37bf5341dc5c4e0ca6292a58eb9ecbc1e1b7d6de/webapp/src/App/Pages/Diagram/Publication/publicationPropertiesGroupExtension/FormGroup/StartEventFormLintRule.js), similar to what was implemented for user tasks in https://github.com/camunda/linting/pull/7.

This is confusing and I think that Web Modeler's StartEventFormLintRule should be moved here for the sake of consistency.

Additionally, we'll have to raise different lint errors for forms linked to user tasks in Web Modeler and Desktop Modeler - we might not want to include rule 1 described in the description at all, see also https://github.com/camunda/web-modeler/issues/6941.

wollefitz commented 11 months ago

@philippfromme @marcellobarile and I had a talk today and we agreed on the following things (see also https://github.com/camunda/web-modeler/issues/6941#issuecomment-1819660684):

we'll move the rule for validating empty JSON configuration on start events to the core
we'll implement all other required rules (see issue description) directly in the core. This is no problem because when bootstrapping the linter, we pass the value web to the modeler parameter so that the linter can only apply the rules that are necessary in Web Modeler
@philippfromme will take over the implementation

philippfromme commented 11 months ago

Will be addressed by

philippfromme commented 11 months ago

One issue with this new approach is that the same diagram produces an error in the desktop modeler but no error in the web modeler, which could confuse users using both modelers. My current assumption is that we'll assume, that no one's using both modelers with the same diagrams.

christian-konrad commented 11 months ago

@philippfromme agree - the ratio of users using the same diagram in both modelers is low, and we do not need to worry

wollefitz commented 11 months ago

Closing this issue as the scope was delivered with https://github.com/camunda/linting/pull/94

Thanks a lot @philippfromme !

camunda / bpmnlint-plugin-camunda-compat