[DROOLS-4698] Managing user defined expressions for Collections #1276
Conversation
| */ | ||
| void initMapStructure(String key, Map<String, String> keyPropertyMap, Map<String, String> valuePropertyMap); | ||
| void initMapStructure(String key, Map<String, String> keyPropertyMap, Map<String, String> valuePropertyMap, boolean isRule); |
There was a problem hiding this comment.
@yesamer
Please replace use of boolean with the ENUM
| @@ -101,10 +98,14 @@ public void setValue(String jsonString) { | |||
| return; | |||
| } | |||
| JSONValue jsonValue = getJSONValue(jsonString); | |||
| if (collectionView.isListWidget()) { | |||
| populateList(jsonValue); | |||
| if (jsonValue instanceof JSONString) { | |||
There was a problem hiding this comment.
@yesamer
I think those nested ifs will lead to multiplication of tests - maybe cleaner to add another method to manage them?
There was a problem hiding this comment.
@gitgabrio I wrapped the logic in a new method, it should be cleaner now.
| @@ -160,10 +161,14 @@ public void addMapItem(Map<String, String> keyPropertiesValues, Map<String, Stri | |||
| public void save() { | |||
| try { | |||
| String updatedValue; | |||
| if (collectionView.isListWidget()) { | |||
| updatedValue = getListValue(); | |||
| if (collectionView.isExpressionWidget()) { | |||
There was a problem hiding this comment.
@yesamer
Same comment as above. It seems there are two paths to manage: the "expressionwidget"(=JSONString) and the others - maybe cleaner to add specific methods
| @DataField("createCollectionRadio") | ||
| protected InputElement createCollectionRadio = Document.get().createRadioInputElement("collectionRadio"); | ||
|
|
||
| @DataField("defineCollectionRadio") |
There was a problem hiding this comment.
@yesamer
What is the difference between those two radio buttons?
There was a problem hiding this comment.
@gitgabrio The first one is to enables the Create Collection guided editor, which is the "old" way to populate the Collection. The second one enables the Define Collection editor, which consists on a TextArea where you can write an expression.
| /** | ||
| * Flag to indicate if this <code>CollectionEditorViewImpl</code> is opened in DMN or RULE scenario | ||
| */ | ||
| protected boolean ruleScenario; |
There was a problem hiding this comment.
@yesamer
Pls replace boolean with ENUM
| presenter.initMapStructure(key, keyPropertyMap, valuePropertyMap, this); | ||
| } | ||
|
|
||
| protected void commonInit(boolean isRule) { |
There was a problem hiding this comment.
@yesamer
As above- pls replace boolean with ENUM
| @@ -174,6 +251,11 @@ public void setValue(String jsonString) { | |||
| presenter.setValue(jsonString); | |||
| } | |||
|
|
|||
| @Override | |||
| public boolean isExpressionWidget() { | |||
There was a problem hiding this comment.
@yesamer
Since three possible paths (list/map/expression) I think it would be cleaner to define (another :) ) enum and use it
There was a problem hiding this comment.
@gitgabrio In this case I'm not agree, the expression property is no related to the Collection type.
Here, I need to know if the user enable the "expression" collection editor, disregarding the Collection type.
There was a problem hiding this comment.
@gitgabrio I create https://issues.redhat.com/browse/DROOLS-4941 to manage this refactor during next sprint
| @@ -318,4 +431,34 @@ protected boolean isShown() { | |||
| protected void toggleRowExpansion(boolean toExpand) { | |||
| CollectionEditorUtils.toggleRowExpansion(faAngleRight, toExpand); | |||
| } | |||
|
|
|||
| protected void checkExpressionSyntax() { | |||
| if (ruleScenario) { | |||
There was a problem hiding this comment.
@yesamer
Pls replace boolean with enum
| JSONValue jsonValue = JSONParser.parseStrict(jsonString); | ||
| if (jsonValue instanceof JSONString) { |
There was a problem hiding this comment.
@yesamer
I think it would be clearer to and an "else" statement, or to create two different methods. Wdyt?
There was a problem hiding this comment.
@gitgabrio I added the else statement
| JSONValue jsonValue = JSONParser.parseStrict(jsonString); | ||
| if (jsonValue instanceof JSONString) { | ||
| String label = isList ? "List() " : "Map() "; | ||
| return label + ConstantHolder.EXPRESSION; |
There was a problem hiding this comment.
I think it is better to return something like List defined with expression (localized too). Wdyt?
There was a problem hiding this comment.
@danielezonca Agree to localize it. I think that List defined with expression it's too long, in a case where we have a lot of column isn't readable in my opinion
I would like to change it, be it should be a short string, in my opinion. Suggestions?
There was a problem hiding this comment.
@danielezonca As agreed I simply put List() or Map().
| @@ -1105,7 +1091,7 @@ public BaseSingletonDOMElementFactory getDOMElementFactory(String className, | |||
| ScenarioSimulationModel.Type modelType, | |||
| FactMappingValueType valueType) { | |||
| boolean isRuleScenario = Objects.equals(ScenarioSimulationModel.Type.RULE, modelType); | |||
| if (ScenarioSimulationSharedUtils.isCollection(className) && Objects.equals(FactMappingValueType.NOT_EXPRESSION, valueType)) { | |||
| if (ScenarioSimulationSharedUtils.isCollection(className)) { | |||
There was a problem hiding this comment.
Are you sure it is not possible to reach this code with valueType == FactMappingValueType.EXPRESSION?
If it is possible you should add back the additional condition because mapped the whole object (i.e. a top level list of string in DMN) to that column we should show the normal edit mode
There was a problem hiding this comment.
@danielezonca This change was required in a previous PR state, where I used FactMappingValueType.EXPRESSION for the Collection expression case too. As you suggested, now I don't use anymore that value for this case, so this change is no longer needed. Removed it.
| * @param expressionValue | ||
| * @return | ||
| */ | ||
| public static String checkExpressionSyntax(String expressionValue) { |
There was a problem hiding this comment.
I think method name checkExpressionSyntax is misleading, what about normalizeExpression or ensureExpressionSyntax?
I usually expect a method that is named check to just perform a validation and return a boolean
There was a problem hiding this comment.
@danielezonca Agree, I changed it to ensureExpressionSyntax. Thx
|
jenkins retest this please |
There was a problem hiding this comment.
Please document that format needed for unary test in expect columns (?=[]), Ideally in the cheatsheet in the panel shown below
Also please be aware of the issues I have reported
Last thing, Anna self-requested review of this PR, please do not merge without her review or discuss with her.
@jomarko Thank you for your review. I analyzed the tickets you created: the first one is solved (I opened a new PR) and the second one is not a bug, in my opinion (I wrote a comment in the ticket).
Excellent idea, I added:
I prefer to refer to |
|
@jomarko I agreed with @danielezonca to move the change on cheatsheet doc in #1277 PR. Thanks |
There was a problem hiding this comment.
Approving to make progress on this, however there is list of jiras to be solved to get feature working completely https://issues.redhat.com/browse/DROOLS-4698
Thank you @yesamer
|
jenkins retest this please |
|
SonarCloud Quality Gate failed.
|
This PR introduce a way to define a user defined expression for collections
@dupliaka @jomarko @danielezonca can you please review and test it?
Examples to test it (Maps) for Given columns (for RULE Scenario)
["Book1" : new com.Book("Bob"), "Book2" : new com.Book("Michael")]OR
a = "Bob"; map = new java.util.HashMap(); map.put("Bob", new com.Book(a)); map.put("Michael", new com.Book("Michael")); map;For Expected columns:
java.util.Objects.equals(actualValue.get("Book1").getName(), "Bob");https://issues.redhat.com/browse/DROOLS-4698
Related PR:https://github.com/kiegroup/drools/pull/2691