sartography / bpmn-js-spiffworkflow

A BPMN.js extension to improve working with SpiffWorkflow - the python BPMN engine.
MIT License
19 stars 5 forks source link

Enhance UX to fix multi instance bug #75

Closed theaubmov closed 1 week ago

theaubmov commented 3 months ago

Summary by CodeRabbit

coderabbitai[bot] commented 3 months ago

Walkthrough

The recent update involves restructuring import paths and enhancing functionality within BPMN workflow components. Changes optimize the organization of key providers like StandardLoopPropertiesProvider and MultiInstancePropertiesProvider, introduce new modules for managing loop properties and synchronization, and refine features for input-output management in the properties panel.

Changes

File Path Change Summary
.../loops/StandardLoopPropertiesProvider.js - Updated import path for LoopProperty helpers
.../loops/helpers.js - Introduced functions for managing loop properties and IO values
.../loops/propertiesPanel/CompletionCondition... - Added CompletionCondition function for managing completion conditions
.../propertiesPanel/InputCollectionEntry.js - Exported InputCollection function with enhanced functionality
.../propertiesPanel/OutputCollectionEntry.js - Introduced OutputCollection function for managing output collections
.../propertiesPanel/LoopCardinalityEntry.js - Added LoopCardinality function for setting loop cardinality properties
.../propertiesPanel/OutputItemEntry.js - Provided functionality for handling output data items in BPMN workflow properties panel
.../propertiesPanel/IsIOSyncEntry.js - Introduced IsOutputElSync function for synchronization of output elements
.../propertiesPanel/CompletionConditionEntry.js - Added CompletionCondition function for handling completion conditions in BPMN elements
.../propertiesPanel/InputItemEntry.js - Implemented InputItem function for managing input data items in a BPMN workflow
.../propertiesPanel/MultiInstancePropertiesProvider.js - Enhanced MultiInstancePropertiesProvider for multi-instance elements in BPMN diagrams

🐰✨

In a land of code and wire,
Changes bloom like spring's first fire.
Syncing outputs, far and wide,
With checkboxes by our side.
Through loops and logs, we hop and twirl,
Crafting worlds in every swirl.
🌟🐾


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share - [X](https://twitter.com/intent/tweet?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A&url=https%3A//coderabbit.ai) - [Mastodon](https://mastodon.social/share?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A%20https%3A%2F%2Fcoderabbit.ai) - [Reddit](https://www.reddit.com/submit?title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&text=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code.%20Check%20it%20out%3A%20https%3A//coderabbit.ai) - [LinkedIn](https://www.linkedin.com/sharing/share-offsite/?url=https%3A%2F%2Fcoderabbit.ai&mini=true&title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&summary=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code)
Tips ### Chat There are 3 ways to chat with [CodeRabbit](https://coderabbit.ai): - Review comments: Directly reply to a review comment made by CodeRabbit. Example: - `I pushed a fix in commit .` - `Generate unit testing code for this file.` - `Open a follow-up GitHub issue for this discussion.` - Files and specific lines of code (under the "Files changed" tab): Tag `@coderabbitai` in a new review comment at the desired location with your query. Examples: - `@coderabbitai generate unit testing code for this file.` - `@coderabbitai modularize this function.` - PR comments: Tag `@coderabbitai` in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples: - `@coderabbitai generate interesting stats about this repository and render them as a table.` - `@coderabbitai show all the console.log statements in this repository.` - `@coderabbitai read src/utils.ts and generate unit testing code.` - `@coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.` Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. ### CodeRabbit Commands (invoked as PR comments) - `@coderabbitai pause` to pause the reviews on a PR. - `@coderabbitai resume` to resume the paused reviews. - `@coderabbitai review` to trigger a review. This is useful when automatic reviews are disabled for the repository. - `@coderabbitai resolve` resolve all the CodeRabbit review comments. - `@coderabbitai help` to get help. Additionally, you can add `@coderabbitai ignore` anywhere in the PR description to prevent this PR from being reviewed. ### CodeRabbit Configration File (`.coderabbit.yaml`) - You can programmatically configure CodeRabbit by adding a `.coderabbit.yaml` file to the root of your repository. - Please see the [configuration documentation](https://docs.coderabbit.ai/guides/configure-coderabbit) for more information. - If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: `# yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json` ### Documentation and Community - Visit our [Documentation](https://coderabbit.ai/docs) for detailed information on how to use CodeRabbit. - Join our [Discord Community](https://discord.com/invite/GsXnASn26c) to get help, request features, and share feedback. - Follow us on [X/Twitter](https://twitter.com/coderabbitai) for updates and announcements.
burnettk commented 3 months ago

what happens when "Output Element is Synchronized with Input Element" is checked? do we definitely want to give users control over this? is this a BPMN spec thing?

theaubmov commented 3 months ago

When the Output Element and Input Element have the same value (variableName), BPMN-JS is unable to parse the Output Element value. The issue arises only if the user wants to use the same value for both the Input and Output Elements. That's why the new checkbox "Output Element is Synchronized with Input Element" has been added to allow users to achieve these use cases. This is not a BPMN spec, it's just a UI enhancement to address these issues.

burnettk commented 2 months ago

@essweine , did you discuss this one with ayoub? it is a valid use case to have a matching Output Element and Input Element in your opinion? you think we should get this change merged?

essweine commented 2 months ago

Yes, we discussed this. It is definitely a valid use case.. However, I ran into a problem doing some testing. I created a simple diagram (had to change the extension to upload) multiinstance.txt that uses two different collections, but when I reloaded it, the output item was lost and the "synchronize" box was checked..

Also, when I remove an output collection, the loopOutputDataRef gets set to undefined, which is going to cause runtime errors (this might be an existing bug, but we should go ahead and fix it).

The third problem I had was that there's an attribute isOutputSynched which doesn't have a namespace. I wonder if we should add a namespace and not try to add the outputDataItem if it is checked (this would require a slight change to the library parser but it would be fairly trivial)? It would also be better to put the attribute on multiInstanceLoopCharacteristics instead of the task.

theaubmov commented 2 months ago

Fixed the issue of getting an 'undefined' value when removing the output collection.

For isOutputSynched I added Spiffworkflow namespace,

When the user check the box of isOutputSync, the current behaviour is adding dataOutput item with same value of input item, so nothing in the backend will change. and for the point of adding isOutputSync attribute to multiInstance LoopCharacterisitcis and not element, it makes sense, but to do so we need to override the default bpmn moddle. so that's why it's still implemented at task level.

For the first issue of losing output element value with that example, I'm not able to regenerate it again by running only bpmn editor, @essweine could you please verify if the issue still exists after last changes.

To optimise the multi instance panel and make it more readable and easier to understand, Should we create a new ticket for it, where we define how the optimised version will look like?

essweine commented 2 weeks ago

For the first issue of losing output element value with that example, I'm not able to regenerate it again by running only bpmn editor, @essweine could you please verify if the issue still exists after last changes.

I was unable to replicate this time either, so I think this is good to go.

To optimise the multi instance panel and make it more readable and easier to understand, Should we create a new ticket for it, where we define how the optimised version will look like?

I'm in favor of a separate ticket for that so we can finally get this merged.