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

Add 'scopes' parameter to allow creation of connector template #229

Closed
wants to merge 17 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@GuillaumeSmaha
Contributor

GuillaumeSmaha commented Apr 3, 2017

No description provided.

@gimbel gimbel added the needs review label Apr 3, 2017

@GuillaumeSmaha

This comment has been minimized.

Show comment
Hide comment
@GuillaumeSmaha

GuillaumeSmaha Apr 3, 2017

Contributor

@nikku I create the PR to discuss about it.
It is not ready to merge.

Contributor

GuillaumeSmaha commented Apr 3, 2017

@nikku I create the PR to discuss about it.
It is not ready to merge.

@GuillaumeSmaha GuillaumeSmaha changed the title from Add scope parameter to allow creation of connector template to Add 'scopes' parameter to allow creation of connector template Apr 4, 2017

@GuillaumeSmaha

This comment has been minimized.

Show comment
Hide comment
@GuillaumeSmaha

GuillaumeSmaha Apr 4, 2017

Contributor

@nikku I added the test for the functionnality in ChangeElementTemplateHandler and Validator

Contributor

GuillaumeSmaha commented Apr 4, 2017

@nikku I added the test for the functionnality in ChangeElementTemplateHandler and Validator

@GuillaumeSmaha

This comment has been minimized.

Show comment
Hide comment
@GuillaumeSmaha

GuillaumeSmaha Apr 4, 2017

Contributor

Error fixed, I made a mistake in mail-task.json

Contributor

GuillaumeSmaha commented Apr 4, 2017

Error fixed, I made a mistake in mail-task.json

@StephenOTT

This comment has been minimized.

Show comment
Hide comment
@StephenOTT

StephenOTT Apr 6, 2017

@nikku any comments on this?

StephenOTT commented Apr 6, 2017

@nikku any comments on this?

@nikku

This comment has been minimized.

Show comment
Hide comment
@nikku

nikku Apr 20, 2017

Member

Sorry for the long waiting time. Will look into this PR tomorrow.

Member

nikku commented Apr 20, 2017

Sorry for the long waiting time. Will look into this PR tomorrow.

@nikku nikku self-assigned this Apr 20, 2017

@nikku

This comment has been minimized.

Show comment
Hide comment
@nikku

nikku Apr 26, 2017

Member

Updated your feature branch with changes from master.

Feedback incoming tomorrow.

Member

nikku commented Apr 26, 2017

Updated your feature branch with changes from master.

Feedback incoming tomorrow.

@GuillaumeSmaha

This comment has been minimized.

Show comment
Hide comment
@GuillaumeSmaha

GuillaumeSmaha Apr 26, 2017

Contributor

@nikku I updated the branch from the master

Contributor

GuillaumeSmaha commented Apr 26, 2017

@nikku I updated the branch from the master

@nikku

A few questions regarding your PR:

  • Why did you choose scopes = [] over scopes = { }? The later could be keyed by scope name, i.e. scopes = { 'camunda:connector': { connectorConfig } } and would ensure that there can be only one configuration per named scope, i.e. connector, messageEventDefinition or whatever else we add in the future.

  • Following up on that, how does the implementation behave, if the user, by accident configures two "connector" scopes?

  • We could and should also validate the allowed scopes, as we do the same kind of validation for properties and so on.

  • What is inside, used in the element template descriptor about and can we get rid of it from the users point of view? As part of the codebase calling it inScope may be a better name. (also added a few pointers in your changes regarding this).

I see the implementation works; Tests are missing though for setting scoped properties, inputOutput, ...

Critical feedback aside, this PR is on a good track.

I like the introduction additional custom field groups, too.

@nikku nikku added ready and removed needs review ready labels Apr 27, 2017

@nikku

This comment has been minimized.

Show comment
Hide comment
@nikku

nikku May 9, 2017

Member

Any plans to act on the review feedback?

Member

nikku commented May 9, 2017

