- Issue created by @jurgenhaas
- 🇧🇪Belgium lammensj
Looking at it further, it might even be that we don't need that cli-plugin but use the modeler-instance directly. See example: https://github.com/bpmn-io/bpmn-js-examples/blob/main/modeling-api/src/snippets/introduction.js
- 🇩🇪Germany jurgenhaas Gottmadingen
Oh, nice, that looks even more promos sing. The CLI part made me feel a bit uncomfortable, but talking to the modeller instance directly like this makes so much sense.
- 🇧🇪Belgium lammensj
I just got a working POC where I was able to "clone" an existing ECA-config by opening it in the BPMN.io-modeler and use underlying modeler to create the necessary elements and connect them to each other.
Now focussing on the auto-layout... Would be cool to show this on DrupalCamp Berlin :-D - 🇧🇪Belgium lammensj
Using bpmn-auto-layout is also working! Now for some more testing...
- 🇧🇪Belgium lammensj
Summary
- Added an additional route and controller which preprocesses the given the ECA-config entity and passes certain mappings via drupalSettings to Javascript
- Registered this new route as entity operation of ECA
- Created an additional Javascript class which processes the mapping by creating "Elements" (events, gateways, tasks and conditions) for the root (the Process) and then properly orders them in the BPMN.io-UIQA
- Use the fork of this module
- Download the package of this ECA-sample: https://ecaguide.org/library/simple/add_role_to_inserted_or_updated_user/
- Unzip it
- Alter the "eca.eca.eca_lib_0007.yml"-file (this the ECA-config file, not the model) by changing the modeller to "core"
- Install the dependencies of this sample (see "dependencies.yml") and the ECA Classic Modeller ( https://www.drupal.org/project/eca_cm → )
- Import the altered config file via the UI
- Go to the overview of ECA-models, you should notice that it was added
- On the right, select "Edit with BPMN.io"TODO
- e2e-test with Nightwatch
- 🇩🇪Germany jurgenhaas Gottmadingen
I've reviewed the code and this is impressive! Great work @lammensj, very exciting. I will give this a try in a minute but wanted to leave to small but general thoughts:
- Is "clone" the correct key for what's happening here?
- This keeps the ID of the original model, which is from a different modeller. That means it overrides a config entity of unknown source. For the context of AI-generated ECA models, that's exactly what we want, but maybe not for others?
That brought me to the following idea: we do have a fallback modeller plugin. How about a convention that AI tools should generate models with that fallback ID and those models will be converted and overridden. All others will really be cloned so that we keep the original.
Again, thank you so much. This won't take long to get merged. And the community will love it.
- 🇩🇪Germany jurgenhaas Gottmadingen
Wow, it works!!!
Just a small issue: it doesn't change the modeller ID when saving the updated ECA model, and it remains at the path
/admin/config/workflow/eca/eca_lib_0029/clone/bpmn-io
which will continue to use that new route which triggers an upstream conversion again.The expected behavior should be that it updates the modeller ID and redirects to
/admin/config/workflow/eca/eca_lib_0029/edit
after the initial save that's following the auto-layout.And an additional idea, but then for a separate issue: the auto-layout feature is probably worth exposing as a function in the canvas in general. What do you think?
- 🇧🇪Belgium lammensj
@jurgenhaas,
1. Do you prefer "convert"? Might capture the intention better... I guess it's a matter of semantics
2. I wasn't aware of the fallback-modeller, so yes, I will definitely make sure that the AI Agent is is generating one that uses that fallback. I wasn't planning on giving the LLM the option to choose.
3. Now that were working towards multiple modellers being able to alter the same ECA-config, the question rises if the admin UI should be altered/optimised? Because you would have multiple rows, concerning the same ECA-config, but different modellers which aren't kept in sync. - 🇩🇪Germany jurgenhaas Gottmadingen
1. Do you prefer "convert"? Might capture the intention better... I guess it's a matter of semantics
That sounds good, yes.
3. Now that were working towards multiple modellers being able to alter the same ECA-config, the question rises if the admin UI should be altered/optimised? Because you would have multiple rows, concerning the same ECA-config, but different modellers which aren't kept in sync.
We probably don't need that. I see 2 different use cases:
The first one is for models that are not created by bpmn_io and not by fallback, they should be converted and saved under a new ID. That way, the original modeller continues ownership and maintenance of its own model and bpmn_io gets a copy of it under it's own regime.
The second one is for models that have been created by fallback, i.e. not really a modeller that has an intent for continued maintenance. For those, we only need to convert, change the modeller to bpmn_io and that model is now owned by bpmn_io.
That solution assumes that if the AI agent creates a model it has no intend to ever update that model, does it?
- 🇧🇪Belgium lammensj
@jurgenhaas, I was able to adjust the Converter and clone the original ECA-entity when it doesn't use the fallback modeller. I'm using the "createDuplicate"-method of the ConfigEntityBase-class. However... The protected $id-property of the ECA-entity doesn't allow null-values, hence the failure in the pipeline. Can this be changed in the ECA-module or should I look for another way of cloning it?
Btw, this can be manually QA'd already, just follow the same steps as above. Just don't forget to temporarily allow null-values for that $id-property.
- 🇩🇪Germany jurgenhaas Gottmadingen
Nice work, again.
The good news is, the ModellerInterfaces comes with a clone method that is really helpful here. Each modeller will handle the cloning and return a prepared ECA entity. And the Fallback modeller returns the ECA entity as is without cloning. So, that gives you everything you need.
I've committed the necessary modifications to the MR. Please feel free to revert that commit, if you don't like it.
- 🇧🇪Belgium lammensj
@jurgenhaas, I've adjusted the existing kernel test and added a couple of nightwatch tests. You can see the results of the conversion and the auto-layout here: https://git.drupalcode.org/issue/bpmn_io-3484063/-/jobs/3400559/artifacts/browse/web/core/reports/nightwatch/screenshots/Tests/bpmn_io/.
Is there anything else? Do you require more manual testing?
- 🇩🇪Germany jurgenhaas Gottmadingen
Thank you @lammensj for all your hard work on this. It looks really great and I've also fixed the remaining PHPCS and PHPStan tests. I don't think we need any further tests at this point.
But I found one odd ECA model, that causes an issue: https://ecaguide.org/library/simple/combined_conditions/
The problem seems to be that there are event objects that have no template associated to them. Do you think that could be fixed?
-
jurgenhaas →
committed ed1341c5 on 2.1.x authored by
lammensj →
Issue #3484063 by lammensj, jurgenhaas: Upstream conversion of ECA...
-
jurgenhaas →
committed ed1341c5 on 2.1.x authored by
lammensj →
- 🇩🇪Germany jurgenhaas Gottmadingen
Turns out, the model above doesn't contain any more elements, so the conversion does exactly what's possible and we merge this now.
Any potential follow-up issues can then be addressed in separate issues.
-
jurgenhaas →
committed ed1341c5 on 2.0.x authored by
lammensj →
Issue #3484063 by lammensj, jurgenhaas: Upstream conversion of ECA...
-
jurgenhaas →
committed ed1341c5 on 2.0.x authored by
lammensj →
Automatically closed - issue fixed for 2 weeks with no activity.