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

fix(cloud-templates): cast default number and boolean numbers to feel #121

Merged
merged 2 commits into from
Sep 13, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions src/cloud-element-templates/Helper.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { getBusinessObject } from 'bpmn-js/lib/util/ModelUtil';
import { is, isAny } from 'bpmn-js/lib/util/ModelUtil';

import { v4 as uuid } from 'uuid';
import { isSpecialFeelProperty, toFeelExpression } from './util/feelUtil';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor nit-pick: Let's rename the utility to CamelCase, i.e. FeelUtil.

I acknowledge that we already have some lowerCaseYesThatIsMe files in the code-base, but let's not introduce too many more of them, as the standard is starting with a capital letter.

Copy link
Collaborator Author

@marstamm marstamm Sep 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that the convention we have? As I see it, the file name reflects the default export. Per convention, Classes and Components are written in PascalCase, hence a lot of our files are also PascalCase.

The utility files do not have a default export or exports a function. Since a function is usually camelCase (ignoring functional components), the file name will reflect this as well.

This is consistent with all existing utility files

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cf. https://github.com/bpmn-io/bpmn-js-element-templates/blob/e8d563a5b4b5abcdaf88ffada30fd9c0c914c95e/src/cloud-element-templates/util/validate.js as an example.

Validator is a class, since it is in Validator.js. When making it a utility validate, the file name will reflect the contents and is named validate.js (lower case)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can only look towards our different utilities in the eco-system, and they all seem to be historically upper-case - https://github.com/bpmn-io/diagram-js/tree/develop/lib/util.

Only recently we started lower-casing things in side-projects, but also not consistently. Predominently, utilities are still upper case, unless imported via index.js directly - https://github.com/bpmn-io/bpmn-js-element-templates/tree/main/src/utils.

👍 on reflecting the name / type of the default export, where it makes sense. But your utility does not provide a default export, but is a collection of various things - https://github.com/bpmn-io/bpmn-js-element-templates/pull/121/files#diff-ed2c638650f4cb053378f288c21bb35939630f122e9d0eb79f4924d62984a084.

Another debate is if "single function per file", with a default context is an appropriate way to slice things. Sometimes (but only sometimes) it makes sense.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that we are not consistent in our naming schemes. In the context of this JSX project, I would argue that using camelCase for utilities helps with understanding the file structure better. Using Upercase primarily for Components helps with knowing what the contents of the file will be without having to open the file itself.

Predominently, utilities are still upper case

Only if they are closely related to a bpmn-js utility they are extending:

https://github.com/bpmn-io/bpmn-js-element-templates/tree/main/src/cloud-element-templates/util

But your utility does not provide a default export, but is a collection of various things

While the utility file does not have a default export, it still only exports functions and should therefore follow the naming convention for functions, rather than Components

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No hard opinion, but if we want to keep naming consistent, I'd follow the unofficial core libraries rule of PascalCase everywhere (cf. bpmn-js and diagram-js).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adjusted to PascalCase

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the end, stuff is usually auto-imported. For that reason, I'd discourage default exports.