Any plans to act on the review feedback?

@StephenOTT

This comment has been minimized.

Show comment
Hide comment
@StephenOTT

StephenOTT May 9, 2017

Yes. Due to the delay in-between initial submission and feedback we had to move on to other work and devs are committed to another sprint at the moment.

StephenOTT commented May 9, 2017

Yes. Due to the delay in-between initial submission and feedback we had to move on to other work and devs are committed to another sprint at the moment.

@nikku

This comment has been minimized.

Show comment
Hide comment
@nikku

nikku May 9, 2017

Member

Thanks for the info and sorry again for the delay in reviewing.

Member

nikku commented May 9, 2017

Thanks for the info and sorry again for the delay in reviewing.

@GuillaumeSmaha

This comment has been minimized.

Show comment
Hide comment
@GuillaumeSmaha

GuillaumeSmaha May 19, 2017

Contributor

@nikku

  • I chose to use scopes= [] because I didn't have a global vision of the project and I didn"t know if the document can contain multiple times the same key.
    I will use scope = {} instead.

  • For multiple connector scope, it will throw an error.

  • The allowed scope are automaticly checked when the insertion command is executed. An error is throwed when the document become invalid.

  • I think inside can be removed from the users point of view and always set as true.

  • I will add more test for setting scoped properties

Contributor

GuillaumeSmaha commented May 19, 2017

@nikku

  • I chose to use scopes= [] because I didn't have a global vision of the project and I didn"t know if the document can contain multiple times the same key.
    I will use scope = {} instead.

  • For multiple connector scope, it will throw an error.

  • The allowed scope are automaticly checked when the insertion command is executed. An error is throwed when the document become invalid.

  • I think inside can be removed from the users point of view and always set as true.

  • I will add more test for setting scoped properties

@GuillaumeSmaha

This comment has been minimized.

Show comment
Hide comment
@GuillaumeSmaha

GuillaumeSmaha May 19, 2017

Contributor

@nikku Edit for my previous note ; I remember why I used inside.

An element <bpmn:extensionElements> contains an element<camunda:inputOutput> and it was stored as a child in the code whereas a <camunda:connector> element contains an element<camunda:inputOutput> as a properties and not as a child.

I think it will be better to change that. So it will be possible to delete inside var and also insideConnector var which used in https://github.com/bpmn-io/bpmn-js-properties-panel/blob/master/lib/helper/InputOutputHelper.js#L32
What do you think to remove insideConnector and to not do a special case for connector element ?

Contributor

GuillaumeSmaha commented May 19, 2017

@nikku Edit for my previous note ; I remember why I used inside.

An element <bpmn:extensionElements> contains an element<camunda:inputOutput> and it was stored as a child in the code whereas a <camunda:connector> element contains an element<camunda:inputOutput> as a properties and not as a child.

I think it will be better to change that. So it will be possible to delete inside var and also insideConnector var which used in https://github.com/bpmn-io/bpmn-js-properties-panel/blob/master/lib/helper/InputOutputHelper.js#L32
What do you think to remove insideConnector and to not do a special case for connector element ?

GuillaumeSmaha added some commits Mar 30, 2017

Add scope parameter to allow creation of connector template
Add Custom fields scope in Properties panel
Add scopes validation in ValidatorSpec
Add test case for ChangeElementTemplateHandler
@GuillaumeSmaha

This comment has been minimized.

Show comment
Hide comment
@GuillaumeSmaha

GuillaumeSmaha May 26, 2017

Contributor

@nikku I updated the PR to use an object for scopes and I add test as you ask.

Contributor

GuillaumeSmaha commented May 26, 2017

@nikku I updated the PR to use an object for scopes and I add test as you ask.

@GuillaumeSmaha

This comment has been minimized.

Show comment
Hide comment
@GuillaumeSmaha

GuillaumeSmaha May 26, 2017

Contributor

