KOGITO-3661 Integrate boxed-expression-component inside dmn-loader and define jsInterop classes for loading/saving expressions #104
Conversation
…chanism to enable the new boxed expression editor as a toggle feature
…ops, when the editor is activated
…n a BPMN Diagram (kiegroup#87)
…l FEEL based cells (kiegroup#83)
# Conflicts: # kie-wb-common-dmn/kie-wb-common-dmn-client/src/main/java/org/kie/workbench/common/dmn/client/editors/expressions/ExpressionEditorViewImpl.html # kie-wb-common-dmn/kie-wb-common-dmn-client/src/main/java/org/kie/workbench/common/dmn/client/editors/expressions/ExpressionEditorViewImpl.java # kie-wb-common-dmn/kie-wb-common-dmn-client/src/test/java/org/kie/workbench/common/dmn/client/editors/expressions/ExpressionEditorViewImplTest.java
…ops, when the editor is activated
Co-authored-by: Kogito Tooling Bot <kietooling@gmail.com>
Ok @danielzhe thanks for your feedback! |
|
@jomarko, let me update you:
Covered Thanks again for your review. Would you mind letting me know if you have any further doubt? |
|
@vpellegrino would you mind to have a look on this bug reported by sonar? https://sonarcloud.io/project/issues?id=org.kie.kogito%3Akogito-editors-java&pullRequest=104&resolved=false&types=BUG |
re review1 - I am fine with separate issue |
There was a problem hiding this comment.
Some comments to code, no more defects found in manual review. Thank you @vpellegrino
| } | ||
|
|
||
| private static Expression buildAndFillNestedExpression(final ExpressionProps props) { | ||
| if (LITERAL_EXPRESSION.getText().equals(props.logicType)) { |
There was a problem hiding this comment.
Could we please update all these if statements to this form:
Objects.equals(LITERAL_EXPRESSION.getText(), props.logicType)
The benefit of the proposed is no NullPointerException is thrown in case (theoretical) getText or logicType is null.
| final String cell = row.length <= columnIndex ? "" : row[columnIndex]; | ||
| final LiteralExpression wrappedExpression = new LiteralExpression(); | ||
| wrappedExpression.setText(new Text(cell)); | ||
| wrappedExpression.setTypeRef(BuiltInType.STRING.asQName()); |
There was a problem hiding this comment.
Are all columns of a relations strings? shouln;t we use a data type of relationProps.columns[columnIndex] rather than STRING?
There was a problem hiding this comment.
In this method, we are setting the typeRef of the wrapped LiteralExpression (used for the cell) to string. I'm going to remove it. Thanks for letting me know
| informationItem.setTypeRef(BuiltInTypeUtils | ||
| .findBuiltInTypeByName(column.dataType) | ||
| .orElse(BuiltInType.UNDEFINED) | ||
| .asQName()); |
There was a problem hiding this comment.
related to previous question, shouldn't we have similar type detection on the previous commented place?
There was a problem hiding this comment.
FYI: This method is about settings name and typeRef for columns. As per #104 (comment), I'm avoiding to set string as typeRef for wrapped expression
| private void assertFormalParameters(FunctionDefinition functionExpression) { | ||
| assertThat(functionExpression.getFormalParameter()).isNotNull(); | ||
| assertThat(functionExpression.getFormalParameter()).hasSize(1); | ||
| assertThat(functionExpression.getFormalParameter()).first().extracting(param -> param.getValue().getValue()).isEqualTo(PARAM_NAME); | ||
| assertThat(functionExpression.getFormalParameter()).first().extracting(param -> param.getTypeRef().getLocalPart()).isEqualTo(PARAM_DATA_TYPE); | ||
| } |
There was a problem hiding this comment.
Generally I think, we overuse assertThat what cause some assertions less readable, one example how they can be simplified:
private void assertFormalParameters(FunctionDefinition functionExpression) {
assertThat(functionExpression.getFormalParameter())
.isNotNull()
.hasSize(1)
.first()
.satisfies(param -> {
assertThat(param.getValue().getValue()).isEqualTo(PARAM_NAME);
assertThat(param.getTypeRef().getLocalPart()).isEqualTo(PARAM_DATA_TYPE);
});
}
@jomarko answered, and accomplished comments as well. |
|
Hi, @vpellegrino ! I've been working with this branch to do undo/redo. Very good job! To reproduce, just put a break point there and set the expression to "Function". You'll notice that it starts to loop forever. |
Hi @danielzhe yeah, I already noticed it. |
There was a problem hiding this comment.
Thank you for this PR, @vpellegrino. Great stuff!
I have only two questions:
1) Enable it on Kogito channels
I've noticed we're removing the display: none property from the ExpressionEditorViewImpl.html template. So, we're actually enabling this component in the editor.
However, only the kie-wb-common-dmn-webapp-kogito-testing web app requires our React assets. Thus, I believe if we build the .vsix extension or other Kogito channel, our component won't load (because Kogito channels rely on the kie-wb-common-dmn-webapp-kogito-runtime module).
Thus, I believe we have two alternatives:
- Add the static assets to the
the kie-wb-common-dmn-webapp-kogito-runtime - Bring the
"display: none"back to theExpressionEditorViewImpl.htmland enable the component in another PR
Wdyt about this?
2) Use of JSNI
We're relying on JSNI in the BoxedExpressionService.java, which is something we must really avoid; instead, we should rely on the JsInterop API like we're doing in the DMNLoader.java. Do you prefer to raise a Jira to handle this in a different PR or handle it here?
--
Thanks a lot for this PR, @vpellegrino! It's really exciting to see the new boxed expression editor alive 🚀
This reverts commit eb014e4
Thanks for your review! |
|
SonarCloud Quality Gate failed. |
Showcase
boxed-expression-component: https://boxed-expression-editor.netlify.app/Tasks solved by this PR: KOGITO-3666, KOGITO-3663, KOGITO-3661
Since the goal of integrating the
boxed-expression-componentinside theDMN Editoris ambitious, the process will be incremental, thanks to the usage of Stable/Beta flags.