[NAE-2454] View Configuration 2.0#460
Conversation
- add params to CreateCaseBody
- create folder configuration template
- add validation in MenuItemService.moveItem - update menu_item to work with menu
- rollback validation
- update logic in ActionDelegate.updateMultichoiceWithCurrentNode to search only for folders - introduce is_folder boolean in menu_item.xml - only folders can have children
- introduce header sort mode direction configuration
- add field for active column sorting
- add translations
- formatting
WalkthroughAdds folder-mode menu items, refactors view-type and view-option APIs, introduces header sort-mode fields for case/task views, and passes create-case params through the workflow controller. ChangesMenu Folder Support
View Type Refactor and View Options
Header Sort-Mode Configuration
CreateCase Params Passthrough
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
- fix translation
coderabbitai
Bot
added
improvement
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/java/com/netgrif/application/engine/menu/service/MenuItemService.java (1)
94-129: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueTODO acknowledges non-transactional multi-case creation.
createMenuItempersists the item case, parent-folder case, view case(s), and URI node across several independentsave/setDatacalls. A failure partway through (e.g. between line 114's save and line 126'ssetDataWithExecute) leaves orphaned/partial cases. The author's own TODO flags this. Want me to sketch a compensating-cleanup or saga-style rollback for this method?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/netgrif/application/engine/menu/service/MenuItemService.java` around lines 94 - 129, `createMenuItem` performs several dependent writes across `workflowService.save`, `appendChildCaseIdAndSave`, `uriService.getOrCreate`, `createView`, and `setDataWithExecute` without a transaction, so a mid-flow failure can leave partial/orphaned state. Make `MenuItemService.createMenuItem` transactional (or otherwise add rollback/compensating cleanup around the whole flow) so the parent case, menu item case, optional view case, and URI node are committed atomically. Use the existing `createMenuItem`, `createView`, and `setDataWithExecute` flow to keep all persisted changes consistent on failure.src/main/resources/petriNets/engine-processes/menu/menu_item.xml (1)
941-960: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUse
getOrCreatefor the destination nodeIf
move_dest_urican point to a path that doesn't exist yet,uriService.findByUri(uriPath)returnsnull, andfindOptionsBasedOnSelectedNode(node)immediately dereferences it vianode.uriPath.MenuItemService.moveItem(...)already usesuriService.getOrCreate(...)for the same destination lookup, so this assign block should match that behavior.🔧 Suggested fix
- node = uriService.findByUri(uriPath) + node = uriService.getOrCreate(uriPath, com.netgrif.application.engine.petrinet.domain.UriContentType.CASE)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/resources/petriNets/engine-processes/menu/menu_item.xml` around lines 941 - 960, The assign block for moveDestUri is still using uriService.findByUri(uriPath), which can return null and break findOptionsBasedOnSelectedNode(node) when the destination path does not exist. Update this logic to match MenuItemService.moveItem(...) by using uriService.getOrCreate(...) for the destination lookup, and then pass the resulting node into change moveDestUri options so the selected-node options are always built from a valid node.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/main/java/com/netgrif/application/engine/menu/domain/configurations/CaseViewBody.java`:
- Around line 28-29: The default value for headersSortModeDirection in
CaseViewBody is mismatched with the configured option key. Update the
initialization in CaseViewBody so the field starts with asc instead of
ascending, keeping it aligned with case_headers_sort_mode_direction and the XML
defaults; this will ensure new views persist a valid selectable value.
In
`@src/main/java/com/netgrif/application/engine/menu/domain/configurations/TaskViewBody.java`:
- Around line 22-23: The default sort direction is using the wrong enumeration
key, so update the default value in TaskViewBody for headersSortModeDirection
from the long-form label to the enum key expected by
task_headers_sort_mode_direction, and apply the same change in CaseViewBody.
Keep the existing field names, but ensure both classes default to the asc option
so the stored ENUMERATION_MAP value matches the available enum entries.
In
`@src/main/java/com/netgrif/application/engine/workflow/web/WorkflowController.java`:
- Line 87: The create-case flow in WorkflowController should guard against
explicit null in body.params before calling workflowService.createCase.
Normalize body.params to an empty map when it is null (or update CreateCaseBody
deserialization/default handling) so CreateCaseEventOutcome creation can safely
proceed. Use the CreateCaseBody and createCase call site as the fix point, and
ensure the service still receives a non-null params map for populateDataSet,
resolveDefaultCaseTitle, and runActions.
In `@src/main/resources/petriNets/engine-processes/menu/menu_item.xml`:
- Around line 43-64: Replace the manual destUriPath parsing in the menu_item
create action with the existing ActionDelegate.groovy splitUriPath(String)
helper so root paths and single-segment URIs are handled consistently. Update
the logic around move_dest_uri in the action block to use the helper’s result
instead of destUriPath.split(uriService.getUriSeparator()) and remove the size()
> 1 workaround. Also drop the unused initializeTrans binding from the action if
it is not needed.
---
Outside diff comments:
In
`@src/main/java/com/netgrif/application/engine/menu/service/MenuItemService.java`:
- Around line 94-129: `createMenuItem` performs several dependent writes across
`workflowService.save`, `appendChildCaseIdAndSave`, `uriService.getOrCreate`,
`createView`, and `setDataWithExecute` without a transaction, so a mid-flow
failure can leave partial/orphaned state. Make `MenuItemService.createMenuItem`
transactional (or otherwise add rollback/compensating cleanup around the whole
flow) so the parent case, menu item case, optional view case, and URI node are
committed atomically. Use the existing `createMenuItem`, `createView`, and
`setDataWithExecute` flow to keep all persisted changes consistent on failure.
In `@src/main/resources/petriNets/engine-processes/menu/menu_item.xml`:
- Around line 941-960: The assign block for moveDestUri is still using
uriService.findByUri(uriPath), which can return null and break
findOptionsBasedOnSelectedNode(node) when the destination path does not exist.
Update this logic to match MenuItemService.moveItem(...) by using
uriService.getOrCreate(...) for the destination lookup, and then pass the
resulting node into change moveDestUri options so the selected-node options are
always built from a valid node.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f93202e3-5fc5-4f1a-89da-a68c0016bfa7
📒 Files selected for processing (16)
src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovysrc/main/java/com/netgrif/application/engine/menu/domain/MenuItemBody.javasrc/main/java/com/netgrif/application/engine/menu/domain/MenuItemConstants.javasrc/main/java/com/netgrif/application/engine/menu/domain/configurations/CaseViewBody.javasrc/main/java/com/netgrif/application/engine/menu/domain/configurations/CaseViewConstants.javasrc/main/java/com/netgrif/application/engine/menu/domain/configurations/TaskViewBody.javasrc/main/java/com/netgrif/application/engine/menu/domain/configurations/TaskViewConstants.javasrc/main/java/com/netgrif/application/engine/menu/domain/templates/FolderTemplate.javasrc/main/java/com/netgrif/application/engine/menu/service/MenuItemService.javasrc/main/java/com/netgrif/application/engine/menu/service/MenuItemTemplateHolder.javasrc/main/java/com/netgrif/application/engine/workflow/web/WorkflowController.javasrc/main/java/com/netgrif/application/engine/workflow/web/requestbodies/CreateCaseBody.javasrc/main/resources/petriNets/engine-processes/menu/case_view_configuration.xmlsrc/main/resources/petriNets/engine-processes/menu/menu_item.xmlsrc/main/resources/petriNets/engine-processes/menu/task_view_configuration.xmlsrc/test/java/com/netgrif/application/engine/menu/MenuItemServiceTest.java
| } | ||
| this.view = viewBody; | ||
| MenuItemViewType viewType = viewBody.getViewType(); | ||
| if (viewType.isTabbed() != viewType.isUntabbed()) { |
There was a problem hiding this comment.
do we need booleans isTabbed and isUntabbed, it isnt possible to use one boolean? or its only bad naming and its used for another purpose?
There was a problem hiding this comment.
We need both. Since some view can be tabbed and untabbed at the same time -> for example CaseViewBody
There was a problem hiding this comment.
I have simplified these attributes -> used new enumeration. And I have simplified other methods, which used these attributes so I think it's fine now
There was a problem hiding this comment.
shouldn't we write descripcion how to make a header identifier? processIdentifier_datafieldIdentifier ? or it should be in documentation?
There was a problem hiding this comment.
In my opinion it should be in a separate documentation. And also which meta headers are available
There was a problem hiding this comment.
but the problem is that documentation doesnt exist now, so nobody know how to write custom headers from processes
- resolve PR comments
- simplify isTabbed/isUntabbed attributes
coderabbitai
Bot
added
breaking change
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/java/com/netgrif/application/engine/menu/domain/MenuItemBody.java (1)
211-237: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuard
this.viewbefore dereferencing it intoDataSetWithView. Folder bodies can haveview == null, so a non-nullviewCasewill NPE here; skip the view-configuration fields whenthis.viewis absent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/netgrif/application/engine/menu/domain/MenuItemBody.java` around lines 211 - 237, In MenuItemBody.toDataSetWithView, guard access to this.view before calling getViewType() because folder menu items can have a null view even when viewCase is present. Update the conditional so the view-configuration fields (FIELD_VIEW_CONFIGURATION_TYPE, FIELD_VIEW_CONFIGURATION_ID, FIELD_VIEW_CONFIGURATION_FORM, FIELD_VIEW_CONFIGURATION_ALL_DATA_FORM) are only populated when this.view is available, while still preserving the existing behavior for the other dataset entries.src/main/resources/petriNets/engine-processes/menu/menu_item.xml (1)
43-64: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAllow root destinations here.
splitUriPath("/")returns["/"], sosize() > 1skipsmove_dest_urifordest_uri_path == "/"and creation can’t move to root. Drop the guard or special-case root if that target should be valid.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/resources/petriNets/engine-processes/menu/menu_item.xml` around lines 43 - 64, The create handler in menu_item_create is incorrectly skipping root destinations because the size() > 1 check prevents move_dest_uri from being set when dest_uri_path is "/". Update the post action in the menu_item_create event so root is treated as a valid destination, either by removing the size guard or explicitly special-casing "/" before calling splitUriPath and change move_dest_uri value.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/main/java/com/netgrif/application/engine/menu/service/interfaces/IMenuItemService.java`:
- Around line 35-44: The Javadoc for getPrimaryViewsAsOptions is stale because
it still mentions “in consideration of input value” even though the method no
longer accepts any parameters. Update the comment in
IMenuItemService#getPrimaryViewsAsOptions so it only describes returning all
primary views as options for MapOptionsField and remove the leftover wording
from the old overload.
- Around line 54-59: The suffix handling in getAvailableViewsAsOptions is too
loose because lastIndexOf("_configuration") can match in the middle of the
identifier instead of only as a real suffix. Update the logic to first verify
the identifier ends with "_configuration" before stripping it, and only then
trim that exact suffix so the parent identifier passed to
MenuItemViewType.findAllByParentIdentifier stays valid.
In
`@src/main/java/com/netgrif/application/engine/workflow/web/WorkflowController.java`:
- Around line 121-148: The `searchPfql` endpoint duplicates the self-link
creation and `PagedModel` assembly logic already used by `search`, which risks
divergence. Extract the common link/resource-building flow into a shared helper
in `WorkflowController` (for example around the
`WebMvcLinkBuilder`/`PagedResourcesAssembler` usage) and have both `search` and
`searchPfql` call it, leaving `searchPfql` only responsible for PFQL
preprocessing and its exception wrapping.
- Around line 141-146: In searchPfql, preserve the original exception cause when
rethrowing. Update the IllegalArgumentException handling in
WorkflowController.searchPfql to use the BadRequestException constructor that
accepts both message and throwable, and add the same String, Throwable
constructor to InternalServerErrorException before rethrowing it from the
generic catch so the original cause is chained.
---
Outside diff comments:
In `@src/main/java/com/netgrif/application/engine/menu/domain/MenuItemBody.java`:
- Around line 211-237: In MenuItemBody.toDataSetWithView, guard access to
this.view before calling getViewType() because folder menu items can have a null
view even when viewCase is present. Update the conditional so the
view-configuration fields (FIELD_VIEW_CONFIGURATION_TYPE,
FIELD_VIEW_CONFIGURATION_ID, FIELD_VIEW_CONFIGURATION_FORM,
FIELD_VIEW_CONFIGURATION_ALL_DATA_FORM) are only populated when this.view is
available, while still preserving the existing behavior for the other dataset
entries.
In `@src/main/resources/petriNets/engine-processes/menu/menu_item.xml`:
- Around line 43-64: The create handler in menu_item_create is incorrectly
skipping root destinations because the size() > 1 check prevents move_dest_uri
from being set when dest_uri_path is "/". Update the post action in the
menu_item_create event so root is treated as a valid destination, either by
removing the size guard or explicitly special-casing "/" before calling
splitUriPath and change move_dest_uri value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d2300e39-785f-4b7a-b654-7b934c241e24
📒 Files selected for processing (13)
src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovysrc/main/java/com/netgrif/application/engine/menu/domain/MenuItemBody.javasrc/main/java/com/netgrif/application/engine/menu/domain/MenuItemViewType.javasrc/main/java/com/netgrif/application/engine/menu/domain/ViewType.javasrc/main/java/com/netgrif/application/engine/menu/domain/configurations/CaseViewBody.javasrc/main/java/com/netgrif/application/engine/menu/domain/configurations/TaskViewBody.javasrc/main/java/com/netgrif/application/engine/menu/service/interfaces/IMenuItemService.javasrc/main/java/com/netgrif/application/engine/workflow/web/WorkflowController.javasrc/main/resources/petriNets/engine-processes/menu/case_view_configuration.xmlsrc/main/resources/petriNets/engine-processes/menu/menu_item.xmlsrc/main/resources/petriNets/engine-processes/menu/tabbed_ticket_view_configuration.xmlsrc/main/resources/petriNets/engine-processes/menu/task_view_configuration.xmlsrc/test/java/com/netgrif/application/engine/menu/MenuItemServiceTest.java
💤 Files with no reviewable changes (1)
- src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/java/com/netgrif/application/engine/menu/domain/MenuItemBody.java (1)
211-237: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuard
this.viewbefore dereferencing it intoDataSetWithView. Folder bodies can haveview == null, so a non-nullviewCasewill NPE here; skip the view-configuration fields whenthis.viewis absent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/netgrif/application/engine/menu/domain/MenuItemBody.java` around lines 211 - 237, In MenuItemBody.toDataSetWithView, guard access to this.view before calling getViewType() because folder menu items can have a null view even when viewCase is present. Update the conditional so the view-configuration fields (FIELD_VIEW_CONFIGURATION_TYPE, FIELD_VIEW_CONFIGURATION_ID, FIELD_VIEW_CONFIGURATION_FORM, FIELD_VIEW_CONFIGURATION_ALL_DATA_FORM) are only populated when this.view is available, while still preserving the existing behavior for the other dataset entries.src/main/resources/petriNets/engine-processes/menu/menu_item.xml (1)
43-64: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAllow root destinations here.
splitUriPath("/")returns["/"], sosize() > 1skipsmove_dest_urifordest_uri_path == "/"and creation can’t move to root. Drop the guard or special-case root if that target should be valid.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/resources/petriNets/engine-processes/menu/menu_item.xml` around lines 43 - 64, The create handler in menu_item_create is incorrectly skipping root destinations because the size() > 1 check prevents move_dest_uri from being set when dest_uri_path is "/". Update the post action in the menu_item_create event so root is treated as a valid destination, either by removing the size guard or explicitly special-casing "/" before calling splitUriPath and change move_dest_uri value.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/main/java/com/netgrif/application/engine/menu/service/interfaces/IMenuItemService.java`:
- Around line 35-44: The Javadoc for getPrimaryViewsAsOptions is stale because
it still mentions “in consideration of input value” even though the method no
longer accepts any parameters. Update the comment in
IMenuItemService#getPrimaryViewsAsOptions so it only describes returning all
primary views as options for MapOptionsField and remove the leftover wording
from the old overload.
- Around line 54-59: The suffix handling in getAvailableViewsAsOptions is too
loose because lastIndexOf("_configuration") can match in the middle of the
identifier instead of only as a real suffix. Update the logic to first verify
the identifier ends with "_configuration" before stripping it, and only then
trim that exact suffix so the parent identifier passed to
MenuItemViewType.findAllByParentIdentifier stays valid.
In
`@src/main/java/com/netgrif/application/engine/workflow/web/WorkflowController.java`:
- Around line 121-148: The `searchPfql` endpoint duplicates the self-link
creation and `PagedModel` assembly logic already used by `search`, which risks
divergence. Extract the common link/resource-building flow into a shared helper
in `WorkflowController` (for example around the
`WebMvcLinkBuilder`/`PagedResourcesAssembler` usage) and have both `search` and
`searchPfql` call it, leaving `searchPfql` only responsible for PFQL
preprocessing and its exception wrapping.
- Around line 141-146: In searchPfql, preserve the original exception cause when
rethrowing. Update the IllegalArgumentException handling in
WorkflowController.searchPfql to use the BadRequestException constructor that
accepts both message and throwable, and add the same String, Throwable
constructor to InternalServerErrorException before rethrowing it from the
generic catch so the original cause is chained.
---
Outside diff comments:
In `@src/main/java/com/netgrif/application/engine/menu/domain/MenuItemBody.java`:
- Around line 211-237: In MenuItemBody.toDataSetWithView, guard access to
this.view before calling getViewType() because folder menu items can have a null
view even when viewCase is present. Update the conditional so the
view-configuration fields (FIELD_VIEW_CONFIGURATION_TYPE,
FIELD_VIEW_CONFIGURATION_ID, FIELD_VIEW_CONFIGURATION_FORM,
FIELD_VIEW_CONFIGURATION_ALL_DATA_FORM) are only populated when this.view is
available, while still preserving the existing behavior for the other dataset
entries.
In `@src/main/resources/petriNets/engine-processes/menu/menu_item.xml`:
- Around line 43-64: The create handler in menu_item_create is incorrectly
skipping root destinations because the size() > 1 check prevents move_dest_uri
from being set when dest_uri_path is "/". Update the post action in the
menu_item_create event so root is treated as a valid destination, either by
removing the size guard or explicitly special-casing "/" before calling
splitUriPath and change move_dest_uri value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d2300e39-785f-4b7a-b654-7b934c241e24
📒 Files selected for processing (13)
src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovysrc/main/java/com/netgrif/application/engine/menu/domain/MenuItemBody.javasrc/main/java/com/netgrif/application/engine/menu/domain/MenuItemViewType.javasrc/main/java/com/netgrif/application/engine/menu/domain/ViewType.javasrc/main/java/com/netgrif/application/engine/menu/domain/configurations/CaseViewBody.javasrc/main/java/com/netgrif/application/engine/menu/domain/configurations/TaskViewBody.javasrc/main/java/com/netgrif/application/engine/menu/service/interfaces/IMenuItemService.javasrc/main/java/com/netgrif/application/engine/workflow/web/WorkflowController.javasrc/main/resources/petriNets/engine-processes/menu/case_view_configuration.xmlsrc/main/resources/petriNets/engine-processes/menu/menu_item.xmlsrc/main/resources/petriNets/engine-processes/menu/tabbed_ticket_view_configuration.xmlsrc/main/resources/petriNets/engine-processes/menu/task_view_configuration.xmlsrc/test/java/com/netgrif/application/engine/menu/MenuItemServiceTest.java
💤 Files with no reviewable changes (1)
- src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy
🛑 Comments failed to post (4)
src/main/java/com/netgrif/application/engine/menu/service/interfaces/IMenuItemService.java (2)
35-44: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Stale javadoc: "in consideration of input value" no longer applies.
getPrimaryViewsAsOptions()takes no parameters; this phrase is leftover from the previous overload.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/netgrif/application/engine/menu/service/interfaces/IMenuItemService.java` around lines 35 - 44, The Javadoc for getPrimaryViewsAsOptions is stale because it still mentions “in consideration of input value” even though the method no longer accepts any parameters. Update the comment in IMenuItemService#getPrimaryViewsAsOptions so it only describes returning all primary views as options for MapOptionsField and remove the leftover wording from the old overload.
54-59: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Suffix stripping via
lastIndexOfis fragile — useendsWith.
lastIndexOf("_configuration")matches the substring anywhere, not just at the end. If it ever appears mid-string (not a true suffix),substring(0, index)silently drops everything after that occurrence, producing a wrong parent identifier (and potentially anIllegalArgumentExceptionfromfromIdentifier). All current callers pass identifiers ending exactly in_configuration, so this doesn't bite today, but the check should validate an actual suffix.♻️ Proposed fix
default Map<String, I18nString> getAvailableViewsAsOptions(String viewIdentifier) { - int index = viewIdentifier.lastIndexOf("_configuration"); - if (index > 0) { - viewIdentifier = viewIdentifier.substring(0, index); - } + String suffix = "_configuration"; + if (viewIdentifier.endsWith(suffix)) { + viewIdentifier = viewIdentifier.substring(0, viewIdentifier.length() - suffix.length()); + } return MenuItemViewType.findAllByParentIdentifier(viewIdentifier).stream() .collect(Collectors.toMap(MenuItemViewType::getIdentifier, MenuItemViewType::getName)); }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.default Map<String, I18nString> getAvailableViewsAsOptions(String viewIdentifier) { String suffix = "_configuration"; if (viewIdentifier.endsWith(suffix)) { viewIdentifier = viewIdentifier.substring(0, viewIdentifier.length() - suffix.length()); } return MenuItemViewType.findAllByParentIdentifier(viewIdentifier).stream()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/netgrif/application/engine/menu/service/interfaces/IMenuItemService.java` around lines 54 - 59, The suffix handling in getAvailableViewsAsOptions is too loose because lastIndexOf("_configuration") can match in the middle of the identifier instead of only as a real suffix. Update the logic to first verify the identifier ends with "_configuration" before stripping it, and only then trim that exact suffix so the parent identifier passed to MenuItemViewType.findAllByParentIdentifier stays valid.src/main/java/com/netgrif/application/engine/workflow/web/WorkflowController.java (2)
121-148: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Consider extracting shared logic between
searchandsearchPfql.
searchPfql(Lines 121-148) largely duplicatessearch's (Lines 108-119) self-link and resource-assembly logic, only adding the PFQL preprocessing step and exception wrapping. Extracting a shared helper would reduce drift risk between the two endpoints.🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 132-132: Avoid LDAP injections
Context: elasticCaseService.search(searchBody.getList(), user, pageable, locale, operation == MergeFilterOperation.AND)
Note: [CWE-90] Improper Neutralization of Special Elements used in an LDAP Query ('LDAP Injection'). Security best practice.(ldap-injection-java)
🪛 PMD (7.25.0)
[Medium] 142-142: PreserveStackTrace (Best Practices): Thrown exception does not preserve the stack trace of exception 'e' on all code paths
(PreserveStackTrace (Best Practices))
[Medium] 145-145: PreserveStackTrace (Best Practices): Thrown exception does not preserve the stack trace of exception 'e' on all code paths
(PreserveStackTrace (Best Practices))
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/netgrif/application/engine/workflow/web/WorkflowController.java` around lines 121 - 148, The `searchPfql` endpoint duplicates the self-link creation and `PagedModel` assembly logic already used by `search`, which risks divergence. Extract the common link/resource-building flow into a shared helper in `WorkflowController` (for example around the `WebMvcLinkBuilder`/`PagedResourcesAssembler` usage) and have both `search` and `searchPfql` call it, leaving `searchPfql` only responsible for PFQL preprocessing and its exception wrapping.
141-146: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Check constructors of BadRequestException and InternalServerErrorException fd -e java BadRequestException InternalServerErrorException | xargs -I{} sh -c 'echo "--- {} ---"; cat {}'Repository: netgrif/application-engine
Length of output: 280
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== Locate exception classes ==" fd -a -t f 'BadRequestException\.java|InternalServerErrorException\.java' src/main/java || true echo echo "== Search for class declarations ==" rg -n "class (BadRequestException|InternalServerErrorException)\b|record (BadRequestException|InternalServerErrorException)\b" src/main/java echo echo "== Search for constructors/usages ==" rg -n "BadRequestException\(|InternalServerErrorException\(" src/main/java | sed -n '1,120p'Repository: netgrif/application-engine
Length of output: 2418
🏁 Script executed:
#!/bin/bash set -euo pipefail # Find the exact files by name, then print their contents if present. for name in BadRequestException InternalServerErrorException; do echo "== $name ==" files=$(fd -a -t f "${name}.java" src/main/java || true) if [ -z "$files" ]; then echo "not found" continue fi while IFS= read -r f; do echo "--- $f ---" sed -n '1,220p' "$f" done <<< "$files" echo doneRepository: netgrif/application-engine
Length of output: 1286
Chain the cause in
searchPfql.BadRequestExceptionalready supports(String, Throwable), so passethere;InternalServerErrorExceptionwill need the same constructor added first if you want the rethrown exception to keep the original cause.🧰 Tools
🪛 PMD (7.25.0)
[Medium] 142-142: PreserveStackTrace (Best Practices): Thrown exception does not preserve the stack trace of exception 'e' on all code paths
(PreserveStackTrace (Best Practices))
[Medium] 145-145: PreserveStackTrace (Best Practices): Thrown exception does not preserve the stack trace of exception 'e' on all code paths
(PreserveStackTrace (Best Practices))
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/netgrif/application/engine/workflow/web/WorkflowController.java` around lines 141 - 146, In searchPfql, preserve the original exception cause when rethrowing. Update the IllegalArgumentException handling in WorkflowController.searchPfql to use the BadRequestException constructor that accepts both message and throwable, and add the same String, Throwable constructor to InternalServerErrorException before rethrowing it from the generic catch so the original cause is chained.Source: Linters/SAST tools
- update description
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/resources/petriNets/engine-processes/menu/case_view_configuration.xml (1)
1060-1091: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSort mode fields are unconditionally required regardless of whether sorting is enabled.
case_headers_sort_mode_activeandcase_headers_sort_mode_directionare markededitable+requiredunconditionally. However,case_headers_modeis amultichoice_mapwhere the user can deselect "sort". When "sort" is removed,updateOptionsBasedOnValuecorrectly removes "sort" fromcase_headers_default_modeoptions, but the sort mode fields remain required — blocking form submission with no way to proceed.Additionally, when
case_is_header_mode_changeableis set tofalse, the existing action hidescase_headers_modeandcase_headers_default_mode, but the sort mode fields stay visible and required, creating an inconsistent UI state.Consider adding a conditional action on
case_headers_mode(and extending thecase_is_header_mode_changeableaction) to make the sort mode fields editable/required only when "sort" is selected, and hidden/optional otherwise — following the same pattern already used forcase_headers_modeandcase_headers_default_mode.🔧 Proposed conditional action for sort mode fields
<dataRef> <id>case_is_header_mode_changeable</id> <logic> <behavior>editable</behavior> <behavior>required</behavior> <action trigger="set"> trans: t.this, isChangeable: f.case_is_header_mode_changeable, mode: f.case_headers_mode, - defaultMode: f.case_headers_default_mode; + defaultMode: f.case_headers_default_mode, + sortActive: f.case_headers_sort_mode_active, + sortDirection: f.case_headers_sort_mode_direction; make [mode, defaultMode], editable on trans when { isChangeable.value } make [mode, defaultMode], required on trans when { isChangeable.value } make [mode, defaultMode], hidden on trans when { !isChangeable.value } make [mode, defaultMode], optional on trans when { !isChangeable.value } + + make [sortActive, sortDirection], editable on trans when { isChangeable.value } + make [sortActive, sortDirection], required on trans when { isChangeable.value } + make [sortActive, sortDirection], hidden on trans when { !isChangeable.value } + make [sortActive, sortDirection], optional on trans when { !isChangeable.value } </action> </logic>Additionally, add an action on
case_headers_modeto conditionally show/require the sort fields based on whether "sort" is selected:<data type="multichoice_map" immediate="true"> <id>case_headers_mode</id> ... <action trigger="set"> headersMode: f.case_headers_mode, defaultMode: f.case_headers_default_mode, - holder: f.headers_options_holder; + holder: f.headers_options_holder, + sortActive: f.case_headers_sort_mode_active, + sortDirection: f.case_headers_sort_mode_direction; + def sortEnabled = headersMode.value != null && headersMode.value.contains("sort") + make [sortActive, sortDirection], editable on trans when { sortEnabled } + make [sortActive, sortDirection], required on trans when { sortEnabled } + make [sortActive, sortDirection], hidden on trans when { !sortEnabled } + make [sortActive, sortDirection], optional on trans when { !sortEnabled } updateOptionsBasedOnValue(defaultMode, headersMode, holder) </action>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/resources/petriNets/engine-processes/menu/case_view_configuration.xml` around lines 1060 - 1091, The sort mode fields are always editable/required even when sorting is not selected, which leaves the form in an invalid state. Update the configuration around case_headers_mode, case_headers_sort_mode_active, and case_headers_sort_mode_direction so these fields are only visible, editable, and required when "sort" is present, and become hidden/optional otherwise. Also extend the existing case_is_header_mode_changeable action so it applies the same hide/require behavior to the sort mode fields when header mode changes are disabled.src/main/resources/petriNets/engine-processes/menu/task_view_configuration.xml (1)
703-734: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSort mode fields are unconditionally required regardless of whether sorting is enabled.
Same issue as in
case_view_configuration.xml:task_headers_sort_mode_activeandtask_headers_sort_mode_directionare markededitable+requiredunconditionally. When "sort" is removed fromtask_headers_mode(amultichoice_mapwith sort/edit options), the sort mode fields remain required, blocking form submission.Additionally, when
task_is_header_mode_changeableisfalse, the existing action hidestask_headers_modeandtask_headers_default_mode, but the sort mode fields stay visible and required.Apply the same conditional action pattern proposed for the case view — make the sort mode fields editable/required only when "sort" is selected and when header mode is changeable.
🔧 Proposed conditional action for sort mode fields
<dataRef> <id>task_is_header_mode_changeable</id> <logic> <behavior>editable</behavior> <action trigger="set"> trans: t.this, isChangeable: f.task_is_header_mode_changeable, mode: f.task_headers_mode, - defaultMode: f.task_headers_default_mode; + defaultMode: f.task_headers_default_mode, + sortActive: f.task_headers_sort_mode_active, + sortDirection: f.task_headers_sort_mode_direction; make [mode, defaultMode], editable on trans when { isChangeable.value } make [mode, defaultMode], required on trans when { isChangeable.value } make [mode, defaultMode], hidden on trans when { !isChangeable.value } make [mode, defaultMode], optional on trans when { !isChangeable.value } + + make [sortActive, sortDirection], editable on trans when { isChangeable.value } + make [sortActive, sortDirection], required on trans when { isChangeable.value } + make [sortActive, sortDirection], hidden on trans when { !isChangeable.value } + make [sortActive, sortDirection], optional on trans when { !isChangeable.value } </action> </logic>Additionally, add an action on
task_headers_mode:<data type="multichoice_map" immediate="true"> <id>task_headers_mode</id> ... <action trigger="set"> headersMode: f.task_headers_mode, defaultMode: f.task_headers_default_mode, - holder: f.headers_options_holder; + holder: f.headers_options_holder, + sortActive: f.task_headers_sort_mode_active, + sortDirection: f.task_headers_sort_mode_direction; + def sortEnabled = headersMode.value != null && headersMode.value.contains("sort") + make [sortActive, sortDirection], editable on trans when { sortEnabled } + make [sortActive, sortDirection], required on trans when { sortEnabled } + make [sortActive, sortDirection], hidden on trans when { !sortEnabled } + make [sortActive, sortDirection], optional on trans when { !sortEnabled } updateOptionsBasedOnValue(defaultMode, headersMode, holder) </action>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/resources/petriNets/engine-processes/menu/task_view_configuration.xml` around lines 703 - 734, The task header sort mode fields are always marked editable and required in task_view_configuration.xml, so they still block submission even when “sort” is not selected or header mode changes are disabled. Update the dataRef definitions for task_headers_sort_mode_active and task_headers_sort_mode_direction to follow the same conditional action pattern used in case_view_configuration.xml, making them editable/required only when task_headers_mode includes sort and when task_is_header_mode_changeable is true. Also ensure the existing action on task_headers_mode toggles these sort fields together with task_headers_default_mode so they are hidden/disabled when sorting is not applicable.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@src/main/resources/petriNets/engine-processes/menu/case_view_configuration.xml`:
- Around line 1060-1091: The sort mode fields are always editable/required even
when sorting is not selected, which leaves the form in an invalid state. Update
the configuration around case_headers_mode, case_headers_sort_mode_active, and
case_headers_sort_mode_direction so these fields are only visible, editable, and
required when "sort" is present, and become hidden/optional otherwise. Also
extend the existing case_is_header_mode_changeable action so it applies the same
hide/require behavior to the sort mode fields when header mode changes are
disabled.
In
`@src/main/resources/petriNets/engine-processes/menu/task_view_configuration.xml`:
- Around line 703-734: The task header sort mode fields are always marked
editable and required in task_view_configuration.xml, so they still block
submission even when “sort” is not selected or header mode changes are disabled.
Update the dataRef definitions for task_headers_sort_mode_active and
task_headers_sort_mode_direction to follow the same conditional action pattern
used in case_view_configuration.xml, making them editable/required only when
task_headers_mode includes sort and when task_is_header_mode_changeable is true.
Also ensure the existing action on task_headers_mode toggles these sort fields
together with task_headers_default_mode so they are hidden/disabled when sorting
is not applicable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 193967d2-da28-474e-a64b-5cc48d1cba8c
📒 Files selected for processing (2)
src/main/resources/petriNets/engine-processes/menu/case_view_configuration.xmlsrc/main/resources/petriNets/engine-processes/menu/task_view_configuration.xml
2 participants
Description
New features
Improvements
Implements NAE-2454
Dependencies
No new dependencies were introduced
Third party dependencies
No new dependencies were introduced
Blocking Pull requests
There are no dependencies on other PR
How Has Been This Tested?
Manually and by unit test: MenuItemServiceTest.java
Test Configuration
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Tests