I tried to remove insideConnector but it needs a lot of work due to the difference between root element containing an extensionElement and camunda:Connector which contains inputOutput in properties

Contributor

GuillaumeSmaha commented May 26, 2017

I tried to remove insideConnector but it needs a lot of work due to the difference between root element containing an extensionElement and camunda:Connector which contains inputOutput in properties

@nikku

This comment has been minimized.

Show comment
Hide comment
@nikku

nikku May 29, 2017

Member

Looking at the existing code base, we already have switches for similar behavior in place, cf. field injection props or the utility you've pointed out. Because of this I would be fine to just rename it to insideConnector for clarity.

Still, can't we assume insideConnector to be true when context != null is being passed (ref)?

Member

nikku commented May 29, 2017

Looking at the existing code base, we already have switches for similar behavior in place, cf. field injection props or the utility you've pointed out. Because of this I would be fine to just rename it to insideConnector for clarity.

Still, can't we assume insideConnector to be true when context != null is being passed (ref)?

@GuillaumeSmaha

This comment has been minimized.

Show comment
Hide comment
@GuillaumeSmaha

GuillaumeSmaha May 29, 2017

Contributor

Still, can't we assume insideConnector to be true when context != null is being passed (ref)?

You have a better knowledge than me. I only focus on camunda:Connector, do you know if there are others elements than Connector inside extensionElements ? If you think no, then I will made the change.

Contributor

GuillaumeSmaha commented May 29, 2017

Still, can't we assume insideConnector to be true when context != null is being passed (ref)?

You have a better knowledge than me. I only focus on camunda:Connector, do you know if there are others elements than Connector inside extensionElements ? If you think no, then I will made the change.

@GuillaumeSmaha

This comment has been minimized.

Show comment
Hide comment
@GuillaumeSmaha

GuillaumeSmaha Jun 5, 2017

Contributor

I tried with this examples and it works ! 👍

  {
    "name": "ConnectorTask",
    "id": "my.connector.Task",
    "appliesTo": [
      "bpmn:ServiceTask"
    ],
    "properties": [],
    "scopes": {
      "camunda:Connector": {
        "insideProperties": true,
        "properties": [
          {
            "label": "ConnectorId",
            "type": "String",
            "value": "My Connector HTTP",
            "binding": {
              "type": "property",
              "name": "connectorId"
            },
            "constraints": {
              "notEmpty": true
            }
          },
          {
            "label": "URL",
            "type": "String",
            "binding": {
              "type": "camunda:inputParameter",
              "name": "recipient"
            },
            "constraints": {
              "notEmpty": true
            }
          },
          {
            "label": "Method",
            "type": "Dropdown",
            "choices": [
              {
                "value": "GET",
                "name":"GET"
              },
              {
                "value": "POST",
                "name": "POST"
              },
              {
                "value": "PUT",
                "name": "PUT"
              },
              {
                "value": "PATCH",
                "name": "PATCH"
              },
              {
                "value": "DELETE",
                "name": "DELETE"
              }
            ],
            "binding": {
              "type": "camunda:inputParameter",
              "name": "method"
            },
            "constraints": {
              "notEmpty": true
            }
          },
          {
            "type": "Hidden",
            "value": "Camunda",
            "binding": {
              "type": "camunda:inputParameter",
              "name": "agent"
            }
          },
          {
            "label": "Response",
            "type": "String",
            "value": "${S(response)}",
            "binding": {
              "type": "camunda:outputParameter",
              "name": "wsResponse",
              "source": "wsResponse"
            }
          }
        ]
      }
    }
  },
Contributor

GuillaumeSmaha commented Jun 5, 2017