/**
* The BPMN 2.0 extension attribute name under
Expand Down Expand Up @@ -177,6 +178,12 @@ export function findZeebeSubscription(message) {

export function getDefaultValue(property) {

if (
isSpecialFeelProperty(property)
) {
return toFeelExpression(property.value, property.type);
}

if (property.value !== undefined) {
return property.value;
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { FeelNumberEntry, NumberFieldEntry } from '@bpmn-io/properties-panel';
import { useService } from 'bpmn-js-properties-panel';
import { isSpecialFeelProperty, propertyValidator, usePropertyAccessors } from './util';
import { propertyValidator, usePropertyAccessors } from './util';
import { isSpecialFeelProperty } from '../../../util/feelUtil';
import { PropertyDescription } from '../../../../components/PropertyDescription';
import { PropertyTooltip } from '../../components/PropertyTooltip';
import { useCallback } from '@bpmn-io/properties-panel/preact/hooks';
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import { find, groupBy } from 'min-dash';
import { getPropertyValue, setPropertyValue, validateProperty } from '../../../util/propertyUtil';
import { isSpecialFeelProperty, toFeelExpression } from '../../../util/feelUtil';

import { useCallback, useState } from '@bpmn-io/properties-panel/preact/hooks';

export function usePropertyAccessors(bpmnFactory, commandStack, element, property) {
Expand Down Expand Up @@ -38,26 +40,6 @@ export function usePropertyAccessors(bpmnFactory, commandStack, element, propert
return [ get, set ];
}

export const isSpecialFeelProperty = (property) => {
return [ 'optional', 'static' ].includes(property.feel) && [ 'Boolean', 'Number' ].includes(property.type);
};

const toFeelExpression = (value, type) => {
if (typeof value === 'string' && value.startsWith('=')) {
return value;
}

if (type === 'Boolean') {
value = value === 'false' ? false : value;
return '=' + !!value;
}

if (typeof value === 'undefined') {
return value;
}

return '=' + value.toString();
};

const fromFeelExpression = (value, type) => {
if (typeof value === 'undefined') {
Expand Down
20 changes: 20 additions & 0 deletions src/cloud-element-templates/util/feelUtil.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
export const isSpecialFeelProperty = (property) => {
return [ 'optional', 'static' ].includes(property.feel) && [ 'Boolean', 'Number' ].includes(property.type);
};

export const toFeelExpression = (value, type) => {
if (typeof value === 'string' && value.startsWith('=')) {
return value;
}

if (type === 'Boolean') {
value = value === 'false' ? false : value;
return '=' + !!value;
}

if (typeof value === 'undefined') {
return value;
}

return '=' + value.toString();
};
Original file line number Diff line number Diff line change
Expand Up @@ -4265,6 +4265,51 @@ describe('cloud-element-templates/cmd - ChangeElementTemplateHandler', function(

});


describe('FEEL Boolean and Numbers', function() {

beforeEach(bootstrap(require('./casted-values.bpmn').default));

describe('Boolean', function() {

const template = require('./casted-values.json')[0];

it('should apply generated value (uuid)', inject(function(elementRegistry) {

// given
let task = elementRegistry.get('Task_1');

// when
task = changeTemplate(task, template);

// then
expect(getZeebeProperty(task, 'StaticBooleanProperty').value).to.eql('=true');
expect(getZeebeProperty(task, 'OptionalBooleanProperty').value).to.eql('=true');
}));

});

describe('Number', function() {

const template = require('./casted-values.json')[1];

it('should apply generated value (uuid)', inject(function(elementRegistry) {

// given
let task = elementRegistry.get('Task_1');

// when
task = changeTemplate(task, template);

// then
expect(getZeebeProperty(task, 'StaticNumberProperty').value).to.eql('=123');
expect(getZeebeProperty(task, 'OptionalNumberProperty').value).to.eql('=123');
}));

});

});

});


Expand Down
17 changes: 17 additions & 0 deletions test/spec/cloud-element-templates/cmd/casted-values.bpmn
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
<?xml version="1.0" encoding="UTF-8"?>
<bpmn:definitions xmlns:bpmn="http://www.omg.org/spec/BPMN/20100524/MODEL" xmlns:bpmndi="http://www.omg.org/spec/BPMN/20100524/DI" xmlns:dc="http://www.omg.org/spec/DD/20100524/DC" id="Definitions_039bbyr" targetNamespace="http://bpmn.io/schema/bpmn" exporter="Camunda Modeler" exporterVersion="4.4.0">
<bpmn:process id="Process_1" isExecutable="true">
<bpmn:serviceTask id="Task_1" />
<bpmn:intermediateThrowEvent id="Event_1" />
</bpmn:process>
<bpmndi:BPMNDiagram id="BPMNDiagram_1">
<bpmndi:BPMNPlane id="BPMNPlane_1" bpmnElement="Process_1">
<bpmndi:BPMNShape id="Task_1_di" bpmnElement="Task_1">
<dc:Bounds x="0" y="0" width="100" height="80" />
</bpmndi:BPMNShape>
<bpmndi:BPMNShape id="Event_0f371po_di" bpmnElement="Event_1">
<dc:Bounds x="92" y="172" width="36" height="36" />
</bpmndi:BPMNShape>
</bpmndi:BPMNPlane>
</bpmndi:BPMNDiagram>
</bpmn:definitions>
62 changes: 62 additions & 0 deletions test/spec/cloud-element-templates/cmd/casted-values.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
[
{
"$schema": "https://unpkg.com/@camunda/zeebe-element-templates-json-schema/resources/schema.json",
"name": "booleanField optional",
"id": "booleanField.feel.optional",
"appliesTo": [
"bpmn:ServiceTask"
],
"properties": [
{
"label": "DefaultStaticBooleanProperty",
"type": "Boolean",
"feel": "static",
"value": true,
"binding": {
"type": "zeebe:property",
"name": "StaticBooleanProperty"
}
},
{
"label": "DefaultOptionalBooleanProperty",
"type": "Boolean",
"feel": "optional",
"value": true,
"binding": {
"type": "zeebe:property",
"name": "OptionalBooleanProperty"
}
}
]
},
{
"$schema": "https://unpkg.com/@camunda/zeebe-element-templates-json-schema/resources/schema.json",
"name": "FEEL optional",
"id": "numberField.feel.optional",
"appliesTo": [
"bpmn:ServiceTask"
],
"properties": [
{
"label": "DefaultStaticNumberProperty",
"type": "Number",
"feel": "static",
"value": 123,
"binding": {
"type": "zeebe:property",
"name": "StaticNumberProperty"
}
},
{
"label": "DefaultOptionalNumberProperty",
"type": "Number",
"feel": "optional",
"value": 123,
"binding": {
"type": "zeebe:property",
"name": "OptionalNumberProperty"
}
}
]
}
]