-
Notifications
You must be signed in to change notification settings - Fork 6
[NAE-2147] Merge 6.5.0 into 7.0.0 #331
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
Conversation
- implement auto-select for menu items
- create processes for managing dashboards - dashboard_management and dashboard_item - create runner for importing dashboard management processes
- added option to load icon from web (image from web) - added option to change dashboard toolbar logo - added option to chane appearance of dashboard toolbar
- added option to change icon and font colors in dashboard items - fix for items ordering
…temoch - fix ActionDelegate.execute method - update ActionDelegate menu item methods - import configuration nets in FilterRunner - implement MenuItemService with new or updated domain models - move menu item domain models to menu package - implement MenuItemView enum - implement MenuItemUtils - rename preference_item to menu_item - add tabbed_case_view_configuration and tabbed_task_view_configuration nets - update MenuItemApiTest according to changes
…temoch - add logging to MenuItemService
…temoch - add or update javadoc
- add translations
- remove redundant MenuItemBody attribute - update menu_item and tabbed_case_view_configuration dataGroups
- update MenuItemService find methods - fix MenuItemApiTest.testDuplicateMenuItem
- update MenuItemBody.setView
- add option to change profile and login urls
- implement TabbedTicketView configuration - implement TabbedSingleTaskView configuration - fix missing entry in ViewBody - fix method call in tabbed_case_view_configuration
- change default value of login url to empty string
- update by PR comments - create findCasesElastic method in ActionDelegate with default page size of 1000 items - check duplicate ids in dashboard_item and dashboard_management in finish action - move importProcess function into ImportHelper and clean runners
- fix setting view_configuration_type
- add translations
- documentation
- implement basic elastic search for title and identifier to ElasticPetriNetService and implement endpoint for elastic search
- remove TODO
- enabled MenuItemApiTest
- change default page size to 100 in findCasesElastic
- resolve PR comments
- add MenuItemView.isPrimary attribute - fix options initializing in configuration
# Conflicts: # src/main/groovy/com/netgrif/application/engine/startup/FilterRunner.groovy
- fix after merge
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review continued from previous batch...
| /** | ||
| * Creates menu item case and it's configuration cases | ||
| * | ||
| * @param body data used for creation | ||
| * @return initialized menu item instance with the provided data | ||
| * @throws IllegalArgumentException if the provided menu identifier already exists | ||
| */ | ||
| @Override | ||
| public Case createMenuItem(MenuItemBody body) throws TransitionNotExecutableException { | ||
| log.debug("Creation of menu item case with identifier [{}] started.", body.getIdentifier()); | ||
| IUser loggedUser = userService.getLoggedOrSystem(); | ||
| String sanitizedIdentifier = MenuItemUtils.sanitize(body.getIdentifier()); | ||
|
|
||
| if (existsMenuItem(sanitizedIdentifier)) { | ||
| throw new IllegalArgumentException(String.format("Menu item identifier %s is not unique!", sanitizedIdentifier)); | ||
| } | ||
|
|
||
| Case parentItemCase = getOrCreateFolderItem(body.getPath()); | ||
|
|
||
| I18nString newName = body.getMenuName(); | ||
| if (newName == null) { | ||
| newName = new I18nString(body.getIdentifier()); | ||
| } | ||
| Case menuItemCase = createCase(FilterRunner.MENU_NET_IDENTIFIER, newName.getDefaultValue(), | ||
| loggedUser.transformToLoggedUser()); | ||
| menuItemCase = workflowService.save(menuItemCase); | ||
|
|
||
| parentItemCase = appendChildCaseIdAndSave(parentItemCase, menuItemCase.getStringId()); | ||
|
|
||
| Case viewCase = null; | ||
| if (body.hasView()) { | ||
| viewCase = createView(body.getView()); | ||
| } | ||
|
|
||
| String nodePath = createNodePath(body.getPath(), sanitizedIdentifier); | ||
|
|
||
| ToDataSetOutcome dataSetOutcome = body.toDataSet(parentItemCase.getStringId(), nodePath, viewCase); | ||
| menuItemCase = setDataWithExecute(menuItemCase, MenuItemConstants.TRANS_INIT_ID, dataSetOutcome.getDataSet()); | ||
| log.debug("Created menu item case [{}] with identifier [{}].", menuItemCase.getStringId(), body.getIdentifier()); | ||
| return menuItemCase; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider adding transaction management for atomicity.
The menu item creation involves multiple operations (case creation, parent update, view creation) that could leave the system in an inconsistent state if any step fails.
Consider wrapping the entire operation in a transaction or implementing compensating actions:
@Transactional
public Case createMenuItem(MenuItemBody body) throws TransitionNotExecutableException {
// existing implementation
}Alternatively, implement rollback logic if view creation fails:
try {
viewCase = createView(body.getView());
} catch (Exception e) {
// Rollback: delete the created menu item and revert parent changes
workflowService.deleteCase(menuItemCase);
// Revert parent folder changes
throw e;
}🤖 Prompt for AI Agents
In
application-engine/src/main/java/com/netgrif/application/engine/menu/services/MenuItemService.java
between lines 85 and 125, the createMenuItem method performs multiple operations
that must be atomic to avoid inconsistent state if any step fails. To fix this,
annotate the method with @Transactional to ensure all database changes are
committed or rolled back together. Alternatively, add explicit rollback logic
around the view creation step by catching exceptions, deleting the created menu
item case, reverting any parent folder changes, and rethrowing the exception to
maintain consistency.
| IUser loggedUser = userService.getLoggedOrSystem(); | ||
| String taskId = MenuItemUtils.findTaskIdInCase(useCase, transId); | ||
| Task task = taskService.findOne(taskId); | ||
| task = taskService.assignTask(task, loggedUser).getTask(); | ||
| task = dataService.setData(task, ImportHelper.populateDataset((Map) dataSet)).getTask(); | ||
| return taskService.finishTask(task, loggedUser).getCase(); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Add null safety checks in setDataWithExecute.
The method assumes the task exists but doesn't handle the case where it might be null.
protected Case setDataWithExecute(Case useCase, String transId, Map<String, Map<String, Object>> dataSet) throws TransitionNotExecutableException {
IUser loggedUser = userService.getLoggedOrSystem();
String taskId = MenuItemUtils.findTaskIdInCase(useCase, transId);
+ if (taskId == null) {
+ throw new IllegalStateException(String.format("Task with transition '%s' not found in case '%s'", transId, useCase.getStringId()));
+ }
Task task = taskService.findOne(taskId);
+ if (task == null) {
+ throw new IllegalStateException(String.format("Task '%s' not found", taskId));
+ }
task = taskService.assignTask(task, loggedUser).getTask();
task = dataService.setData(task, ImportHelper.populateDataset((Map) dataSet)).getTask();
return taskService.finishTask(task, loggedUser).getCase();
}📝 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.
| IUser loggedUser = userService.getLoggedOrSystem(); | |
| String taskId = MenuItemUtils.findTaskIdInCase(useCase, transId); | |
| Task task = taskService.findOne(taskId); | |
| task = taskService.assignTask(task, loggedUser).getTask(); | |
| task = dataService.setData(task, ImportHelper.populateDataset((Map) dataSet)).getTask(); | |
| return taskService.finishTask(task, loggedUser).getCase(); | |
| } | |
| IUser loggedUser = userService.getLoggedOrSystem(); | |
| String taskId = MenuItemUtils.findTaskIdInCase(useCase, transId); | |
| if (taskId == null) { | |
| throw new IllegalStateException(String.format( | |
| "Task with transition '%s' not found in case '%s'", | |
| transId, useCase.getStringId())); | |
| } | |
| Task task = taskService.findOne(taskId); | |
| if (task == null) { | |
| throw new IllegalStateException(String.format( | |
| "Task '%s' not found", | |
| taskId)); | |
| } | |
| task = taskService.assignTask(task, loggedUser).getTask(); | |
| task = dataService.setData( | |
| task, | |
| ImportHelper.populateDataset((Map) dataSet) | |
| ).getTask(); | |
| return taskService.finishTask(task, loggedUser).getCase(); | |
| } |
🤖 Prompt for AI Agents
In
application-engine/src/main/java/com/netgrif/application/engine/menu/services/MenuItemService.java
around lines 719 to 725, the method setDataWithExecute assumes the task
retrieved by taskService.findOne(taskId) is never null, which can cause
NullPointerExceptions. Add a null check after retrieving the task; if the task
is null, handle it appropriately, such as throwing an exception or returning
early. This ensures subsequent calls on the task object are safe.
| <event type="delete"> | ||
| <id>view_delete</id> | ||
| <actions phase="pre"> | ||
| <action id="0"> | ||
| removeViewCase() | ||
| </action> | ||
| </actions> | ||
| </event> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider error handling for async view case deletion.
The removeViewCase function runs asynchronously but doesn't handle potential errors. If the deletion fails, it could leave orphaned cases.
Consider adding error handling:
async.run {
try {
workflowService.deleteCase(viewIdAsList[0])
} catch (Exception e) {
log.error("Failed to delete associated view case: ${viewIdAsList[0]}", e)
}
}Also applies to: 78-90
🤖 Prompt for AI Agents
In
application-engine/src/main/resources/petriNets/engine-processes/menu/tabbed_ticket_view_configuration.xml
around lines 34 to 41, the removeViewCase function is called asynchronously
without error handling, risking orphaned cases if deletion fails. Modify the
action to run the deletion inside an async block with a try-catch that logs any
exceptions, similar to the provided example. Apply the same error handling
pattern to lines 78 to 90 where similar async deletions occur.
| <function scope="process" name="removeViewCase"> | ||
| { -> | ||
|
|
||
| def viewIdAsList = useCase.dataSet['view_configuration_id'].value | ||
| if (viewIdAsList == null || viewIdAsList.isEmpty()) { | ||
| return | ||
| } | ||
|
|
||
| async.run { | ||
| workflowService.deleteCase(viewIdAsList[0]) | ||
| } | ||
| } | ||
| </function> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add error handling and logging to async view deletion.
The asynchronous removeViewCase function lacks error handling and logging, which could lead to silent failures and orphaned view cases.
<function scope="process" name="removeViewCase">
{ ->
def viewIdAsList = useCase.dataSet['view_configuration_id'].value
if (viewIdAsList == null || viewIdAsList.isEmpty()) {
return
}
async.run {
+ try {
+ log.info("Deleting associated view case: {}", viewIdAsList[0])
workflowService.deleteCase(viewIdAsList[0])
+ log.info("Successfully deleted view case: {}", viewIdAsList[0])
+ } catch (Exception e) {
+ log.error("Failed to delete associated view case: {}", viewIdAsList[0], e)
+ // Consider notifying administrators or implementing retry logic
+ }
}
}
</function>📝 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.
| <function scope="process" name="removeViewCase"> | |
| { -> | |
| def viewIdAsList = useCase.dataSet['view_configuration_id'].value | |
| if (viewIdAsList == null || viewIdAsList.isEmpty()) { | |
| return | |
| } | |
| async.run { | |
| workflowService.deleteCase(viewIdAsList[0]) | |
| } | |
| } | |
| </function> | |
| <function scope="process" name="removeViewCase"> | |
| { -> | |
| def viewIdAsList = useCase.dataSet['view_configuration_id'].value | |
| if (viewIdAsList == null || viewIdAsList.isEmpty()) { | |
| return | |
| } | |
| async.run { | |
| try { | |
| log.info("Deleting associated view case: {}", viewIdAsList[0]) | |
| workflowService.deleteCase(viewIdAsList[0]) | |
| log.info("Successfully deleted view case: {}", viewIdAsList[0]) | |
| } catch (Exception e) { | |
| log.error("Failed to delete associated view case: {}", viewIdAsList[0], e) | |
| // Consider notifying administrators or implementing retry logic | |
| } | |
| } | |
| } | |
| </function> |
🤖 Prompt for AI Agents
In
application-engine/src/main/resources/petriNets/engine-processes/menu/tabbed_ticket_view_configuration.xml
around lines 78 to 90, the async block in the removeViewCase function does not
handle errors or log failures. Wrap the call to workflowService.deleteCase in a
try-catch block inside the async.run closure, log any caught exceptions with
relevant error details, and ensure errors do not fail silently.
| public ToDataSetOutcome toDataSet(String parentId, String nodePath, Case viewCase) { | ||
| ToDataSetOutcome outcome = new ToDataSetOutcome(); | ||
|
|
||
| // GENERIC | ||
| ArrayList<String> filterIdCaseRefValue = new ArrayList<>(); | ||
| if (this.filter != null) { | ||
| filterIdCaseRefValue.add(this.filter.getStringId()); | ||
| } | ||
| ArrayList<String> parentIdCaseRef = new ArrayList<>(); | ||
| if (parentId != null) { | ||
| parentIdCaseRef.add(parentId); | ||
| outcome.putDataSetEntry(MenuItemConstants.FIELD_PARENT_ID, FieldType.CASE_REF, List.of(parentId)); | ||
| } | ||
|
|
||
| if (nodePath != null) { | ||
| putDataSetEntry(dataSet, MenuItemConstants.PREFERENCE_ITEM_FIELD_NODE_PATH, FieldType.TEXT, nodePath); | ||
| } | ||
| if (!ignoreParentId) { | ||
| putDataSetEntry(dataSet, MenuItemConstants.PREFERENCE_ITEM_FIELD_PARENT_ID, FieldType.CASE_REF, parentIdCaseRef); | ||
| outcome.putDataSetEntry(MenuItemConstants.FIELD_NODE_PATH, FieldType.TEXT, nodePath); | ||
| } | ||
| putDataSetEntry(dataSet, MenuItemConstants.PREFERENCE_ITEM_FIELD_MENU_NAME, FieldType.I18N, this.menuName); | ||
| putDataSetEntry(dataSet, MenuItemConstants.PREFERENCE_ITEM_FIELD_MENU_ICON, FieldType.TEXT, this.menuIcon); | ||
| putDataSetEntry(dataSet, MenuItemConstants.PREFERENCE_ITEM_FIELD_TAB_NAME, FieldType.I18N, this.tabName); | ||
| putDataSetEntry(dataSet, MenuItemConstants.PREFERENCE_ITEM_FIELD_TAB_ICON, FieldType.TEXT, this.tabIcon); | ||
| outcome.putDataSetEntry(MenuItemConstants.FIELD_MENU_NAME, FieldType.I18N, this.menuName); | ||
| outcome.putDataSetEntry(MenuItemConstants.FIELD_MENU_ICON, FieldType.TEXT, this.menuIcon); | ||
| outcome.putDataSetEntry(MenuItemConstants.FIELD_USE_TABBED_VIEW, FieldType.BOOLEAN, this.useTabbedView); | ||
| outcome.putDataSetEntry(MenuItemConstants.FIELD_TAB_NAME, FieldType.I18N, this.tabName); | ||
| outcome.putDataSetEntry(MenuItemConstants.FIELD_TAB_ICON, FieldType.TEXT, this.tabIcon); | ||
| if (this.identifier != null) { | ||
| putDataSetEntry(dataSet, MenuItemConstants.PREFERENCE_ITEM_FIELD_IDENTIFIER, FieldType.TEXT, this.getIdentifier()); | ||
| outcome.putDataSetEntry(MenuItemConstants.FIELD_IDENTIFIER, FieldType.TEXT, this.getIdentifier()); | ||
| } | ||
| putDataSetEntry(dataSet, MenuItemConstants.PREFERENCE_ITEM_FIELD_FILTER_CASE, FieldType.CASE_REF, filterIdCaseRefValue); | ||
| putDataSetEntry(dataSet, MenuItemConstants.PREFERENCE_ITEM_FIELD_USE_TAB_ICON, FieldType.BOOLEAN, this.useTabIcon); | ||
| putDataSetEntry(dataSet, MenuItemConstants.PREFERENCE_ITEM_FIELD_USE_CUSTOM_VIEW, FieldType.BOOLEAN, | ||
| outcome.putDataSetEntry(MenuItemConstants.FIELD_USE_TAB_ICON, FieldType.BOOLEAN, this.useTabIcon); | ||
| outcome.putDataSetEntry(MenuItemConstants.FIELD_USE_CUSTOM_VIEW, FieldType.BOOLEAN, | ||
| this.useCustomView); | ||
| putDataSetEntry(dataSet, MenuItemConstants.PREFERENCE_ITEM_FIELD_CUSTOM_VIEW_SELECTOR, FieldType.TEXT, | ||
| outcome.putDataSetEntry(MenuItemConstants.FIELD_CUSTOM_VIEW_SELECTOR, FieldType.TEXT, | ||
| this.customViewSelector); | ||
|
|
||
| // CASE | ||
| putDataSetEntry(dataSet, MenuItemConstants.PREFERENCE_ITEM_FIELD_CASE_VIEW_SEARCH_TYPE, FieldType.ENUMERATION_MAP, | ||
| this.caseViewSearchType); | ||
| putDataSetEntry(dataSet, MenuItemConstants.PREFERENCE_ITEM_FIELD_CREATE_CASE_BUTTON_TITLE, FieldType.TEXT, | ||
| this.createCaseButtonTitle); | ||
| putDataSetEntry(dataSet, MenuItemConstants.PREFERENCE_ITEM_FIELD_CREATE_CASE_BUTTON_ICON, FieldType.TEXT, | ||
| this.createCaseButtonIcon); | ||
| putDataSetEntry(dataSet, MenuItemConstants.PREFERENCE_ITEM_FIELD_REQUIRE_TITLE_IN_CREATION, FieldType.BOOLEAN, | ||
| this.caseRequireTitleInCreation); | ||
| putDataSetEntry(dataSet, MenuItemConstants.PREFERENCE_ITEM_FIELD_SHOW_CREATE_CASE_BUTTON, FieldType.BOOLEAN, | ||
| this.showCreateCaseButton); | ||
| putDataSetEntry(dataSet, MenuItemConstants.PREFERENCE_ITEM_FIELD_BANNED_NETS_IN_CREATION, FieldType.TEXT, | ||
| this.bannedNetsInCreation); | ||
| putDataSetEntry(dataSet, MenuItemConstants.PREFERENCE_ITEM_FIELD_CASE_SHOW_MORE_MENU, FieldType.BOOLEAN, | ||
| this.caseShowMoreMenu); | ||
| putDataSetEntry(dataSet, MenuItemConstants.PREFERENCE_ITEM_FIELD_CASE_ALLOW_HEADER_TABLE_MODE, FieldType.BOOLEAN, | ||
| this.caseAllowHeaderTableMode); | ||
| putDataSetEntry(dataSet, MenuItemConstants.PREFERENCE_ITEM_FIELD_CASE_HEADERS_MODE, FieldType.MULTICHOICE_MAP, | ||
| this.caseHeadersMode == null ? new ArrayList<>() : this.caseHeadersMode); | ||
| putDataSetEntry(dataSet, MenuItemConstants.PREFERENCE_ITEM_FIELD_CASE_HEADERS_DEFAULT_MODE, FieldType.ENUMERATION_MAP, | ||
| this.caseHeadersDefaultMode); | ||
| putDataSetEntry(dataSet, MenuItemConstants.PREFERENCE_ITEM_FIELD_CASE_DEFAULT_HEADERS, FieldType.TEXT, | ||
| this.caseDefaultHeaders != null ? String.join(",", this.caseDefaultHeaders) : null); | ||
| putDataSetEntry(dataSet, MenuItemConstants.PREFERENCE_ITEM_FIELD_CASE_IS_HEADER_MODE_CHANGEABLE, FieldType.BOOLEAN, | ||
| this.caseIsHeaderModeChangeable); | ||
| putDataSetEntry(dataSet, MenuItemConstants.PREFERENCE_ITEM_FIELD_USE_CASE_DEFAULT_HEADERS, FieldType.BOOLEAN, | ||
| this.caseUseDefaultHeaders); | ||
|
|
||
| // TASK | ||
| ArrayList<String> additionalFilterIdCaseRefValue = new ArrayList<>(); | ||
| if (this.additionalFilter != null) { | ||
| additionalFilterIdCaseRefValue.add(this.additionalFilter.getStringId()); | ||
| outcome.putDataSetEntry(MenuItemConstants.FIELD_IS_AUTO_SELECT, FieldType.BOOLEAN, this.isAutoSelect); | ||
| outcome.putDataSetEntryOptions(MenuItemConstants.FIELD_ALLOWED_ROLES, FieldType.MULTICHOICE_MAP, this.allowedRoles); | ||
| outcome.putDataSetEntryOptions(MenuItemConstants.FIELD_BANNED_ROLES, FieldType.MULTICHOICE_MAP, this.bannedRoles); | ||
|
|
||
| if (viewCase != null) { | ||
| outcome.putDataSetEntry(MenuItemConstants.FIELD_VIEW_CONFIGURATION_TYPE, FieldType.ENUMERATION_MAP, | ||
| this.view.getViewType().getIdentifier()); | ||
| outcome.putDataSetEntry(MenuItemConstants.FIELD_VIEW_CONFIGURATION_ID, FieldType.CASE_REF, | ||
| List.of(viewCase.getStringId())); | ||
| String taskId = MenuItemUtils.findTaskIdInCase(viewCase, ViewConstants.TRANS_SETTINGS_ID); | ||
| outcome.putDataSetEntry(MenuItemConstants.FIELD_VIEW_CONFIGURATION_FORM, FieldType.TASK_REF, List.of(taskId)); | ||
| } | ||
|
|
||
| putDataSetEntry(dataSet, MenuItemConstants.PREFERENCE_ITEM_FIELD_ADDITIONAL_FILTER_CASE, FieldType.CASE_REF, | ||
| additionalFilterIdCaseRefValue); | ||
| putDataSetEntry(dataSet, MenuItemConstants.PREFERENCE_ITEM_FIELD_MERGE_FILTERS, FieldType.BOOLEAN, | ||
| this.mergeFilters); | ||
| putDataSetEntry(dataSet, MenuItemConstants.PREFERENCE_ITEM_FIELD_TASK_VIEW_SEARCH_TYPE, FieldType.ENUMERATION_MAP, | ||
| this.taskViewSearchType); | ||
| putDataSetEntry(dataSet, MenuItemConstants.PREFERENCE_ITEM_FIELD_TASK_HEADERS_MODE, FieldType.MULTICHOICE_MAP, | ||
| this.taskHeadersMode == null ? new ArrayList<>() : this.taskHeadersMode); | ||
| putDataSetEntry(dataSet, MenuItemConstants.PREFERENCE_ITEM_FIELD_TASK_HEADERS_DEFAULT_MODE, FieldType.ENUMERATION_MAP, | ||
| this.taskHeadersDefaultMode); | ||
| putDataSetEntry(dataSet, MenuItemConstants.PREFERENCE_ITEM_FIELD_TASK_IS_HEADER_MODE_CHANGEABLE, FieldType.BOOLEAN, | ||
| this.taskIsHeaderModeChangeable); | ||
| putDataSetEntry(dataSet, MenuItemConstants.PREFERENCE_ITEM_FIELD_TASK_ALLOW_HEADER_TABLE_MODE, FieldType.BOOLEAN, | ||
| this.taskAllowHeaderTableMode); | ||
| putDataSetEntry(dataSet, MenuItemConstants.PREFERENCE_ITEM_FIELD_USE_TASK_DEFAULT_HEADERS, FieldType.BOOLEAN, | ||
| this.taskUseDefaultHeaders); | ||
| putDataSetEntry(dataSet, MenuItemConstants.PREFERENCE_ITEM_FIELD_TASK_DEFAULT_HEADERS, FieldType.TEXT, | ||
| this.taskDefaultHeaders != null ? String.join(",", this.taskDefaultHeaders) : null); | ||
| putDataSetEntry(dataSet, MenuItemConstants.PREFERENCE_ITEM_FIELD_TASK_SHOW_MORE_MENU, FieldType.BOOLEAN, | ||
| this.taskShowMoreMenu); | ||
|
|
||
| return dataSet; | ||
| return outcome; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider adding validation for required fields in toDataSet.
While the method handles optional fields well, it doesn't validate that required fields (like menuName) are not null.
Add validation at the beginning of the main toDataSet method:
public ToDataSetOutcome toDataSet(String parentId, String nodePath, Case viewCase) {
+ if (this.menuName == null || this.menuName.getDefaultValue() == null || this.menuName.getDefaultValue().isEmpty()) {
+ throw new IllegalStateException("Menu name is required but not set");
+ }
ToDataSetOutcome outcome = new ToDataSetOutcome();Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemBody.java
between lines 148 and 184, the toDataSet method lacks validation for required
fields such as menuName. Add validation at the start of the method to check if
menuName (and any other required fields) is null, and if so, throw an
appropriate exception or handle the error to prevent proceeding with invalid
data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 35
🔭 Outside diff range comments (3)
application-engine/src/test/groovy/com/netgrif/application/engine/action/MenuItemApiTest.groovy (1)
202-205: Update to use new MenuItemConstants.The code still references old
PREFERENCE_ITEM_FIELD_*constants. These should be updated to use the newFIELD_*constants for consistency.- Case folderCase = findCasesElastic("processIdentifier:$FilterRunner.PREFERRED_ITEM_NET_IDENTIFIER AND dataSet.${MenuItemConstants.PREFERENCE_ITEM_FIELD_NODE_PATH.attributeId}.textValue:\"/netgrif2\"", PageRequest.of(0, 1))[0] + Case folderCase = findCasesElastic("processIdentifier:$FilterRunner.MENU_NET_IDENTIFIER AND dataSet.${MenuItemConstants.FIELD_NODE_PATH}.textValue:\"/netgrif2\"", PageRequest.of(0, 1))[0] - ArrayList<String> childIds = folderCase.dataSet[MenuItemConstants.PREFERENCE_ITEM_FIELD_CHILD_ITEM_IDS.attributeId].value as ArrayList<String> + ArrayList<String> childIds = folderCase.dataSet[MenuItemConstants.FIELD_CHILD_ITEM_IDS].value as ArrayList<String>application-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy (1)
2412-2421: Type inconsistency in role collection method.The ProcessRole version returns plain strings in the map values, but the method signature indicates it should return
Map<String, I18nString>. This will cause a ClassCastException at runtime.Apply this fix to properly return I18nString objects:
Map<String, I18nString> collectRolesForPreferenceItem(List<ProcessRole> roles) { return roles.collectEntries { ProcessRole role -> if (role.isGlobal()) { - return [(role.importId + ":" + GLOBAL_ROLE), ("$role.name (🌍 Global role)" as String)] + return [(role.importId + ":" + GLOBAL_ROLE), new I18nString("$role.name (🌍 Global role)" as String)] } else { PetriNet net = petriNetService.get(new ObjectId(role.netId)) - return [(role.importId + ":" + net.identifier), ("$role.name ($net.title)" as String)] + return [(role.importId + ":" + net.identifier), new I18nString("$role.name ($net.title)" as String)] } } as Map<String, I18nString> }nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticPetriNet.java (1)
51-58: Inconsistent title assignment in update method.The
updatemethod assigns the title directly from the parameter without using thetransformToFieldmethod, unlike the constructor. This inconsistency could lead to type mismatches since the constructor expectsI18nStringbut transforms it toI18nField, whileupdateexpects the parameter to already beI18nField.Consider applying this fix to ensure consistency:
public void update(ElasticPetriNet net) { this.version = net.getVersion(); if (net.getUriNodeId() != null) { this.uriNodeId = net.getUriNodeId(); } - this.title = net.getTitle(); + // Note: Title is already I18nField type in the parameter + this.title = net.getTitle(); this.initials = net.getInitials(); }Or if the parameter should contain
I18nString, the method signature and implementation need to be updated accordingly.
🧹 Nitpick comments (26)
application-engine/src/main/resources/application.yaml (1)
161-162: Prefer list syntax foractions.importsto aid readability & diff-friendlinessCurrent single-line value is valid but hard to maintain when classes change.
- actions: - imports: com.netgrif.application.engine.objects.workflow.domain.menu.MenuItemBody,com.netgrif.application.engine.objects.petrinet.domain.I18nString,com.netgrif.application.engine.objects.workflow.domain.menu.dashboard.DashboardManagementBody + actions: + imports: + - com.netgrif.application.engine.objects.workflow.domain.menu.MenuItemBody + - com.netgrif.application.engine.objects.petrinet.domain.I18nString + - com.netgrif.application.engine.objects.workflow.domain.menu.dashboard.DashboardManagementBodyAlso verify that another
netgrif.engine.actions.importsproperty is not defined elsewhere to avoid accidental override.application-engine/CHANGELOG.md (1)
8-21: Duplicate/incorrect “Full Changelog” linkLine 21 still points to
v7.0.0inside the new 6.5.0 section; readers may be confused.- - Full Changelog: https://github.com/netgrif/application-engine/commits/v7.0.0 + - Full Changelog: https://github.com/netgrif/application-engine/commits/v6.5.0nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticPetriNet.java (2)
15-16: Remove unused imports.The imports for
HashSetandSetare not used in this class.-import java.util.HashSet; -import java.util.Set;
15-16: Remove unused imports.The imports for
HashSetandSetare not used in this class and should be removed to keep the code clean.-import java.util.HashSet; -import java.util.Set;nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/dashboard/DashboardToDataSetOutcome.java (1)
21-34: LGTM! Good null safety and structure, but consider removing redundant initialization.The method handles null options properly and uses appropriate data structures. However, there's a redundant operation on lines 25-27.
fieldMap.put("type", fieldType.getName()); - if (fieldType.equals(FieldType.MULTICHOICE_MAP)) { - fieldMap.put("value", Set.of()); - } if (options == null) { options = new HashMap<>(); } fieldMap.put("options", options); fieldMap.put("value", fieldValue);The empty Set initialization is immediately overwritten by
fieldValueon line 32, making it redundant. If you need special handling for MULTICHOICE_MAP whenfieldValueis null, consider:fieldMap.put("options", options); fieldMap.put("value", fieldType.equals(FieldType.MULTICHOICE_MAP) && fieldValue == null ? Set.of() : fieldValue);application-engine/src/main/groovy/com/netgrif/application/engine/startup/DefaultDashboardRunner.groovy (1)
28-33: Consider making helper method private and handling return value.The
createMainDashboardManagementItem()method is only used internally and its return value is unused in therun()method.Apply this diff to improve encapsulation:
- def createMainDashboardManagementItem() { + private def createMainDashboardManagementItem() { def dashboardItemBody = new DashboardManagementBody("main_dashboard", new I18nString("Main Dashboard",Map.of("sk","Hlavný Dashboard","de","Haupt-Dashboard"))) dashboardItemBody.setLogoutDashboard(true) - return dashboardManagementService.createDashboardManagement(dashboardItemBody) + dashboardManagementService.createDashboardManagement(dashboardItemBody) }nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/dashboard/DashboardItemConstants.java (2)
4-4: Fix the comment to reference the correct process.The comment mentions "dashboard_management.xml" but should reference "dashboard_item.xml" based on the
PROCESS_IDENTIFIERconstant.-/** - * Here are declared constants of process dashboard_management.xml. - */ +/** + * Here are declared constants of process dashboard_item.xml. + */
9-9: Add space for formatting consistency.Missing space after the equals sign for consistency with other field declarations.
- public static final String FIELD_ID= "item_id"; + public static final String FIELD_ID = "item_id";nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/FilterBody.java (1)
50-50: Fix Javadoc reference.The Javadoc comment references
IDataServicebut the class is not imported. Either add the import or use the fully qualified class name in the Javadoc.- * Transforms attributes into dataSet for {@link IDataService#setData} + * Transforms attributes into dataSet for IDataService#setDatanae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/filter/FilterConstants.java (1)
3-3: Remove unused Lombok import.The
@Getterannotation from Lombok is imported but not used in this constants class.- import lombok.Getter;nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/ToDataSetOutcome.java (1)
44-55: Consider making MULTICHOICE_MAP handling more extensible.The special case handling for MULTICHOICE_MAP field type is hardcoded. Consider making this more configurable or document why this specific type requires empty set initialization.
public void putDataSetEntryOptions(String fieldId, FieldType fieldType, @Nullable Map<String, I18nString> options) { + if (fieldId == null || fieldId.trim().isEmpty()) { + throw new IllegalArgumentException("fieldId cannot be null or empty"); + } Map<String, Object> fieldMap = new LinkedHashMap<>(); fieldMap.put("type", fieldType.getName()); if (fieldType.equals(FieldType.MULTICHOICE_MAP)) { + // Initialize with empty set for MULTICHOICE_MAP to ensure proper UI behavior fieldMap.put("value", Set.of()); } if (options == null) { options = new HashMap<>(); } fieldMap.put("options", options); this.dataSet.put(fieldId, fieldMap); }application-engine/src/main/java/com/netgrif/application/engine/menu/services/DashboardItemServiceImpl.java (1)
100-103: Remove unused method.The
setDatamethod is not used anywhere in this class and appears to be dead code.Consider removing this method if it's not needed, or document its intended use if it's meant for future functionality.
nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemView.java (1)
52-59: Improve exception message for better debugging.The IllegalArgumentException should include a descriptive message.
public static MenuItemView fromIdentifier(String identifier) { for (MenuItemView view : MenuItemView.values()) { if (view.identifier.equals(identifier)) { return view; } } - throw new IllegalArgumentException(identifier); + throw new IllegalArgumentException("Unknown MenuItemView identifier: " + identifier); }nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/configurations/ViewBody.java (1)
54-56: Remove redundant semicolon.public ToDataSetOutcome toDataSet() { return toDataSet(null, null); -}; +}application-engine/src/main/java/com/netgrif/application/engine/menu/services/interfaces/IMenuItemService.java (1)
16-40: Add JavaDoc documentation for interface methods.The interface methods lack documentation. Consider adding JavaDoc to describe the purpose, parameters, return values, and exception conditions for each method.
Example for one method:
/** * Creates a new filter case based on the provided filter body. * * @param body the filter configuration containing all necessary data * @return the created filter case * @throws TransitionNotExecutableException if the filter creation workflow cannot be executed */ Case createFilter(FilterBody body) throws TransitionNotExecutableException;nae-object-library/src/main/java/com/netgrif/application/engine/objects/utils/MenuItemUtils.java (1)
73-79: Avoid catching NullPointerException.Catching
NullPointerExceptionis generally considered bad practice. Consider explicit null checking instead.public static List<String> getCaseIdsFromCaseRef(Case useCase, String caseRefId) { - try { - return (List<String>) useCase.getFieldValue(caseRefId); - } catch (NullPointerException ignore) { - return null; - } + if (useCase == null || caseRefId == null) { + return null; + } + Object fieldValue = useCase.getFieldValue(caseRefId); + return fieldValue instanceof List ? (List<String>) fieldValue : null; }application-engine/src/main/java/com/netgrif/application/engine/menu/services/DashboardManagementServiceImpl.java (1)
40-45: Consider using constructor injection over field injectionThe class uses field injection with
@RequiredArgsConstructor. While this works with Lombok, constructor injection is generally preferred as it makes dependencies explicit and facilitates testing.application-engine/src/main/resources/petriNets/engine-processes/menu/tabbed_single_task_view_configuration.xml (1)
42-67: Improve null check consistency in filter functionThe null check on line 59 uses both
!=for null comparison. Consider using a more consistent and explicit approach.- return findFilters(filterAutocomplete.value != null ? filterAutocomplete.value : "") + return findFilters(filterAutocomplete.value ?: "")nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemBody.java (1)
39-87: Consider using Builder pattern instead of multiple constructorsThe class has 6 constructors with different parameter combinations, which can be confusing for API users. Consider implementing a Builder pattern for better maintainability and clearer object construction.
Would you like me to generate a Builder implementation for this class?
application-engine/src/main/java/com/netgrif/application/engine/menu/services/MenuItemService.java (1)
47-48: Consider moving DEFAULT_FOLDER_ICON to MenuItemConstants.For better organization and consistency with other constants in the codebase, consider moving this static field to the
MenuItemConstantsclass.application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticPetriNetService.java (2)
92-124: Consider performance impact of double-fetch pattern.The method first searches in Elasticsearch then fetches full PetriNet objects by ID. While this ensures data consistency, it may impact performance for large result sets. Consider caching or returning lightweight references if full objects aren't always needed.
168-168: Wildcard queries may impact performance.Using leading wildcards (
*searchText*) can be very inefficient in Elasticsearch as it prevents the use of indexes effectively. Consider using edge n-gram tokenizers or other text analysis strategies for better performance.application-engine/src/main/resources/petriNets/engine-processes/dashboard_management.xml (1)
109-110: Use more idiomatic Groovy for map population.The forEach loops can be replaced with more idiomatic Groovy code.
- keysDashboard.forEach { newOrderDashboard[it] = dashboard_item_list.options[it]} - keysMenu.forEach { newOrderMenu[it] = dashboard_item_to_menu_item.options[it]} + keysDashboard.each { key -> newOrderDashboard[key] = dashboard_item_list.options[key] } + keysMenu.each { key -> newOrderMenu[key] = dashboard_item_to_menu_item.options[key] }Also applies to: 147-148
application-engine/src/main/resources/petriNets/engine-processes/menu/tabbed_case_view_configuration.xml (1)
418-427: Redundant i18n key – potential translation mismatchBoth
use_default_headersanduse_case_default_headersappear in the Slovak
bundle, but only the latter corresponds to an existing data field
(id="use_case_default_headers"). Keeping unused keys adds noise and increases
maintenance overhead. Please remove the superfluous entry or confirm it is
indeed required elsewhere.application-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy (1)
1723-1827: Legacy createMenuItem methods properly deprecated and refactored.The deprecated methods correctly delegate to the new service while maintaining backward compatibility. Consider adding deprecation messages with migration guidance to help users transition to the new API patterns.
application-engine/src/main/java/com/netgrif/application/engine/menu/services/interfaces/DashboardItemService.java (1)
7-14: Add JavaDoc documentation for better API clarity.The interface methods would benefit from JavaDoc documentation to clarify their behavior, parameters, return values, and exceptions.
+/** + * Service interface for managing dashboard item cases. + */ public interface DashboardItemService { + /** + * Gets an existing dashboard item case or creates a new one if it doesn't exist. + * + * @param body the dashboard item body containing configuration + * @return the existing or newly created case + * @throws TransitionNotExecutableException if case creation/update fails + */ Case getOrCreate(DashboardItemBody body) throws TransitionNotExecutableException; + /** + * Updates an existing dashboard item case with new configuration. + * + * @param itemCase the existing case to update + * @param body the new configuration + * @return the updated case + * @throws TransitionNotExecutableException if the update fails + */ Case update(Case itemCase, DashboardItemBody body) throws TransitionNotExecutableException; + /** + * Finds a dashboard item case by its identifier. + * + * @param identifier the case identifier + * @return the found case or null if not found + */ Case findById(String identifier); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (62)
application-engine/CHANGELOG.md(1 hunks)application-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy(40 hunks)application-engine/src/main/groovy/com/netgrif/application/engine/startup/DashboardManagementRunner.groovy(1 hunks)application-engine/src/main/groovy/com/netgrif/application/engine/startup/DefaultDashboardRunner.groovy(1 hunks)application-engine/src/main/groovy/com/netgrif/application/engine/startup/ImportHelper.groovy(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticPetriNetService.java(3 hunks)application-engine/src/main/java/com/netgrif/application/engine/elastic/service/interfaces/IElasticPetriNetService.java(2 hunks)application-engine/src/main/java/com/netgrif/application/engine/menu/services/DashboardItemServiceImpl.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/menu/services/DashboardManagementServiceImpl.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/menu/services/MenuItemService.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/menu/services/interfaces/DashboardItemService.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/menu/services/interfaces/DashboardManagementService.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/menu/services/interfaces/IMenuItemService.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/petrinet/web/PetriNetController.java(5 hunks)application-engine/src/main/java/com/netgrif/application/engine/petrinet/web/responsebodies/PetriNetReferenceResourceAssembler.java(0 hunks)application-engine/src/main/java/com/netgrif/application/engine/startup/runner/DashboardRunner.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/startup/runner/FilterRunner.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/domain/menu/Menu.java(0 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/domain/menu/MenuAndFilters.java(0 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/domain/menu/MenuEntry.java(0 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/domain/menu/MenuEntryRole.java(0 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/domain/menu/MenuItemBody.java(0 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/domain/menu/MenuItemConstants.java(0 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/domain/menu/MenuList.java(0 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/web/WorkflowController.java(1 hunks)application-engine/src/main/resources/application.yaml(1 hunks)application-engine/src/main/resources/petriNets/engine-processes/dashboard_item.xml(1 hunks)application-engine/src/main/resources/petriNets/engine-processes/dashboard_management.xml(1 hunks)application-engine/src/main/resources/petriNets/engine-processes/menu/menu_item.xml(1 hunks)application-engine/src/main/resources/petriNets/engine-processes/menu/tabbed_case_view_configuration.xml(1 hunks)application-engine/src/main/resources/petriNets/engine-processes/menu/tabbed_single_task_view_configuration.xml(1 hunks)application-engine/src/main/resources/petriNets/engine-processes/menu/tabbed_task_view_configuration.xml(1 hunks)application-engine/src/main/resources/petriNets/engine-processes/menu/tabbed_ticket_view_configuration.xml(1 hunks)application-engine/src/main/resources/petriNets/engine-processes/preference_filter_item.xml(1 hunks)application-engine/src/main/resources/petriNets/engine-processes/preference_item.xml(0 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/action/MenuItemApiTest.groovy(7 hunks)application-engine/src/test/resources/dashboard_management_test.xml(1 hunks)application-engine/src/test/resources/petriNets/filter_api_test.xml(1 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticPetriNet.java(4 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/utils/MenuItemUtils.java(1 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/filter/FilterConstants.java(1 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/FilterBody.java(1 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemBody.java(3 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java(1 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemView.java(1 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/ToDataSetOutcome.java(1 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/configurations/TabbedCaseViewBody.java(1 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/configurations/TabbedCaseViewConstants.java(1 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/configurations/TabbedSingleTaskViewBody.java(1 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/configurations/TabbedSingleTaskViewConstants.java(1 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/configurations/TabbedTaskViewBody.java(1 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/configurations/TabbedTaskViewConstants.java(1 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/configurations/TabbedTicketViewBody.java(1 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/configurations/TabbedTicketViewConstants.java(1 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/configurations/ViewBody.java(1 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/configurations/ViewConstants.java(1 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/dashboard/DashboardItemBody.java(1 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/dashboard/DashboardItemConstants.java(1 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/dashboard/DashboardManagementBody.java(1 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/dashboard/DashboardManagementConstants.java(1 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/dashboard/DashboardToDataSetOutcome.java(1 hunks)nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/ElasticPetriNet.java(3 hunks)
💤 Files with no reviewable changes (9)
- application-engine/src/main/java/com/netgrif/application/engine/workflow/domain/menu/MenuAndFilters.java
- application-engine/src/main/java/com/netgrif/application/engine/workflow/domain/menu/Menu.java
- application-engine/src/main/java/com/netgrif/application/engine/workflow/domain/menu/MenuEntryRole.java
- application-engine/src/main/java/com/netgrif/application/engine/workflow/domain/menu/MenuEntry.java
- application-engine/src/main/java/com/netgrif/application/engine/workflow/domain/menu/MenuList.java
- application-engine/src/main/resources/petriNets/engine-processes/preference_item.xml
- application-engine/src/main/java/com/netgrif/application/engine/workflow/domain/menu/MenuItemConstants.java
- application-engine/src/main/java/com/netgrif/application/engine/workflow/domain/menu/MenuItemBody.java
- application-engine/src/main/java/com/netgrif/application/engine/petrinet/web/responsebodies/PetriNetReferenceResourceAssembler.java
🧰 Additional context used
🧠 Learnings (26)
📓 Common learnings
Learnt from: renczesstefan
PR: netgrif/application-engine#318
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java:60-62
Timestamp: 2025-06-23T13:30:13.096Z
Learning: In MenuItemConstants enum in nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java, the field `attributeId` will be renamed to `value` to make it more generic and appropriate for both dataset attribute identifiers and technical constants like PATH_SEPARATOR.
nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/configurations/TabbedTaskViewConstants.java (1)
Learnt from: renczesstefan
PR: #318
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java:60-62
Timestamp: 2025-06-23T13:30:13.096Z
Learning: In MenuItemConstants enum in nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java, the field attributeId will be renamed to value to make it more generic and appropriate for both dataset attribute identifiers and technical constants like PATH_SEPARATOR.
nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/configurations/TabbedTicketViewConstants.java (1)
Learnt from: renczesstefan
PR: #318
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java:60-62
Timestamp: 2025-06-23T13:30:13.096Z
Learning: In MenuItemConstants enum in nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java, the field attributeId will be renamed to value to make it more generic and appropriate for both dataset attribute identifiers and technical constants like PATH_SEPARATOR.
application-engine/src/main/resources/application.yaml (1)
Learnt from: renczesstefan
PR: #318
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java:60-62
Timestamp: 2025-06-23T13:30:13.096Z
Learning: In MenuItemConstants enum in nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java, the field attributeId will be renamed to value to make it more generic and appropriate for both dataset attribute identifiers and technical constants like PATH_SEPARATOR.
nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/configurations/ViewConstants.java (1)
Learnt from: renczesstefan
PR: #318
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java:60-62
Timestamp: 2025-06-23T13:30:13.096Z
Learning: In MenuItemConstants enum in nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java, the field attributeId will be renamed to value to make it more generic and appropriate for both dataset attribute identifiers and technical constants like PATH_SEPARATOR.
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticPetriNet.java (1)
Learnt from: renczesstefan
PR: #318
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java:60-62
Timestamp: 2025-06-23T13:30:13.096Z
Learning: In MenuItemConstants enum in nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java, the field attributeId will be renamed to value to make it more generic and appropriate for both dataset attribute identifiers and technical constants like PATH_SEPARATOR.
nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/dashboard/DashboardItemConstants.java (1)
Learnt from: renczesstefan
PR: #318
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java:60-62
Timestamp: 2025-06-23T13:30:13.096Z
Learning: In MenuItemConstants enum in nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java, the field attributeId will be renamed to value to make it more generic and appropriate for both dataset attribute identifiers and technical constants like PATH_SEPARATOR.
nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/dashboard/DashboardItemBody.java (1)
Learnt from: renczesstefan
PR: #318
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java:60-62
Timestamp: 2025-06-23T13:30:13.096Z
Learning: In MenuItemConstants enum in nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java, the field attributeId will be renamed to value to make it more generic and appropriate for both dataset attribute identifiers and technical constants like PATH_SEPARATOR.
nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/configurations/TabbedTaskViewBody.java (1)
Learnt from: renczesstefan
PR: #318
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java:60-62
Timestamp: 2025-06-23T13:30:13.096Z
Learning: In MenuItemConstants enum in nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java, the field attributeId will be renamed to value to make it more generic and appropriate for both dataset attribute identifiers and technical constants like PATH_SEPARATOR.
nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/filter/FilterConstants.java (1)
Learnt from: renczesstefan
PR: #318
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java:60-62
Timestamp: 2025-06-23T13:30:13.096Z
Learning: In MenuItemConstants enum in nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java, the field attributeId will be renamed to value to make it more generic and appropriate for both dataset attribute identifiers and technical constants like PATH_SEPARATOR.
nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/dashboard/DashboardManagementConstants.java (1)
Learnt from: renczesstefan
PR: #318
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java:60-62
Timestamp: 2025-06-23T13:30:13.096Z
Learning: In MenuItemConstants enum in nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java, the field attributeId will be renamed to value to make it more generic and appropriate for both dataset attribute identifiers and technical constants like PATH_SEPARATOR.
nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemBody.java (1)
Learnt from: renczesstefan
PR: #318
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java:60-62
Timestamp: 2025-06-23T13:30:13.096Z
Learning: In MenuItemConstants enum in nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java, the field attributeId will be renamed to value to make it more generic and appropriate for both dataset attribute identifiers and technical constants like PATH_SEPARATOR.
nae-object-library/src/main/java/com/netgrif/application/engine/objects/utils/MenuItemUtils.java (1)
Learnt from: renczesstefan
PR: #318
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java:60-62
Timestamp: 2025-06-23T13:30:13.096Z
Learning: In MenuItemConstants enum in nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java, the field attributeId will be renamed to value to make it more generic and appropriate for both dataset attribute identifiers and technical constants like PATH_SEPARATOR.
nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemView.java (1)
Learnt from: renczesstefan
PR: #318
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java:60-62
Timestamp: 2025-06-23T13:30:13.096Z
Learning: In MenuItemConstants enum in nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java, the field attributeId will be renamed to value to make it more generic and appropriate for both dataset attribute identifiers and technical constants like PATH_SEPARATOR.
application-engine/src/test/resources/petriNets/filter_api_test.xml (1)
Learnt from: renczesstefan
PR: #318
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java:60-62
Timestamp: 2025-06-23T13:30:13.096Z
Learning: In MenuItemConstants enum in nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java, the field attributeId will be renamed to value to make it more generic and appropriate for both dataset attribute identifiers and technical constants like PATH_SEPARATOR.
application-engine/src/test/groovy/com/netgrif/application/engine/action/MenuItemApiTest.groovy (1)
Learnt from: renczesstefan
PR: #318
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java:60-62
Timestamp: 2025-06-23T13:30:13.096Z
Learning: In MenuItemConstants enum in nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java, the field attributeId will be renamed to value to make it more generic and appropriate for both dataset attribute identifiers and technical constants like PATH_SEPARATOR.
application-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy (1)
Learnt from: renczesstefan
PR: #318
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java:60-62
Timestamp: 2025-06-23T13:30:13.096Z
Learning: In MenuItemConstants enum in nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java, the field attributeId will be renamed to value to make it more generic and appropriate for both dataset attribute identifiers and technical constants like PATH_SEPARATOR.
nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/dashboard/DashboardToDataSetOutcome.java (1)
Learnt from: renczesstefan
PR: #318
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java:60-62
Timestamp: 2025-06-23T13:30:13.096Z
Learning: In MenuItemConstants enum in nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java, the field attributeId will be renamed to value to make it more generic and appropriate for both dataset attribute identifiers and technical constants like PATH_SEPARATOR.
nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/configurations/TabbedSingleTaskViewBody.java (1)
Learnt from: renczesstefan
PR: #318
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java:60-62
Timestamp: 2025-06-23T13:30:13.096Z
Learning: In MenuItemConstants enum in nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java, the field attributeId will be renamed to value to make it more generic and appropriate for both dataset attribute identifiers and technical constants like PATH_SEPARATOR.
application-engine/src/main/java/com/netgrif/application/engine/menu/services/MenuItemService.java (1)
Learnt from: renczesstefan
PR: #318
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java:60-62
Timestamp: 2025-06-23T13:30:13.096Z
Learning: In MenuItemConstants enum in nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java, the field attributeId will be renamed to value to make it more generic and appropriate for both dataset attribute identifiers and technical constants like PATH_SEPARATOR.
application-engine/src/main/resources/petriNets/engine-processes/preference_filter_item.xml (1)
Learnt from: renczesstefan
PR: #318
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java:60-62
Timestamp: 2025-06-23T13:30:13.096Z
Learning: In MenuItemConstants enum in nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java, the field attributeId will be renamed to value to make it more generic and appropriate for both dataset attribute identifiers and technical constants like PATH_SEPARATOR.
nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/configurations/TabbedSingleTaskViewConstants.java (1)
Learnt from: renczesstefan
PR: #318
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java:60-62
Timestamp: 2025-06-23T13:30:13.096Z
Learning: In MenuItemConstants enum in nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java, the field attributeId will be renamed to value to make it more generic and appropriate for both dataset attribute identifiers and technical constants like PATH_SEPARATOR.
nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java (1)
Learnt from: renczesstefan
PR: #318
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java:60-62
Timestamp: 2025-06-23T13:30:13.096Z
Learning: In MenuItemConstants enum in nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java, the field attributeId will be renamed to value to make it more generic and appropriate for both dataset attribute identifiers and technical constants like PATH_SEPARATOR.
nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/configurations/TabbedCaseViewConstants.java (1)
Learnt from: renczesstefan
PR: #318
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java:60-62
Timestamp: 2025-06-23T13:30:13.096Z
Learning: In MenuItemConstants enum in nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java, the field attributeId will be renamed to value to make it more generic and appropriate for both dataset attribute identifiers and technical constants like PATH_SEPARATOR.
application-engine/src/main/java/com/netgrif/application/engine/menu/services/interfaces/IMenuItemService.java (1)
Learnt from: renczesstefan
PR: #318
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java:60-62
Timestamp: 2025-06-23T13:30:13.096Z
Learning: In MenuItemConstants enum in nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java, the field attributeId will be renamed to value to make it more generic and appropriate for both dataset attribute identifiers and technical constants like PATH_SEPARATOR.
application-engine/src/main/resources/petriNets/engine-processes/menu/menu_item.xml (1)
Learnt from: renczesstefan
PR: #318
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java:60-62
Timestamp: 2025-06-23T13:30:13.096Z
Learning: In MenuItemConstants enum in nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java, the field attributeId will be renamed to value to make it more generic and appropriate for both dataset attribute identifiers and technical constants like PATH_SEPARATOR.
🪛 GitHub Actions: Pull Request
nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/dashboard/DashboardManagementBody.java
[warning] 42-54: @builder will ignore the initializing expression entirely. Add @Builder.Default or make the field final to fix this.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
🔇 Additional comments (71)
application-engine/src/main/java/com/netgrif/application/engine/workflow/web/WorkflowController.java (1)
122-124: No-op change – looks goodOnly an extra blank line was introduced; there is no impact on behaviour or style consistency.
nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/configurations/TabbedTicketViewConstants.java (1)
1-5: Empty constants class – is it required?
TabbedTicketViewConstantsadds no fields beyond its parent. If nothing is planned shortly, consider removing it to avoid dead code.nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/configurations/TabbedSingleTaskViewConstants.java (1)
4-6: Constant definition is concise and correctThe field name and casing are consistent with other constants classes in the module.
nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/ElasticPetriNet.java (1)
67-73: LGTM! Well-implemented transformation method.The
transformToFieldmethod correctly transforms anI18nStringto anI18nFieldby properly extracting keys, values, and translations while ensuring encapsulation through new collection instances.application-engine/src/main/resources/petriNets/engine-processes/preference_filter_item.xml (1)
419-419: LGTM! Correct adaptation to updated object model.The change from
it._id.eq(...)toit._id.objectId.eq(...)properly aligns with the updated object model where_idnow contains anobjectIdproperty. This ensures correct case matching in the filter initialization logic.application-engine/src/main/java/com/netgrif/application/engine/elastic/service/interfaces/IElasticPetriNetService.java (1)
22-22: LGTM! Well-designed search interface.The new
searchmethod provides a comprehensive interface for Petri net searching with proper parameters for criteria, user context, pagination, locale, and intersection logic. The return typePage<PetriNetReference>follows Spring Data conventions for paginated results.application-engine/src/main/groovy/com/netgrif/application/engine/startup/ImportHelper.groovy (1)
249-263: LGTM! Excellent centralization of import logic.The new
importProcessmethod provides a clean, reusable approach to Petri net importing with proper existence checking, error handling, and logging. This centralization will improve consistency across startup runners and reduce code duplication.nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/configurations/ViewConstants.java (2)
1-16: LGTM! Well-structured constants class following good practices.The constants are well-organized with clear naming conventions and serve as a centralized container for menu item configuration values. The separation between field constants (FIELD_) and transition constants (TRANS_) provides good logical grouping.
1-16: LGTM! Well-structured constants class.The implementation follows Java best practices for constants classes with clear naming conventions and proper documentation. The logical grouping of field constants (lines 7-11) and transition constants (lines 13-15) makes the class easy to maintain and understand.
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticPetriNet.java (2)
60-60: LGTM! Good abstraction for type transformation.The abstract method enforces subclasses to provide their own transformation logic from
I18nStringtoI18nField, which maintains flexibility while ensuring type safety.
60-60: Good abstraction design.The abstract method
transformToFieldproperly enforces subclasses to provide the transformation logic fromI18nStringtoI18nField, which aligns well with the refactoring goals.nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/configurations/TabbedCaseViewBody.java (3)
13-31: LGTM! Well-structured configuration class with sensible defaults.The class design follows good practices with proper use of Lombok annotations, inheritance, and composition via the chainedView property. The default values are reasonable for a tabbed case view configuration.
44-74: Excellent dataset transformation implementation.The
toDataSetInternalmethod properly handles the conversion of all configuration properties to dataset entries with appropriate field types. The use of constants and null-safe operations shows good defensive programming practices.
16-41: Well-designed view configuration class.The class follows good object-oriented principles with:
- Appropriate use of inheritance from
ViewBody- Sensible default values for configuration properties
- Clean implementation of abstract methods
- Support for view chaining through
chainedViewpropertyapplication-engine/src/main/java/com/netgrif/application/engine/menu/services/interfaces/DashboardItemService.java (2)
7-14: LGTM! Clean and well-designed service interface.The interface follows good practices with:
- Clear method naming that indicates purpose
- Consistent use of
DashboardItemBodyas the data transfer object- Appropriate exception handling for workflow operations
- Standard service pattern implementation
The method signatures are logical and align well with the service layer architecture for dashboard management.
1-6: Well-structured service interface.The interface follows good service layer design principles with clear method signatures, appropriate use of domain objects, and proper exception handling.
nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/configurations/TabbedTaskViewConstants.java (1)
1-16: LGTM! Well-structured constants class.The implementation follows Java best practices with proper constant declarations, consistent naming conventions, and clear documentation referencing the associated process file.
application-engine/src/main/groovy/com/netgrif/application/engine/startup/DashboardManagementRunner.groovy (1)
10-14: Good use of conditional startup runner.The implementation properly uses
@ConditionalOnPropertywithmatchIfMissing = trueto allow the feature to be disabled while defaulting to enabled. The runner order ensures proper sequencing during startup.nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/configurations/TabbedSingleTaskViewBody.java (1)
10-31: LGTM! Clean implementation following established patterns.The class properly extends
ViewBodywith appropriate Lombok annotations and method overrides. The implementation is consistent with the view configuration pattern used throughout the codebase.application-engine/src/main/java/com/netgrif/application/engine/menu/services/interfaces/DashboardManagementService.java (1)
7-14: LGTM! Well-designed service interface.The interface follows clean architecture principles with clear method signatures, proper exception handling, and focused responsibility for dashboard management operations.
nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/configurations/TabbedTicketViewBody.java (1)
1-30: LGTM! Clean implementation following established patterns.The
TabbedTicketViewBodyclass properly extendsViewBodyand implements the required abstract methods correctly. The use of Lombok annotations is appropriate, and the chained view functionality aligns with the broader menu item view configuration architecture.nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/dashboard/DashboardManagementConstants.java (1)
1-20: LGTM! Well-structured constants class.The constants are well-organized and follow consistent naming conventions. The class provides a centralized reference for dashboard management process identifiers and field names, which will ensure consistency across the codebase.
application-engine/src/test/resources/dashboard_management_test.xml (1)
1-55: LGTM! Comprehensive integration test for dashboard management.The test resource effectively exercises the new dashboard management functionality, including:
- Filter and menu item creation with chained view configuration
- Dashboard item creation and association
- Dashboard management case updates
The Groovy script demonstrates proper usage of the new domain classes and service methods introduced in the refactoring.
application-engine/src/main/java/com/netgrif/application/engine/petrinet/web/PetriNetController.java (1)
73-75: LGTM! Proper dependency injection.The new
IElasticPetriNetServicedependency is properly injected and will be used by the new elastic search endpoint.nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/configurations/TabbedCaseViewConstants.java (1)
1-22: LGTM! Well-structured constants class.The constants are well-organized with consistent naming conventions and comprehensive coverage of tabbed case view configuration fields. The documentation clearly explains the purpose, and the structure aligns with the broader refactoring effort.
nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/FilterBody.java (1)
54-72: LGTM! Solid data transformation implementation.The
toDataSet()method properly maps all fields to their corresponding dataset entries with appropriate field types. The null metadata handling with fallback to defaults is well-implemented.nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/dashboard/DashboardItemBody.java (1)
37-53: Well-structured data transformation method.The
toDataSet()method provides comprehensive mapping of all dashboard item properties to dataset entries. The implementation is consistent and follows the established pattern.nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/filter/FilterConstants.java (1)
5-22: LGTM! Comprehensive filter constants.The constants are well-organized and provide comprehensive coverage of filter-related identifiers, transitions, and configuration values. The naming convention is consistent and the values are appropriate for their intended usage.
nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/configurations/TabbedTaskViewBody.java (2)
29-32: Verify the null return in getAssociatedViewBody().The method returns
nullwhich may be intentional, but please verify this is the correct behavior for tabbed task views, especially since other view body implementations might have associated views.
40-62: LGTM! Solid data transformation implementation.The
toDataSetInternal()method properly maps all configuration fields to dataset entries with appropriate field types. The null handling for collections (lines 47, 57) is well-implemented, ensuring safe data transformation.application-engine/src/main/java/com/netgrif/application/engine/startup/runner/DashboardRunner.java (1)
23-27: LGTM!Constants are properly defined with clear naming conventions and consistent file path patterns.
application-engine/src/test/groovy/com/netgrif/application/engine/action/MenuItemApiTest.groovy (4)
76-103: LGTM!The test correctly validates menu item creation with the new constants and properly asserts the tabbed view configuration, including the hierarchical relationship between tabbed case and task views.
127-169: LGTM!The test properly validates that view configurations are preserved during menu item updates, ensuring backward compatibility and data integrity.
265-296: LGTM!The test correctly validates menu item duplication with proper error handling and field validation using the new constants.
327-356: LGTM!Excellent test coverage for the deletion cascade, properly verifying that all related entities (menu items, tabbed case views, and tabbed task views) are deleted.
application-engine/src/main/java/com/netgrif/application/engine/startup/runner/FilterRunner.java (2)
60-74: LGTM!The dynamic import of configuration nets based on
MenuItemViewenum values is well-implemented with proper stream operations and error handling.
52-58: LGTM!The visibility change to package-private improves testability, and returning
Optional<PetriNet>provides better API design by making the nullable return explicit.application-engine/src/main/java/com/netgrif/application/engine/menu/services/interfaces/IMenuItemService.java (2)
14-41: LGTM!The interface provides a comprehensive API for menu item management with well-defined methods and appropriate exception handling.
50-70: LGTM!The default methods provide convenient helpers for view retrieval with proper filtering and transformation logic.
nae-object-library/src/main/java/com/netgrif/application/engine/objects/utils/MenuItemUtils.java (4)
19-28: LGTM!The sanitization method properly handles null input and correctly removes diacritical marks, special characters, and normalizes the string.
37-51: LGTM!The method correctly finds tasks by transition ID with proper null handling and defensive programming.
98-100: LGTM!Clear and concise helper method that properly delegates to the underlying utility method.
106-109: LGTM!The method correctly checks for folder children with proper null and empty list handling.
nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java (1)
7-39: LGTM! Well-structured constants classThe refactoring from enum to a constants class with static final strings is implemented correctly. This aligns with the previous learning about making the structure more generic and flexible for different use cases.
application-engine/src/main/resources/petriNets/engine-processes/menu/tabbed_single_task_view_configuration.xml (1)
226-278: Well-structured transition configurationThe settings transition is properly restricted to admin users with appropriate permissions (perform, view, cancel, assign). The UI layout using a grid system with material design templates is clean and maintainable.
nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemBody.java (1)
89-91: Avoid side effects in getter methodThe
getIdentifier()method callsMenuItemUtils.sanitize()which may modify the identifier. Getters should be pure functions without side effects. Consider sanitizing the identifier when it's set, not when it's retrieved.+private String sanitizedIdentifier; + +public void setIdentifier(String identifier) { + this.identifier = identifier; + this.sanitizedIdentifier = MenuItemUtils.sanitize(identifier); +} + public String getIdentifier() { - return MenuItemUtils.sanitize(this.identifier); + return this.sanitizedIdentifier != null ? this.sanitizedIdentifier : MenuItemUtils.sanitize(this.identifier); }⛔ Skipped due to learnings
Learnt from: renczesstefan PR: netgrif/application-engine#318 File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java:60-62 Timestamp: 2025-06-23T13:30:13.096Z Learning: In MenuItemConstants enum in nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java, the field `attributeId` will be renamed to `value` to make it more generic and appropriate for both dataset attribute identifiers and technical constants like PATH_SEPARATOR.application-engine/src/main/resources/petriNets/engine-processes/dashboard_item.xml (2)
701-730: Well-implemented validation logicThe finish event validation properly checks all required fields based on whether the item is internal or external, with clear error messages for each validation failure.
47-65: Improve robustness of string manipulation in delete handlerThe string split/join operations for
items_orderfield could be more robust with null checks and error handling.def itemsOrder = refCase.dataSet["items_order"].value.split(",") as ArrayList -itemsOrder.remove(useCase.stringId) +if (itemsOrder != null && !itemsOrder.isEmpty()) { + itemsOrder.remove(useCase.stringId) +} def dashboardItemsListOptions = refCase.dataSet["dashboard_item_list"].options -dashboardItemsListOptions.remove(useCase.stringId) +if (dashboardItemsListOptions != null) { + dashboardItemsListOptions.remove(useCase.stringId) +}Likely an incorrect or invalid review comment.
application-engine/src/main/java/com/netgrif/application/engine/menu/services/MenuItemService.java (3)
55-66: LGTM!The filter creation logic is well-structured with proper error handling and logging.
92-125: LGTM!Well-implemented menu item creation with proper validation, parent-child relationship handling, and optional view support.
712-715: LGTM!The unchecked warnings suppression is justified for these generic Map operations.
application-engine/src/main/resources/petriNets/engine-processes/menu/tabbed_task_view_configuration.xml (2)
42-68: LGTM!The filter autocomplete function correctly validates filter types and handles edge cases properly.
373-718: LGTM!The Petri net structure is well-defined with appropriate role restrictions and proper state transitions.
application-engine/src/main/resources/petriNets/engine-processes/menu/tabbed_ticket_view_configuration.xml (1)
158-163: LGTM!The filter type validation correctly ensures only Case-type filters are used for the ticket view.
application-engine/src/main/resources/petriNets/engine-processes/dashboard_management.xml (2)
40-55: LGTM!The dashboard ID uniqueness validation is properly implemented with checks at both set and finish events, ensuring data integrity.
Also applies to: 690-698
199-269: LGTM!The add/remove item logic properly maintains data consistency and handles both internal and external dashboard items correctly.
application-engine/src/main/resources/petriNets/engine-processes/menu/menu_item.xml (1)
296-307:menu_name_as_visibleuses enumeration but is mutated like enumeration_mapThe script calls
change menu_name_as_visible choices { … }, yet
menu_name_as_visibleis declared astype="enumeration". Standard PetriFlow
enumeration fields expose the propertyoptions, notchoices.If the engine treats this as an error it will silently ignore the update and
the menu entry will never display the translated title.Confirm the intended type:
-<data type="enumeration"> +<data type="enumeration_map">or adjust the script to
options.application-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy (13)
29-31: LGTM! Import additions align with the new service architecture.The new imports properly support the transition to a service-oriented approach for menu item, filter, and dashboard management. The domain model imports and utility classes are appropriately added for the refactored functionality.
Also applies to: 35-38, 47-47, 56-64, 74-74
205-212: Service dependencies properly injected.The autowired services follow Spring conventions and support the delegation pattern for menu item, filter, and dashboard management operations.
933-936: Convenient case lookup method added.Good addition of an intuitive findCase(String) method that improves API usability.
1617-1628: Filter creation properly refactored to use service delegation.The implementation correctly builds a FilterBody and delegates to the menu item service, maintaining backward compatibility while embracing the new architecture.
1879-1925: Menu item change operations properly updated for new constants structure.The refactoring correctly uses the new MenuItemConstants structure and maintains consistent delegation patterns. The removal of
.valuesuffix improves code readability.
2110-2207: Well-structured view configuration helper methods.The new static helper methods provide clean abstractions for creating different view configurations. They properly support the tabbed view architecture and chained view patterns.
2339-2361: Dashboard management methods properly implemented.The new dashboard-related methods follow consistent patterns and provide clean delegation to the respective services.
2242-2302: Menu item operations properly delegated to service layer.All menu item operations now consistently delegate to the menuItemService, improving separation of concerns and maintainability.
1970-2095: Filter creation methods properly migrated to use FilterBody.The methods correctly build FilterBody objects and maintain backward compatibility. The deprecation of legacy signatures guides users toward the newer patterns.
2495-2642: Create-or-update methods properly refactored with clear deprecation timeline.Good use of
@Deprecated(since = "6.5.0")to provide version context. The methods correctly delegate to the service layer while maintaining backward compatibility.
2210-2224: Path manipulation methods properly updated for new constants.The removal of
.valuesuffix from MenuItemConstants usage improves code readability while maintaining the same functionality.Also applies to: 2714-2757
2694-2696: Default filter metadata logic properly centralized.Good delegation to FilterBody.getDefaultMetadata(), keeping the metadata logic where it belongs.
2698-2700: Menu hierarchy management method added.The new method properly delegates parent-child relationship management to the service layer.
nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/dashboard/DashboardToDataSetOutcome.java (1)
17-20: Good extension design with proper annotations.The class properly extends
ToDataSetOutcomewith appropriate Lombok annotations and follows the established pattern for dataset outcome classes.
...gine/src/main/groovy/com/netgrif/application/engine/startup/DashboardManagementRunner.groovy
Outdated
Show resolved
Hide resolved
| @RequiredArgsConstructor | ||
| class DefaultDashboardRunner implements ApplicationEngineStartupRunner { | ||
|
|
||
| @Autowired | ||
| private DashboardManagementService dashboardManagementService |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Remove redundant dependency injection annotations.
Using both @RequiredArgsConstructor and @Autowired is redundant. Choose one approach for consistency.
Apply this diff to use constructor injection consistently:
-@RequiredArgsConstructor
class DefaultDashboardRunner implements ApplicationEngineStartupRunner {
- @Autowired
- private DashboardManagementService dashboardManagementService
+ private final DashboardManagementService dashboardManagementService
+
+ DefaultDashboardRunner(DashboardManagementService dashboardManagementService) {
+ this.dashboardManagementService = dashboardManagementService
+ }🤖 Prompt for AI Agents
In
application-engine/src/main/groovy/com/netgrif/application/engine/startup/DefaultDashboardRunner.groovy
around lines 17 to 21, remove the @Autowired annotation from the
DashboardManagementService field since @RequiredArgsConstructor already
generates a constructor for dependency injection. Use only constructor injection
by relying on @RequiredArgsConstructor and make the field final if needed for
clarity.
...ine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticPetriNetService.java
Show resolved
Hide resolved
| public Case createDashboardManagement(DashboardManagementBody body) throws TransitionNotExecutableException { | ||
| Case managementCase; | ||
| MenuItemUtils.sanitize(body.getId()); | ||
|
|
||
| managementCase = findDashboardManagement(body.getId()); | ||
| if (managementCase != null) { | ||
| log.info("Dashboard management with id:{} already exists", body.getId()); | ||
| return managementCase; | ||
| } | ||
| addReferencedMenuItems(body); | ||
| LoggedUser loggedUser = userService.getLoggedOrSystem().transformToLoggedUser(); | ||
| managementCase = workflowService.createCase(petriNetService.getNewestVersionByIdentifier(DashboardManagementConstants.PROCESS_IDENTIFIER).getStringId(), body.getName().getDefaultValue(), "", loggedUser).getCase(); | ||
| ToDataSetOutcome outcome = body.toDataSet(); | ||
| managementCase = setDataWithExecute(managementCase, DashboardItemConstants.TASK_CONFIGURE, outcome.getDataSet()); | ||
| return managementCase; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Verification agent
🧩 Analysis chain
Handle potential null PetriNet and clarify parameter mutation
- The
MenuItemUtils.sanitize(body.getId())call on line 58 appears to modify thebodyobject's ID, which is a side effect not indicated by the method signature. - The
petriNetService.getNewestVersionByIdentifier()call on line 67 could potentially return null if no PetriNet is found.
🏁 Script executed:
#!/bin/bash
# Check if MenuItemUtils.sanitize modifies the input or returns a sanitized value
ast-grep --pattern 'class MenuItemUtils {
$$$
sanitize($_) {
$$$
}
$$$
}'
# Check if getNewestVersionByIdentifier can return null
rg -A 5 "getNewestVersionByIdentifier" --type javaLength of output: 16918
I’ll check how sanitize is implemented to confirm if it mutates its input.
🏁 Script executed:
#!/bin/bash
# Locate the MenuItemUtils source file
file=$(rg -l "class MenuItemUtils" -n)
echo "Found MenuItemUtils at: $file"
# Show the sanitize method definition and body
rg -A5 -n "sanitize" "$file"Length of output: 862
Assign the sanitized ID and guard against missing PetriNet
The call to MenuItemUtils.sanitize(body.getId()) returns a sanitized String and does not mutate the body object—its return value is currently ignored. Also, getNewestVersionByIdentifier(...) can return null, leading to an NPE on .getStringId().
Please update:
- Replace the standalone sanitize call with an assignment (or setter) so you actually use the sanitized ID.
- Add a null check for the returned
PetriNetbefore calling.getStringId(), throwing or handling the error appropriately.
Locations to update:
- application-engine/src/main/java/com/netgrif/application/engine/menu/services/DashboardManagementServiceImpl.java
Suggested diff:
@@ createDashboardManagement(DashboardManagementBody body)
- MenuItemUtils.sanitize(body.getId());
+ // actually apply the sanitized ID
+ body.setId(MenuItemUtils.sanitize(body.getId()));
@@
- managementCase = workflowService.createCase(
- petriNetService.getNewestVersionByIdentifier(DashboardManagementConstants.PROCESS_IDENTIFIER)
- .getStringId(),
- body.getName().getDefaultValue(), "", loggedUser)
- .getCase();
+ // guard against missing process definition
+ PetriNet net = petriNetService.getNewestVersionByIdentifier(
+ DashboardManagementConstants.PROCESS_IDENTIFIER);
+ if (net == null) {
+ throw new IllegalStateException("Missing PetriNet for identifier: "
+ + DashboardManagementConstants.PROCESS_IDENTIFIER);
+ }
+ managementCase = workflowService.createCase(
+ net.getStringId(),
+ body.getName().getDefaultValue(), "", loggedUser)
+ .getCase();📝 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.
| public Case createDashboardManagement(DashboardManagementBody body) throws TransitionNotExecutableException { | |
| Case managementCase; | |
| MenuItemUtils.sanitize(body.getId()); | |
| managementCase = findDashboardManagement(body.getId()); | |
| if (managementCase != null) { | |
| log.info("Dashboard management with id:{} already exists", body.getId()); | |
| return managementCase; | |
| } | |
| addReferencedMenuItems(body); | |
| LoggedUser loggedUser = userService.getLoggedOrSystem().transformToLoggedUser(); | |
| managementCase = workflowService.createCase(petriNetService.getNewestVersionByIdentifier(DashboardManagementConstants.PROCESS_IDENTIFIER).getStringId(), body.getName().getDefaultValue(), "", loggedUser).getCase(); | |
| ToDataSetOutcome outcome = body.toDataSet(); | |
| managementCase = setDataWithExecute(managementCase, DashboardItemConstants.TASK_CONFIGURE, outcome.getDataSet()); | |
| return managementCase; | |
| } | |
| public Case createDashboardManagement(DashboardManagementBody body) throws TransitionNotExecutableException { | |
| Case managementCase; | |
| // actually apply the sanitized ID | |
| body.setId(MenuItemUtils.sanitize(body.getId())); | |
| managementCase = findDashboardManagement(body.getId()); | |
| if (managementCase != null) { | |
| log.info("Dashboard management with id:{} already exists", body.getId()); | |
| return managementCase; | |
| } | |
| addReferencedMenuItems(body); | |
| LoggedUser loggedUser = userService.getLoggedOrSystem().transformToLoggedUser(); | |
| // guard against missing process definition | |
| PetriNet net = petriNetService.getNewestVersionByIdentifier( | |
| DashboardManagementConstants.PROCESS_IDENTIFIER); | |
| if (net == null) { | |
| throw new IllegalStateException("Missing PetriNet for identifier: " | |
| + DashboardManagementConstants.PROCESS_IDENTIFIER); | |
| } | |
| managementCase = workflowService.createCase( | |
| net.getStringId(), | |
| body.getName().getDefaultValue(), | |
| "", | |
| loggedUser) | |
| .getCase(); | |
| ToDataSetOutcome outcome = body.toDataSet(); | |
| managementCase = setDataWithExecute(managementCase, DashboardItemConstants.TASK_CONFIGURE, outcome.getDataSet()); | |
| return managementCase; | |
| } |
🤖 Prompt for AI Agents
In
application-engine/src/main/java/com/netgrif/application/engine/menu/services/DashboardManagementServiceImpl.java
around lines 56 to 71, the sanitized ID returned by
MenuItemUtils.sanitize(body.getId()) is not assigned back to the body, so the
sanitized value is not used; assign the sanitized string back to the body ID
using the appropriate setter or assignment. Additionally, the call to
petriNetService.getNewestVersionByIdentifier(...) can return null, which causes
a NullPointerException when calling getStringId(); add a null check for the
returned PetriNet object and handle the null case by throwing an exception or
returning an error before proceeding.
| @Override | ||
| public Case updateDashboardManagement(Case managementCase, DashboardManagementBody body) throws TransitionNotExecutableException { | ||
| MenuItemUtils.sanitize(body.getId()); | ||
| addReferencedMenuItems(body); | ||
| ToDataSetOutcome outcome = body.toDataSet(); | ||
| managementCase = setDataWithExecute(managementCase, DashboardItemConstants.TASK_CONFIGURE, outcome.getDataSet()); | ||
| return managementCase; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add null check for managementCase parameter
The method should validate that the managementCase parameter is not null before proceeding with the update.
public Case updateDashboardManagement(Case managementCase, DashboardManagementBody body) throws TransitionNotExecutableException {
+ if (managementCase == null) {
+ throw new IllegalArgumentException("Management case cannot be null");
+ }
MenuItemUtils.sanitize(body.getId());📝 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.
| @Override | |
| public Case updateDashboardManagement(Case managementCase, DashboardManagementBody body) throws TransitionNotExecutableException { | |
| MenuItemUtils.sanitize(body.getId()); | |
| addReferencedMenuItems(body); | |
| ToDataSetOutcome outcome = body.toDataSet(); | |
| managementCase = setDataWithExecute(managementCase, DashboardItemConstants.TASK_CONFIGURE, outcome.getDataSet()); | |
| return managementCase; | |
| } | |
| @Override | |
| public Case updateDashboardManagement(Case managementCase, DashboardManagementBody body) throws TransitionNotExecutableException { | |
| if (managementCase == null) { | |
| throw new IllegalArgumentException("Management case cannot be null"); | |
| } | |
| MenuItemUtils.sanitize(body.getId()); | |
| addReferencedMenuItems(body); | |
| ToDataSetOutcome outcome = body.toDataSet(); | |
| managementCase = setDataWithExecute(managementCase, DashboardItemConstants.TASK_CONFIGURE, outcome.getDataSet()); | |
| return managementCase; | |
| } |
🤖 Prompt for AI Agents
In
application-engine/src/main/java/com/netgrif/application/engine/menu/services/DashboardManagementServiceImpl.java
around lines 80 to 87, add a null check for the managementCase parameter at the
start of the updateDashboardManagement method. If managementCase is null, throw
an appropriate exception or handle the case to prevent NullPointerExceptions
later in the method. This ensures the method only proceeds when managementCase
is valid.
| if (this.id != null) { | ||
| outcome.putDataSetEntry(DashboardManagementConstants.FIELD_NAME, FieldType.I18N, this.name); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix null check condition for name field.
The condition incorrectly checks this.id instead of this.name.
-if (this.id != null) {
+if (this.name != null) {
outcome.putDataSetEntry(DashboardManagementConstants.FIELD_NAME, FieldType.I18N, this.name);
}📝 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.
| if (this.id != null) { | |
| outcome.putDataSetEntry(DashboardManagementConstants.FIELD_NAME, FieldType.I18N, this.name); | |
| } | |
| if (this.name != null) { | |
| outcome.putDataSetEntry(DashboardManagementConstants.FIELD_NAME, FieldType.I18N, this.name); | |
| } |
🤖 Prompt for AI Agents
In
nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/dashboard/DashboardManagementBody.java
around lines 67 to 69, the null check condition incorrectly checks this.id
instead of this.name before putting the data set entry. Change the condition to
check if this.name is not null before calling outcome.putDataSetEntry with
this.name.
| } | ||
| outcome.putDataSetEntry(DashboardManagementConstants.FIELD_SIMPLE_DASHBOARD, FieldType.BOOLEAN, this.simpleDashboard); | ||
| outcome.putDataSetEntry(DashboardManagementConstants.FIELD_PROFILE_DASHBOARD, FieldType.BOOLEAN, this.profileDashboard); | ||
| outcome.putDataSetEntry(DashboardManagementConstants.FIELD_LANGUAGE_DASHBOARD, FieldType.BOOLEAN, this.profileDashboard); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use correct field value for language dashboard.
The method uses this.profileDashboard instead of this.languageDashboard.
-outcome.putDataSetEntry(DashboardManagementConstants.FIELD_LANGUAGE_DASHBOARD, FieldType.BOOLEAN, this.profileDashboard);
+outcome.putDataSetEntry(DashboardManagementConstants.FIELD_LANGUAGE_DASHBOARD, FieldType.BOOLEAN, this.languageDashboard);📝 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.
| outcome.putDataSetEntry(DashboardManagementConstants.FIELD_LANGUAGE_DASHBOARD, FieldType.BOOLEAN, this.profileDashboard); | |
| outcome.putDataSetEntry(DashboardManagementConstants.FIELD_LANGUAGE_DASHBOARD, FieldType.BOOLEAN, this.languageDashboard); |
🤖 Prompt for AI Agents
In
nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/dashboard/DashboardManagementBody.java
at line 75, the code incorrectly uses this.profileDashboard for the language
dashboard field. Replace this.profileDashboard with this.languageDashboard in
the outcome.putDataSetEntry call to use the correct field value.
| public void putDataSetEntryWithOptions(String fieldId, FieldType fieldType, @Nullable Map<String, I18nString> options, @Nullable Object fieldValue){ | ||
| Map<String, Object> fieldMap = new LinkedHashMap<>(); | ||
|
|
||
| fieldMap.put("type", fieldType.getName()); | ||
| if (fieldType.equals(FieldType.MULTICHOICE_MAP)) { | ||
| fieldMap.put("value", Set.of()); | ||
| } | ||
| if (options == null) { | ||
| options = new HashMap<>(); | ||
| } | ||
| fieldMap.put("options", options); | ||
| fieldMap.put("value", fieldValue); | ||
| this.getDataSet().put(fieldId, fieldMap); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Potential logic issue with MULTICHOICE_MAP handling.
The method initializes the "value" key with an empty set for MULTICHOICE_MAP field type (line 26), but then immediately overwrites it with fieldValue (line 32). This could lead to unexpected behavior if fieldValue is not a Set or is null for multichoice fields.
Consider handling the MULTICHOICE_MAP case more explicitly:
fieldMap.put("type", fieldType.getName());
-if (fieldType.equals(FieldType.MULTICHOICE_MAP)) {
- fieldMap.put("value", Set.of());
-}
if (options == null) {
options = new HashMap<>();
}
fieldMap.put("options", options);
-fieldMap.put("value", fieldValue);
+if (fieldType.equals(FieldType.MULTICHOICE_MAP)) {
+ fieldMap.put("value", fieldValue != null ? fieldValue : Set.of());
+} else {
+ fieldMap.put("value", fieldValue);
+}📝 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.
| public void putDataSetEntryWithOptions(String fieldId, FieldType fieldType, @Nullable Map<String, I18nString> options, @Nullable Object fieldValue){ | |
| Map<String, Object> fieldMap = new LinkedHashMap<>(); | |
| fieldMap.put("type", fieldType.getName()); | |
| if (fieldType.equals(FieldType.MULTICHOICE_MAP)) { | |
| fieldMap.put("value", Set.of()); | |
| } | |
| if (options == null) { | |
| options = new HashMap<>(); | |
| } | |
| fieldMap.put("options", options); | |
| fieldMap.put("value", fieldValue); | |
| this.getDataSet().put(fieldId, fieldMap); | |
| } | |
| public void putDataSetEntryWithOptions(String fieldId, FieldType fieldType, @Nullable Map<String, I18nString> options, @Nullable Object fieldValue){ | |
| Map<String, Object> fieldMap = new LinkedHashMap<>(); | |
| fieldMap.put("type", fieldType.getName()); | |
| if (options == null) { | |
| options = new HashMap<>(); | |
| } | |
| fieldMap.put("options", options); | |
| if (fieldType.equals(FieldType.MULTICHOICE_MAP)) { | |
| fieldMap.put("value", fieldValue != null ? fieldValue : Set.of()); | |
| } else { | |
| fieldMap.put("value", fieldValue); | |
| } | |
| this.getDataSet().put(fieldId, fieldMap); | |
| } |
🤖 Prompt for AI Agents
In
nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/dashboard/DashboardToDataSetOutcome.java
around lines 21 to 34, the "value" for MULTICHOICE_MAP is first set to an empty
set but then overwritten by fieldValue, which may cause issues if fieldValue is
null or not a set. To fix this, add explicit handling for MULTICHOICE_MAP by
checking if fieldValue is a valid set; if not, initialize it as an empty set,
and only then put it into the map, avoiding overwriting the initial empty set
with an invalid value.
| if (viewCase != null) { | ||
| outcome.putDataSetEntry(MenuItemConstants.FIELD_VIEW_CONFIGURATION_TYPE, FieldType.ENUMERATION_MAP, | ||
| this.view.getViewType().getIdentifier()); | ||
| outcome.putDataSetEntry(MenuItemConstants.FIELD_VIEW_CONFIGURATION_ID, FieldType.CASE_REF, | ||
| List.of(viewCase.getStringId())); | ||
| String taskId = MenuItemUtils.findTaskIdInCase(viewCase, ViewConstants.TRANS_SETTINGS_ID); | ||
| outcome.putDataSetEntry(MenuItemConstants.FIELD_VIEW_CONFIGURATION_FORM, FieldType.TASK_REF, List.of(taskId)); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add null check for taskId retrieval
The MenuItemUtils.findTaskIdInCase call on line 179 might return null, which would cause issues when added to the dataset.
if (viewCase != null) {
outcome.putDataSetEntry(MenuItemConstants.FIELD_VIEW_CONFIGURATION_TYPE, FieldType.ENUMERATION_MAP,
this.view.getViewType().getIdentifier());
outcome.putDataSetEntry(MenuItemConstants.FIELD_VIEW_CONFIGURATION_ID, FieldType.CASE_REF,
List.of(viewCase.getStringId()));
String taskId = MenuItemUtils.findTaskIdInCase(viewCase, ViewConstants.TRANS_SETTINGS_ID);
+ if (taskId == null) {
+ throw new IllegalStateException("Settings task not found in view case");
+ }
outcome.putDataSetEntry(MenuItemConstants.FIELD_VIEW_CONFIGURATION_FORM, FieldType.TASK_REF, List.of(taskId));
}📝 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.
| if (viewCase != null) { | |
| outcome.putDataSetEntry(MenuItemConstants.FIELD_VIEW_CONFIGURATION_TYPE, FieldType.ENUMERATION_MAP, | |
| this.view.getViewType().getIdentifier()); | |
| outcome.putDataSetEntry(MenuItemConstants.FIELD_VIEW_CONFIGURATION_ID, FieldType.CASE_REF, | |
| List.of(viewCase.getStringId())); | |
| String taskId = MenuItemUtils.findTaskIdInCase(viewCase, ViewConstants.TRANS_SETTINGS_ID); | |
| outcome.putDataSetEntry(MenuItemConstants.FIELD_VIEW_CONFIGURATION_FORM, FieldType.TASK_REF, List.of(taskId)); | |
| } | |
| if (viewCase != null) { | |
| outcome.putDataSetEntry(MenuItemConstants.FIELD_VIEW_CONFIGURATION_TYPE, FieldType.ENUMERATION_MAP, | |
| this.view.getViewType().getIdentifier()); | |
| outcome.putDataSetEntry(MenuItemConstants.FIELD_VIEW_CONFIGURATION_ID, FieldType.CASE_REF, | |
| List.of(viewCase.getStringId())); | |
| String taskId = MenuItemUtils.findTaskIdInCase(viewCase, ViewConstants.TRANS_SETTINGS_ID); | |
| if (taskId == null) { | |
| throw new IllegalStateException("Settings task not found in view case"); | |
| } | |
| outcome.putDataSetEntry(MenuItemConstants.FIELD_VIEW_CONFIGURATION_FORM, FieldType.TASK_REF, List.of(taskId)); | |
| } |
🤖 Prompt for AI Agents
In
nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemBody.java
around lines 174 to 181, the taskId retrieved by MenuItemUtils.findTaskIdInCase
may be null, causing issues when added to the dataset. Add a null check for
taskId before calling outcome.putDataSetEntry with it, and only add the entry if
taskId is not null.
| public void putDataSetEntry(String fieldId, FieldType fieldType, @Nullable Object fieldValue) { | ||
| Map<String, Object> fieldMap = new LinkedHashMap<>(); | ||
| fieldMap.put("type", fieldType.getName()); | ||
| fieldMap.put("value", fieldValue); | ||
| this.dataSet.put(fieldId, fieldMap); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Add null/empty validation for fieldId parameter.
Consider adding validation to prevent null or empty fieldId values from being added to the dataset.
public void putDataSetEntry(String fieldId, FieldType fieldType, @Nullable Object fieldValue) {
+ if (fieldId == null || fieldId.trim().isEmpty()) {
+ throw new IllegalArgumentException("fieldId cannot be null or empty");
+ }
Map<String, Object> fieldMap = new LinkedHashMap<>();
fieldMap.put("type", fieldType.getName());
fieldMap.put("value", fieldValue);
this.dataSet.put(fieldId, fieldMap);
}📝 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.
| public void putDataSetEntry(String fieldId, FieldType fieldType, @Nullable Object fieldValue) { | |
| Map<String, Object> fieldMap = new LinkedHashMap<>(); | |
| fieldMap.put("type", fieldType.getName()); | |
| fieldMap.put("value", fieldValue); | |
| this.dataSet.put(fieldId, fieldMap); | |
| } | |
| public void putDataSetEntry(String fieldId, FieldType fieldType, @Nullable Object fieldValue) { | |
| if (fieldId == null || fieldId.trim().isEmpty()) { | |
| throw new IllegalArgumentException("fieldId cannot be null or empty"); | |
| } | |
| Map<String, Object> fieldMap = new LinkedHashMap<>(); | |
| fieldMap.put("type", fieldType.getName()); | |
| fieldMap.put("value", fieldValue); | |
| this.dataSet.put(fieldId, fieldMap); | |
| } |
🤖 Prompt for AI Agents
In
nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/ToDataSetOutcome.java
around lines 30 to 35, add validation to check if the fieldId parameter is null
or empty before proceeding. If fieldId is null or empty, throw an
IllegalArgumentException or handle it appropriately to prevent invalid entries
from being added to the dataSet map.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 33
🔭 Outside diff range comments (2)
application-engine/src/test/groovy/com/netgrif/application/engine/action/MenuItemApiTest.groovy (1)
202-205: Fix inconsistent constant usage.The test mixes old (
PREFERRED_ITEM_NET_IDENTIFIER,PREFERENCE_ITEM_FIELD_*) and new (MenuItemConstants.FIELD_*) constant patterns, which creates inconsistency.-Case folderCase = findCasesElastic("processIdentifier:$FilterRunner.PREFERRED_ITEM_NET_IDENTIFIER AND dataSet.${MenuItemConstants.PREFERENCE_ITEM_FIELD_NODE_PATH.attributeId}.textValue:\"/netgrif2\"", PageRequest.of(0, 1))[0] +Case folderCase = findCasesElastic("processIdentifier:$FilterRunner.MENU_NET_IDENTIFIER AND dataSet.${MenuItemConstants.FIELD_NODE_PATH}.textValue:\"/netgrif2\"", PageRequest.of(0, 1))[0] -ArrayList<String> childIds = folderCase.dataSet[MenuItemConstants.PREFERENCE_ITEM_FIELD_CHILD_ITEM_IDS.attributeId].value as ArrayList<String> +ArrayList<String> childIds = folderCase.dataSet[MenuItemConstants.FIELD_CHILD_ITEM_IDS].value as ArrayList<String>application-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy (1)
2226-2231: Missing .value property access on PATH_SEPARATOR constant.Consistency issue with PATH_SEPARATOR usage.
- if (path == null || path == MenuItemConstants.PATH_SEPARATOR || path.length() == 0) { + if (path == null || path == MenuItemConstants.PATH_SEPARATOR.value || path.length() == 0) { return false }
🧹 Nitpick comments (38)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/configurations/TabbedTicketViewConstants.java (1)
4-5: Utility class should befinalwith a private constructorPrevent instantiation and subclassing of a constants-only class.
-public class TabbedTicketViewConstants extends ViewConstants { -} +public final class TabbedTicketViewConstants extends ViewConstants { + + private TabbedTicketViewConstants() { + // no-instantiation + } +}nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/configurations/TabbedSingleTaskViewConstants.java (1)
4-6: Mark constants holder asfinaland add private constructorSame rationale as for other constants classes.
-public class TabbedSingleTaskViewConstants extends ViewConstants { - public static final String FIELD_TRANSITION_ID = "transition_id"; -} +public final class TabbedSingleTaskViewConstants extends ViewConstants { + + public static final String FIELD_TRANSITION_ID = "transition_id"; + + private TabbedSingleTaskViewConstants() { + // no-instantiation + } +}nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/dashboard/DashboardManagementConstants.java (1)
1-21: Well-structured constants class with excellent organization.The class follows Java best practices with proper naming conventions, clear documentation, and logical grouping of related constants. This centralized approach eliminates magic strings and improves maintainability across the dashboard management functionality.
Consider adding brief inline comments for complex field names to improve readability:
public static final String FIELD_DASHBOARD_ITEM_TO_MENU_ITEM_LIST = "dashboard_item_to_menu_item"; +// Toolbar configuration fields public static final String FIELD_SIMPLE_DASHBOARD = "simple_dashboard_toolbar"; public static final String FIELD_PROFILE_DASHBOARD = "profile_dashboard_toolbar"; public static final String FIELD_LANGUAGE_DASHBOARD = "language_dashboard_toolbar"; public static final String FIELD_LOGOUT_DASHBOARD = "logout_dashboard_toolbar";nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticPetriNet.java (1)
15-16: Remove unused imports.The
HashSetandSetimports are not used in this class and should be removed to maintain clean code.-import java.util.HashSet; -import java.util.Set;nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/filter/FilterConstants.java (1)
3-3: Remove unused Lombok import.The
lombok.Getterimport is not used since there's no@Getterannotation on the class or any fields that would benefit from it.-import lombok.Getter;nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/configurations/TabbedCaseViewBody.java (2)
25-25: Consider defensive copying for mutable collection initialization.The
headersModefield is initialized with a mutableArrayListthat could be modified externally, potentially affecting all instances.- private List<String> headersMode = new ArrayList<>(List.of("sort", "edit", "search")); + private List<String> headersMode = new ArrayList<>(List.of("sort", "edit", "search"));Or use an immutable approach:
- private List<String> headersMode = new ArrayList<>(List.of("sort", "edit", "search")); + private List<String> headersMode = List.of("sort", "edit", "search");
17-26: Consider validation for enum-like string fields.Fields like
viewSearchTypeandheadersDefaultModeappear to have constrained values but lack validation. Consider adding validation or using actual enums if the values are predefined.// Consider creating enums for constrained values public enum ViewSearchType { FULLTEXT_ADVANCED("fulltext_advanced"), // other values... private final String value; // constructor and getter }application-engine/src/main/groovy/com/netgrif/application/engine/startup/DefaultDashboardRunner.groovy (1)
29-29: Consider externalizing localized strings.The hardcoded localized strings should be externalized to resource files for better maintainability and internationalization support.
Consider using a configuration properties approach or resource bundles for localized strings instead of hardcoding them in the Map.
application-engine/src/main/groovy/com/netgrif/application/engine/startup/ImportHelper.groovy (1)
249-263: Improve variable naming and add documentation.The method implementation is solid, but there are some improvements to consider:
- The variable name
filteris misleading since this method handles any process type, not just filters- Missing JavaDoc documentation for this utility method
Apply these improvements:
+ /** + * Imports a Petri net process if it doesn't already exist. + * @param message descriptive message for logging + * @param netIdentifier the unique identifier of the Petri net + * @param netFileName the filename of the Petri net XML file + * @return Optional containing the imported or existing PetriNet + */ Optional<PetriNet> importProcess(String message, String netIdentifier, String netFileName) { - PetriNet filter = petriNetService.getNewestVersionByIdentifier(netIdentifier) - if (filter != null) { + PetriNet existingNet = petriNetService.getNewestVersionByIdentifier(netIdentifier) + if (existingNet != null) { log.info("${message} has already been imported.") - return Optional.of(filter) + return Optional.of(existingNet) } - Optional<PetriNet> filterNet = this.createNet(netFileName, VersionType.MAJOR, userService.getSystem().transformToLoggedUser()) + Optional<PetriNet> importedNet = this.createNet(netFileName, VersionType.MAJOR, userService.getSystem().transformToLoggedUser()) - if (!filterNet.isPresent()) { + if (!importedNet.isPresent()) { log.error("Import of ${message} failed!") } - return filterNet + return importedNet }nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/configurations/TabbedSingleTaskViewBody.java (1)
16-19: Document the intentional null return.The
getAssociatedViewBody()method always returnsnull. This should be documented to clarify that single task views don't have associated views by design.Add documentation:
@Override + /** + * Single task views do not have associated views. + * @return always null + */ public ViewBody getAssociatedViewBody() { return null; }nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/configurations/TabbedTaskViewConstants.java (1)
6-16: Hide constructor in utility‐constants class
TabbedTaskViewConstantsshould not be instantiated. Add a private constructor to communicate intent and prevent accidental instantiation.public class TabbedTaskViewConstants extends ViewConstants { + private TabbedTaskViewConstants() { + /* static holder – no instances */ + } public static final String FIELD_MERGE_FILTERS = "merge_filters"; public static final String FIELD_VIEW_SEARCH_TYPE = "view_search_type"; public static final String FIELD_DEFAULT_HEADERS = "default_headers"; public static final String FIELD_HEADERS_MODE = "headers_mode"; @@ public static final String FIELD_USE_DEFAULT_HEADERS = "use_default_headers"; public static final String FIELD_SHOW_MORE_MENU = "show_more_menu"; }nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/configurations/TabbedTicketViewBody.java (1)
14-29: Expose builder / constructor for safer initialisation
chainedViewis mutable and may remainnull, causing NPEs whengetAssociatedViewBody()is used downstream.
Consider providing an all-args constructor or a builder to enforce non-null initialisation, or guard againstnullingetAssociatedViewBody().- private ViewBody chainedView; + @NonNull + private ViewBody chainedView; @Override public ViewBody getAssociatedViewBody() { - return this.chainedView; + return Objects.requireNonNull(this.chainedView, + "chainedView must be initialised before use"); }nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/configurations/ViewConstants.java (1)
6-16: Add private constructor to prevent instantiation
ViewConstantsis a holder of static constants. Instantiation provides no value and should be disallowed.public class ViewConstants { + private ViewConstants() { + /* static holder */ + } public static final String FIELD_CONFIGURATION_TYPE = "view_configuration_type"; public static final String FIELD_VIEW_CONFIGURATION_ID = "view_configuration_id"; @@ public static final String TRANS_SYNC_ID = "data_sync"; }nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/dashboard/DashboardItemConstants.java (1)
6-23: Utility class should not be instantiatedSame pattern applies here—add a private constructor.
public class DashboardItemConstants { + private DashboardItemConstants() { + /* static holder */ + } public static final String PROCESS_IDENTIFIER = "dashboard_item"; public static final String TASK_CONFIGURE = "configuration"; @@ public static final String FIELD_IS_ACTIVE= "is_active"; }nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/configurations/TabbedCaseViewConstants.java (1)
7-22: Prevent instantiation of constants holderAs with other constants classes, add a private constructor.
public class TabbedCaseViewConstants extends ViewConstants { + private TabbedCaseViewConstants() { + /* static holder */ + } public static final String FIELD_NEW_FILTER_ID = "new_filter_id"; public static final String FIELD_DEFAULT_HEADERS = "default_headers"; @@ public static final String FIELD_USE_CASE_DEFAULT_HEADERS = "use_case_default_headers"; }application-engine/src/test/resources/dashboard_management_test.xml (1)
14-50: Consider adding error handling and validation to the embedded script.The Groovy script creates dashboard components programmatically but lacks error handling and validation. Consider the following improvements:
- Add null checks and validation for created objects
- Handle potential exceptions during object creation
- Consider extracting hardcoded values to configurable parameters
<![CDATA[ +try { def filterBody = new com.netgrif.application.engine.menu.domain.FilterBody() + if (filterBody == null) { + throw new IllegalStateException("Failed to create FilterBody") + } filterBody.setTitle(new com.netgrif.application.engine.petrinet.domain.I18nString("Main")) // ... rest of the configuration def menuItem = createOrIgnoreMenuItem(menuItemBody) + if (menuItem == null) { + throw new IllegalStateException("Failed to create menu item") + } // ... rest of the script +} catch (Exception e) { + log.error("Failed to create dashboard components: ${e.message}", e) + throw e +} ]]>nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/dashboard/DashboardItemBody.java (1)
29-35: Consider initializing remaining fields in custom constructor.The custom constructor only initializes 5 out of 13 fields, leaving others with default values. This could lead to unexpected behavior if the remaining fields are required.
public DashboardItemBody(String id, String menuItems, String itemIcon, I18nString name, boolean isInternal) { this.id = id; this.menuItems = menuItems; this.isInternal = isInternal; this.itemIcon = itemIcon; this.name = name; + // Initialize remaining fields with sensible defaults + this.inheritIcon = false; + this.inheritName = false; + this.isActive = true; }application-engine/src/main/java/com/netgrif/application/engine/startup/runner/DashboardRunner.java (1)
32-34: Consider adding error handling for import operations.The import operations are called directly without error handling. If import fails, it could cause application startup to fail silently or with unclear error messages.
@Override public void run(ApplicationArguments args) throws Exception { + try { helper.importProcess("Petri net for filters", DASHBOARD_NET_IDENTIFIER, DASHBOARD_FILE_NAME); helper.importProcess("Petri net for filter preferences", DASHBOARD_TILE_NET_IDENTIFIER, DASHBOARD_TILE_FILE_NAME); + } catch (Exception e) { + log.error("Failed to import dashboard processes", e); + throw e; + } }nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/FilterBody.java (1)
37-47: Consider making default metadata configurable.The
getDefaultMetadatamethod returns hardcoded values that might need to be configurable in different environments or use cases.Consider extracting these values to configuration properties:
public static Map<String, Object> getDefaultMetadata(String type) { Map<String, Object> resultMap = new HashMap<>(); - - resultMap.put("searchCategories", List.of()); - resultMap.put("predicateMetadata", List.of()); - resultMap.put("filterType", type); - resultMap.put("defaultSearchCategories", true); - resultMap.put("inheritAllowedNets", false); + + resultMap.put("searchCategories", getDefaultSearchCategories()); + resultMap.put("predicateMetadata", getDefaultPredicateMetadata()); + resultMap.put("filterType", type); + resultMap.put("defaultSearchCategories", getDefaultSearchCategoriesFlag()); + resultMap.put("inheritAllowedNets", getDefaultInheritAllowedNetsFlag()); return resultMap; }application-engine/src/test/groovy/com/netgrif/application/engine/action/MenuItemApiTest.groovy (1)
252-252: Document the purpose of Thread.sleep in tests.The
Thread.sleep(2000)call lacks explanation for why the delay is necessary. This could indicate timing dependencies that might make tests flaky.+// Wait for background processing to complete before assertions Thread.sleep(2000)Consider using more reliable synchronization mechanisms if possible, such as polling for expected conditions rather than fixed delays.
application-engine/src/main/java/com/netgrif/application/engine/menu/services/DashboardItemServiceImpl.java (1)
115-124: Add debug logging for search results.Consider adding debug logging to help troubleshoot when cases are not found.
protected Case findCase(String processIdentifier, String query) { CaseSearchRequest request = CaseSearchRequest.builder() .process(Collections.singletonList(new CaseSearchRequest.PetriNet(processIdentifier))) .query(query) .build(); Page<Case> resultPage = elasticCaseService.search(java.util.List.of(request), userService.getLoggedOrSystem().transformToLoggedUser(), PageRequest.of(0, 1), Locale.getDefault(), false); - return resultPage.hasContent() ? resultPage.getContent().get(0) : null; + if (resultPage.hasContent()) { + return resultPage.getContent().get(0); + } else { + log.debug("No case found for processIdentifier: {} with query: {}", processIdentifier, query); + return null; + } }application-engine/src/main/java/com/netgrif/application/engine/petrinet/web/PetriNetController.java (1)
206-206: Address TODO: Implement merge filters functionality.The TODO comment indicates missing merge filters functionality which might be critical for proper search behavior.
Would you like me to help implement the merge filters functionality or create an issue to track this TODO?
nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/configurations/TabbedTaskViewBody.java (1)
18-19: Consider initializing the filter field or documenting its nullable behavior.The
filterfield is not initialized and could be null. Ensure this is handled properly in all usages or consider initializing it.public class TabbedTaskViewBody extends ViewBody { - private Case filter; + private Case filter = null; // Explicitly nullable, handled in toDataSetInternal private boolean mergeFilters = true;nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemView.java (1)
83-89: Document the potential IllegalArgumentException.The method can throw IllegalArgumentException if parentIdentifier is invalid. Consider documenting this in JavaDoc.
/** * Finds all enum values, that are tabbed or non-tabbed and are defined in parent view as {@link #allowedAssociatedViews} * * @param isTabbed if true, set of views is reduced to only tabbed views * @param parentIdentifier identifier of the view, that contains returned views in {@link #allowedAssociatedViews} * * @return List of views based on {@link #isTabbed} and {@link #allowedAssociatedViews} + * @throws IllegalArgumentException if parentIdentifier is not a valid view identifier * */ public static List<MenuItemView> findAllByIsTabbedAndParentIdentifier(boolean isTabbed, String parentIdentifier) {application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticPetriNetService.java (2)
115-116: Consider batch size limits for ID-based fetching.When fetching PetriNets by their IDs after the search, ensure the list size doesn't exceed reasonable limits to avoid performance issues.
SearchHits<ElasticPetriNet> hits = template.search(query, ElasticPetriNet.class, IndexCoordinates.of(elasticsearchConfiguration.elasticPetriNetIndex())); Page<ElasticPetriNet> indexedNets = (Page) SearchHitSupport.unwrapSearchHits(SearchHitSupport.searchPageFor(hits, query.getPageable())); -netPage = petriNetService.findAllById(indexedNets.get().map(ElasticPetriNet::getStringId).collect(Collectors.toList())); +List<String> ids = indexedNets.get().map(ElasticPetriNet::getStringId).collect(Collectors.toList()); +if (ids.size() > 1000) { + log.warn("Large number of IDs to fetch: {}", ids.size()); +} +netPage = petriNetService.findAllById(ids); total = indexedNets.getTotalElements();
167-172: Address TODO: Move search fields to configuration.The search fields are hardcoded and should be moved to properties as indicated by the TODO comment.
Would you like me to help refactor this to use configuration properties or create an issue to track this TODO?
Also, consider the performance impact of wildcard searches on large datasets.
nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/ToDataSetOutcome.java (2)
44-55: Consider defensive copying for the options parameter.The method modifies the input parameter semantics by reassigning
optionswhen null. This could be confusing to callers who might expect the parameter to remain unchanged.Consider using a local variable for the null check:
public void putDataSetEntryOptions(String fieldId, FieldType fieldType, @Nullable Map<String, I18nString> options) { Map<String, Object> fieldMap = new LinkedHashMap<>(); fieldMap.put("type", fieldType.getName()); if (fieldType.equals(FieldType.MULTICHOICE_MAP)) { fieldMap.put("value", Set.of()); } - if (options == null) { - options = new HashMap<>(); - } - fieldMap.put("options", options); + Map<String, I18nString> optionsToUse = options != null ? options : new HashMap<>(); + fieldMap.put("options", optionsToUse); this.dataSet.put(fieldId, fieldMap); }
16-21: Document thread safety considerations.The class exposes a mutable Map through getters (via @DaTa). If instances are shared between threads, consider documenting thread safety requirements or making the class immutable.
nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/configurations/ViewBody.java (2)
50-51: JavaDoc references non-imported class.The JavaDoc mentions
IDataServicebut this class is not imported. Consider either importing it or using a fully qualified name in the documentation.
56-56: Remove unnecessary semicolon.- public ToDataSetOutcome toDataSet() { - return toDataSet(null, null); - }; + public ToDataSetOutcome toDataSet() { + return toDataSet(null, null); + }application-engine/src/main/java/com/netgrif/application/engine/menu/services/interfaces/IMenuItemService.java (2)
24-26: Add Javadoc to clarify the difference between createOrUpdateMenuItem and createOrIgnoreMenuItemThese two methods have similar names but presumably different behaviors. Consider adding Javadoc comments to clarify:
- When to use
createOrUpdateMenuItemvscreateOrIgnoreMenuItem- What happens when a menu item already exists in each case
- The return value in different scenarios
63-70: Consider extracting the "_configuration" suffix as a constantThe hardcoded string
"_configuration"appears to be a naming convention. Consider extracting it as a constant to improve maintainability and make the convention explicit.+ private static final String CONFIGURATION_SUFFIX = "_configuration"; + default Map<String, I18nString> getAvailableViewsAsOptions(boolean isTabbed, String viewIdentifier) { - int index = viewIdentifier.lastIndexOf("_configuration"); + int index = viewIdentifier.lastIndexOf(CONFIGURATION_SUFFIX); if (index > 0) { viewIdentifier = viewIdentifier.substring(0, index); }application-engine/src/main/java/com/netgrif/application/engine/menu/services/MenuItemService.java (1)
618-656: Remove unused duplicate methodThe
getOrCreateFolderRecursive2method appears to be an unused alternative implementation ofgetOrCreateFolderRecursive. It should be removed to avoid confusion.- protected Case getOrCreateFolderRecursive2(String path, MenuItemBody body, Case childFolderCase) throws TransitionNotExecutableException { - Case folderCase = findFolderCase(path); - if (folderCase != null) { - if (childFolderCase != null) { - folderCase = appendChildCaseIdAndSave(folderCase, childFolderCase.getStringId()); - initializeParentId(childFolderCase, folderCase.getStringId()); - } - return folderCase; - } - - folderCase = createCase(FilterRunner.MENU_NET_IDENTIFIER, body.getMenuName().toString(), userService.getLoggedOrSystem().transformToLoggedUser()); - if (childFolderCase != null) { - folderCase = appendChildCaseIdAndSave(folderCase, childFolderCase.getStringId()); - initializeParentId(childFolderCase, folderCase.getStringId()); - } else { - folderCase = workflowService.save(folderCase); - } - - Optional<TaskPair> newItemTask = folderCase.getTasks().stream().filter(taskPair -> taskPair.getTransition().equals(MenuItemConstants.TRANS_INIT_ID)).findFirst(); - - ToDataSetOutcome dataSetOutcome = body.toDataSet(null, path, null); - if (childFolderCase != null) { - appendChildCaseIdInDataSet(folderCase, childFolderCase.getStringId(), dataSetOutcome.getDataSet()); - } - - if (newItemTask.isPresent()) { - setDataWithExecute(folderCase, MenuItemConstants.TRANS_INIT_ID, dataSetOutcome.getDataSet()); - } - - if (hasParent(path)) { - body = new com.netgrif.application.engine.objects.workflow.domain.menu.MenuItemBody(new I18nString(nameFromPath(path)), "folder"); - String parentPath = parentPath(path); - getOrCreateFolderRecursive(parentPath, body, folderCase); - } - - folderCase = setDataWithExecute(folderCase, MenuItemConstants.TRANS_INIT_ID, dataSetOutcome.getDataSet()); - - return folderCase; - }application-engine/src/main/resources/petriNets/engine-processes/menu/tabbed_case_view_configuration.xml (1)
60-65: Consider toggling visibility instead of editability forupdate_filterbutton
make updateBtn,editable on trans when { true }exposes the button but still leaves it hidden if the field logic marks it as invisible elsewhere. In typical Petriflow UI behaviour, “editable” presumes the data element is already visible; users never see a non-visible but editable control.
Switching the rule tovisible(and keeping the complementary rule in theelsebranch) makes the intent explicit and prevents confusion in complex layouts.- make updateBtn,editable on trans when { true } + make updateBtn,visible on trans when { true }application-engine/src/main/resources/petriNets/engine-processes/menu/menu_item.xml (1)
521-551: Ordering logic duplicated – extract to utility to keep the process maintainableBoth
order_downandorder_uprepeat near-identical swap / setData code. Duplicated Groovy snippets inside XML are notoriously painful to patch and test.Consider a single process-scoped function
reorderChildren(idxDelta)receiving+1or-1to perform the swap and update the parent once. It will:
- Reduce risk of future off-by-one errors
- Keep transitions lean and readable
- Make unit testing possible outside the XML
application-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy (3)
1723-1738: Proper use of @deprecated annotation for backward compatibility.These deprecated methods correctly delegate to the new service-based implementation while maintaining the old API signatures. Consider adding
@Deprecated(since = "6.5.0")annotations with version information for better tracking.- @Deprecated + @Deprecated(since = "6.5.0") Case createMenuItem(String uri, String identifier, Case filter, String groupName, Map<String, String> allowedRoles, Map<String, String> bannedRoles = [:], List<String> caseDefaultHeaders = [], List<String> taskDefaultHeaders = []) {Also applies to: 1750-1767, 1781-1797, 1810-1826
2496-2514: Inconsistent deprecation annotations.Some methods have
@Deprecated(since = "6.5.0")while others just have@Deprecated. Consider making all deprecation annotations consistent with version information.- @Deprecated + @Deprecated(since = "6.5.0") Case createOrUpdateMenuItem(String id, String uri, String type, String query, String icon, String title, List<String> allowedNets,Also applies to: 2533-2548
2132-2134: Incomplete implementation - TODO for task filter query.The task filter query is empty with a TODO comment. This needs to be implemented to properly filter tasks by case ID and transition ID.
Would you like me to help implement the query construction for filtering tasks by case ID and transition ID?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (62)
application-engine/CHANGELOG.md(1 hunks)application-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy(40 hunks)application-engine/src/main/groovy/com/netgrif/application/engine/startup/DashboardManagementRunner.groovy(1 hunks)application-engine/src/main/groovy/com/netgrif/application/engine/startup/DefaultDashboardRunner.groovy(1 hunks)application-engine/src/main/groovy/com/netgrif/application/engine/startup/ImportHelper.groovy(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticPetriNetService.java(3 hunks)application-engine/src/main/java/com/netgrif/application/engine/elastic/service/interfaces/IElasticPetriNetService.java(2 hunks)application-engine/src/main/java/com/netgrif/application/engine/menu/services/DashboardItemServiceImpl.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/menu/services/DashboardManagementServiceImpl.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/menu/services/MenuItemService.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/menu/services/interfaces/DashboardItemService.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/menu/services/interfaces/DashboardManagementService.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/menu/services/interfaces/IMenuItemService.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/petrinet/web/PetriNetController.java(5 hunks)application-engine/src/main/java/com/netgrif/application/engine/petrinet/web/responsebodies/PetriNetReferenceResourceAssembler.java(0 hunks)application-engine/src/main/java/com/netgrif/application/engine/startup/runner/DashboardRunner.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/startup/runner/FilterRunner.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/domain/menu/Menu.java(0 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/domain/menu/MenuAndFilters.java(0 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/domain/menu/MenuEntry.java(0 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/domain/menu/MenuEntryRole.java(0 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/domain/menu/MenuItemBody.java(0 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/domain/menu/MenuItemConstants.java(0 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/domain/menu/MenuList.java(0 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/web/WorkflowController.java(1 hunks)application-engine/src/main/resources/application.yaml(1 hunks)application-engine/src/main/resources/petriNets/engine-processes/dashboard_item.xml(1 hunks)application-engine/src/main/resources/petriNets/engine-processes/dashboard_management.xml(1 hunks)application-engine/src/main/resources/petriNets/engine-processes/menu/menu_item.xml(1 hunks)application-engine/src/main/resources/petriNets/engine-processes/menu/tabbed_case_view_configuration.xml(1 hunks)application-engine/src/main/resources/petriNets/engine-processes/menu/tabbed_single_task_view_configuration.xml(1 hunks)application-engine/src/main/resources/petriNets/engine-processes/menu/tabbed_task_view_configuration.xml(1 hunks)application-engine/src/main/resources/petriNets/engine-processes/menu/tabbed_ticket_view_configuration.xml(1 hunks)application-engine/src/main/resources/petriNets/engine-processes/preference_filter_item.xml(1 hunks)application-engine/src/main/resources/petriNets/engine-processes/preference_item.xml(0 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/action/MenuItemApiTest.groovy(7 hunks)application-engine/src/test/resources/dashboard_management_test.xml(1 hunks)application-engine/src/test/resources/petriNets/filter_api_test.xml(1 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticPetriNet.java(4 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/utils/MenuItemUtils.java(1 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/filter/FilterConstants.java(1 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/FilterBody.java(1 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemBody.java(3 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java(1 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemView.java(1 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/ToDataSetOutcome.java(1 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/configurations/TabbedCaseViewBody.java(1 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/configurations/TabbedCaseViewConstants.java(1 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/configurations/TabbedSingleTaskViewBody.java(1 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/configurations/TabbedSingleTaskViewConstants.java(1 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/configurations/TabbedTaskViewBody.java(1 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/configurations/TabbedTaskViewConstants.java(1 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/configurations/TabbedTicketViewBody.java(1 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/configurations/TabbedTicketViewConstants.java(1 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/configurations/ViewBody.java(1 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/configurations/ViewConstants.java(1 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/dashboard/DashboardItemBody.java(1 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/dashboard/DashboardItemConstants.java(1 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/dashboard/DashboardManagementBody.java(1 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/dashboard/DashboardManagementConstants.java(1 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/dashboard/DashboardToDataSetOutcome.java(1 hunks)nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/ElasticPetriNet.java(3 hunks)
💤 Files with no reviewable changes (9)
- application-engine/src/main/java/com/netgrif/application/engine/workflow/domain/menu/MenuAndFilters.java
- application-engine/src/main/java/com/netgrif/application/engine/workflow/domain/menu/MenuEntryRole.java
- application-engine/src/main/java/com/netgrif/application/engine/workflow/domain/menu/MenuItemBody.java
- application-engine/src/main/resources/petriNets/engine-processes/preference_item.xml
- application-engine/src/main/java/com/netgrif/application/engine/workflow/domain/menu/MenuList.java
- application-engine/src/main/java/com/netgrif/application/engine/workflow/domain/menu/Menu.java
- application-engine/src/main/java/com/netgrif/application/engine/petrinet/web/responsebodies/PetriNetReferenceResourceAssembler.java
- application-engine/src/main/java/com/netgrif/application/engine/workflow/domain/menu/MenuEntry.java
- application-engine/src/main/java/com/netgrif/application/engine/workflow/domain/menu/MenuItemConstants.java
🧰 Additional context used
🧠 Learnings (26)
📓 Common learnings
Learnt from: renczesstefan
PR: netgrif/application-engine#318
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java:60-62
Timestamp: 2025-06-23T13:30:13.096Z
Learning: In MenuItemConstants enum in nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java, the field `attributeId` will be renamed to `value` to make it more generic and appropriate for both dataset attribute identifiers and technical constants like PATH_SEPARATOR.
application-engine/src/main/resources/application.yaml (1)
Learnt from: renczesstefan
PR: #318
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java:60-62
Timestamp: 2025-06-23T13:30:13.096Z
Learning: In MenuItemConstants enum in nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java, the field attributeId will be renamed to value to make it more generic and appropriate for both dataset attribute identifiers and technical constants like PATH_SEPARATOR.
nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/configurations/TabbedTaskViewConstants.java (1)
Learnt from: renczesstefan
PR: #318
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java:60-62
Timestamp: 2025-06-23T13:30:13.096Z
Learning: In MenuItemConstants enum in nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java, the field attributeId will be renamed to value to make it more generic and appropriate for both dataset attribute identifiers and technical constants like PATH_SEPARATOR.
application-engine/src/main/resources/petriNets/engine-processes/preference_filter_item.xml (1)
Learnt from: renczesstefan
PR: #318
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java:60-62
Timestamp: 2025-06-23T13:30:13.096Z
Learning: In MenuItemConstants enum in nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java, the field attributeId will be renamed to value to make it more generic and appropriate for both dataset attribute identifiers and technical constants like PATH_SEPARATOR.
nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/filter/FilterConstants.java (1)
Learnt from: renczesstefan
PR: #318
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java:60-62
Timestamp: 2025-06-23T13:30:13.096Z
Learning: In MenuItemConstants enum in nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java, the field attributeId will be renamed to value to make it more generic and appropriate for both dataset attribute identifiers and technical constants like PATH_SEPARATOR.
nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/dashboard/DashboardManagementConstants.java (1)
Learnt from: renczesstefan
PR: #318
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java:60-62
Timestamp: 2025-06-23T13:30:13.096Z
Learning: In MenuItemConstants enum in nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java, the field attributeId will be renamed to value to make it more generic and appropriate for both dataset attribute identifiers and technical constants like PATH_SEPARATOR.
nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/dashboard/DashboardItemBody.java (1)
Learnt from: renczesstefan
PR: #318
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java:60-62
Timestamp: 2025-06-23T13:30:13.096Z
Learning: In MenuItemConstants enum in nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java, the field attributeId will be renamed to value to make it more generic and appropriate for both dataset attribute identifiers and technical constants like PATH_SEPARATOR.
nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java (1)
Learnt from: renczesstefan
PR: #318
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java:60-62
Timestamp: 2025-06-23T13:30:13.096Z
Learning: In MenuItemConstants enum in nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java, the field attributeId will be renamed to value to make it more generic and appropriate for both dataset attribute identifiers and technical constants like PATH_SEPARATOR.
application-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy (1)
Learnt from: renczesstefan
PR: #318
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java:60-62
Timestamp: 2025-06-23T13:30:13.096Z
Learning: In MenuItemConstants enum in nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java, the field attributeId will be renamed to value to make it more generic and appropriate for both dataset attribute identifiers and technical constants like PATH_SEPARATOR.
nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/configurations/TabbedSingleTaskViewBody.java (1)
Learnt from: renczesstefan
PR: #318
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java:60-62
Timestamp: 2025-06-23T13:30:13.096Z
Learning: In MenuItemConstants enum in nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java, the field attributeId will be renamed to value to make it more generic and appropriate for both dataset attribute identifiers and technical constants like PATH_SEPARATOR.
nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/configurations/TabbedTicketViewConstants.java (1)
Learnt from: renczesstefan
PR: #318
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java:60-62
Timestamp: 2025-06-23T13:30:13.096Z
Learning: In MenuItemConstants enum in nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java, the field attributeId will be renamed to value to make it more generic and appropriate for both dataset attribute identifiers and technical constants like PATH_SEPARATOR.
nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/configurations/TabbedSingleTaskViewConstants.java (1)
Learnt from: renczesstefan
PR: #318
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java:60-62
Timestamp: 2025-06-23T13:30:13.096Z
Learning: In MenuItemConstants enum in nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java, the field attributeId will be renamed to value to make it more generic and appropriate for both dataset attribute identifiers and technical constants like PATH_SEPARATOR.
nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/ToDataSetOutcome.java (1)
Learnt from: renczesstefan
PR: #318
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java:60-62
Timestamp: 2025-06-23T13:30:13.096Z
Learning: In MenuItemConstants enum in nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java, the field attributeId will be renamed to value to make it more generic and appropriate for both dataset attribute identifiers and technical constants like PATH_SEPARATOR.
application-engine/src/test/resources/petriNets/filter_api_test.xml (1)
Learnt from: renczesstefan
PR: #318
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java:60-62
Timestamp: 2025-06-23T13:30:13.096Z
Learning: In MenuItemConstants enum in nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java, the field attributeId will be renamed to value to make it more generic and appropriate for both dataset attribute identifiers and technical constants like PATH_SEPARATOR.
nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/dashboard/DashboardItemConstants.java (1)
Learnt from: renczesstefan
PR: #318
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java:60-62
Timestamp: 2025-06-23T13:30:13.096Z
Learning: In MenuItemConstants enum in nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java, the field attributeId will be renamed to value to make it more generic and appropriate for both dataset attribute identifiers and technical constants like PATH_SEPARATOR.
nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/configurations/TabbedCaseViewConstants.java (1)
Learnt from: renczesstefan
PR: #318
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java:60-62
Timestamp: 2025-06-23T13:30:13.096Z
Learning: In MenuItemConstants enum in nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java, the field attributeId will be renamed to value to make it more generic and appropriate for both dataset attribute identifiers and technical constants like PATH_SEPARATOR.
application-engine/src/test/groovy/com/netgrif/application/engine/action/MenuItemApiTest.groovy (1)
Learnt from: renczesstefan
PR: #318
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java:60-62
Timestamp: 2025-06-23T13:30:13.096Z
Learning: In MenuItemConstants enum in nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java, the field attributeId will be renamed to value to make it more generic and appropriate for both dataset attribute identifiers and technical constants like PATH_SEPARATOR.
nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/dashboard/DashboardToDataSetOutcome.java (1)
Learnt from: renczesstefan
PR: #318
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java:60-62
Timestamp: 2025-06-23T13:30:13.096Z
Learning: In MenuItemConstants enum in nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java, the field attributeId will be renamed to value to make it more generic and appropriate for both dataset attribute identifiers and technical constants like PATH_SEPARATOR.
nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/configurations/ViewConstants.java (1)
Learnt from: renczesstefan
PR: #318
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java:60-62
Timestamp: 2025-06-23T13:30:13.096Z
Learning: In MenuItemConstants enum in nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java, the field attributeId will be renamed to value to make it more generic and appropriate for both dataset attribute identifiers and technical constants like PATH_SEPARATOR.
application-engine/src/main/java/com/netgrif/application/engine/menu/services/interfaces/IMenuItemService.java (1)
Learnt from: renczesstefan
PR: #318
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java:60-62
Timestamp: 2025-06-23T13:30:13.096Z
Learning: In MenuItemConstants enum in nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java, the field attributeId will be renamed to value to make it more generic and appropriate for both dataset attribute identifiers and technical constants like PATH_SEPARATOR.
nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemView.java (1)
Learnt from: renczesstefan
PR: #318
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java:60-62
Timestamp: 2025-06-23T13:30:13.096Z
Learning: In MenuItemConstants enum in nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java, the field attributeId will be renamed to value to make it more generic and appropriate for both dataset attribute identifiers and technical constants like PATH_SEPARATOR.
application-engine/src/main/java/com/netgrif/application/engine/menu/services/MenuItemService.java (1)
Learnt from: renczesstefan
PR: #318
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java:60-62
Timestamp: 2025-06-23T13:30:13.096Z
Learning: In MenuItemConstants enum in nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java, the field attributeId will be renamed to value to make it more generic and appropriate for both dataset attribute identifiers and technical constants like PATH_SEPARATOR.
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticPetriNet.java (1)
Learnt from: renczesstefan
PR: #318
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java:60-62
Timestamp: 2025-06-23T13:30:13.096Z
Learning: In MenuItemConstants enum in nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java, the field attributeId will be renamed to value to make it more generic and appropriate for both dataset attribute identifiers and technical constants like PATH_SEPARATOR.
application-engine/src/main/resources/petriNets/engine-processes/menu/menu_item.xml (1)
Learnt from: renczesstefan
PR: #318
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java:60-62
Timestamp: 2025-06-23T13:30:13.096Z
Learning: In MenuItemConstants enum in nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java, the field attributeId will be renamed to value to make it more generic and appropriate for both dataset attribute identifiers and technical constants like PATH_SEPARATOR.
nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemBody.java (1)
Learnt from: renczesstefan
PR: #318
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java:60-62
Timestamp: 2025-06-23T13:30:13.096Z
Learning: In MenuItemConstants enum in nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java, the field attributeId will be renamed to value to make it more generic and appropriate for both dataset attribute identifiers and technical constants like PATH_SEPARATOR.
nae-object-library/src/main/java/com/netgrif/application/engine/objects/utils/MenuItemUtils.java (1)
Learnt from: renczesstefan
PR: #318
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java:60-62
Timestamp: 2025-06-23T13:30:13.096Z
Learning: In MenuItemConstants enum in nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java, the field attributeId will be renamed to value to make it more generic and appropriate for both dataset attribute identifiers and technical constants like PATH_SEPARATOR.
🪛 GitHub Actions: Pull Request
nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/dashboard/DashboardManagementBody.java
[warning] 42-54: @builder will ignore the initializing expression entirely on multiple fields. Add @Builder.Default or make the field final to fix.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
🔇 Additional comments (59)
application-engine/src/main/java/com/netgrif/application/engine/workflow/web/WorkflowController.java (1)
123-123: No functional impact – formatting only
The extra blank line is harmless and does not warrant a change.application-engine/src/main/resources/application.yaml (1)
161-162:actionssection misplaced – will NOT be picked up by Spring logging config
actions:is indented at the same level aslogging:instead of being nested underlogging.level.
Spring will therefore ignore the customimports, and log-parsing or classpath scanning that relies on it will silently fail.- actions: - imports: com.netgrif.application.engine.objects.workflow.domain.menu.MenuItemBody,com.netgrif.application.engine.objects.petrinet.domain.I18nString,com.netgrif.application.engine.objects.workflow.domain.menu.dashboard.DashboardManagementBody + level: + actions: + imports: com.netgrif.application.engine.objects.workflow.domain.menu.MenuItemBody,com.netgrif.application.engine.objects.petrinet.domain.I18nString,com.netgrif.application.engine.objects.workflow.domain.menu.dashboard.DashboardManagementBodyLikely an incorrect or invalid review comment.
application-engine/CHANGELOG.md (1)
8-21: Documentation only – looks good
The updated links and release notes read correctly.application-engine/src/main/java/com/netgrif/application/engine/elastic/service/interfaces/IElasticPetriNetService.java (2)
3-11: LGTM! Well-designed search interface.The new imports and interface method follow Spring conventions well. The search method signature includes all necessary parameters for comprehensive Petri net searching: search criteria, user context, pagination, localization, and intersection logic.
22-22: Comprehensive search method signature.The search method provides a well-structured interface with appropriate parameters for filtering, pagination, and user context. The
Boolean isIntersectionparameter adds flexibility for different search modes.nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/ElasticPetriNet.java (2)
13-16: Appropriate imports for I18n transformation.The new imports support the internationalization field transformation functionality, adding necessary collection types and I18nString handling.
67-73: Well-implemented I18n transformation method.The
transformToFieldmethod correctly extracts internationalization data fromI18nStringand creates a structuredI18nField. The implementation properly:
- Extracts translation keys and values
- Creates a copy of the translations map
- Includes the default value in the values set
- Returns a properly constructed I18nField
This aligns well with the broader internationalization refactoring effort.
application-engine/src/main/resources/petriNets/engine-processes/preference_filter_item.xml (1)
419-419: Correct case ID access pattern.The change from
it._id.eq(...)toit._id.objectId.eq(...)properly accesses theobjectIdproperty for case comparison. This refinement aligns with the broader refactoring to useProcessResourceIdobjects consistently and ensures correct case matching logic.application-engine/src/main/groovy/com/netgrif/application/engine/startup/DashboardManagementRunner.groovy (3)
10-14: Well-structured startup runner.The class follows established patterns with appropriate annotations:
@RunnerOrder(141)provides clear execution ordering@ConditionalOnPropertyallows flexible enabling/disabling- Component registration is correct
19-24: Good constant definitions.The constants are well-named and follow naming conventions. Having both public identifiers and private file names provides good encapsulation while allowing external access to the net identifiers when needed.
26-29: Clean import implementation.The run method properly delegates to
ImportHelperfor both dashboard management and dashboard item Petri nets. The descriptive names in the import calls improve logging and debugging.application-engine/src/main/java/com/netgrif/application/engine/menu/services/interfaces/DashboardItemService.java (2)
1-6: Appropriate imports and package structure.The imports are minimal and focused, including only necessary types for the interface. The package structure follows the established pattern for service interfaces.
7-14: Well-designed service interface.The interface follows established service patterns:
getOrCreatemethod provides idempotent entity creationupdatemethod takes both case and body for proper updatesfindByIdprovides simple lookup functionality- Appropriate use of
TransitionNotExecutableExceptionfor workflow-related errors- Clean method signatures with clear return types
application-engine/src/test/resources/petriNets/filter_api_test.xml (1)
113-122: LGTM! Excellent consolidation of filter and menu item management.The refactoring from multiple granular calls to a single
createOrUpdateMenuItemAndFilterservice method improves code maintainability and aligns with the new service-based architecture. The metadata structure properly encapsulates filter configuration properties, and all necessary parameters are correctly passed to the unified method.nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticPetriNet.java (1)
60-60: Well-designed abstract method for type transformation.The abstract
transformToFieldmethod properly enforces subclasses to implement the conversion logic fromI18nStringtoI18nField, supporting the internationalization enhancements across the application.nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/filter/FilterConstants.java (1)
1-22: Excellent constants organization with clear logical grouping.The class follows Java best practices with proper naming conventions and centralizes filter-related constants effectively. The logical grouping (transitions, field IDs, ISO codes, filter types, visibility) enhances readability and maintainability.
nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/configurations/TabbedCaseViewBody.java (2)
16-32: Excellent view configuration design with sensible defaults.The class properly extends
ViewBodyand provides well-thought-out default values for tabbed case view properties. The use of descriptive field names and appropriate data types enhances maintainability.
44-74: Robust dataset transformation with proper null handling.The
toDataSetInternalmethod correctly maps all fields to their appropriate dataset types and includes proper null-safe handling for collections and optional fields.application-engine/src/test/resources/dashboard_management_test.xml (1)
1-8: LGTM: Well-structured XML process definition.The XML structure follows the Petriflow schema correctly with appropriate metadata fields for the dashboard import process.
nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/dashboard/DashboardItemBody.java (1)
10-13: LGTM: Appropriate use of Lombok annotations.The combination of
@Data,@Builder, and@AllArgsConstructorprovides comprehensive boilerplate generation for the domain class.nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/FilterBody.java (1)
14-16: LGTM: Clean domain class structure.The class uses appropriate Lombok annotations and follows good practices for a data transfer object.
application-engine/src/test/groovy/com/netgrif/application/engine/action/MenuItemApiTest.groovy (2)
76-82: LGTM: Updated to use new field constants.The test correctly uses the new
MenuItemConstants.FIELD_*constants, aligning with the refactored domain model.
89-102: Excellent: Enhanced validation for tabbed view configurations.The new assertions properly validate the nested tabbed view structure and relationships using
MenuItemUtils.getCaseIdFromCaseRef, providing comprehensive test coverage for the new view configuration feature.application-engine/src/main/java/com/netgrif/application/engine/petrinet/web/PetriNetController.java (1)
203-204: Ensure security annotation onsearchElasticPetriNets
We observed three methods in PetriNetController (lines 96, 179, 217) secured with@PreAuthorize, but thesearchElasticPetriNetsendpoint at line 203 currently has none. Please confirm whether it should enforce the same authorization (for example,@authorizationService.hasAuthority('ADMIN')) or another appropriate check:
- application-engine/src/main/java/com/netgrif/application/engine/petrinet/web/PetriNetController.java: insert an
@PreAuthorizeannotation above thesearchElasticPetriNetsmethod.nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/configurations/TabbedTaskViewBody.java (1)
40-62: Consider adding handling for thefilterfield in toDataSetInternalWe ran an AST search for other
ViewBodysubclasses but didn’t find any implementations to compare against. Without further evidence, it’s unclear whether omittingfilteris intentional. Please:
- Verify if the
filterproperty on this class should be included in the dataset.- If it’s required, add a
outcome.putDataSetEntry(...)call forfilter.- Otherwise, document why it’s deliberately excluded.
nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/configurations/ViewBody.java (1)
66-82: Well-structured dataset transformation logic.The method properly handles null cases and delegates specific transformations to subclasses through the abstract
toDataSetInternalmethod. Good separation of concerns.nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java (1)
7-39: Clean refactoring to static constants.The conversion from enum to a final class with static String constants is appropriate. The consistent naming convention with
FIELD_andTRANS_prefixes improves code clarity and aligns with the learning about making the constants more generic.application-engine/src/main/java/com/netgrif/application/engine/startup/runner/FilterRunner.java (1)
60-74: All configuration XML files are presentWe’ve confirmed that for each
MenuItemViewidentifier (tabbed_case_view,tabbed_task_view,tabbed_ticket_view,tabbed_single_task_view), there is a matching_configuration.xmlfile under
application-engine/src/main/resources/petriNets/engine-processes/menu/. No missing files detected.application-engine/src/main/resources/petriNets/engine-processes/dashboard_management.xml (1)
683-700: Consider transaction safety for the is_active flag updateThe
is_activeflag is set to true (line 697) before the validation completes. If the validation fails and throws an exception, the flag might remain in an inconsistent state. Consider validating first, then updating the flag as the last operation.<actions phase="pre"> <action> dashboard_id: f.dashboard_id, dashboard_name: f.dashboard_name, is_active: f.is_active; if (dashboard_id.value == null || dashboard_id.value == "") { throw new IllegalArgumentException("Dashboard ID cannot be empty.") } def dashboardWithIdExists = findCaseElastic("processIdentifier:\"dashboard_management\" AND dataSet.dashboard_id.textValue.keyword:\"${dashboard_id.value}\" AND dataSet.is_active.booleanValue:true" as String) if (dashboardWithIdExists != null) { throw new IllegalArgumentException("Dashboard with given ID already exists.") } + // Set is_active only after all validations pass change is_active value { true } </action> </actions>Likely an incorrect or invalid review comment.
application-engine/src/main/resources/petriNets/engine-processes/menu/tabbed_task_view_configuration.xml (6)
1-41: LGTM!The document structure and role definitions follow standard Petri net configuration patterns with appropriate permission levels.
42-93: LGTM!The Groovy functions correctly handle dynamic filter autocomplete and option synchronization with proper null checks and XML-encoded operators.
95-224: LGTM!Filter configuration data fields are properly structured with appropriate validation, type restrictions, and action triggers for filter management.
226-317: LGTM!Task view configuration fields provide comprehensive display options with proper data validation and real-time updates through immediate flags.
319-371: LGTM!Complete i18n implementation with consistent translations for both Slovak and German locales.
373-718: LGTM!Process flow definition with transitions, places, and arcs is properly structured with appropriate role-based access control and comprehensive UI layout configuration.
application-engine/src/main/resources/petriNets/engine-processes/dashboard_item.xml (4)
1-46: LGTM!Document structure with appropriate role definitions and delete event handler for maintaining referential integrity.
47-65: LGTM!The removeRefFromParent function properly maintains referential integrity by updating all dashboard_management cases that reference the deleted item.
387-441: LGTM!Complete i18n implementation with consistent Slovak and German translations.
443-762: LGTM!Well-structured configuration transition with comprehensive validation and proper state management through the is_active flag.
nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemBody.java (4)
1-37: LGTM!Good refactoring to consolidate view-related fields into ViewBody abstraction and initialize maps to prevent null pointer exceptions.
39-88: LGTM!Well-designed constructor overloads providing flexibility while ensuring proper initialization of all required fields.
89-119: LGTM!Methods properly handle null cases and correctly derive the useTabbedView flag from the view type.
121-184: LGTM!Well-structured toDataSet methods with progressive enhancement through overloads and improved type safety using ToDataSetOutcome.
application-engine/src/main/resources/petriNets/engine-processes/menu/tabbed_ticket_view_configuration.xml (6)
1-51: LGTM!Standard document structure with appropriate role definitions and async cleanup on delete.
52-90: LGTM!Functions properly validate filter types and use async operations for non-blocking cleanup.
92-194: LGTM!Filter configuration properly validates for Case type filters with appropriate null handling.
196-222: LGTM!Associated view configuration supports nested view structures with dynamic option loading.
224-255: LGTM!Minimal view configuration with complete internationalization support.
257-405: LGTM!Well-structured transitions with proper lifecycle management for associated view configurations.
application-engine/src/main/java/com/netgrif/application/engine/menu/services/MenuItemService.java (7)
1-48: LGTM!Well-structured service class with appropriate dependencies and Spring annotations.
49-83: LGTM!Filter operations properly handle user context and use appropriate abstractions.
85-186: LGTM!Comprehensive CRUD operations with proper validation, automatic parent folder creation, and flexible creation patterns.
188-239: LGTM!Efficient search operations with proper Elasticsearch query construction and optimized existence checks.
252-252: Address the TODO commentThere's an unimplemented TODO that should be addressed or removed if no longer relevant.
241-289: LGTM!The moveItem method properly validates cyclic paths and maintains parent-child relationships with recursive updates.
385-747: LGTM!Comprehensive helper methods for folder management, view lifecycle, and data operations with proper error handling.
application-engine/src/main/resources/petriNets/engine-processes/menu/menu_item.xml (1)
85-93: Ensure Lucene query parameters are properly escaped
IfuseCase.stringIdcontains special Lucene characters (+ - && || ! ( ) { } [ ] ^ " ~ * ? : \ /), the unescaped string may break the query or broaden matches.• Wrap the dynamic value in an escape helper, e.g.:
def escapedId = org.apache.lucene.queryparser.classic.QueryParser.escape(useCase.stringId) def cases = findCasesElastic( "processIdentifier:\"dashboard_item\" AND dataSet.menu_items_list.keyValue:\"${escapedId}\"" )• Alternatively, use any existing
escapeLucene()or parameter-binding method provided by your Elastic service.
• Verify inActionDelegate.findCasesElastic(…)that queries are built with escaped inputs or safe parameter binding.application-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy (2)
1619-1628: Good refactoring to service-based approach.The method now properly uses the
FilterBodydomain object and delegates tomenuItemService.createFilter(). This is a clean abstraction that improves modularity and maintainability.
1879-1926: Well-structured refactoring of changeMenuItem method.The method now properly uses the new
MenuItemConstantsfields and delegates URI changes tomenuItemService.moveItem(). This ensures proper validation and consistency.
| if (path.lastIndexOf(MenuItemConstants.PATH_SEPARATOR) == 0) { | ||
| return path.replace(MenuItemConstants.PATH_SEPARATOR, "") | ||
| } | ||
| return path.substring(path.lastIndexOf(MenuItemConstants.PATH_SEPARATOR.value)) | ||
| return path.substring(path.lastIndexOf(MenuItemConstants.PATH_SEPARATOR)) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing .value property access on PATH_SEPARATOR constant.
The code is inconsistent with PATH_SEPARATOR usage. Based on other usages in the file, it should access the .value property.
- if (path.lastIndexOf(MenuItemConstants.PATH_SEPARATOR) == 0) {
- return path.replace(MenuItemConstants.PATH_SEPARATOR, "")
+ if (path.lastIndexOf(MenuItemConstants.PATH_SEPARATOR.value) == 0) {
+ return path.replace(MenuItemConstants.PATH_SEPARATOR.value, "")
}
- return path.substring(path.lastIndexOf(MenuItemConstants.PATH_SEPARATOR))
+ return path.substring(path.lastIndexOf(MenuItemConstants.PATH_SEPARATOR.value) + 1)📝 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.
| if (path.lastIndexOf(MenuItemConstants.PATH_SEPARATOR) == 0) { | |
| return path.replace(MenuItemConstants.PATH_SEPARATOR, "") | |
| } | |
| return path.substring(path.lastIndexOf(MenuItemConstants.PATH_SEPARATOR.value)) | |
| return path.substring(path.lastIndexOf(MenuItemConstants.PATH_SEPARATOR)) | |
| } | |
| if (path.lastIndexOf(MenuItemConstants.PATH_SEPARATOR.value) == 0) { | |
| return path.replace(MenuItemConstants.PATH_SEPARATOR.value, "") | |
| } | |
| return path.substring(path.lastIndexOf(MenuItemConstants.PATH_SEPARATOR.value) + 1) | |
| } |
🤖 Prompt for AI Agents
In
application-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy
around lines 2213 to 2217, the constant MenuItemConstants.PATH_SEPARATOR is used
without accessing its .value property, which is inconsistent with other usages
in the file. Update all occurrences of MenuItemConstants.PATH_SEPARATOR in this
code block to MenuItemConstants.PATH_SEPARATOR.value to correctly access the
string value of the constant.
| protected String parentPath(String path) { | ||
| if (path == null || path == MenuItemConstants.PATH_SEPARATOR.value || path.length() == 0 || path.lastIndexOf(MenuItemConstants.PATH_SEPARATOR.value) == 0) { | ||
| return MenuItemConstants.PATH_SEPARATOR.value | ||
| if (path == null || path == MenuItemConstants.PATH_SEPARATOR || path.length() == 0 || path.lastIndexOf(MenuItemConstants.PATH_SEPARATOR) == 0) { | ||
| return MenuItemConstants.PATH_SEPARATOR | ||
| } | ||
| return path.substring(0, path.lastIndexOf(MenuItemConstants.PATH_SEPARATOR.value)) | ||
| return path.substring(0, path.lastIndexOf(MenuItemConstants.PATH_SEPARATOR)) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing .value property access on PATH_SEPARATOR constant.
Same issue as in nameFromPath method - missing .value property access.
- if (path == null || path == MenuItemConstants.PATH_SEPARATOR || path.length() == 0 || path.lastIndexOf(MenuItemConstants.PATH_SEPARATOR) == 0) {
- return MenuItemConstants.PATH_SEPARATOR
+ if (path == null || path == MenuItemConstants.PATH_SEPARATOR.value || path.length() == 0 || path.lastIndexOf(MenuItemConstants.PATH_SEPARATOR.value) == 0) {
+ return MenuItemConstants.PATH_SEPARATOR.value
}
- return path.substring(0, path.lastIndexOf(MenuItemConstants.PATH_SEPARATOR))
+ return path.substring(0, path.lastIndexOf(MenuItemConstants.PATH_SEPARATOR.value))🤖 Prompt for AI Agents
In
application-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy
around lines 2219 to 2224, the PATH_SEPARATOR constant is used without accessing
its .value property, which is required. Update all occurrences of
MenuItemConstants.PATH_SEPARATOR to MenuItemConstants.PATH_SEPARATOR.value to
correctly reference the string value of the constant.
| List<String> splitUriPath(String path) { | ||
| String rootPath = MenuItemConstants.PATH_SEPARATOR.value | ||
| String[] splitPath = path.split(MenuItemConstants.PATH_SEPARATOR.value) | ||
| String rootPath = MenuItemConstants.PATH_SEPARATOR | ||
| String[] splitPath = path.split(MenuItemConstants.PATH_SEPARATOR) | ||
| if (splitPath.length == 0 && path == rootPath) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing .value property access on PATH_SEPARATOR constant.
Consistency issue with PATH_SEPARATOR usage.
- String rootPath = MenuItemConstants.PATH_SEPARATOR
- String[] splitPath = path.split(MenuItemConstants.PATH_SEPARATOR)
+ String rootPath = MenuItemConstants.PATH_SEPARATOR.value
+ String[] splitPath = path.split(MenuItemConstants.PATH_SEPARATOR.value)🤖 Prompt for AI Agents
In
application-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy
around lines 2713 to 2716, the PATH_SEPARATOR constant is used without accessing
its .value property, causing inconsistency. Update all occurrences of
MenuItemConstants.PATH_SEPARATOR to MenuItemConstants.PATH_SEPARATOR.value to
ensure consistent and correct usage of the constant's value.
| @RequiredArgsConstructor | ||
| class DefaultDashboardRunner implements ApplicationEngineStartupRunner { | ||
|
|
||
| @Autowired | ||
| private DashboardManagementService dashboardManagementService |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Use consistent dependency injection pattern.
The class uses both constructor injection (@RequiredArgsConstructor) and field injection (@Autowired) which is inconsistent. Choose one pattern for clarity and maintainability.
Use constructor injection consistently:
@Slf4j
@Component
@RunnerOrder(142)
-@RequiredArgsConstructor
-class DefaultDashboardRunner implements ApplicationEngineStartupRunner {
+class DefaultDashboardRunner implements ApplicationEngineStartupRunner {
- @Autowired
- private DashboardManagementService dashboardManagementService
+ private final DashboardManagementService dashboardManagementService
+
+ DefaultDashboardRunner(DashboardManagementService dashboardManagementService) {
+ this.dashboardManagementService = dashboardManagementService
+ }🤖 Prompt for AI Agents
In
application-engine/src/main/groovy/com/netgrif/application/engine/startup/DefaultDashboardRunner.groovy
around lines 17 to 21, the class mixes constructor injection
(@RequiredArgsConstructor) and field injection (@Autowired) for dependencies. To
fix this, remove the @Autowired annotation and the field injection, and instead
declare the DashboardManagementService as a final field initialized via the
constructor generated by @RequiredArgsConstructor, ensuring consistent use of
constructor injection throughout the class.
...-engine/src/main/groovy/com/netgrif/application/engine/startup/DefaultDashboardRunner.groovy
Outdated
Show resolved
Hide resolved
| if (this.id != null) { | ||
| outcome.putDataSetEntry(DashboardManagementConstants.FIELD_ID, FieldType.TEXT, this.id); | ||
| } | ||
| if (this.id != null) { | ||
| outcome.putDataSetEntry(DashboardManagementConstants.FIELD_NAME, FieldType.I18N, this.name); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix copy-paste error in null check.
Line 67 checks this.id instead of this.name, which appears to be a copy-paste error.
if (this.id != null) {
outcome.putDataSetEntry(DashboardManagementConstants.FIELD_ID, FieldType.TEXT, this.id);
}
-if (this.id != null) {
+if (this.name != null) {
outcome.putDataSetEntry(DashboardManagementConstants.FIELD_NAME, FieldType.I18N, this.name);
}📝 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.
| if (this.id != null) { | |
| outcome.putDataSetEntry(DashboardManagementConstants.FIELD_ID, FieldType.TEXT, this.id); | |
| } | |
| if (this.id != null) { | |
| outcome.putDataSetEntry(DashboardManagementConstants.FIELD_NAME, FieldType.I18N, this.name); | |
| } | |
| if (this.id != null) { | |
| outcome.putDataSetEntry(DashboardManagementConstants.FIELD_ID, FieldType.TEXT, this.id); | |
| } | |
| if (this.name != null) { | |
| outcome.putDataSetEntry(DashboardManagementConstants.FIELD_NAME, FieldType.I18N, this.name); | |
| } |
🤖 Prompt for AI Agents
In
nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/dashboard/DashboardManagementBody.java
around lines 64 to 69, the null check before adding the FIELD_NAME entry
incorrectly checks this.id instead of this.name. Change the condition on line 67
to check if this.name is not null before calling outcome.putDataSetEntry for
FIELD_NAME.
| } | ||
| outcome.putDataSetEntry(DashboardManagementConstants.FIELD_SIMPLE_DASHBOARD, FieldType.BOOLEAN, this.simpleDashboard); | ||
| outcome.putDataSetEntry(DashboardManagementConstants.FIELD_PROFILE_DASHBOARD, FieldType.BOOLEAN, this.profileDashboard); | ||
| outcome.putDataSetEntry(DashboardManagementConstants.FIELD_LANGUAGE_DASHBOARD, FieldType.BOOLEAN, this.profileDashboard); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incorrect field value for language dashboard.
The language dashboard field is being set with profileDashboard value instead of languageDashboard.
-outcome.putDataSetEntry(DashboardManagementConstants.FIELD_LANGUAGE_DASHBOARD, FieldType.BOOLEAN, this.profileDashboard);
+outcome.putDataSetEntry(DashboardManagementConstants.FIELD_LANGUAGE_DASHBOARD, FieldType.BOOLEAN, this.languageDashboard);📝 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.
| outcome.putDataSetEntry(DashboardManagementConstants.FIELD_LANGUAGE_DASHBOARD, FieldType.BOOLEAN, this.profileDashboard); | |
| outcome.putDataSetEntry(DashboardManagementConstants.FIELD_LANGUAGE_DASHBOARD, FieldType.BOOLEAN, this.languageDashboard); |
🤖 Prompt for AI Agents
In
nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/dashboard/DashboardManagementBody.java
at line 75, the field for the language dashboard is incorrectly assigned the
value of profileDashboard. Replace the value passed to putDataSetEntry for
FIELD_LANGUAGE_DASHBOARD from this.profileDashboard to this.languageDashboard to
correctly reflect the language dashboard state.
| public void putDataSetEntryWithOptions(String fieldId, FieldType fieldType, @Nullable Map<String, I18nString> options, @Nullable Object fieldValue){ | ||
| Map<String, Object> fieldMap = new LinkedHashMap<>(); | ||
|
|
||
| fieldMap.put("type", fieldType.getName()); | ||
| if (fieldType.equals(FieldType.MULTICHOICE_MAP)) { | ||
| fieldMap.put("value", Set.of()); | ||
| } | ||
| if (options == null) { | ||
| options = new HashMap<>(); | ||
| } | ||
| fieldMap.put("options", options); | ||
| fieldMap.put("value", fieldValue); | ||
| this.getDataSet().put(fieldId, fieldMap); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clarify MULTICHOICE_MAP handling logic and add documentation.
The method has a logical issue with MULTICHOICE_MAP handling and lacks documentation:
- Lines 25-27 set the value to an empty Set for MULTICHOICE_MAP, but line 32 immediately overwrites it with
fieldValue - Missing JavaDoc documentation explaining the method's purpose and parameters
Fix the MULTICHOICE_MAP logic and add documentation:
+ /**
+ * Adds a dataset entry with field type, options, and value to the dataset.
+ * @param fieldId the field identifier
+ * @param fieldType the type of the field
+ * @param options optional field options map, null will be converted to empty map
+ * @param fieldValue the field value to set
+ */
public void putDataSetEntryWithOptions(String fieldId, FieldType fieldType, @Nullable Map<String, I18nString> options, @Nullable Object fieldValue){
Map<String, Object> fieldMap = new LinkedHashMap<>();
fieldMap.put("type", fieldType.getName());
- if (fieldType.equals(FieldType.MULTICHOICE_MAP)) {
- fieldMap.put("value", Set.of());
- }
if (options == null) {
options = new HashMap<>();
}
fieldMap.put("options", options);
- fieldMap.put("value", fieldValue);
+ // Set default empty set for MULTICHOICE_MAP if fieldValue is null
+ if (fieldType.equals(FieldType.MULTICHOICE_MAP) && fieldValue == null) {
+ fieldMap.put("value", Set.of());
+ } else {
+ fieldMap.put("value", fieldValue);
+ }
this.getDataSet().put(fieldId, fieldMap);
}📝 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.
| public void putDataSetEntryWithOptions(String fieldId, FieldType fieldType, @Nullable Map<String, I18nString> options, @Nullable Object fieldValue){ | |
| Map<String, Object> fieldMap = new LinkedHashMap<>(); | |
| fieldMap.put("type", fieldType.getName()); | |
| if (fieldType.equals(FieldType.MULTICHOICE_MAP)) { | |
| fieldMap.put("value", Set.of()); | |
| } | |
| if (options == null) { | |
| options = new HashMap<>(); | |
| } | |
| fieldMap.put("options", options); | |
| fieldMap.put("value", fieldValue); | |
| this.getDataSet().put(fieldId, fieldMap); | |
| } | |
| /** | |
| * Adds a dataset entry with field type, options, and value to the dataset. | |
| * @param fieldId the field identifier | |
| * @param fieldType the type of the field | |
| * @param options optional field options map, null will be converted to empty map | |
| * @param fieldValue the field value to set | |
| */ | |
| public void putDataSetEntryWithOptions(String fieldId, FieldType fieldType, @Nullable Map<String, I18nString> options, @Nullable Object fieldValue){ | |
| Map<String, Object> fieldMap = new LinkedHashMap<>(); | |
| fieldMap.put("type", fieldType.getName()); | |
| if (options == null) { | |
| options = new HashMap<>(); | |
| } | |
| fieldMap.put("options", options); | |
| // Set default empty set for MULTICHOICE_MAP if fieldValue is null | |
| if (fieldType.equals(FieldType.MULTICHOICE_MAP) && fieldValue == null) { | |
| fieldMap.put("value", Set.of()); | |
| } else { | |
| fieldMap.put("value", fieldValue); | |
| } | |
| this.getDataSet().put(fieldId, fieldMap); | |
| } |
🤖 Prompt for AI Agents
In
nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/dashboard/DashboardToDataSetOutcome.java
between lines 21 and 34, the MULTICHOICE_MAP handling sets the value to an empty
Set but then immediately overwrites it with fieldValue, making the initial
assignment ineffective. Fix this by setting the value to an empty Set only if
fieldValue is null or not provided for MULTICHOICE_MAP. Also, add JavaDoc
comments above the method to explain its purpose, parameters, and behavior
clearly.
| public ToDataSetOutcome toDataSet() { | ||
| ToDataSetOutcome outcome = new ToDataSetOutcome(); | ||
|
|
||
| outcome.putDataSetEntry(FilterConstants.FILTER_TYPE_FIELD_ID, FieldType.ENUMERATION_MAP, this.type); | ||
| outcome.putDataSetEntry(FilterConstants.FILTER_VISIBILITY_FIELD_ID, FieldType.ENUMERATION_MAP, this.visibility); | ||
| outcome.putDataSetEntry(FilterConstants.FILTER_I18N_TITLE_FIELD_ID, FieldType.I18N, this.title); | ||
| Map<String, Object> metadata = this.metadata; | ||
| if (metadata == null) { | ||
| metadata = getDefaultMetadata(this.type); | ||
| } | ||
| outcome.getDataSet().put(FilterConstants.FILTER_FIELD_ID, Map.of( | ||
| "type", "filter", | ||
| "value", this.query, | ||
| "allowedNets", this.allowedNets, | ||
| "filterMetadata", metadata | ||
| )); | ||
|
|
||
| return outcome; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Add validation for required fields in toDataSet method.
The method converts fields to dataset entries but doesn't validate that required fields like type and query are not null or empty.
public ToDataSetOutcome toDataSet() {
+ if (this.type == null || this.type.trim().isEmpty()) {
+ throw new IllegalStateException("Filter type is required");
+ }
+ if (this.query == null || this.query.trim().isEmpty()) {
+ throw new IllegalStateException("Filter query is required");
+ }
+
ToDataSetOutcome outcome = new ToDataSetOutcome();
// ... rest of the method📝 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.
| public ToDataSetOutcome toDataSet() { | |
| ToDataSetOutcome outcome = new ToDataSetOutcome(); | |
| outcome.putDataSetEntry(FilterConstants.FILTER_TYPE_FIELD_ID, FieldType.ENUMERATION_MAP, this.type); | |
| outcome.putDataSetEntry(FilterConstants.FILTER_VISIBILITY_FIELD_ID, FieldType.ENUMERATION_MAP, this.visibility); | |
| outcome.putDataSetEntry(FilterConstants.FILTER_I18N_TITLE_FIELD_ID, FieldType.I18N, this.title); | |
| Map<String, Object> metadata = this.metadata; | |
| if (metadata == null) { | |
| metadata = getDefaultMetadata(this.type); | |
| } | |
| outcome.getDataSet().put(FilterConstants.FILTER_FIELD_ID, Map.of( | |
| "type", "filter", | |
| "value", this.query, | |
| "allowedNets", this.allowedNets, | |
| "filterMetadata", metadata | |
| )); | |
| return outcome; | |
| } | |
| public ToDataSetOutcome toDataSet() { | |
| if (this.type == null || this.type.trim().isEmpty()) { | |
| throw new IllegalStateException("Filter type is required"); | |
| } | |
| if (this.query == null || this.query.trim().isEmpty()) { | |
| throw new IllegalStateException("Filter query is required"); | |
| } | |
| ToDataSetOutcome outcome = new ToDataSetOutcome(); | |
| outcome.putDataSetEntry(FilterConstants.FILTER_TYPE_FIELD_ID, FieldType.ENUMERATION_MAP, this.type); | |
| outcome.putDataSetEntry(FilterConstants.FILTER_VISIBILITY_FIELD_ID, FieldType.ENUMERATION_MAP, this.visibility); | |
| outcome.putDataSetEntry(FilterConstants.FILTER_I18N_TITLE_FIELD_ID, FieldType.I18N, this.title); | |
| Map<String, Object> metadata = this.metadata; | |
| if (metadata == null) { | |
| metadata = getDefaultMetadata(this.type); | |
| } | |
| outcome.getDataSet().put(FilterConstants.FILTER_FIELD_ID, Map.of( | |
| "type", "filter", | |
| "value", this.query, | |
| "allowedNets", this.allowedNets, | |
| "filterMetadata", metadata | |
| )); | |
| return outcome; | |
| } |
🤖 Prompt for AI Agents
In
nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/FilterBody.java
between lines 54 and 72, the toDataSet method lacks validation for required
fields such as type and query. Add checks at the start of the method to ensure
that type and query are not null or empty, and throw an appropriate exception or
handle the error if they are invalid. This will prevent null or empty values
from being processed and ensure data integrity.
| public static MenuItemView fromIdentifier(String identifier) { | ||
| for (MenuItemView view : MenuItemView.values()) { | ||
| if (view.identifier.equals(identifier)) { | ||
| return view; | ||
| } | ||
| } | ||
| throw new IllegalArgumentException(identifier); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Add null check and improve error message.
The method should validate the input parameter and provide a more descriptive error message.
public static MenuItemView fromIdentifier(String identifier) {
+ if (identifier == null) {
+ throw new IllegalArgumentException("Identifier cannot be null");
+ }
for (MenuItemView view : MenuItemView.values()) {
if (view.identifier.equals(identifier)) {
return view;
}
}
- throw new IllegalArgumentException(identifier);
+ throw new IllegalArgumentException("Unknown MenuItemView identifier: " + identifier);
}📝 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.
| public static MenuItemView fromIdentifier(String identifier) { | |
| for (MenuItemView view : MenuItemView.values()) { | |
| if (view.identifier.equals(identifier)) { | |
| return view; | |
| } | |
| } | |
| throw new IllegalArgumentException(identifier); | |
| } | |
| public static MenuItemView fromIdentifier(String identifier) { | |
| if (identifier == null) { | |
| throw new IllegalArgumentException("Identifier cannot be null"); | |
| } | |
| for (MenuItemView view : MenuItemView.values()) { | |
| if (view.identifier.equals(identifier)) { | |
| return view; | |
| } | |
| } | |
| throw new IllegalArgumentException("Unknown MenuItemView identifier: " + identifier); | |
| } |
🤖 Prompt for AI Agents
In
nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemView.java
around lines 52 to 59, add a null check for the identifier parameter at the
start of the fromIdentifier method. If the identifier is null, throw an
IllegalArgumentException with a clear message indicating that the identifier
cannot be null. Also, improve the existing exception message to include the
invalid identifier value for better clarity.
- Remove unused `uriService` references and outdated assertions. - Simplify folder case retrieval and path handling logic. - Adjust duplicate menu item node path creation. - Streamline `filter_api_test.xml` to remove unnecessary node lookups.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Nitpick comments (1)
application-engine/src/main/java/com/netgrif/application/engine/menu/services/interfaces/IMenuItemService.java (1)
66-69: Extract magic string to constantThe string
"_configuration"should be defined as a constant to improve maintainability and avoid typos.+ private static final String CONFIGURATION_SUFFIX = "_configuration"; + default Map<String, I18nString> getAvailableViewsAsOptions(boolean isTabbed, String viewIdentifier) { - int index = viewIdentifier.lastIndexOf("_configuration"); + int index = viewIdentifier.lastIndexOf(CONFIGURATION_SUFFIX); if (index > 0) { viewIdentifier = viewIdentifier.substring(0, index); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticPetriNetService.java(3 hunks)application-engine/src/main/java/com/netgrif/application/engine/menu/services/MenuItemService.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/menu/services/interfaces/IMenuItemService.java(1 hunks)application-engine/src/main/resources/petriNets/engine-processes/menu/tabbed_case_view_configuration.xml(1 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/action/MenuItemApiTest.groovy(9 hunks)application-engine/src/test/resources/petriNets/filter_api_test.xml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- application-engine/src/test/resources/petriNets/filter_api_test.xml
- application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticPetriNetService.java
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: renczesstefan
PR: netgrif/application-engine#318
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java:60-62
Timestamp: 2025-06-23T13:30:13.096Z
Learning: In MenuItemConstants enum in nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java, the field `attributeId` will be renamed to `value` to make it more generic and appropriate for both dataset attribute identifiers and technical constants like PATH_SEPARATOR.
application-engine/src/main/java/com/netgrif/application/engine/menu/services/interfaces/IMenuItemService.java (1)
Learnt from: renczesstefan
PR: #318
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java:60-62
Timestamp: 2025-06-23T13:30:13.096Z
Learning: In MenuItemConstants enum in nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java, the field attributeId will be renamed to value to make it more generic and appropriate for both dataset attribute identifiers and technical constants like PATH_SEPARATOR.
application-engine/src/test/groovy/com/netgrif/application/engine/action/MenuItemApiTest.groovy (1)
Learnt from: renczesstefan
PR: #318
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java:60-62
Timestamp: 2025-06-23T13:30:13.096Z
Learning: In MenuItemConstants enum in nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java, the field attributeId will be renamed to value to make it more generic and appropriate for both dataset attribute identifiers and technical constants like PATH_SEPARATOR.
application-engine/src/main/java/com/netgrif/application/engine/menu/services/MenuItemService.java (1)
Learnt from: renczesstefan
PR: #318
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java:60-62
Timestamp: 2025-06-23T13:30:13.096Z
Learning: In MenuItemConstants enum in nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java, the field attributeId will be renamed to value to make it more generic and appropriate for both dataset attribute identifiers and technical constants like PATH_SEPARATOR.
🔇 Additional comments (4)
application-engine/src/test/groovy/com/netgrif/application/engine/action/MenuItemApiTest.groovy (2)
259-271: Good test coverage for validation scenariosThe test properly validates error cases for empty titles and duplicate identifiers using
assertThrows. This ensures the duplication logic correctly handles invalid inputs.
330-351: Excellent test coverage for cascade deletionThe test thoroughly verifies that removing a menu item properly cascades to delete associated tabbed views (both case and task views). This ensures data consistency is maintained.
application-engine/src/main/java/com/netgrif/application/engine/menu/services/MenuItemService.java (2)
452-466: Well-structured view lifecycle managementThe
handleViewmethod and its associatedmust*condition checker methods provide a clear and maintainable approach to managing view lifecycle states. The separation of condition checking from action execution follows good design principles.Also applies to: 523-545
382-392: Good defensive programming in parent-child relationship managementThe method properly handles null/empty cases and maintains data consistency by updating both the child list and the
has_childrenflag.
| protected String nameFromPath(String path) { | ||
| if (path == null || path.equals(MenuItemConstants.PATH_SEPARATOR) || path.isEmpty()) { | ||
| return ""; | ||
| } | ||
| if (path.lastIndexOf(MenuItemConstants.PATH_SEPARATOR) == 0) { | ||
| return path.replace(MenuItemConstants.PATH_SEPARATOR, ""); | ||
| } | ||
| return path.substring(path.lastIndexOf(MenuItemConstants.PATH_SEPARATOR)); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add bounds checking for substring operation
The nameFromPath method on line 743 could throw StringIndexOutOfBoundsException if lastIndexOf returns the last position in the string.
protected String nameFromPath(String path) {
if (path == null || path.equals(MenuItemConstants.PATH_SEPARATOR) || path.isEmpty()) {
return "";
}
if (path.lastIndexOf(MenuItemConstants.PATH_SEPARATOR) == 0) {
return path.replace(MenuItemConstants.PATH_SEPARATOR, "");
}
- return path.substring(path.lastIndexOf(MenuItemConstants.PATH_SEPARATOR));
+ int lastIndex = path.lastIndexOf(MenuItemConstants.PATH_SEPARATOR);
+ return lastIndex < path.length() - 1 ? path.substring(lastIndex + 1) : "";
}📝 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.
| protected String nameFromPath(String path) { | |
| if (path == null || path.equals(MenuItemConstants.PATH_SEPARATOR) || path.isEmpty()) { | |
| return ""; | |
| } | |
| if (path.lastIndexOf(MenuItemConstants.PATH_SEPARATOR) == 0) { | |
| return path.replace(MenuItemConstants.PATH_SEPARATOR, ""); | |
| } | |
| return path.substring(path.lastIndexOf(MenuItemConstants.PATH_SEPARATOR)); | |
| } | |
| protected String nameFromPath(String path) { | |
| if (path == null || path.equals(MenuItemConstants.PATH_SEPARATOR) || path.isEmpty()) { | |
| return ""; | |
| } | |
| if (path.lastIndexOf(MenuItemConstants.PATH_SEPARATOR) == 0) { | |
| return path.replace(MenuItemConstants.PATH_SEPARATOR, ""); | |
| } | |
| int lastIndex = path.lastIndexOf(MenuItemConstants.PATH_SEPARATOR); | |
| return lastIndex < path.length() - 1 | |
| ? path.substring(lastIndex + 1) | |
| : ""; | |
| } |
🤖 Prompt for AI Agents
In
application-engine/src/main/java/com/netgrif/application/engine/menu/services/MenuItemService.java
around lines 736 to 744, the substring call uses lastIndexOf without checking if
it is the last character index, which can cause StringIndexOutOfBoundsException.
Fix this by adding bounds checking before calling substring to ensure the index
is not the last character position or adjust the substring parameters
accordingly to avoid the exception.
| String trimmed = bannedNets.value?.replaceAll("\\s","") | ||
| if (bannedNets.value != trimmed) { | ||
| change bannedNets value { trimmed } | ||
| } | ||
| </action> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Whitespace removal is too aggressive
The current implementation removes ALL whitespace including spaces within identifiers. This would transform "my net 1, my net 2" into "mynet1,mynet2" which may not be intended.
-String trimmed = bannedNets.value?.replaceAll("\\s","")
+String trimmed = bannedNets.value?.split(",").collect { it.trim() }.join(",")The same issue applies to the default_headers field trimming on lines 364-368.
Also applies to: 364-368
🤖 Prompt for AI Agents
In
application-engine/src/main/resources/petriNets/engine-processes/menu/tabbed_case_view_configuration.xml
around lines 280 to 284 and 364 to 368, the current code removes all whitespace
characters from the bannedNets and default_headers values, which unintentionally
removes spaces within identifiers. To fix this, modify the whitespace trimming
logic to only remove leading and trailing whitespace instead of all whitespace
characters, preserving spaces inside the strings. Use a method or pattern that
trims spaces at the start and end but leaves internal spaces intact.
| Case item = getMenuItem(caze) | ||
| Case filter = getFilter(caze) | ||
|
|
||
| Thread.sleep(4000) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Replace Thread.sleep with proper synchronization
Using Thread.sleep(4000) makes tests slow and potentially flaky. Consider using a polling mechanism or wait conditions to ensure the data is properly indexed.
- Thread.sleep(4000)
+ // Wait for elastic indexing with timeout
+ def maxAttempts = 20
+ def attempt = 0
+ while (attempt < maxAttempts) {
+ def result = findCasesElastic("processIdentifier:$FilterRunner.MENU_NET_IDENTIFIER", PageRequest.of(0, 1))
+ if (!result.isEmpty()) break
+ Thread.sleep(200)
+ attempt++
+ }📝 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.
| Thread.sleep(4000) | |
| // Wait for elastic indexing with timeout | |
| def maxAttempts = 20 | |
| def attempt = 0 | |
| while (attempt < maxAttempts) { | |
| def result = findCasesElastic("processIdentifier:$FilterRunner.MENU_NET_IDENTIFIER", PageRequest.of(0, 1)) | |
| if (!result.isEmpty()) break | |
| Thread.sleep(200) | |
| attempt++ | |
| } |
🤖 Prompt for AI Agents
In
application-engine/src/test/groovy/com/netgrif/application/engine/action/MenuItemApiTest.groovy
at line 80, replace the use of Thread.sleep(4000) with a proper synchronization
method such as a polling loop or wait condition that repeatedly checks if the
data is indexed and ready, proceeding as soon as the condition is met to avoid
unnecessary delays and reduce test flakiness.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.github/workflows/nexus-release.yml(1 hunks)application-engine/pom.xml(1 hunks)nae-object-library/pom.xml(1 hunks)nae-spring-core-adapter/pom.xml(1 hunks)nae-user-ce/pom.xml(1 hunks)nae-user-common/pom.xml(1 hunks)pom.xml(1 hunks)
✅ Files skipped from review due to trivial changes (6)
- nae-user-common/pom.xml
- nae-user-ce/pom.xml
- nae-object-library/pom.xml
- application-engine/pom.xml
- pom.xml
- nae-spring-core-adapter/pom.xml
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: renczesstefan
PR: netgrif/application-engine#318
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java:60-62
Timestamp: 2025-06-23T13:30:13.096Z
Learning: In MenuItemConstants enum in nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java, the field `attributeId` will be renamed to `value` to make it more generic and appropriate for both dataset attribute identifiers and technical constants like PATH_SEPARATOR.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
🔇 Additional comments (1)
.github/workflows/nexus-release.yml (1)
52-54: Confirmserver-idchange aligns with<distributionManagement>
server-idis nownetgrif-nexus-releases. Ensure every module’spom.xmlhas a matching<id>under<distributionManagement>. Otherwisemvn deploywill skip credentials and fail.
| - name: DEBUG PRINT | ||
| run: echo "$MAVEN_USERNAME / $MAVEN_PASSWORD" | ||
| shell: bash | ||
| env: | ||
| MAVEN_USERNAME: ${{ secrets.NEXUS_USERNAME }} | ||
| MAVEN_PASSWORD: ${{ secrets.NEXUS_PASSWORD }} | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Credentials echoed to logs – remove immediately
echo "$MAVEN_USERNAME / $MAVEN_PASSWORD" leaks raw credentials into the Actions log. Even if GitHub masks secrets, any mis-configuration or later copy-paste can expose them.
Delete this debug step or output only a placeholder.
- - name: DEBUG PRINT
- run: echo "$MAVEN_USERNAME / $MAVEN_PASSWORD"
- shell: bash
- env:
- MAVEN_USERNAME: ${{ secrets.NEXUS_USERNAME }}
- MAVEN_PASSWORD: ${{ secrets.NEXUS_PASSWORD }}📝 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.
| - name: DEBUG PRINT | |
| run: echo "$MAVEN_USERNAME / $MAVEN_PASSWORD" | |
| shell: bash | |
| env: | |
| MAVEN_USERNAME: ${{ secrets.NEXUS_USERNAME }} | |
| MAVEN_PASSWORD: ${{ secrets.NEXUS_PASSWORD }} |
🤖 Prompt for AI Agents
In .github/workflows/nexus-release.yml around lines 39 to 45, the step that
echoes MAVEN_USERNAME and MAVEN_PASSWORD outputs sensitive credentials to the
logs. Remove the entire debug print step or replace the echo command with a
placeholder message that does not reveal any secret values to prevent credential
leakage.
- Refactor `DashboardManagementRunner` to update process descriptions for better clarity. - Replace `@Autowired` with constructor injection in `DefaultDashboardRunner`. - Improve javadoc comments in `DashboardManagementService` interface. - Simplify path separator handling in `ActionDelegate`.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (4)
application-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy (4)
2735-2735: Potential query injection vulnerability.The elastic query construction using string concatenation could be vulnerable to injection if the
pathparameter contains special characters or query syntax. This issue was previously identified and remains unresolved.
2213-2217: Missing .value property access on PATH_SEPARATOR constant.The code is inconsistent with PATH_SEPARATOR usage. This issue was previously identified and remains unresolved.
2219-2224: Missing .value property access on PATH_SEPARATOR constant.Same issue as in
nameFromPathmethod - missing.valueproperty access. This was previously identified.
2713-2716: Missing .value property access on PATH_SEPARATOR constant.Consistency issue with PATH_SEPARATOR usage. This was previously identified in the review.
🧹 Nitpick comments (1)
application-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy (1)
1-2765: Consider breaking up this large class for better maintainability.While the refactoring to service-based architecture is well done, the
ActionDelegateclass remains very large at 2700+ lines. Consider splitting it into smaller, more focused classes (e.g.,FilterActionDelegate,MenuItemActionDelegate,DashboardActionDelegate) to improve maintainability and readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
application-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy(40 hunks)application-engine/src/main/groovy/com/netgrif/application/engine/startup/DashboardManagementRunner.groovy(1 hunks)application-engine/src/main/groovy/com/netgrif/application/engine/startup/DefaultDashboardRunner.groovy(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/menu/services/interfaces/DashboardManagementService.java(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- application-engine/src/main/java/com/netgrif/application/engine/menu/services/interfaces/DashboardManagementService.java
🚧 Files skipped from review as they are similar to previous changes (2)
- application-engine/src/main/groovy/com/netgrif/application/engine/startup/DashboardManagementRunner.groovy
- application-engine/src/main/groovy/com/netgrif/application/engine/startup/DefaultDashboardRunner.groovy
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: renczesstefan
PR: netgrif/application-engine#318
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java:60-62
Timestamp: 2025-06-23T13:30:13.096Z
Learning: In MenuItemConstants enum in nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java, the field `attributeId` will be renamed to `value` to make it more generic and appropriate for both dataset attribute identifiers and technical constants like PATH_SEPARATOR.
application-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy (1)
Learnt from: renczesstefan
PR: #318
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java:60-62
Timestamp: 2025-06-23T13:30:13.096Z
Learning: In MenuItemConstants enum in nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java, the field attributeId will be renamed to value to make it more generic and appropriate for both dataset attribute identifiers and technical constants like PATH_SEPARATOR.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
🔇 Additional comments (7)
application-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy (7)
29-31: Good architectural improvement with service-based approach.The introduction of
IMenuItemService,DashboardManagementService, andDashboardItemServiceproperly aligns with the PR's objective to refactor menu items and dashboards into a service-based architecture. The autowiring follows Spring conventions correctly.Also applies to: 205-213
933-936: Useful convenience method addition.The new
findCase(String stringId)method provides a cleaner API for finding cases by ID, improving code readability.
1618-1628: Well-executed refactoring to service-based architecture.The filter and menu item creation methods have been properly refactored to use
FilterBody,MenuItemBody, and the new services. The code maintains backward compatibility while improving modularity and maintainability.Also applies to: 1733-1738, 1765-1767, 1794-1797, 1821-1826, 1862-1864
2339-2361: Clean implementation of dashboard management methods.The new dashboard management methods properly delegate to their respective services and follow the established pattern. This maintains consistency with the menu item refactoring approach.
1722-1722: Good deprecation strategy for backward compatibility.The deprecated methods are properly marked with
@Deprecatedannotations and include version information (since = "6.5.0"). They continue to work by delegating to the new service-based implementations, providing a smooth migration path.Also applies to: 1750-1750, 1780-1780, 1809-1809, 2322-2322, 2407-2407, 2533-2533, 2575-2575, 2626-2626, 2663-2663
2110-2207: Well-structured view configuration helper methods.The new helper methods (
createSingleTaskViewBody,createDefaultViewBody,createLegacyMenuItemViews) properly encapsulate the view configuration logic and create appropriateViewBodyobjects for different scenarios. The chaining of views for tabbed interfaces is handled cleanly.
2379-2382: Useful convenience overload with sensible defaults.The new
findCasesElasticoverload with a default page size of 100 provides a cleaner API for common use cases.
# Conflicts: # application-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy # application-engine/src/test/groovy/com/netgrif/application/engine/action/MenuItemApiTest.groovy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
application-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy (2)
2747-2757: Critical: Inconsistent PATH_SEPARATOR usage in getCorrectedUri.Line 2748 correctly accesses
PATH_SEPARATOR.valuebut line 2752 is missing.value, which will cause a runtime error:- int lastIdx = uncheckedPath.lastIndexOf(MenuItemConstants.PATH_SEPARATOR) + int lastIdx = uncheckedPath.lastIndexOf(MenuItemConstants.PATH_SEPARATOR.value)
2730-2745: Incorrect property access on field constants.Lines 2740-2741 incorrectly append
.valueto the field constants when accessing dataSet fields:- options.put(childFolderCase.dataSet[MenuItemConstants.FIELD_CHILD_ITEM_IDS.value].value as String, new I18nString(childFolderCase.dataSet[MenuItemConstants.PREFERENCE_ITEM_FIELD_NODE_NAME.value].value as String)) + options.put(childFolderCase.dataSet[MenuItemConstants.FIELD_CHILD_ITEM_IDS].value as String, new I18nString(childFolderCase.dataSet[MenuItemConstants.PREFERENCE_ITEM_FIELD_NODE_NAME].value as String))The constants should be used directly to access dataSet fields, not their
.valueproperty.
♻️ Duplicate comments (5)
application-engine/src/test/groovy/com/netgrif/application/engine/action/MenuItemApiTest.groovy (1)
81-81: Replace Thread.sleep with proper synchronization mechanism.This Thread.sleep(4000) makes tests slow and potentially flaky. A polling mechanism was already suggested in previous reviews to wait for elastic indexing with timeout.
application-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy (4)
2735-2735: Security: Query injection vulnerability needs to be addressed.The elastic query construction remains vulnerable to injection attacks through the
pathparameter. This critical issue was previously identified and still needs to be fixed by escaping special characters.
2213-2217: Missing .value property access on PATH_SEPARATOR constant.The PATH_SEPARATOR constant is used without accessing its .value property, which is inconsistent with other usages in the file.
2219-2224: Missing .value property access on PATH_SEPARATOR constant.Multiple occurrences of PATH_SEPARATOR without .value property access in the parentPath method.
2713-2716: Missing .value property access on PATH_SEPARATOR constant.The splitUriPath method uses PATH_SEPARATOR without accessing .value property.
🧹 Nitpick comments (1)
application-engine/src/test/groovy/com/netgrif/application/engine/action/MenuItemApiTest.groovy (1)
248-248: Consider replacing Thread.sleep with proper synchronization.Similar to line 81, this
Thread.sleep(2000)can make tests slower and potentially flaky. Consider using a polling mechanism or wait condition instead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
application-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy(39 hunks)application-engine/src/main/groovy/com/netgrif/application/engine/startup/ImportHelper.groovy(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/petrinet/web/PetriNetController.java(5 hunks)application-engine/src/test/groovy/com/netgrif/application/engine/action/MenuItemApiTest.groovy(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- application-engine/src/main/java/com/netgrif/application/engine/petrinet/web/PetriNetController.java
- application-engine/src/main/groovy/com/netgrif/application/engine/startup/ImportHelper.groovy
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: renczesstefan
PR: netgrif/application-engine#318
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java:60-62
Timestamp: 2025-06-23T13:30:13.096Z
Learning: In MenuItemConstants enum in nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java, the field `attributeId` will be renamed to `value` to make it more generic and appropriate for both dataset attribute identifiers and technical constants like PATH_SEPARATOR.
application-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy (1)
Learnt from: renczesstefan
PR: #318
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java:60-62
Timestamp: 2025-06-23T13:30:13.096Z
Learning: In MenuItemConstants enum in nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java, the field attributeId will be renamed to value to make it more generic and appropriate for both dataset attribute identifiers and technical constants like PATH_SEPARATOR.
application-engine/src/test/groovy/com/netgrif/application/engine/action/MenuItemApiTest.groovy (1)
Learnt from: renczesstefan
PR: #318
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java:60-62
Timestamp: 2025-06-23T13:30:13.096Z
Learning: In MenuItemConstants enum in nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java, the field attributeId will be renamed to value to make it more generic and appropriate for both dataset attribute identifiers and technical constants like PATH_SEPARATOR.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
🔇 Additional comments (14)
application-engine/src/test/groovy/com/netgrif/application/engine/action/MenuItemApiTest.groovy (7)
10-16: LGTM! New imports support enhanced menu functionality.The added imports align with the refactoring from preference items to menu items and support the new tabbed view configurations and utility methods.
82-88: LGTM! Correct migration to new MenuItemConstants.The migration from old
PREFERENCE_ITEM_FIELD_*constants to newFIELD_*constants is consistent with the refactoring, and the newMenuItemView.TABBED_CASE_VIEW.identifierusage properly supports the enhanced view configuration system.
95-108: LGTM! Enhanced tabbed view validation logic.The new test logic properly validates the nested tabbed view configurations using the
MenuItemUtils.getCaseIdFromCaseRefutility method and ensures correct configuration of both tabbed case and task views.
110-112: LGTM! Updated queries use correct new identifiers.The elastic search queries correctly use the new
FilterRunner.MENU_NET_IDENTIFIERandMenuItemConstants.FIELD_NODE_PATHconstants, maintaining functionality while aligning with the refactoring.
128-169: LGTM! Comprehensive validation of menu item changes.The enhanced test logic properly validates that tabbed view configurations persist correctly during menu item changes, using the
MenuItemUtils.getCaseIdFromCaseRefutility method and ensuring referential integrity of nested view configurations.
279-292: LGTM! Correct validation of menu item duplication logic.The test properly validates the duplication functionality with updated constants and correctly checks for empty string values in duplicate fields after completion, aligning with the new duplication logic.
321-352: LGTM! Comprehensive validation of cascade deletion.The enhanced removal test properly validates that when a menu item is deleted, all associated tabbed views (both case and task views) are also properly deleted, ensuring no orphaned view configurations remain in the system.
application-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy (7)
933-936: Good addition of convenience method.The new
findCase(String stringId)method provides a cleaner API for finding cases by ID, complementing the existing predicate-basedfindCasemethod.
1619-1628: Clean refactoring to service-based approach.The migration to use
FilterBodyand delegate tomenuItemService.createFilter()properly encapsulates the filter creation logic and follows good separation of concerns.
1723-1827: Proper deprecation and migration to service-based menu item creation.The deprecated
createMenuItemmethods correctly maintain backward compatibility while delegating to the new service. The consistent use ofsetUseTabbedView(true)indicates a standardization on tabbed views.
1879-1926: Well-structured menu item update methods.The
changeMenuItemimplementation properly handles different attribute updates and delegates to appropriate services. Good use of constants for field and transition identifiers.
2339-2361: Good addition of dashboard management API methods.The new dashboard-related methods provide a consistent API for dashboard management and properly delegate to specialized services.
2110-2207: Well-designed view configuration builder methods.The new view creation methods properly implement the builder pattern for complex view configurations. The chained view approach allows flexible composition of case and task views.
2533-2642: Excellent deprecation strategy for menu item methods.The deprecated methods maintain backward compatibility while properly migrating to the new service-based architecture. The consistent use of
@Deprecated(since = "6.5.0")provides clear version tracking.
- Refactor user transformation logic to use `ActorTransformer.toLoggedUser` across services. Remove outdated TODO comments and unused imports.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
♻️ Duplicate comments (7)
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticPetriNetService.java (2)
18-18: Remove unused Lazy importThe import
org.springframework.context.annotation.Lazyis not used in any@Lazyannotations in this file. The@Lazyannotation on line 58 uses the already imported annotation.-import org.springframework.context.annotation.Lazy;
193-193: Add pagination limits to prevent memory issuesUsing
FullPageRequestwithout limits could retrieve an unbounded number of results, potentially causing memory issues with large datasets.-List<PetriNetReference> groupProcesses = this.petriNetService.search(processQuery, user, new FullPageRequest(), locale).getContent(); +// Limit to reasonable number of group processes +Page<PetriNetReference> groupProcessPage = this.petriNetService.search(processQuery, user, PageRequest.of(0, 1000), locale); +if (groupProcessPage.getTotalElements() > 1000) { + log.warn("Large number of group processes found: {}. Consider implementing streaming or chunked processing.", groupProcessPage.getTotalElements()); +} +List<PetriNetReference> groupProcesses = groupProcessPage.getContent();Also add the import:
import org.springframework.data.domain.PageRequest;application-engine/src/main/java/com/netgrif/application/engine/menu/services/MenuItemService.java (5)
197-199: Potential query injection vulnerability in findMenuItem.The elastic search query directly interpolates the identifier without escaping, which could lead to query manipulation.
Escape the identifier before interpolation:
+String escapedIdentifier = identifier.replaceAll("[\\\\\"]", "\\\\$0"); String query = String.format("processIdentifier:%s AND dataSet.%s.textValue.keyword:\"%s\"", - FilterRunner.MENU_NET_IDENTIFIER, MenuItemConstants.FIELD_IDENTIFIER, identifier); + FilterRunner.MENU_NET_IDENTIFIER, MenuItemConstants.FIELD_IDENTIFIER, escapedIdentifier);
212-214: Query injection vulnerabilities in findMenuItem overload.Both name and path parameters need escaping before interpolation.
+String escapedName = name.replaceAll("[\\\\\"]", "\\\\$0"); +String escapedPath = path.replaceAll("[\\\\\"]", "\\\\$0"); String query = String.format("processIdentifier:%s AND title.keyword:\"%s\" AND nodePath:\"%s\"", - FilterRunner.MENU_NET_IDENTIFIER, name, path); + FilterRunner.MENU_NET_IDENTIFIER, escapedName, escapedPath);
621-659: Remove duplicate method getOrCreateFolderRecursive2.This appears to be dead code or an experimental version. Having two methods with nearly identical names violates clean code principles and causes confusion.
Remove the entire
getOrCreateFolderRecursive2method to eliminate confusion and adhere to clean code principles.
721-727: Add null safety checks in setDataWithExecute.The method assumes the task exists but doesn't handle the case where it might be null.
protected Case setDataWithExecute(Case useCase, String transId, Map<String, Map<String, Object>> dataSet) throws TransitionNotExecutableException { AbstractUser loggedUser = userService.getLoggedOrSystem(); String taskId = MenuItemUtils.findTaskIdInCase(useCase, transId); + if (taskId == null) { + throw new IllegalStateException(String.format("Task with transition '%s' not found in case '%s'", transId, useCase.getStringId())); + } Task task = taskService.findOne(taskId); + if (task == null) { + throw new IllegalStateException(String.format("Task '%s' not found", taskId)); + } task = taskService.assignTask(task, loggedUser).getTask(); task = dataService.setData(task, ImportHelper.populateDataset((Map) dataSet)).getTask(); return taskService.finishTask(task, loggedUser).getCase(); }
730-738: Add bounds checking for substring operation.The
nameFromPathmethod could throwStringIndexOutOfBoundsExceptioniflastIndexOfreturns the last position in the string.protected String nameFromPath(String path) { if (path == null || path.equals(MenuItemConstants.PATH_SEPARATOR) || path.isEmpty()) { return ""; } if (path.lastIndexOf(MenuItemConstants.PATH_SEPARATOR) == 0) { return path.replace(MenuItemConstants.PATH_SEPARATOR, ""); } - return path.substring(path.lastIndexOf(MenuItemConstants.PATH_SEPARATOR)); + int lastIndex = path.lastIndexOf(MenuItemConstants.PATH_SEPARATOR); + return lastIndex < path.length() - 1 ? path.substring(lastIndex + 1) : ""; }
🧹 Nitpick comments (5)
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticPetriNetService.java (2)
114-116: Track the TODO for impersonation featureThere's a TODO comment indicating that impersonation functionality is not yet implemented. Consider creating a tracking issue for this feature if it's required.
Would you like me to create an issue to track the implementation of the impersonation feature?
175-180: Consider making search fields configurableThe TODO comment correctly identifies that the search fields and their boost values are hardcoded. Consider moving these to configuration properties for better flexibility.
Would you like me to help refactor this to use configuration properties for the search fields and boost values?
application-engine/src/main/java/com/netgrif/application/engine/menu/services/MenuItemService.java (3)
211-211: Address the TODO comment.The TODO comment indicates retry functionality should be implemented for this method.
Would you like me to implement the retry mechanism similar to the
findMenuItem(String identifier, boolean retry)method?
225-225: Address the TODO comment.The TODO comment needs clarification or implementation.
Please clarify what specific functionality needs to be implemented for the folder case finding logic.
239-246: Inconsistent query approach in existsMenuItem.The method uses MongoDB query while other methods use Elasticsearch. Consider consistency or document the reasoning.
Either use the commented Elasticsearch approach consistently:
-//TODO mongo -Case menuItem = this.workflowService.searchOne(QCase.case$.processIdentifier.eq("menu_item") - .and(QCase.case$.dataSet.get(MenuItemConstants.FIELD_IDENTIFIER).value.eq(identifier))); -return menuItem != null; -// String query = String.format("processIdentifier:%s AND dataSet.%s.textValue.keyword:\"%s\"", -// FilterRunner.MENU_NET_IDENTIFIER, MenuItemConstants.FIELD_IDENTIFIER, identifier); -// return countCases(FilterRunner.MENU_NET_IDENTIFIER, query) > 0; +String escapedIdentifier = identifier.replaceAll("[\\\\\"]", "\\\\$0"); +String query = String.format("processIdentifier:%s AND dataSet.%s.textValue.keyword:\"%s\"", + FilterRunner.MENU_NET_IDENTIFIER, MenuItemConstants.FIELD_IDENTIFIER, escapedIdentifier); +return countCases(FilterRunner.MENU_NET_IDENTIFIER, query) > 0;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
application-engine/src/main/groovy/com/netgrif/application/engine/startup/ImportHelper.groovy(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticPetriNetService.java(3 hunks)application-engine/src/main/java/com/netgrif/application/engine/menu/services/DashboardItemServiceImpl.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/menu/services/DashboardManagementServiceImpl.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/menu/services/MenuItemService.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- application-engine/src/main/java/com/netgrif/application/engine/menu/services/DashboardItemServiceImpl.java
- application-engine/src/main/groovy/com/netgrif/application/engine/startup/ImportHelper.groovy
- application-engine/src/main/java/com/netgrif/application/engine/menu/services/DashboardManagementServiceImpl.java
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: renczesstefan
PR: netgrif/application-engine#318
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java:60-62
Timestamp: 2025-06-23T13:30:13.096Z
Learning: In MenuItemConstants enum in nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java, the field `attributeId` will be renamed to `value` to make it more generic and appropriate for both dataset attribute identifiers and technical constants like PATH_SEPARATOR.
application-engine/src/main/java/com/netgrif/application/engine/menu/services/MenuItemService.java (1)
Learnt from: renczesstefan
PR: #318
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java:60-62
Timestamp: 2025-06-23T13:30:13.096Z
Learning: In MenuItemConstants enum in nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java, the field attributeId will be renamed to value to make it more generic and appropriate for both dataset attribute identifiers and technical constants like PATH_SEPARATOR.
🧬 Code Graph Analysis (1)
application-engine/src/main/java/com/netgrif/application/engine/menu/services/MenuItemService.java (4)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/ActorTransformer.java (1)
ActorTransformer(10-131)nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java (1)
MenuItemConstants(7-39)application-engine/src/main/java/com/netgrif/application/engine/menu/services/DashboardItemServiceImpl.java (1)
Slf4j(33-126)application-engine/src/main/java/com/netgrif/application/engine/menu/services/DashboardManagementServiceImpl.java (1)
Slf4j(36-140)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
🔇 Additional comments (4)
application-engine/src/main/java/com/netgrif/application/engine/menu/services/MenuItemService.java (4)
39-50: LGTM!The class structure, annotations, and dependency injection setup are well-implemented using Spring best practices.
57-84: LGTM!The filter creation and update operations are well-implemented with appropriate data handling, transaction management, and logging.
429-539: LGTM!The view and filter handling methods are well-structured with clear separation of concerns and appropriate error handling.
740-751: LGTM!The path utility methods are well-implemented with appropriate null checks and edge case handling.
| } | ||
|
|
||
| // TODO refactor to properties | ||
| String searchText = "*" + request.getTitle() + "*"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sanitize search input to prevent query injection
The search text is directly concatenated with wildcards without escaping special characters. This could lead to query errors or injection issues if the title contains Elasticsearch query syntax characters.
Consider escaping special characters:
-String searchText = "*" + request.getTitle() + "*";
+String searchText = "*" + QueryStringQuery.escape(request.getTitle()) + "*";Or use a more robust approach with match queries instead of wildcards for better performance:
-String searchText = "*" + request.getTitle() + "*";
-QueryStringQuery fullTextQuery = QueryStringQuery.of(builder -> builder.fields(fullTextFields).query(searchText));
+QueryStringQuery fullTextQuery = QueryStringQuery.of(builder ->
+ builder.fields(fullTextFields)
+ .query(request.getTitle())
+ .defaultOperator(Operator.And)
+ .analyzeWildcard(true)
+ .fuzziness("AUTO"));Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticPetriNetService.java
at line 176, the searchText is constructed by directly concatenating wildcards
to the request title without escaping special characters, which risks query
injection or errors. Fix this by sanitizing the input string to escape
Elasticsearch special characters before adding wildcards, or refactor the query
to use a match query instead of wildcard queries for safer and more efficient
searching.
| @Override | ||
| public void moveItem(Case itemCase, String destUri) throws TransitionNotExecutableException { | ||
|
|
||
| log.debug("Move of menu item case [{}] started. Destination path [{}]", itemCase.getStringId(), destUri); | ||
| if (MenuItemUtils.isCyclicNodePath(itemCase, destUri)) { | ||
| throw new IllegalArgumentException(String.format("Cyclic path not supported. Destination path: %s", destUri)); | ||
| } | ||
| List<Case> casesToSave = new ArrayList<>(); | ||
|
|
||
| List<String> parentIdList = MenuItemUtils.getCaseIdsFromCaseRef(itemCase, MenuItemConstants.FIELD_PARENT_ID); | ||
| if (parentIdList != null && !parentIdList.isEmpty()) { | ||
| Case oldParent = removeChildItemFromParent(parentIdList.get(0), itemCase); | ||
| casesToSave.add(oldParent); | ||
| } | ||
|
|
||
| Case newParent = getOrCreateFolderItem(destUri); | ||
| if (newParent != null) { | ||
| itemCase.getDataField(MenuItemConstants.FIELD_PARENT_ID).setValue(List.of(newParent.getStringId())); | ||
| appendChildCaseIdInMemory(newParent, itemCase.getStringId()); | ||
| casesToSave.add(newParent); | ||
| } else { | ||
| itemCase.getDataField(MenuItemConstants.FIELD_PARENT_ID).setValue(null); | ||
| } | ||
|
|
||
| resolveAndHandleNewNodePath(itemCase, destUri); | ||
| casesToSave.add(itemCase); | ||
|
|
||
| if (MenuItemUtils.hasFolderChildren(itemCase)) { | ||
| List<Case> childrenToSave = updateNodeInChildrenFoldersRecursive(itemCase); | ||
| casesToSave.addAll(childrenToSave); | ||
| } | ||
|
|
||
| for (Case useCase : casesToSave) { | ||
| if (useCase != null) { | ||
| workflowService.save(useCase); | ||
| } | ||
| } | ||
| log.debug("Moved menu item case [{}]. Destination path was [{}]", itemCase.getStringId(), destUri); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider adding transaction management for atomicity.
The moveItem operation involves multiple case updates that could leave the system in an inconsistent state if any step fails.
Consider wrapping the entire operation in a transaction:
@Transactional
public void moveItem(Case itemCase, String destUri) throws TransitionNotExecutableException {
// existing implementation
}🤖 Prompt for AI Agents
In
application-engine/src/main/java/com/netgrif/application/engine/menu/services/MenuItemService.java
around lines 257 to 295, the moveItem method updates multiple cases but lacks
transaction management, risking partial updates on failure. To fix this,
annotate the moveItem method with @Transactional to ensure all database
operations within it are atomic and rollback on exceptions, preserving data
consistency.
...ation-engine/src/main/java/com/netgrif/application/engine/menu/services/MenuItemService.java
Show resolved
Hide resolved
...gine/src/main/groovy/com/netgrif/application/engine/startup/DashboardManagementRunner.groovy
Outdated
Show resolved
Hide resolved
application-engine/src/main/groovy/com/netgrif/application/engine/startup/ImportHelper.groovy
Show resolved
Hide resolved
| import org.springframework.context.annotation.Lazy; | ||
| import org.springframework.dao.InvalidDataAccessApiUsageException; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The annotation is presnet at line 58.
| private IPetriNetService petriNetService; | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
petriNetService properties is initialized with lazy setter at line 60.
...ine/src/main/java/com/netgrif/application/engine/menu/services/DashboardItemServiceImpl.java
Show resolved
Hide resolved
...c/main/java/com/netgrif/application/engine/menu/services/DashboardManagementServiceImpl.java
Show resolved
Hide resolved
...ation-engine/src/main/java/com/netgrif/application/engine/menu/services/MenuItemService.java
Show resolved
Hide resolved
- Refactor startup runners and improve configuration handling - Migrated Groovy-based runners to Java for consistency and maintainability. - Introduced `MenuProcessRunner` to manage menu-related processes separately. - Updated dashboard-related property keys and added conditional annotations for more precise feature control. - Ensured clarity in YAML configuration with new `dashboard.enabled` property.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (2)
application-engine/src/main/java/com/netgrif/application/engine/menu/services/MenuItemService.java (2)
368-752: Helper methods provide essential functionality but past issues remain unresolved.The helper methods support the core service functionality well, including:
- View and filter lifecycle management
- Recursive folder creation and management
- Path manipulation utilities
- Case relationship management
However, several issues flagged in previous reviews persist:
- Unused duplicate method
getOrCreateFolderRecursive2(lines 622-660)- Potential StringIndexOutOfBoundsException in
nameFromPath(line 738)- Missing null safety checks in
setDataWithExecute
185-247: Query injection vulnerabilities persist from previous reviews.The Elasticsearch query methods still directly interpolate user input without escaping, creating potential injection vulnerabilities:
- Line 198-199:
identifierparameter needs escaping- Line 213-214:
nameandpathparameters need escapingAdditionally, there are unresolved TODO comments indicating incomplete implementation.
Apply proper escaping before query interpolation:
+String escapedIdentifier = identifier.replaceAll("[\\\\\"]", "\\\\$0"); String query = String.format("processIdentifier:%s AND dataSet.%s.textValue.keyword:\"%s\"", - MenuProcessRunner.MENU_NET_IDENTIFIER, MenuItemConstants.FIELD_IDENTIFIER, identifier); + MenuProcessRunner.MENU_NET_IDENTIFIER, MenuItemConstants.FIELD_IDENTIFIER, escapedIdentifier);
🧹 Nitpick comments (1)
application-engine/src/main/java/com/netgrif/application/engine/startup/runner/DefaultDashboardRunner.java (1)
1-43: LGTM! Well-designed default dashboard creation with good internationalization.The DefaultDashboardRunner is well-implemented with:
- Proper execution order (142) after DashboardManagementRunner
- Good internationalization support (English, Slovak, German)
- Sensible error handling that logs issues without breaking startup
- Clean delegation to the service layer
- Appropriate conditional enabling
Consider making the
createMainDashboardManagementItemmethod private since it's only used internally:- Case createMainDashboardManagementItem() throws TransitionNotExecutableException { + private Case createMainDashboardManagementItem() throws TransitionNotExecutableException {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
application-engine/src/main/java/com/netgrif/application/engine/menu/services/DashboardManagementServiceImpl.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/menu/services/MenuItemService.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/startup/runner/DashboardManagementRunner.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/startup/runner/DashboardRunner.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/startup/runner/DefaultDashboardRunner.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/startup/runner/FilterRunner.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/startup/runner/MenuProcessRunner.java(1 hunks)application-engine/src/main/resources/application-dev.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- application-engine/src/main/resources/application-dev.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- application-engine/src/main/java/com/netgrif/application/engine/menu/services/DashboardManagementServiceImpl.java
- application-engine/src/main/java/com/netgrif/application/engine/startup/runner/DashboardRunner.java
- application-engine/src/main/java/com/netgrif/application/engine/startup/runner/FilterRunner.java
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: renczesstefan
PR: netgrif/application-engine#318
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java:60-62
Timestamp: 2025-06-23T13:30:13.096Z
Learning: In MenuItemConstants enum in nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java, the field `attributeId` will be renamed to `value` to make it more generic and appropriate for both dataset attribute identifiers and technical constants like PATH_SEPARATOR.
application-engine/src/main/java/com/netgrif/application/engine/menu/services/MenuItemService.java (1)
Learnt from: renczesstefan
PR: #318
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java:60-62
Timestamp: 2025-06-23T13:30:13.096Z
Learning: In MenuItemConstants enum in nae-object-library/src/main/java/com/netgrif/application/engine/objects/workflow/domain/menu/MenuItemConstants.java, the field attributeId will be renamed to value to make it more generic and appropriate for both dataset attribute identifiers and technical constants like PATH_SEPARATOR.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
🔇 Additional comments (7)
application-engine/src/main/java/com/netgrif/application/engine/menu/services/MenuItemService.java (5)
1-52: LGTM! Well-structured service class.The class follows proper Spring patterns with dependency injection, logging, and clear organization. The imports are appropriate and the class structure is clean.
53-86: LGTM! Clean filter management implementation.The filter creation and update methods follow consistent patterns with proper user handling, data transformation, and logging. The separation of concerns is well-maintained.
87-184: LGTM! Comprehensive menu item management with good API design.The menu item creation and update methods handle complex scenarios well, including folder hierarchy management, view configuration, and identifier validation. The convenience methods (createOrUpdate, createOrIgnore) provide good API flexibility.
Note: Past reviews have flagged the need for transaction management in createMenuItem due to multiple operations that could leave the system in an inconsistent state if any step fails. This remains a valid concern for production robustness.
248-296: LGTM! Well-implemented move functionality with proper safeguards.The moveItem method handles the complex scenario of moving menu items between folders correctly, including:
- Cyclic path prevention
- Parent-child relationship updates
- Recursive node path updates for folder hierarchies
- Efficient bulk saving approach
The implementation is thorough and handles edge cases appropriately.
297-367: LGTM! Comprehensive item duplication with recursive view handling.The duplicateItem method handles complex duplication scenarios well, including:
- Input validation and uniqueness checks
- Recursive view configuration duplication
- Proper parent-child relationship establishment
- Dataset manipulation for the new item
The implementation is thorough and maintains data integrity during duplication.
Note: Line 336 still contains an unused method call that was flagged in previous reviews.
application-engine/src/main/java/com/netgrif/application/engine/startup/runner/DashboardManagementRunner.java (1)
1-33: LGTM! Clean and well-structured startup runner implementation.The DashboardManagementRunner follows established patterns with:
- Proper conditional enabling via configuration property
- Appropriate execution order (141)
- Clean dependency injection using ImportHelper
- Good use of constants for identifiers and file paths
- Simple, focused responsibility for importing dashboard Petri nets
The implementation is consistent with other startup runners in the codebase.
application-engine/src/main/java/com/netgrif/application/engine/startup/runner/MenuProcessRunner.java (1)
1-52: LGTM! Excellent dynamic Petri net import implementation.The MenuProcessRunner demonstrates excellent design with:
- Early execution order (101) appropriate for core menu functionality
- Clean separation between fixed import and dynamic configuration creation
- Elegant use of streams to process MenuItemView enum values
- Proper error handling by filtering out failed imports
- Good maintainability - adding new menu view types only requires enum updates
The dynamic approach for configuration nets is particularly well-implemented and avoids code duplication.
- Update version to 7.0.0-SNAPSHOT and modify runners - Bumped parent version across multiple modules to 7.0.0-SNAPSHOT. - Made `DefaultDashboardRunner` and `DashboardManagementRunner` public for broader usage. - Ensures proper alignment with new versioning and access changes.
Description
Implements NAE-2147
Dependencies
Third party dependencies
No new dependencies were introduced
Blocking Pull requests
There are no dependencies on other PR
How Has Been This Tested?
Manually
Checklist:
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores
Documentation
Tests