I tried with this examples and it works ! 👍

  {
    "name": "ConnectorTask",
    "id": "my.connector.Task",
    "appliesTo": [
      "bpmn:ServiceTask"
    ],
    "properties": [],
    "scopes": {
      "camunda:Connector": {
        "insideProperties": true,
        "properties": [
          {
            "label": "ConnectorId",
            "type": "String",
            "value": "My Connector HTTP",
            "binding": {
              "type": "property",
              "name": "connectorId"
            },
            "constraints": {
              "notEmpty": true
            }
          },
          {
            "label": "URL",
            "type": "String",
            "binding": {
              "type": "camunda:inputParameter",
              "name": "recipient"
            },
            "constraints": {
              "notEmpty": true
            }
          },
          {
            "label": "Method",
            "type": "Dropdown",
            "choices": [
              {
                "value": "GET",
                "name":"GET"
              },
              {
                "value": "POST",
                "name": "POST"
              },
              {
                "value": "PUT",
                "name": "PUT"
              },
              {
                "value": "PATCH",
                "name": "PATCH"
              },
              {
                "value": "DELETE",
                "name": "DELETE"
              }
            ],
            "binding": {
              "type": "camunda:inputParameter",
              "name": "method"
            },
            "constraints": {
              "notEmpty": true
            }
          },
          {
            "type": "Hidden",
            "value": "Camunda",
            "binding": {
              "type": "camunda:inputParameter",
              "name": "agent"
            }
          },
          {
            "label": "Response",
            "type": "String",
            "value": "${S(response)}",
            "binding": {
              "type": "camunda:outputParameter",
              "name": "wsResponse",
              "source": "wsResponse"
            }
          }
        ]
      }
    }
  },

@nikku nikku dismissed their stale review Jun 9, 2017

Outdated

@nikku

This comment has been minimized.

Show comment
Hide comment
@nikku

nikku Jun 9, 2017

Member

We're getting closer to merging...

Member

nikku commented Jun 9, 2017

We're getting closer to merging...

@nikku

This comment has been minimized.

Show comment
Hide comment
@nikku

nikku Jun 9, 2017

Member

While working on a few more improvements I recognized that the example connector CustomProps.bpmn you've added is invalid: It does not contain a value for a property that, by template definition, must be provided.

As a result one of the test cases fails unless we apply another fix.

My question is: Should we deal with broken (without value) connector definitions? If yes, we'd need to pull in e18e33d. Otherwise you should change the CustomProps.bpmn and provide actual values.

Member

nikku commented Jun 9, 2017

While working on a few more improvements I recognized that the example connector CustomProps.bpmn you've added is invalid: It does not contain a value for a property that, by template definition, must be provided.

As a result one of the test cases fails unless we apply another fix.

My question is: Should we deal with broken (without value) connector definitions? If yes, we'd need to pull in e18e33d. Otherwise you should change the CustomProps.bpmn and provide actual values.

@nikku

This comment has been minimized.

Show comment
Hide comment
@nikku

nikku Jun 9, 2017

Member

One last thing I'd like you to look into:

As a pattern in CustomProps, we'll always create the whole model tree when setting an individual property. That said, when the user provides an incomplete BPMN 2.0 XML reading and setting properties should still work for individual entries. An example XML could be this:

<bpmn:ServiceTask id="ConnectorTask" name="Connector Task" camunda:modelerTemplate="my.connector.Task">
</bpmn:ServiceTask>

When the user changes a property such as url, CustomProps does make sure the respective model structure is created:

<bpmn:ServiceTask id="ConnectorTask" name="Connector Task" camunda:modelerTemplate="my.connector.Task">
  <bpmn:extensionElements>
    <camunda:connector>
      <camunda:inputOutput>
        <camunda:inputParameter name="url"></camunda:inputParameter>
      </camunda:inputOutput>
      <camunda:connectorId>My Connector HTTP</camunda:connectorId>
    </camunda:connector>
  </bpmn:extensionElements>
</bpmn:ServiceTask>

Your implementation right now assumes that at least the following stub is always given:

