AF-2354: Create Initial integration MVP with Work Item Handlers #3034
Conversation
There was a problem hiding this comment.
LGTM as this is a first version. On future versions we should combine all the *.wid files found and improve the parser. Great job @jesuino!
| @@ -36,17 +39,40 @@ | |||
|
|
|||
| private final Promises promises; | |||
| private final WorkItemDefinitionCacheRegistry registry; | |||
| private final WorkItemDefinitionParser parser; | |||
| private ResourceContentService resourceContentService; | |||
|
Adding @inodeman on the review loop, as he'll be working on next kogito - stunner tasks, so better having him updated on all this stuff. thx! |
There was a problem hiding this comment.
Hey @jesuino
First of all, good job! Thanks! 👍
I added some review comments on the code side, I did not yet tested the runtime UI, but let's do it one step at time... :)
So in general all good, just added a few comments on the code I think we should revisit.
Also I think we should refactor the modules where new WID parser and service are located - Actually I think you put those in the kie-wb-common-bpmn-kogito-runtime module, right? In fact I would like to rename that modlue, probably it's not properly self-explanatory, but the goal for this module is only providing a webapp for the BPMN editor targeting kogito environments (VSCode, Che, GitHub...). So not sure if that makes sense, but I would prefer moving those classes to some other module, probably on the kie-wb-common-stunner-bpmn-client one? This way, on the KIE workbench environment, we could "override" this "default" service by the real one actualy used on our workbench (which calls the backend VFS...) . This way, by default, once using the BPMN editor, it will use your "new" WID approach (parser and service), but on the KIE workbench world, it'll still use the old VFS services...
I don't say that's something we HAVE TO do, probably makese sense even keeping that classes here, only targeting KOGITO, but I think it's probably something you could look at and we can discuss too in some call, if you need.
Thanks!
Roger
|
|
||
| private String retrieveParameters(Queue<String> objectQueue) { | ||
| String param = objectQueue.poll(); | ||
| StringBuffer sbParams = new StringBuffer(param); |
There was a problem hiding this comment.
Please do NOT use StringBuffer... at least here. There exist several downsides related to it if you don't use it properly, mostly because it's thread safe.
So, at least in client side code in general, better moving to StringBuilder or even better just using regular java operators on Strings. Even GWT transpilation and the JVM finally will optimizie your StringBuijlder to java operators, mostly, so it makes not much different on resutls.
But anyway, please avoid using StringBuffer instances...
| } | ||
|
|
||
| protected String cleanProp(String prop) { | ||
| return prop.trim().replaceAll("\"", "").replaceAll(",", ""); |
There was a problem hiding this comment.
can it be static and private?
| return prop.trim().replaceAll("\"", "").replaceAll(",", ""); | ||
| } | ||
|
|
||
| protected boolean isStartingObject(String line) { |
There was a problem hiding this comment.
can it be static and private?
| return line.trim().startsWith("["); | ||
| } | ||
|
|
||
| protected boolean isEndingObject(String line) { |
There was a problem hiding this comment.
can it be static and private?
| return widList; | ||
| } | ||
|
|
||
| private boolean empty(String line) { |
| @ApplicationScoped | ||
| public class WorkItemDefinitionParser { | ||
|
|
||
| public List<WorkItemDefinition> parse(String widStr) { |
There was a problem hiding this comment.
in general you're also calling String#trim() too many times along code below... probably better doing the trim once, and passing it accross helper methods to avoid unnecessary computation time
| return line == null || line.trim().equals(""); | ||
| } | ||
|
|
||
| private WorkItemDefinition parseWorkItemDefinitionObject(Queue<String> objectQueue) { |
There was a problem hiding this comment.
can it be static and private?
...rkbench/common/stunner/kogito/client/services/WorkItemDefinitionStandaloneClientService.java
Show resolved
Hide resolved
|
Hello @romartin , Thanks for your valuable feedback. Please see my response inline
I moved the parser and its tests to
AFAIK in BC distribution the WorkItemDefinitionClientService implementation is using the BC infrastructure already, hence this should not impact BC at all. I made the parser improvements that you suggested. Would you please take a look again? See my latest commit -> fb8ae2e Thanks! |
There was a problem hiding this comment.
Hey @jesuino
Great job here too, new commits looks great, although the parser not being properly implemented, it's fine to go ahead with this code, thanks for the changes 👍
I will approve guys, code looks good, but I didn't tested the runtime. The WAR for testing this is not generated on the Jenkins builds, so would appreciate if some of you can test the runtime - the whole picture (vscode + bpmn + wid files...) in addition to @jesuino , maybe @ederign or @inodeman you've time for it too?
| return widList; | ||
| } | ||
|
|
||
| private boolean empty(String line) { |
There was a problem hiding this comment.
Thank you, added this improvement and also removed an old test that was breaking the build
|
Thanks for your feedback, @romartin I built the packages (Chrome and VSCode) and made it available here: https://drive.google.com/file/d/1AHGxzViHx_OdDapdwy8arkKrLyCVzPWk/view?usp=sharing The VSCode extension I installed and tested, didn't try the Chrome extension from the ZIP above. Thanks! |
|
jenkins retest this |
|
|
||
| public List<WorkItemDefinition> parse(String widStr) { | ||
|
|
||
| if (widStr == null || "".equals(widStr.trim())) { |
There was a problem hiding this comment.
Please update the private static method empty and reuse it here.
| public void widParseTest() { | ||
| List<WorkItemDefinition> defs = parser.parse(WID); | ||
| assertEquals(7, defs.size()); | ||
| WorkItemDefinition wid1 = defs.get(0); | ||
| assertEquals("Email", wid1.getName()); | ||
| assertEquals("Email", wid1.getDisplayName()); | ||
| assertEquals("defaultemailicon.gif", wid1.getIconDefinition().getUri()); | ||
| assertTrue(wid1.getResults().isEmpty()); | ||
| assertEquals(EMAIL_WID_EXTRACTED_PARAMETERS, wid1.getParameters()); | ||
|
|
||
| WorkItemDefinition wid2 = defs.get(3); | ||
|
|
||
| assertEquals("Rest", wid2.getName()); | ||
| assertEquals("REST", wid2.getDisplayName()); | ||
| assertEquals("defaultservicenodeicon.png", wid2.getIconDefinition().getUri()); | ||
| assertTrue(wid1.getResults().isEmpty()); | ||
|
|
||
| assertEquals(REST_WID_EXTRACTED_PARAMETERS, wid2.getParameters()); | ||
| assertEquals("\"Result\" : new ObjectDataType(),", wid2.getResults()); | ||
|
|
||
| } |
There was a problem hiding this comment.
I believe we should assert all 7 definitions
| resourceContentService.list("**/*.wid").then(paths -> { | ||
| if (paths.length > 0) { | ||
| String path = paths[0]; // getting first WID found | ||
| DomGlobal.console.log("Loading WIDs from " + path); | ||
| resourceContentService.get(path).then(value -> { | ||
| Collection<WorkItemDefinition> wids = parser.parse(value); | ||
| wids.forEach(registry::register); | ||
| success.onInvoke(wids); | ||
| return null; | ||
| }); | ||
| } else { | ||
| DomGlobal.console.log("No WIDs to load"); | ||
| success.onInvoke(Collections.emptyList()); | ||
| } | ||
| return null; | ||
| }); | ||
| }); |
|
Jenkins please retest this |
There was a problem hiding this comment.
Hey @jesuino
I think the builds are failing because you're using different branch names for the related PRs... So see:
- AF-2354: Create Initial integration MVP with Work Item Handlers #3034 ->
AF-2353 - AF-2350: Resource Content API Implementation apache/incubator-kie-tools#40 ->
AF-2350 - AF-2353: Adding kogito-appformer-bridge appformer#854 ->
AF-2353
So Jenkins is not able to checkout and build the whole assembly. Please fix the branch names ASAP, the must be using the same one, so we can go ahead with the build and test it.
Thanks
|
@romartin The tests were failing for this project because we added tests that broke the parser, but after a discussion we decided that we will keep the parser as it is and discuss how it should evolve in a new JIRA. The kogito-tooling PR and this PR are independents. This PR depends on appformer and droolsjbpm-build-bootstrap PRs hence why it is on AF-2353 branch. @jomarko thanks again for your review. I removed the number of WIDs in test and found that category was not being filled, so I fixed this issue and kept only three WIDs in the test, covering all properties. Let me know if you have any question. Thanks! |
|
Jenkins execute full downstream build |
1 similar comment
|
Jenkins execute full downstream build |
|
jenkins please retest this |
|
jenkins execute full downstream build |
|
Jenkins execute full downstream build |
|
jenkins retest this |
|
It was failing due the lack of rebase, which is now done |
|
jenkins retest this |
|
Jenkins execute full downstream build |
|
ahh ok @jesuino , thanks for the answers, it makes sense, sorry I got confused. I'm jsut checking out this work for testing it, I'll let you know something in a while 👍 Thx1 |
|
Hey @jesuino I jsut found some time to test this stuff, and it still doesn't work for me, either I'm missing something or is there some other issue... lemme explain what I did and plz lemme know: I downloaded your custom vscode extension and installed it. I also created a process which contains a service task (lLog), and added both The process editor is being opened, but the
Am I missing something? Any ideas? Here are my source files: bpmn2 & wid Thanks! |
|
Hello Roger,
Thanks for the feedback.
Sometimes it happens to me for any component and it seems to be a GWT issue
with some sort of cache. For me it shows when I resize the properties pane.
Please let me know if it works for you.
Thanks!
…On Tue, Dec 10, 2019 at 11:17 PM Roger Martínez ***@***.***> wrote:
Hey @jesuino <https://github.com/jesuino>
I jsut found some time to test this stuff, and it still doesn't work for
me, either I'm missing something or is there some other issue... lemme
explain what I did and plz lemme know:
I downloaded your custom vscode extension
<https://drive.google.com/file/d/1AHGxzViHx_OdDapdwy8arkKrLyCVzPWk/view?usp=sharing>
and installed it. I also created a process which contains a service task
(lLog), and added both bpmn2 and wid files in a folder, which I added
then in the VSCode workspace.
The process editor is being opened, but the Task is not a Log (service
task) type, as it is in the source BPMN file. I understand there is not
icon for service tasks yet, but it you notice, also when selecting the
task, the properties are not the right ones for the Log one. No errors
found on the console... see:
[image: Screenshot from 2019-12-11 03-10-47]
<https://user-images.githubusercontent.com/4602417/70585475-a68cb280-1bc4-11ea-9e0c-e6ac55b72990.png>
PS: Here are my source files -
Am I missing something? Any ideas?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3034?email_source=notifications&email_token=AACX3DF4RFQWGPKZ4BUURULQYBES3A5CNFSM4JQHU4DKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEGRT4DI#issuecomment-564346381>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AACX3DEZLT3ZZR63RKNOAMTQYBES3ANCNFSM4JQHU4DA>
.
--
William Siqueira
Software Engineer
Red Hat
<https://www.redhat.com>
M: +55-11999232248
<https://red.ht/sig>
|
|
Hey @jesuino I don't think this is realted to resizing the properties panel... the issue is that the task imported is a regular task, not a Log task type... I'm closing the properties panel and opening it again and the task remains the same What am I missing? |
|
Hey @jesuino What I suggest is just to use my BTW - There will be still missing to use the right service task icons. This PR includes the loading for the Thx in advance! |
|
Hi @romartin , regarding your comment "BTW - There will be still missing to use the right service task icons. This PR includes the loading for the WID declarations, but there is still missing to use the new Resource API to fetch the right icon for each service task, and display them properly in the process editor. Is this something you @jesuino will also help on, in some other ticket//PR too? Otherwise, plz lemme know so we can re-assign it. CC @ederign @porcelli @krisv" As we already discussed our implementation is just a draft parser in order to validate the Resource Content API. IMO, any further work or functionality should be prioritised and done by someone on stunner team (@jesuino needs to work on other priorities). (The icon part for instance, probably will be a time consuming task) My suggestion is if you are ok with the rest of the code (excluding the parsers), approve the resource content API code, create JIRAs/epic for a 'production ready' version of WID parser and assign to someone of stunner team. Makes sense? If it's possible we really would like to merge this tomorrow! Thanks again! |
|
Hello @romartin Thanks for the clarification. I thought you were facing a caching issue, but then you clarified that the problem was that the task was not recognized. I made a few improvements in the parser:
This, however, does not fix the issue you are seeing. But I think the problem is not related to the parsing because the generated BPMN with a custom task dragged from the services tasks pallet looks like the same one generated in Business Central. Therefore the issue could be related to Stunner Kogito version because you should be able to reproduce same behavior even if you return a hardcoded list of WIDs in WorkItemDefinitionStandaloneClientService. Please let me know if you have any question. Thanks! |
|
Jenkins execute full downstream build |
|
ok @ederign @jesuino thanks for the updates! Approving it, although not really sure if we want to merge this yet. I mean, it partially works, it needs a few more love (probably re-implementing the parser and adding support for icons, plus bug fixing), so it's not really yet "ready" IMO for the next kogito release. The plan would be next week @inodeman continuing this job, so we have both options: even merging now or keeping this PR open, and then @inodeman can work on top of it and complete it properly during next week. What do you prefer? CC @ederign @porcelli @krisv @lazarotti Thanks! |
|
Let you know guys that me and @inodeman are working on KOGITO-277, by starting on top of this PR stuff. So it you agree, not still merging because it's still not clear if we'll deliver a new PR or just another one on top yet, does that works / makes sense for you guys? We'll let you know something ASAP Thx in advance! |
|
Sure thing @romartin feel free to close this or create over the top! Whatever is best for you guys! |
Merge after kiegroup/appformer#854 is merged
Related PRs:
apache/incubator-kie-tools#40
kiegroup/appformer#854