JNG-5833 ui operations #286
Conversation
noherczeg
requested review from
robertcsakany,
bencegelei,
gaborflorian and
Madmat8
|
Warning Rate limit exceeded@noherczeg has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 34 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThe pull request introduces comprehensive enhancements to the Judo Tatami JSL-to-UI transformation framework. The changes primarily focus on expanding the capabilities of action declarations, introducing new methods for handling transfer actions, view actions, and UI elements. Multiple Epsilon transformation and operation files have been modified to support more dynamic and flexible action handling across different UI components, including views, tables, groups, and tabs. A new test class has also been added to validate these new operations and transformations. Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (13)
judo-tatami-jsl-jsl2ui/src/main/epsilon/transformations/ui/modules/structure/transferActionDeclaration.etl (2)
1-30: Consider handling an additional else scenario foroperationType.
Ifself.eContainer.mapis not defined,operationTyperemains unset. You may want to set a default type or throw an error to avoid unexpected null references.
67-84: Potential spelling issue in property name.
The property is spelledparamaterName, which might be a typo. Consider renaming the property toparameterNameif it aligns with your domain.- t.name = s.paramaterName; + t.name = s.parameterName;judo-tatami-jsl-jsl2ui/src/main/epsilon/transformations/ui/modules/view/viewActionDeclaration.etl (2)
1-41: Avoid overwriting thedataElementassignment.
You assignt.dataElement = s.equivalent("OperationType")at line 20, then reassign it tos.transferAction.target.equivalent("OperationType")at line 36. Unless both references are guaranteed identical, consider removing or clarifying the first assignment to avoid confusion.- t.dataElement = s.equivalent("OperationType"); ... ... t.dataElement = s.transferAction.target.equivalent("OperationType");
66-75: Review commented-out assignment oftargetType.
The commented-out logic at line 72 may be needed for some scenarios. Verify whether this is intentionally left out or should be restored.judo-tatami-jsl-jsl2ui/src/main/epsilon/operations/jsl/ui/viewTabsDeclaration.eol (1)
49-53: Consider documenting the reason for commented-out code.The commented-out code for handling
UIViewLinkDeclarationsuggests potential future functionality. If this is intended for future implementation, consider adding a TODO comment explaining why it's commented out and when it should be uncommented.judo-tatami-jsl-jsl2ui/src/main/epsilon/operations/jsl/ui/viewGroupDeclaration.eol (1)
57-80: Consider documenting the reason for commented-out code.Similar to viewTabsDeclaration.eol, there's commented-out code for UIViewLinkDeclaration. Consider adding a TODO comment if this is intended for future implementation.
judo-tatami-jsl-jsl2ui/src/test/java/hu/blackbelt/judo/tatami/jsl/jsl2ui/application/JslModel2UiOperationsTest.java (7)
266-376: Consider extracting helper methods for better readability.While the test is thorough, it could benefit from helper methods to encapsulate common assertion patterns and improve readability.
Consider extracting helper methods for common patterns:
+ private void assertOperationParameters(OperationType operation, String expectedInputFQName, String expectedOutputFQName) { + OperationParameterType input = operation.getInput(); + OperationParameterType output = operation.getOutput(); + + assertEquals(expectedInputFQName, input.getFQName()); + assertEquals(expectedOutputFQName, output.getFQName()); + } + + private void assertTableButtons(Table table, List<String> expectedTableButtons, List<String> expectedRowButtons) { + List<Button> tableButtons = table.getTableActionButtonGroup().getButtons(); + List<Button> rowButtons = table.getRowActionButtonGroup().getButtons(); + + assertEquals(expectedTableButtons, tableButtons.stream().map(NamedElement::getFQName).sorted().toList()); + assertEquals(expectedRowButtons, rowButtons.stream().map(NamedElement::getFQName).sorted().toList()); + }
378-430: Consider extracting helper methods for better readability.Similar to the previous test, this method could benefit from helper methods to encapsulate common assertion patterns.
Consider extracting helper methods for common patterns:
+ private void assertPageActions(PageDefinition page, List<String> expectedActionFQNames) { + List<Action> actions = page.getActions(); + assertEquals(expectedActionFQNames, actions.stream().map(NamedElement::getFQName).sorted().toList()); + } + + private void assertCallOperationAction(Action action, OperationType expectedOperation, PageDefinition expectedPage) { + assertEquals(expectedOperation, action.getTargetDataElement()); + assertEquals(expectedPage, action.getTargetPageDefinition()); + }
129-230: Consider improving test organization and readability.The test is comprehensive but could benefit from:
- Moving test data setup to a separate method.
- Grouping assertions into logical sections with descriptive comments.
Example refactor:
+ private void assertDataElements(Application application) { + assertEquals(List.of( + "A::OperationsOnViews::A", + "A::OperationsOnViews::Error1", + "A::OperationsOnViews::ErrorWithDefaults", + "A::OperationsOnViews::Transfer1", + "A::OperationsOnViews::Transfer2", + "A::OperationsOnViews::TransferX" + ), application.getDataElements().stream().map(NamedElement::getFQName).sorted().toList()); + } + @Test void testOperationsOnViews() throws Exception { - jslModel = JslParser.getModelFromStrings("OperationsOnViews", List.of(createModelString("OperationsOnViews"))); + // Setup + setupTestModel("OperationsOnViews"); - transform(); + // When + Application application = transformAndGetApplication(); - List<Application> apps = uiModelWrapper.getStreamOfUiApplication().toList(); - assertEquals(1, apps.size()); - Application application = apps.get(0); - - assertEquals(List.of( - "A::OperationsOnViews::A", - // ... - ), application.getDataElements().stream().map(NamedElement::getFQName).sorted().toList()); + // Then + assertDataElements(application); // ... rest of the assertions }
232-264: Add descriptive messages to assertions.The test would be more maintainable with descriptive messages in assertions to help identify failures.
Example refactor:
- assertNull(myAction1.getInput()); - assertNull(myAction1.getOutput()); + assertNull(myAction1.getInput(), "Parameterless operation should not have input"); + assertNull(myAction1.getOutput(), "Void operation should not have output");
266-376: Extract helper methods for common operations.The test has repeated code for finding elements by FQName. Consider extracting these into helper methods.
Example refactor:
+ private <T extends NamedElement> T findByFQName(List<T> elements, String fqName) { + return elements.stream() + .filter(e -> e.getFQName().equals(fqName)) + .findFirst() + .orElseThrow(() -> new AssertionError("Element not found: " + fqName)); + } + @Test void testOperationsOnViewsWithInputSelectors() throws Exception { // ... setup code ... - ClassType transferX = classTypes.stream() - .filter(c -> c.getFQName().equals("A::OperationsOnViewsWithInputSelectors::TransferX")) - .findFirst().orElseThrow(); + ClassType transferX = findByFQName(classTypes, "A::OperationsOnViewsWithInputSelectors::TransferX");
378-430: Consider using test data builders.The test could benefit from using test data builders to make the test setup more maintainable and readable.
Example refactor:
+ private static class TestModelBuilder { + private final StringBuilder model = new StringBuilder(); + + public TestModelBuilder withEntity(String name) { + model.append("entity ").append(name).append(" {\n"); + return this; + } + + public String build() { + return model.toString(); + } + } + @Test void testOperationsOnViewsWithInputForms() throws Exception { - jslModel = JslParser.getModelFromStrings("OperationsOnViewsWithInputForms", - List.of(createModelString("OperationsOnViewsWithInputForms"))); + jslModel = JslParser.getModelFromStrings("OperationsOnViewsWithInputForms", + List.of(new TestModelBuilder() + .withEntity("Entity1") + // ... configure test data ... + .build()));
129-230: Consider adding test cases for error scenarios.The test thoroughly validates the happy path but could benefit from additional test cases:
- Invalid model configurations
- Missing required elements
- Edge cases in operation definitions
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
judo-tatami-jsl-jsl2ui/src/main/epsilon/operations/jsl/data/_importData.eol(1 hunks)judo-tatami-jsl-jsl2ui/src/main/epsilon/operations/jsl/data/transferActionDeclaration.eol(1 hunks)judo-tatami-jsl-jsl2ui/src/main/epsilon/operations/jsl/data/transferDeclaration.eol(4 hunks)judo-tatami-jsl-jsl2ui/src/main/epsilon/operations/jsl/ui/rowDeclaration.eol(1 hunks)judo-tatami-jsl-jsl2ui/src/main/epsilon/operations/jsl/ui/viewActionDeclaration.eol(1 hunks)judo-tatami-jsl-jsl2ui/src/main/epsilon/operations/jsl/ui/viewDeclaration.eol(3 hunks)judo-tatami-jsl-jsl2ui/src/main/epsilon/operations/jsl/ui/viewGroupDeclaration.eol(1 hunks)judo-tatami-jsl-jsl2ui/src/main/epsilon/operations/jsl/ui/viewTableDeclaration.eol(2 hunks)judo-tatami-jsl-jsl2ui/src/main/epsilon/operations/jsl/ui/viewTabsDeclaration.eol(1 hunks)judo-tatami-jsl-jsl2ui/src/main/epsilon/transformations/ui/jslToUi.etl(2 hunks)judo-tatami-jsl-jsl2ui/src/main/epsilon/transformations/ui/modules/structure/transferActionDeclaration.etl(1 hunks)judo-tatami-jsl-jsl2ui/src/main/epsilon/transformations/ui/modules/view/menuTableDeclarationViewPage.etl(1 hunks)judo-tatami-jsl-jsl2ui/src/main/epsilon/transformations/ui/modules/view/viewActionDeclaration.etl(1 hunks)judo-tatami-jsl-jsl2ui/src/test/java/hu/blackbelt/judo/tatami/jsl/jsl2ui/application/JslModel2UiOperationsTest.java(1 hunks)
👮 Files not reviewed due to content moderation or server errors (3)
- judo-tatami-jsl-jsl2ui/src/main/epsilon/operations/jsl/data/transferDeclaration.eol
- judo-tatami-jsl-jsl2ui/src/main/epsilon/transformations/ui/modules/view/menuTableDeclarationViewPage.etl
- judo-tatami-jsl-jsl2ui/src/test/java/hu/blackbelt/judo/tatami/jsl/jsl2ui/application/JslModel2UiOperationsTest.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: ⏳ Build, test and deploy artifacts
🔇 Additional comments (33)
judo-tatami-jsl-jsl2ui/src/main/epsilon/transformations/ui/modules/structure/transferActionDeclaration.etl (3)
32-41: No issues found.
This rule correctly extends the abstract rule and sets the ID and adds the operation to the corresponding class type.
43-65: No issues found.
The transformation logic for output parameters appears consistent with the domain (refresh, update, delete behaviors).
86-98: No issues found.
The transformation for error declarations into fault parameters looks correct.judo-tatami-jsl-jsl2ui/src/main/epsilon/operations/jsl/data/_importData.eol (1)
7-7: Import statement looks valid.
No circular dependencies are apparent. The new import should allow referencing additional operations fromtransferActionDeclaration.eol.judo-tatami-jsl-jsl2ui/src/main/epsilon/operations/jsl/ui/rowDeclaration.eol (2)
21-24: LGTM! Good use of recursion for collecting visual elements.The implementation correctly collects action declarations and their nested visual elements.
29-34: LGTM! Good use of caching for performance optimization.The implementation efficiently collects action declarations using a cached operation.
judo-tatami-jsl-jsl2ui/src/main/epsilon/operations/jsl/ui/viewActionDeclaration.eol (3)
2-9: LGTM! Consistent naming convention.The renaming from ViewActionDeclaration to UIActionDeclaration aligns with the framework's naming patterns.
11-35: LGTM! Clear and specific action type checks.The implementation provides well-defined checks for different action types with appropriate caching.
37-56: LGTM! Comprehensive visual elements collection.The implementation handles all cases (form, selector table, return) correctly and uses recursion appropriately.
judo-tatami-jsl-jsl2ui/src/main/epsilon/operations/jsl/data/transferActionDeclaration.eol (1)
1-54: LGTM! Comprehensive transfer action operations.The implementation provides well-structured operations with appropriate caching and edge case handling.
judo-tatami-jsl-jsl2ui/src/main/epsilon/operations/jsl/ui/viewTableDeclaration.eol (2)
40-43: LGTM! Consistent with rowDeclaration implementation.The implementation follows the same pattern as in rowDeclaration.eol for collecting action declarations.
63-70: LGTM! Consistent with rowDeclaration implementation.The implementation follows the same pattern as in rowDeclaration.eol for collecting action declarations.
judo-tatami-jsl-jsl2ui/src/main/epsilon/operations/jsl/ui/viewTabsDeclaration.eol (1)
46-69: LGTM! Well-structured implementation with performance optimization.The implementation follows the same pattern as
getExposedVisualElements()and uses@cachedappropriately for performance optimization. The method effectively aggregates action declarations from all relevant UI components.judo-tatami-jsl-jsl2ui/src/main/epsilon/operations/jsl/ui/viewGroupDeclaration.eol (1)
47-50: LGTM! Good addition to exposed visual elements.The addition of action declarations to
getExposedVisualElements()maintains consistency with the UI component hierarchy.judo-tatami-jsl-jsl2ui/src/main/epsilon/operations/jsl/ui/viewDeclaration.eol (2)
42-44: LGTM! Good addition to handle nested action declarations.The addition ensures that exposed visual elements from nested action declarations are properly included.
99-114: LGTM! Well-implemented with deterministic ordering.The implementation effectively collects action declarations and provides a deterministic order by sorting. The return type of List (vs Set in other similar methods) is appropriate here due to the sorting requirement.
judo-tatami-jsl-jsl2ui/src/main/epsilon/transformations/ui/jslToUi.etl (1)
7-7: LGTM! Appropriate imports for action handling.The addition of
transferActionDeclaration.etlandviewActionDeclaration.etlimports aligns with the enhanced action declaration handling implemented in other files.Also applies to: 27-27
judo-tatami-jsl-jsl2ui/src/main/epsilon/operations/jsl/data/transferDeclaration.eol (8)
79-82: LGTM!The function is well-optimized with
@cacheddecorator and correctly filters members by type.
214-217: LGTM!The function is well-optimized with
@cacheddecorator and correctly checks for sortable fields.
219-222: LGTM!The function is well-optimized with
@cacheddecorator and correctly checks for filterable fields.
15-15: LGTM! The changes correctly handle action-related transfer objects.The function now properly includes transfer objects from actions, ensuring that error types, parameter types, and return types are correctly exposed.
Also applies to: 27-35
79-82: LGTM! The function follows the established pattern.The implementation is clean, efficient (using
@cached), and follows the same pattern as other similar functions in the file.
15-15: LGTM! The changes correctly handle action-related transfer objects.The function now properly collects transfer objects from action errors, parameter types, and return types, following the same pattern used for relations.
Also applies to: 27-35
79-82: LGTM! The function follows the established pattern.The function is correctly cached and follows the same pattern as other member retrieval functions in the file.
214-222: LGTM! The functions correctly check for field properties.Both functions are properly cached and follow the same pattern for checking field properties.
judo-tatami-jsl-jsl2ui/src/main/epsilon/transformations/ui/modules/view/menuTableDeclarationViewPage.etl (3)
72-77: Address the TODO comment for tabular reference field actions.The code correctly processes action declarations, but there's a TODO comment indicating that additional logic is needed for tabular reference field actions.
Would you like me to help implement the if-else conditions for tabular reference field actions? This would ensure proper handling of actions based on their context.
72-77: Address the TODO comment for tabular reference field actions.The implementation correctly adds view actions, but there's a TODO comment indicating that additional logic is needed for tabular reference field actions.
Would you like me to help implement the if-else conditions for tabular reference field actions? This would ensure proper handling of actions based on their context.
72-77: Address the TODO comment for tabular reference field actions.The action handling logic is correct, but there's a TODO comment indicating that additional logic is needed for tabular reference field actions.
Do you want me to help implement the if-else conditions for tabular reference field actions?
judo-tatami-jsl-jsl2ui/src/test/java/hu/blackbelt/judo/tatami/jsl/jsl2ui/application/JslModel2UiOperationsTest.java (5)
129-230: LGTM!The test method provides comprehensive coverage of basic operations on views with well-structured assertions.
232-264: LGTM!The test method effectively validates parameterless void operations and their fault types.
232-264: LGTM! The test effectively validates parameterless void operations.The test provides good coverage of input, output, and fault parameters for parameterless void operations.
266-376: LGTM! The test provides comprehensive coverage of input selector operations.The test thoroughly validates all aspects of operations with input selectors, including UI elements, actions, and relationships.
378-430: LGTM! The test effectively validates input form operations.The test provides thorough coverage of operations with input forms, including form structure, actions, and relationships.
...tatami-jsl-jsl2ui/src/main/epsilon/transformations/ui/modules/view/viewActionDeclaration.etl
Outdated
Show resolved
Hide resolved
...tatami-jsl-jsl2ui/src/main/epsilon/transformations/ui/modules/view/viewActionDeclaration.etl
Outdated
Show resolved
Hide resolved
...tatami-jsl-jsl2ui/src/main/epsilon/transformations/ui/modules/view/viewActionDeclaration.etl
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
judo-tatami-jsl-jsl2ui/src/main/epsilon/transformations/ui/modules/view/viewActionDeclaration.etl (2)
146-160:⚠️ Potential issueRemove duplicate rule "OperationInputFormCallAction".
The rule appears twice in the file with similar logic. This duplication can lead to unexpected behavior during transformation.
Keep only one instance of the rule and update all references to use the single implementation. Consider keeping the second instance (lines 309-323) as it has additional logic for handling target data elements.
-@lazy -rule OperationInputFormCallAction - transform s: JSL!UIActionDeclaration - to t: UI!ui::Action { - t.setId(rootMenu.name + "/(jsl/" + s.getId() + ")/OperationInputFormCallAction"); - t.name = s.name + "::CallOperation"; - t.ownerDataElement = s.transferAction.target.equivalent("OperationType"); - t.actionDefinition = s.equivalent("OperationInputFormCallActionDefinition"); - if (s.hasOutput()) { - t.targetPageDefinition = s.equivalent("OperationOutputPageDefinition"); - } - - log.debug("OperationInputFormCallAction: " + t.name); -}Also applies to: 309-323
359-361:⚠️ Potential issueFix undefined variable "op" in condition.
The variable
opis undefined and should beuiOpbased on the context.Apply this fix:
- if (op?.input?.target == ad?.getOperationReference()?.getMember()?.input?.target) { + if (uiOp?.input?.target == ad?.getOperationReference()?.getMember()?.input?.target) {
🧹 Nitpick comments (2)
judo-tatami-jsl-jsl2ui/src/main/epsilon/transformations/ui/modules/view/viewActionDeclaration.etl (2)
1-41: Consider adding input validation for button properties.The
ViewActionDeclarationButtonrule should validate the input parameters before transformation.Add validation for:
- Undefined or empty name
- Invalid icon format
- Missing or invalid action definition
rule ViewActionDeclarationButton transform s: JSL!UIActionDeclaration to t: UI!ui::Button { guard: rootMenu.containsVisualElement(s) + + if (s.name.isUndefined() or s.name.trim().isEmpty()) { + throw "Button name cannot be empty for " + s.getId(); + } t.setId(rootMenu.name + "/(jsl/" + s.getId() + ")/ViewActionDeclarationButton");
1-435: Consider organizing rules into logical groups.The file contains many related rules that could be organized better for maintainability.
Consider grouping rules into categories using comments:
- Button-related rules
- Action definition rules
- Page definition rules
- Operation-specific rules
Example:
+// ============================================ +// Button-related transformation rules +// ============================================ rule ViewActionDeclarationButton transform s: JSL!UIActionDeclaration to t: UI!ui::Button {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
judo-tatami-jsl-jsl2ui/src/main/epsilon/transformations/ui/modules/structure/transferActionDeclaration.etl(1 hunks)judo-tatami-jsl-jsl2ui/src/main/epsilon/transformations/ui/modules/view/viewActionDeclaration.etl(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- judo-tatami-jsl-jsl2ui/src/main/epsilon/transformations/ui/modules/structure/transferActionDeclaration.etl
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: ⏳ Build, test and deploy artifacts
...tatami-jsl-jsl2ui/src/main/epsilon/transformations/ui/modules/view/viewActionDeclaration.etl
Outdated
Show resolved
Hide resolved
...tatami-jsl-jsl2ui/src/main/epsilon/transformations/ui/modules/view/viewActionDeclaration.etl
Show resolved
Hide resolved
judo-tatami-jsl-jsl2ui/src/main/epsilon/operations/jsl/ui/viewGroupDeclaration.eol
Show resolved
Hide resolved
judo-tatami-jsl-jsl2ui/src/main/epsilon/operations/jsl/ui/viewTabsDeclaration.eol
Show resolved
Hide resolved
Will be extended later.