<bpmn:ServiceTask id="ConnectorTask_NoData" name="Connector Task NoData" camunda:modelerTemplate="my.connector.Task">
  <bpmn:extensionElements>
    <camunda:connector>
      <camunda:inputOutput />
    </camunda:connector>
  </bpmn:extensionElements>
</bpmn:ServiceTask>

Could you update this to create the whole structure on the fly, in case it does not exist yet?

Member

nikku commented Jun 9, 2017

One last thing I'd like you to look into:

As a pattern in CustomProps, we'll always create the whole model tree when setting an individual property. That said, when the user provides an incomplete BPMN 2.0 XML reading and setting properties should still work for individual entries. An example XML could be this:

<bpmn:ServiceTask id="ConnectorTask" name="Connector Task" camunda:modelerTemplate="my.connector.Task">
</bpmn:ServiceTask>

When the user changes a property such as url, CustomProps does make sure the respective model structure is created:

<bpmn:ServiceTask id="ConnectorTask" name="Connector Task" camunda:modelerTemplate="my.connector.Task">
  <bpmn:extensionElements>
    <camunda:connector>
      <camunda:inputOutput>
        <camunda:inputParameter name="url"></camunda:inputParameter>
      </camunda:inputOutput>
      <camunda:connectorId>My Connector HTTP</camunda:connectorId>
    </camunda:connector>
  </bpmn:extensionElements>
</bpmn:ServiceTask>

Your implementation right now assumes that at least the following stub is always given:

<bpmn:ServiceTask id="ConnectorTask_NoData" name="Connector Task NoData" camunda:modelerTemplate="my.connector.Task">
  <bpmn:extensionElements>
    <camunda:connector>
      <camunda:inputOutput />
    </camunda:connector>
  </bpmn:extensionElements>
</bpmn:ServiceTask>

Could you update this to create the whole structure on the fly, in case it does not exist yet?

@felix-mueller

This comment has been minimized.

Show comment
Hide comment
@felix-mueller

felix-mueller Jul 18, 2017

Contributor

Hi @GuillaumeSmaha
any chance to check Nikku's proposal? I was also looking for element templates for connectors today so maybe we can move this forward?

Contributor

felix-mueller commented Jul 18, 2017

Hi @GuillaumeSmaha
any chance to check Nikku's proposal? I was also looking for element templates for connectors today so maybe we can move this forward?

@GuillaumeSmaha

This comment has been minimized.

Show comment
Hide comment
@GuillaumeSmaha

GuillaumeSmaha Aug 17, 2017

Contributor

@nikku @felix-mueller I will work on it in the next days

Contributor

GuillaumeSmaha commented Aug 17, 2017

@nikku @felix-mueller I will work on it in the next days

@firle

This comment has been minimized.

Show comment
Hide comment
@firle

firle Sep 7, 2017

Hey Guys!
Currently we developing with Camunda and to simplify our workflows it would be great to use templates for connectors. Since the last reply is 3 weeks ago i want to ask how long it will take to include this feature discussed here to include on the master branch. This will push us forward!
Thanks and Thanks to your work guys!

firle commented Sep 7, 2017

Hey Guys!
Currently we developing with Camunda and to simplify our workflows it would be great to use templates for connectors. Since the last reply is 3 weeks ago i want to ask how long it will take to include this feature discussed here to include on the master branch. This will push us forward!
Thanks and Thanks to your work guys!

@GuillaumeSmaha

This comment has been minimized.

Show comment
Hide comment
@GuillaumeSmaha

GuillaumeSmaha Sep 7, 2017

Contributor

I am still working on this on my free time.

Contributor

GuillaumeSmaha commented Sep 7, 2017

I am still working on this on my free time.

@nikku nikku removed the needs review label Oct 4, 2017

@GuillaumeSmaha

This comment has been minimized.

Show comment
Hide comment
@GuillaumeSmaha

GuillaumeSmaha Oct 6, 2017

Contributor

@nikku I fixed CustomProp to generate the whole model tree

Contributor

