Fixes modeler properties panel #485
Conversation
|
@korthout could you have a look at these changes from engineering perspective. thanks |
There was a problem hiding this comment.
Thanks @Hafflgav. Nice work 👏
Please have a look at my comments and questions.
Note, that I've used the emoji-code (as used by the Zeebe-team) to try to clarify the intentions of my comments.
| Define [variable mappings](../../service-tasks/service-tasks#variable-mappings) to transform the | ||
| Define [variable mappings](/components/concepts/variables.md#inputoutput-variable-mappings) to transform the |
There was a problem hiding this comment.
❓ Can you explain your reasoning for this change? It's not entirely clear to me.
There was a problem hiding this comment.
To me it was weird that we reference variable mappings twice on the docu page with different links. To me the "overall" description of variable mappings seems way easier to understand than the one for the service task.
There was a problem hiding this comment.
My reasoning is that this is reference documentation. The Business Rule Tasks work exactly the same as the Service Task and thus when looking up the reference of the business rule task, we need to refer to the reference docs on the service task.
We can however add a link to variable mappings concept as well, to help point users to explain what they are looking at.
docs/reference/bpmn-processes/call-activities/call-activities.md
Outdated
Show resolved
Hide resolved
docs/reference/bpmn-processes/call-activities/call-activities.md
Outdated
Show resolved
Hide resolved
| By default, all job variables merge into the process instance. This behavior can be customized by defining an output mapping at the service task. | ||
| Input/output variable mappings can be used to create new variables or customize how variables are merged into the process instance. |
There was a problem hiding this comment.
❌ We should not lose the description of the default variable merging behavior.
By default, all job variables merge into the process instance.
Note, that this behavior is altered as soon as variable mappings are defined.
Please also bring back this description in the other pages.
| Input mappings can be used to transform the variables into a format accepted by the job worker. | ||
| To use the variable mapping, the Zeebe extension element ioMapping must be added to the element. It can contain multiple input and output elements that specify which variables should be mapped. The `Local Input Variable` denotes the variable name inside the activity (a local variable to be created), whereas the `Process Variable Name` of an output denotes the variable name outside of the activity. |
There was a problem hiding this comment.
I like where this is going, but:
❌ There is no Local Input Variable and no Process Variable Name. Please have a look at how the modeler sets the source and target of the input and output, when specified.
<zeebe:ioMapping>
<zeebe:input source="= source" target="InputVariable_0in6t79" />
<zeebe:output source="= source" target="OutputVariable_302pclo" />
</zeebe:ioMapping>Please also update this description in the other pages.
There was a problem hiding this comment.
In terms of the BPMN XML you are right. But from a users point of view he will see the Local Input Variable and Process Variable Name in the properties panel. I would not expect him to look in the XML representation.
In my opinion it would make sense to describe a bit more clearly what variable mapping are from a user point of view. Or we should maybe wipe out these naming differences in the properties panel and BPMN XML.
There was a problem hiding this comment.
@korthout - any input from your side on my comment here?
There was a problem hiding this comment.
@Hafflgav I see your point, but not all users use the Camunda Modeler to model their process (e.g. they produce their own XML or use zeebe-bpmn-model to generate it). Zeebe requires XML that adheres to a specific spec, which is currently described in these reference docs. We should not remove that. We can however expand the docs to help out users and I agree that something like that is needed.
Note that the general concept of variable mappings is currently explained in Concepts, which also already uses terms like ioMapping, source and target. We should probably change that first.
AFAIK, the references pages, that this service-task page belongs to, have recently been moved entirely under the Modeler components docs, but IMO not all of it belongs there. My preference would be that the concept is described in the Concept. The Camunda Modeler specific docs on variable mappings should be described in the Modeler component docs, while the way that Zeebe deals with variable mapping should be described in the Zeebe component docs. A guide on using variable mappings should be available in the Guides. I assume that this is also the longer term vision of @akeller and @christinaausley (please correct me if I'm wrong).
As a first step towards that, it might be useful to differentiate in this docs the difference between the concepts that are described (e.g.Process Variable Name and Input Source or Input Variable Expression, etc) and how these are named in different parts (e.g. source and target in xml and Local Input Variable and Process Variable Name in the Modeler). Personally I would then first change the concepts page to use the correct terms. We can then change the reference docs on service tasks accordingly.
How do you see this @Hafflgav, is that the way to go or would you do it differently? Sorry about the long text, I just wanted to share my thoughts to make it clear why I'm against just changing these names only on these references pages, but also provide a way to move forward. I hope it helps.
There was a problem hiding this comment.
I think at guide for variable mappings make a lot of sense, I'll get that added into our backlog where we can figure out who will write that content.
Regarding BPMN documentation and it's location, I'm happy to talk more about this separately.
|
Are these changes ready to be reviewed? I wasn't sure because there were still comments not resolved. We also moved the URLs for the BPMN content, so there are a few conflicts now 🙈 |
|
@Hafflgav I've answered on your open comments. Please let me know when I need to have another look. |
|
|
||
| For more information about this topic visit the documentation about [Input/output variable mappings](/components/concepts/variables.md#inputoutput-variable-mappings). |
There was a problem hiding this comment.
I would probably say here "For more information about this topic, visit the documentation on input and output variable mappings."
| the type of job workers should subscribe to (e.g. `script`). | ||
|
|
||
| Use [task headers](../../service-tasks/service-tasks#task-headers) to pass static parameters to the job | ||
| worker (e.g. the script to evaluate). | ||
| worker (e.g. the script to evaluate). The community extension [Zeebe Script Worker](https://github.com/camunda-community-hub/zeebe-script-worker) requires certain attributes to be set in the task headers. | ||
| Take a look at the link for more information. |
There was a problem hiding this comment.
I think you can remove line 33, as we already provide the link above and the user should already understand they can click on it for additional information.
|
Closing this PR: |
Changed:
A rather major change has been made to the paragraph of variable mappings.
Please review these changes and rephrase if necessary. :)
It would be awesome if you could leave this branch open since I am going to do some more stuff in here.