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

Support camunda:executionListener with implementationType property binding #17

Merged
merged 4 commits into from
Oct 12, 2023

Conversation

AlexanderSkrock
Copy link
Contributor

@AlexanderSkrock AlexanderSkrock commented Sep 26, 2023

Before this change we only evaluated the special case for scripts. For everything else we always used the value property. But for some cases e. g. class-based execution listeners we need actually need to set the "class" attribute instead. Also, this align with the json schema definition.

Closes #13

Note: I had to reopen, because I deleted my fork when cleaning up my repos... Forgot about the open pull request. See #15

@barmac barmac self-requested a review September 27, 2023 09:04
@barmac
Copy link
Member

barmac commented Sep 28, 2023

Hi, thanks for the PR. In order to make the feature complete, we'd also need to:

@AlexanderSkrock
Copy link
Contributor Author

AlexanderSkrock commented Sep 28, 2023

Hi, @barmac ,
of course I would take on these two tasks as well!

Changes to the json schema should be proposed with a pull request here, right?
=> https://github.com/camunda/element-templates-json-schema

Could you give me a hint which repository contains the documentation, I was not able to find any files for this in the camunda-modeler repo.
=> Edit: This repo, right? https://github.com/camunda/camunda-platform-docs/tree/main/docs/components/modeler/desktop-modeler/element-templates

Kind regards
Alexander Skrock

@barmac
Copy link
Member

barmac commented Sep 28, 2023

Hi,

I am happy that you want to complete the loop :)

Both repos are the ones where we'd need to implement the changes. For the documentation, you'd need to look into this file specifically: https://github.com/camunda/camunda-platform-docs/blob/main/docs/components/modeler/desktop-modeler/element-templates/c7-defining-templates.md

Looking forward to your contributions!

@AlexanderSkrock
Copy link
Contributor Author

AlexanderSkrock commented Sep 28, 2023

Hi again @barmac ,

sorry for coming back to you again.

I just attempted to write documentation and realized that while name totally makes sense from the implementation side, it is not as intuitive for using it.

Once again name aligns with the attribute that is used for defining properties with a binding and basically that is what we do.
On the other side, from user perspective, it rather is the implementationType (type is already used). Available types besides script are class, expression and delegateExpression.

So my question to you is, should we stick to name as I initially proposed because it works the same as already known for property bindings or should we be more specific to this use-case?

I am fine with both approaches. I would appreciate your opinion on this, before I finalize my contribution.

Kind regards
Alexander Skrock

@barmac
Copy link
Member

barmac commented Sep 29, 2023

Hi,

Thanks for coming back. I looked into this more deeply, and indeed implementationType sounds like a more reasonable choice. I'd suggest to have the following rules in the JSON schema:

  • If implementationType is set on the binding, it has to have one of the values: [ 'script', 'class', 'expression', 'delegateExpression' ].
  • If implementationType=script is set, scriptFormat is required.

So let's keep the backwards compatibility, but also enforce correct values in the future templates.

@AlexanderSkrock
Copy link
Contributor Author

AlexanderSkrock commented Oct 6, 2023

Now, everything should be ready for review, @barmac :

@barmac
Copy link
Member

barmac commented Oct 10, 2023

Thanks for the heads-up. I will look into these items today.

@barmac barmac changed the title Named property binding Support camunda:executionListener with implementationType property binding Oct 10, 2023
@barmac
Copy link
Member

barmac commented Oct 10, 2023

We will probably need to release a new version of bpmn-io/element-templates-validator for this.

@AlexanderSkrock
Copy link
Contributor Author

AlexanderSkrock commented Oct 10, 2023

What is the matter with the current version? Maybe, I could do necessary changes.

@barmac
Copy link
Member

barmac commented Oct 10, 2023

It's OK. The validator depends on the JSON schema, so I need to update the JSON schema there in order to update it. I am in the process of merging your changes ^^. Thank you so much for your contributions

@barmac
Copy link
Member

barmac commented Oct 10, 2023

Validator update for reference: bpmn-io/element-templates-validator#24

@barmac
Copy link
Member

barmac commented Oct 10, 2023

So once that PR is merged and validator is released, I will incorporate the update into this PR and merge it.

@AlexanderSkrock
Copy link
Contributor Author

Thank you a lot for the effort!

@barmac barmac merged commit f0b5f43 into bpmn-io:main Oct 12, 2023
3 checks passed
@barmac
Copy link
Member

barmac commented Oct 12, 2023

OK this is now merged and I will publish it soon. Thank you @AlexanderSkrock for your amazing contribution!

@AlexanderSkrock AlexanderSkrock deleted the named-property-binding branch October 12, 2023 11:06
@nikku
Copy link
Member

nikku commented Oct 12, 2023

Thanks indeed for this contribution. Well done 👏

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 bindings of type "camunda:executionListener" ignores name property
3 participants