GuillaumeSmaha commented Oct 6, 2017

@nikku I fixed CustomProp to generate the whole model tree

@nikku

This comment has been minimized.

Show comment
Hide comment
@nikku

nikku Oct 6, 2017

Member

Thanks for the changes. I'll have a look.

Member

nikku commented Oct 6, 2017

Thanks for the changes. I'll have a look.

@StephenOTT

This comment has been minimized.

Show comment
Hide comment
@StephenOTT

StephenOTT commented Oct 11, 2017

@nikku any updates?

@nikku

This comment has been minimized.

Show comment
Hide comment
@nikku

nikku Oct 11, 2017

Member

Yep. CI is failing for this PR. I'll have a look, as it does not seem to be related to the added functionality.

Member

nikku commented Oct 11, 2017

Yep. CI is failing for this PR. I'll have a look, as it does not seem to be related to the added functionality.

@GuillaumeSmaha

This comment has been minimized.

Show comment
Hide comment
@GuillaumeSmaha

GuillaumeSmaha Oct 11, 2017

Contributor

@nikku @StephenOTT I fix CI test (Merge and Fix for syntax)

Contributor

GuillaumeSmaha commented Oct 11, 2017

@nikku @StephenOTT I fix CI test (Merge and Fix for syntax)

@nikku

This comment has been minimized.

Show comment
Hide comment
@nikku

nikku Oct 12, 2017

Member

I've consolidated your changes on the add-scopes-snapshot branch. There are a couple of issues with our CI, unrelated to this PR. I'm about to fix these and we're ready to merge.

In general I'd ask you to follow our Contribution Guidelines to make reviewing and merging easier. This includes running npm run all to ensure code is properly formated and working against our test suite, providing commit messages adhering to our conventions and using the rebase workflow instead of merge commits to pull in upstream changes (read more about it here).

Member

nikku commented Oct 12, 2017

I've consolidated your changes on the add-scopes-snapshot branch. There are a couple of issues with our CI, unrelated to this PR. I'm about to fix these and we're ready to merge.

In general I'd ask you to follow our Contribution Guidelines to make reviewing and merging easier. This includes running npm run all to ensure code is properly formated and working against our test suite, providing commit messages adhering to our conventions and using the rebase workflow instead of merge commits to pull in upstream changes (read more about it here).

@nikku

This comment has been minimized.

Show comment
Hide comment
@nikku

nikku Oct 12, 2017

Member

Merged via 3d38955.

Thank you @GuillaumeSmaha and congratulations to your first big contribution 🏆.

Member

nikku commented Oct 12, 2017

Merged via 3d38955.

Thank you @GuillaumeSmaha and congratulations to your first big contribution 🏆.

@nikku nikku closed this Oct 12, 2017

@nikku

This comment has been minimized.

Show comment
Hide comment
@nikku
Member

nikku commented Oct 12, 2017

@felix-mueller

This comment has been minimized.

Show comment
Hide comment
@felix-mueller

felix-mueller Oct 12, 2017

Contributor

Awesome, thank you @GuillaumeSmaha and @nikku !

Contributor

felix-mueller commented Oct 12, 2017

Awesome, thank you @GuillaumeSmaha and @nikku !

@GuillaumeSmaha

This comment has been minimized.

Show comment
Hide comment
@GuillaumeSmaha

GuillaumeSmaha Oct 12, 2017

Contributor

@nikku Thanks :)

Contributor

GuillaumeSmaha commented Oct 12, 2017

@nikku Thanks :)

@StephenOTT

This comment has been minimized.

Show comment
Hide comment
@StephenOTT

StephenOTT Oct 12, 2017

@GuillaumeSmaha Great work! 🎉

@nikku would the new feature be in the nightly of Camunda Modeler?

StephenOTT commented Oct 12, 2017

@GuillaumeSmaha Great work! 🎉

@nikku would the new feature be in the nightly of Camunda Modeler?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment