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

DROOLS-3719: [DMN Designer] Included Models - Add the 'Included Model dialog' to the 'Included Models tab' #2562

Merged
merged 3 commits into from Apr 4, 2019

Conversation

Projects
None yet
4 participants

@karreiro karreiro requested review from danielzhe and jomarko Mar 29, 2019

@karreiro karreiro force-pushed the karreiro:DROOLS-3719 branch from 34c837b to 86151e0 Mar 29, 2019

@jomarko
Copy link
Contributor

left a comment

Manual review notes

Some minor comments inline. I continue with manual check.

@jomarko

This comment has been minimized.

Copy link
Contributor

commented Apr 1, 2019

Manual review notes

I spotted two issues

  1. Saving a file Create two empty dmn models (a.dmn, b.dmn). Then in one of them include the other one and click save button of the dmn editor. The error [1] will appear in the server log.
  2. Renaming included model Open a model with existing included models. Rename some of them and try to save. No unsaved changes dialog will appear.

[1]

13:32:05,601 ERROR [org.kie.dmn.core.compiler.DMNCompilerImpl] (EJB default - 9) Required import not found: Could not locate required dependency while importing DMN for namespace: https://github.com/kiegroup/drools/kie-dmn/_0C5353A4-0B49-4D1B-A22B-24C60E46F1A7, name: first, modelName: null. for node 'org.kie.dmn.model.v1_2.TImport@268ce829' 

@manstis manstis self-requested a review Apr 1, 2019

@karreiro

This comment has been minimized.

Copy link
Contributor Author

commented Apr 2, 2019

Thank you for the review, @jomarko.

Regarding the raised issues:

Saving a file Create two empty dmn models (a.dmn, b.dmn). Then in one of them include the other one and click save button of the dmn editor. The error [1] will appear in the server log.

I've quickly debugged it, and it's not that simple. When a DMN file is saved, the project is automatically built (by using the incremental approach). I've noticed that in this build, external resources are not being loaded. This bug wasn't introduced by this PR, so I've opened a JIRA for this https://issues.jboss.org/browse/DROOLS-3841.

Renaming included model Open a model with existing included models. Rename some of them and try to save. No unsaved changes dialog will appear.

Fixed in the new commit:
2019-04-02 13 56 11

@jomarko

jomarko approved these changes Apr 3, 2019

Copy link
Contributor

left a comment

The issue I reported is fixed now, thank you @karreiro .

@manstis
Copy link
Member

left a comment

LGTM on the whole :-)

Show resolved Hide resolved ...st/java/org/kie/workbench/common/dmn/api/definition/v1_1/ImportTest.java Outdated
Show resolved Hide resolved ...e/workbench/common/dmn/backend/editors/types/DMNIncludeModelFactory.java
Show resolved Hide resolved ...kbench/common/dmn/backend/editors/types/DMNIncludeModelsServiceImpl.java
Show resolved Hide resolved ...kbench/common/dmn/backend/editors/types/DMNIncludeModelsServiceImpl.java

return Optional.of(new DMNIncludeModel(fileName, modelPackage, pathURI, namespace));
} catch (final Exception e) {
return Optional.empty();

This comment has been minimized.

Copy link
@manstis

manstis Apr 3, 2019

Member

What scenarios lead to this being thrown!!?

I'm guessing you hit a problem when testing and this was the safest solution?

This comment has been minimized.

Copy link
@karreiro

karreiro Apr 3, 2019

Author Contributor

The operation of getting paths and opening files is not atomic. So this catch prevents errors when some is invalid. However, I totally agree that factory class returning an optional object is odd, I've refactored it a little bit to fix it ;-)

Show resolved Hide resolved ...kbench/common/dmn/backend/editors/types/DMNIncludeModelsServiceImpl.java Outdated
Show resolved Hide resolved ...mon/dmn/backend/editors/types/query/DMNValueRepositoryRootIndexTerm.java
Show resolved Hide resolved ...e/workbench/common/widgets/client/assets/dropdown/KieAssetsDropdown.java
Show resolved Hide resolved ...rkbench/common/widgets/client/assets/dropdown/KieAssetsDropdownView.java Outdated
@karreiro

This comment has been minimized.

Copy link
Contributor Author

commented Apr 3, 2019

Thank you for the review @manstis. Changes applied :-)

@manstis

manstis approved these changes Apr 3, 2019

Copy link
Member

left a comment

Thanks @karreiro :-)

@karreiro karreiro merged commit 34fea1f into kiegroup:master Apr 4, 2019

1 check passed

Linux Build finished. 12845 tests run, 30 skipped, 0 failed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.