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

Correct Dropdown rendering on undefined zebee:input#source #824

Merged
merged 9 commits into from
Dec 15, 2022

Conversation

smbea
Copy link
Contributor

@smbea smbea commented Dec 1, 2022

@bpmn-io-tasks bpmn-io-tasks bot added the needs review Review pending label Dec 1, 2022
@smbea smbea marked this pull request as draft December 1, 2022 14:29
@bpmn-io-tasks bpmn-io-tasks bot added in progress Currently worked on and removed needs review Review pending labels Dec 1, 2022
@smbea smbea marked this pull request as ready for review December 1, 2022 16:07
@bpmn-io-tasks bpmn-io-tasks bot added needs review Review pending and removed in progress Currently worked on labels Dec 1, 2022
@smbea smbea requested a review from nikku December 1, 2022 16:07
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.

Great stuff. With this we're now consistent across the board, which is 🏆.

Just two minor remarks (we can ignore these and await user feedback).

@smbea
Copy link
Contributor Author

smbea commented Dec 1, 2022

If I understood correctly, it was an easy fix by adding isDefined there

@smbea smbea requested a review from nikku December 1, 2022 19:59
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.

I did a roundtrip test with the newly added undefined-input-binding.json template.

Result:

  • Initial binding works fine (fixed in this PR)
  • Removing values leads to left-overs though where the expected behavior would be to remove the bindings from XML (similar to initial creation)

However that implies we actually know what we're doing: What exactly are the semantics of input and output bindings?

Let's hold this for a moment though as I try to clarify the direction that we want to head to with Zeebe input bindings, camunda/camunda#11163.


    <bpmn:serviceTask id="Activity_1m5q40l" zeebe:modelerTemplate="undefined-input-binding">
      <bpmn:extensionElements>
        <zeebe:ioMapping>
          <zeebe:input source="set-2" target="ignore" />
          <zeebe:input target="test" />
          <zeebe:output source="test" />
        </zeebe:ioMapping>
        <zeebe:taskHeaders />
        <zeebe:properties>
          <zeebe:property name="test" value="" />
        </zeebe:properties>
        <zeebe:taskDefinition />
      </bpmn:extensionElements>
    </bpmn:serviceTask>

@bpmn-io-tasks bpmn-io-tasks bot added in progress Currently worked on and removed needs review Review pending labels Dec 2, 2022
@nikku
Copy link
Member

nikku commented Dec 5, 2022

@smbea Let's proceed regarding the discovery in #824 (review):

Removing values leads to left-overs though where the expected behavior would be to remove the bindings from XML (similar to initial creation).

Let's update this PR and ensure on removal the whole input / property / ... is being removed, not only the value. This way we are consistent across create + apply + removal.

Properly handling task input in a no-magic fashion (camunda/camunda#11163) on the engine side is likely not built upon "empty" inputs (<zeebe:input target="test" />).

@nikku nikku marked this pull request as draft December 8, 2022 14:07
@smbea
Copy link
Contributor Author

smbea commented Dec 9, 2022

@nikku could you let me know how you reached those leftovers scenario? Was changing between templates? I wasn't able to reproduce it yet

@nikku
Copy link
Member

nikku commented Dec 12, 2022

Related bug, found through #824 (review) and hopefully properly reproducible (against this branch): #830.

@smbea smbea marked this pull request as ready for review December 13, 2022 12:38
@bpmn-io-tasks bpmn-io-tasks bot added needs review Review pending and removed in progress Currently worked on labels Dec 13, 2022
@smbea smbea requested a review from nikku December 13, 2022 12:38
@nikku nikku force-pushed the fix-binding-initialization branch 2 times, most recently from b47370b to bb309f4 Compare December 14, 2022 12:49
@nikku
Copy link
Member

nikku commented Dec 14, 2022

As indicated here #830 is a won't fix / not a bug, as I missed optional completely when reproducing this issue:

  • 🅰️ Element template properties with binding zeebe:input, zeebe:property and zeebe:output may be marked as optional. In that case, if they are falsy (empty string) then they must not be serialized into the XML. If they are not marked as optional they must be serialized into the XML as empty properties.
  • 🅱️ Optional is not supported for zeebe:taskHeader (why?) and zeebe:taskDefinition:type (makes sense?)

To validate 🅰️ this I did the following:

  • Add dedicated templates for the no variables case (with or without optional) ✔️
  • Validate it works as intended using a test diagram => Optional removes empty properties, while mandatory keeps (even if properties are broken / diagram may not deploy) ✔️

@nikku
Copy link
Member

nikku commented Dec 14, 2022

Additional findings of the integration test:

@nikku
Copy link
Member

nikku commented Dec 14, 2022

This one is also a won't fix, as it does break the optional contract.

Actual fix for the (visual) glitch reported via camunda/camunda-modeler#3327 is bpmn-io/properties-panel#203.

TLDR: We did not properly convert undefined values (i.e. input being non-existent or missing source to an empty string, not setting the select value, causing the browser default behavior to render the first select option.

@nikku nikku force-pushed the fix-binding-initialization branch 3 times, most recently from 9b6e98a to cc4a3b7 Compare December 14, 2022 22:27
@nikku nikku self-requested a review December 14, 2022 22:28
@nikku nikku changed the title Only add zeebe:input/output/zeebe:property binding if value is defined Correct Dropdown rendering on undefined zebee:input#source Dec 15, 2022
@nikku
Copy link
Member

nikku commented Dec 15, 2022

Updated this PR to contain the properties panel patch bump (cd1b51e) + test improvements only.

@nikku nikku merged commit 3408f79 into master Dec 15, 2022
@nikku nikku deleted the fix-binding-initialization branch December 15, 2022 14:23
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label Dec 15, 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