[ETASK-23] Dynamic view configuration#447
Open
Retoocs wants to merge 51 commits into
Open
Conversation
- update text translations
- fix null node - add todo
- update petri net of menu_item.xml - fix handling of uri - update role permission in menu_item.xml
- refactor menu item view forms - implement MenuController.getMenuItemData - implement MenuItemServiceTest.getMenuItemDataTest
- fix resolving menu item in the menu - add todos
- fix changing menu item by ActionDelegate - fix menu_item.manageBehaviorOfTabFields - fix menu_item move_add_node set action - improve menu_item initialization when creating by forms - fix next view selection in ticket view
- make single task view as a primary view - update init values in menu_item.xml - update tabbed_single_task_view_configuration configurations
- make single task view as untabbed too
- make task view as untabbed too
- make case view as untabbed too
- fix field ids
- introduce empty content configuration - fix null node
- fix test - update doc
- begin updating filter
- introduce menu item configuration templates
- finish implementation of template configurations
- resolve todos - fix behavior in menu_item.xml
- add menu item body validation - add tags in menu_item.xml transitions for better filtering
- change default headers field type to string collection - introduce CustomViewTemplate - update default filter bodies to exclude allowed nets
- update CustomViewTemplate - introduce allAllowedNets metadata for filter - fix behavior in menu_item.xml
- update templates configuration - fix custom view creation via template - rework settings parts UX
- change menu item process identifier constant - add duplicity check on create item transition
- implement menu item data index creation and usage
- menu item identifier and uri fixes - refactor MenuItemService.updateMenuItem - fix MenuItemService.findMenuItem - improve identifier behavior on creation task
- remove filter case - remove deprecated methods in ActionDelegate - update menu configurations for direct use of filter field
- force menu item database indexes
- rename MenuItemView to MenuItemViewType - fix menu name null node - update MenuItemServiceTest - optimize tests by ensuring mongo db indexes
- finish implementation of create menu item test - introduce temporary method MenuItemService.collectRoles
- implement more tests in MenuItemServiceTest - remove unnecessary attributes in case_view_configuration.xml
- fix allowExport after merge
- introduce new filter fields - remove old filter field
- remove transactional in Importer - handle deprecated filter data type when importing process - remove use of deleted filter attributes
- implement FilterTest
- force component use for dedicated filter fields - fix cloning filter fields
- close input stream - disable MenuItemApiTest.groovy
# Conflicts: # src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy # src/main/java/com/netgrif/application/engine/menu/domain/FilterBody.java # src/main/java/com/netgrif/application/engine/menu/service/MenuItemService.java # src/main/java/com/netgrif/application/engine/workflow/service/FilterImportExportService.java # src/main/java/com/netgrif/application/engine/workflow/service/interfaces/IFilterImportExportService.java # src/main/resources/petriNets/engine-processes/import_filters.xml # src/test/groovy/com/netgrif/application/engine/action/MenuItemApiTest.groovy # src/test/groovy/com/netgrif/application/engine/filters/FilterImportExportTest.groovy
- add todos - resolve todos - introduce allowed net fields for view configurations - update FilterBody attributes - improve initial values for filter fields
- remove unused field from task_view_configuration - make editable boolean field in task_view_configuration
- fix MenuItemService.moveItem case override
- enhance MenuItemTemplateHolder methods
- fix MenuItemServiceTest
- set up headers for task lists
- fix uri change of menu item on creation
- add translation - update doc
WalkthroughReplaces legacy filter model with Case/Task/Process filter fields, refactors menu domain/types/templates and MenuItemService to Mongo with indexes, updates Petri net menu/view configs and startup runners, removes filter import/export/search services, adds MenuController and new menu APIs, and updates tests and docs. ChangesCore menu and filter refactor
Sequence Diagram(s)sequenceDiagram
rect rgba(66, 135, 245, 0.5)
participant Client
participant MenuController
end
rect rgba(40, 167, 69, 0.5)
participant IMenuItemService
participant MongoTemplate
participant WorkflowService
end
Client->>MenuController: GET /api/menu/{encodedCaseId}
MenuController->>IMenuItemService: getMenuItemData(caseId, locale)
IMenuItemService->>MongoTemplate: query menu case by indexed fields
IMenuItemService->>WorkflowService: resolve task/data groups
IMenuItemService-->>MenuController: List<DataGroup>
MenuController-->>Client: HAL JSON (EntityModel)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
|
coderabbitai
Bot
added
bugfix
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 25
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/main/resources/petriNets/engine-processes/menu/menu_item.xml (1)
203-216:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject URI separators in
move_dest_uri_new_node.This value is appended verbatim, so entering
foo/baror/foocreates multiple path segments even though the UI asks for a single node. That can silently reshape the URI tree and move the item under an unintended branch.Proposed fix
<action trigger="set"> newNodeName: f.move_dest_uri_new_node, selectedUri: f.move_dest_uri; if (newNodeName.value == null || newNodeName.value == "") { return } + + if (newNodeName.value.contains(uriService.getUriSeparator())) { + throw new IllegalArgumentException("New node name must not contain URI separators") + } String prefixUri = selectedUri.value.join("/") prefixUri = prefixUri.replace("//","/")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/resources/petriNets/engine-processes/menu/menu_item.xml` around lines 203 - 216, Reject values for move_dest_uri_new_node that contain URI separator characters instead of appending them verbatim: in the action that reads newNodeName (symbol newNodeName / move_dest_uri_new_node) validate newNodeName.value for occurrences of "/" and uriService.getUriSeparator() and if found either return early (like existing empty check) or surface a validation error; alternatively trim any leading/trailing separators before building prefixUri so the code that constructs newUri (uses selectedUri, uriService.getUriSeparator(), and uriService.getOrCreate) cannot create extra path segments. Ensure the check/sanitization occurs before concatenating prefixUri and calling uriService.getOrCreate.src/test/groovy/com/netgrif/application/engine/menu/MenuImportExportTest.groovy (1)
183-184:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix missing
menuImportExportServicecollaborator in disabled menu import/export test (lines 183-184)
MenuImportExportTeststill callsmenuImportExportService.invokeMethod(...)at lines 183–184, but the test class has no@Autowired/field/property namedmenuImportExportService(only this usage). With the test enabled, it will fail at runtime (missing property/bean). TheMenuAndFilterstype/import is still present, so that part is not an issue.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/groovy/com/netgrif/application/engine/menu/MenuImportExportTest.groovy` around lines 183 - 184, The test calls menuImportExportService.invokeMethod("loadFromXML", ...) but the test class lacks a menuImportExportService collaborator; add a collaborator field or bean for MenuImportExportService (e.g., declare and annotate a MenuImportExportService menuImportExportService with `@Autowired` or initialize a mock and inject it) so the invokeMethod calls on menuImportExportService (used to loadFromXML) resolve at runtime; ensure the field name matches menuImportExportService and that the service provides the loadFromXML behavior expected by the test.src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/FieldType.groovy (1)
24-37:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMatch serialized field names in
fromString().Line 37 breaks the new filter types:
"caseFilter","taskFilter", and"processFilter"are converted toCASEFILTER/TASKFILTER/PROCESSFILTER, which do not match the new enum constants with underscores. Any path that round-tripsgetName()back throughfromString()will now fail for these fields.💡 Proposed fix
+import java.util.Arrays + enum FieldType { @@ static FieldType fromString(String name) { - return valueOf(name.toUpperCase()) + return Arrays.stream(values()) + .filter(type -> type.name.equals(name) || type.name().equalsIgnoreCase(name) || type.name().equalsIgnoreCase(name)) + .findFirst() + .orElseThrow(() -> new IllegalArgumentException("Unknown field type: " + name)) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/FieldType.groovy` around lines 24 - 37, fromString(String name) currently calls valueOf(name.toUpperCase()) which converts serialized names like "caseFilter"/"taskFilter"/"processFilter" into CASEFILTER/TASKFILTER/PROCESSFILTER and fails to match the enum constants; update FieldType.fromString to lookup by the enum instance's serialized name property instead (e.g., iterate FieldType.values() and return the entry whose 'name' field equals the input, with a case-insensitive comparison or sensible fallback), and keep a fallback to valueOf(name.toUpperCase()) only if necessary to preserve backwards compatibility.src/main/java/com/netgrif/application/engine/menu/domain/MenuItemViewType.java (1)
74-94: 🧹 Nitpick | 🔵 Trivial | ⚖️ Poor tradeoffSimplify the tabbed/untabbed filter predicate; current form has a latent correctness gap.
The predicate
(view.isTabbed == isTabbed || view.isUntabbed != isTabbed)is hard to reason about and does not express the apparent intent ("when querying tabbed, match tabbed-capable views; when querying non-tabbed, match untabbed-capable views"). It is masked today only because every enum value hasisTabbed == true, but it is fragile: a future view withisTabbed=false, isUntabbed=falsewould be incorrectly returned whenisTabbed=falseis requested (sinceisUntabbed != false→false != falseisfalse... converselyview.isUntabbed != isTabbedevaluates tofalse, butview.isTabbed == isTabbed→false == falseistrue, so it is wrongly included). The same predicate is duplicated infindAllByIsTabbedAndParentIdentifier.Consider replacing both occurrences with an explicit selector.
♻️ Proposed clarification
- .filter(view -> (view.isTabbed == isTabbed || view.isUntabbed != isTabbed) && view.isPrimary == isPrimary) + .filter(view -> (isTabbed ? view.isTabbed : view.isUntabbed) && view.isPrimary == isPrimary)- .filter(view -> (view.isTabbed == isTabbed || view.isUntabbed != isTabbed) + .filter(view -> (isTabbed ? view.isTabbed : view.isUntabbed) && parentView.getAllowedAssociatedViews().contains(view.identifier))Please confirm the intended semantics: should a view be selectable in non-tabbed mode strictly when
isUntabbed == true?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/netgrif/application/engine/menu/domain/MenuItemViewType.java` around lines 74 - 94, The tabbed/untabbed predicate is unclear and brittle; update both findAllByIsTabbedAndIsPrimary and findAllByIsTabbedAndParentIdentifier to explicitly match the intended semantics: when isTabbed==true filter by view.isTabbed==true, otherwise filter by view.isUntabbed==true; keep the other condition in each method (isPrimary match or parentView.getAllowedAssociatedViews().contains(view.identifier)) unchanged so only the tabbed check is simplified and made explicit using the view.isTabbed and view.isUntabbed fields.src/main/java/com/netgrif/application/engine/menu/domain/configurations/ViewBody.java (1)
70-78: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winDowngrade NPE concern in
ViewBody.toDataSet(Case)(safe in current call path, optional robustness guard)
ViewBody.toDataSet(Case associatedViewCase)can NPE whenassociatedViewCase != nullbutthis.getAssociatedViewBody()isnull(e.g.,TaskViewBody), but current caller path inMenuItemService.createView(...)only creates/passesassociatedViewCasewhenbody.hasAssociatedView()istrue, which impliesbody.getAssociatedViewBody() != null.- Optional hardening: make the guard consistent with the dereference.
🛡️ Suggested guard
- if (associatedViewCase != null) { + if (associatedViewCase != null && hasAssociatedView()) { outcome.putDataSetEntry(ViewConstants.FIELD_CONFIGURATION_TYPE, FieldType.ENUMERATION_MAP, this.getAssociatedViewBody().getViewType().getIdentifier());🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/netgrif/application/engine/menu/domain/configurations/ViewBody.java` around lines 70 - 78, In ViewBody.toDataSet(Case) the code dereferences this.getAssociatedViewBody() while only checking associatedViewCase != null; change the guard to ensure the associated view body is non-null as well (e.g., if (associatedViewCase != null && this.getAssociatedViewBody() != null) or use this.hasAssociatedView()) so you don't risk NPE when getAssociatedViewBody() can be null (references: ViewBody.toDataSet(Case), getAssociatedViewBody(), hasAssociatedView(), MenuItemService.createView(...)).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/groovy/com/netgrif/application/engine/startup/MenuRunner.groovy`:
- Around line 32-38: In createConfigurationNets() the code uses
MenuItemViewType.values().each { ... }.collect() which returns the original
enums instead of the imported PetriNet objects; change the iteration to use
collect with a closure that returns the result of helper.importProcess so the
method actually returns List<PetriNet> (e.g. MenuItemViewType.values().collect {
view -> helper.importProcess(...) }), or alternatively if the returned list is
unused by run(), change createConfigurationNets() signature to void and keep the
current each block that only calls helper.importProcess for side effects.
In `@src/main/groovy/com/netgrif/application/engine/startup/MongoDbRunner.groovy`:
- Around line 27-28: MongoDbRunner currently runs at startup and can trigger a
MongoDB drop under test configs; modify MongoDbRunner to avoid dropping
non-isolated DBs by adding a guard: detect active profiles or the property
spring.profiles.active and skip the drop when the "test" profile is active (or
alternatively annotate the class with `@Profile`("!test")), or read
spring.data.mongodb.drop and the resolved database name from Environment and
only perform the drop if drop==true AND the database name clearly indicates an
isolated test DB (e.g., endsWith("_test") or matches a CI-only pattern);
implement the check in MongoDbRunner's startup method (the run/init method in
class MongoDbRunner) so the drop is skipped unless the safe conditions are met.
In
`@src/main/java/com/netgrif/application/engine/importer/service/FieldFactory.java`:
- Around line 796-802: resolveAttributeValues currently only sets allowedNets
when field.getType().equals(FieldType.CASE_REF), which means immediate FILTER
fields (or any FieldWithAllowedNets used as immediate data fields) won't get
allowedNets populated; change resolveAttributeValues in FieldFactory so that it
assigns allowedNets to any field implementing FieldWithAllowedNets (use
instanceof FieldWithAllowedNets) rather than only for CASE_REF, or alternatively
call resolveAllowedNets(...) from buildImmediateField to ensure consistency with
buildField; update references to DataField.getAllowedNets() and
((FieldWithAllowedNets) field).setAllowedNets(...) accordingly.
In `@src/main/java/com/netgrif/application/engine/importer/service/Importer.java`:
- Line 243: The method signature change to use CaseActorRef/ActorRef breaks
compilation because those types are not present in
com.netgrif.application.engine.importer.model; restore the original generated
model types (the previous importer model classes used by resolveUserRef and any
other affected methods such as the one at line ~732) or add the corresponding
model/schema classes to com.netgrif.application.engine.importer.model in this PR
so callers and method signatures remain consistent; update the signatures of
resolveUserRef and any other methods that reference CaseActorRef/ActorRef back
to the original model types (or ensure the newly added classes match the
expected names and packages) and re-run compilation to confirm the pipeline
passes.
In
`@src/main/java/com/netgrif/application/engine/menu/domain/configurations/CaseViewBody.java`:
- Line 29: Rename the boolean field isHeaderModeChangeable in class CaseViewBody
to headerModeChangeable to make Lombok generate predictable accessors (getter
isHeaderModeChangeable() and setter setHeaderModeChangeable(...) currently
confuse callers/serializers); update all usages of isHeaderModeChangeable, any
direct field references, and any JSON bindings or tests to use
headerModeChangeable so accessor names and serialized property match the other
boolean fields (e.g., follow the pattern used for other booleans in
CaseViewBody).
- Around line 69-98: The code unconditionally writes nullable Text fields
bannedNetsInCreation and emptyContentIcon into the dataset via
ToDataSetOutcome.putDataSetEntry (in CaseViewBody), causing explicit null
entries; instead guard those entries like other optional fields and only call
putDataSetEntry for CaseViewConstants.FIELD_BANNED_NETS_IN_CREATION and
CaseViewConstants.FIELD_EMPTY_CONTENT_ICON when bannedNetsInCreation and
emptyContentIcon are non-null (respectively), so the dataset omits missing
values rather than persisting value: null.
In
`@src/main/java/com/netgrif/application/engine/menu/domain/configurations/TaskViewBody.java`:
- Around line 76-81: TaskViewBody.toDataSetInternal always emits
TaskViewConstants.FIELD_EMPTY_CONTENT_ICON even when emptyContentIcon is null;
add the same null guard used for emptyContentText so the icon entry is only put
when non-null. Modify the code path in TaskViewBody.toDataSetInternal to check
this.emptyContentIcon != null before calling
outcome.putDataSetEntry(TaskViewConstants.FIELD_EMPTY_CONTENT_ICON,
FieldType.TEXT, this.emptyContentIcon), and apply the identical guard to
CaseViewBody where the same asymmetry exists.
In
`@src/main/java/com/netgrif/application/engine/menu/domain/ConfigurationTemplateOutcome.java`:
- Around line 16-20: The constructor
ConfigurationTemplateOutcome(ToDataSetOutcome toDataSetOutcome) should
defensively validate inputs: first check toDataSetOutcome and
toDataSetOutcome.getDataSet() for null before iterating; inside the forEach
ensure each fieldMap is non-null and contains the "value" key (and that
fieldMap.get("value") is non-null if you don't want nulls stored) before calling
this.mapping.put(fieldId, ...). Update the constructor to skip or handle entries
that fail those checks so no NullPointerException is thrown when
ToDataSetOutcome, the dataset, a fieldMap, or the "value" entry is missing.
- Line 10: The public mutable Map field mapping on ConfigurationTemplateOutcome
breaks encapsulation; make the field private (private final Map<String,Object>
mapping) and add a public accessor method (e.g., getMapping()) that returns
either an unmodifiable view (Collections.unmodifiableMap(mapping)) or a
defensive copy to prevent external mutation; update any direct accesses to the
mapping field to use getMapping() so callers cannot mutate the internal state
directly.
In `@src/main/java/com/netgrif/application/engine/menu/domain/FilterBody.java`:
- Around line 22-29: In FilterBody.toDataSet(), guard against a null type to
avoid NPE: check if this.type is null before calling this.type.getName() and
handle it defensively (e.g., compute a safe typeName = (this.type != null ?
this.type.getName() : null) or throw a clear IllegalStateException); then put
that safe typeName into the dataSetValues map and continue to populate value and
insert into viewDataSetOutcome.getDataSet(). Also consider adding a unit test
for FilterBody.toDataSet() when type is null; reference: FilterBody.toDataSet(),
the field type, and the flow where ViewBody.toDataSet() calls
filterBody.setType(getFilterType()).
In `@src/main/java/com/netgrif/application/engine/menu/domain/MenuItemBody.java`:
- Around line 112-119: The conditional in MenuItemBody that sets useTabbedView
only when viewType.isTabbed() != viewType.isUntabbed() is subtle; update the
code comments (or add a small helper like MenuItemViewType.isDeterminate()) to
explicitly state that isTabbed==isUntabbed indicates an indeterminate/neutral
view type and therefore useTabbedView must remain unchanged, and add Javadoc to
MenuItemViewType explaining why both flags might be equal (e.g., legacy/neutral
types or mutually inclusive flags) so future readers understand the intended
behavior; reference the viewBody assignment, MenuItemViewType,
viewType.isTabbed(), viewType.isUntabbed(), and the useTabbedView field when
adding the explanation.
- Line 189: The call
outcome.putDataSetEntry(MenuItemConstants.FIELD_CONFIGURATION_TEMPLATES,
FieldType.ENUMERATION_MAP, this.configurationTemplateIdentifier) in MenuItemBody
may store a null; add a null-safety guard so you only call putDataSetEntry when
configurationTemplateIdentifier is non-null (or pass a safe default/empty map
instead). Locate the occurrence in the MenuItemBody class and update the logic
around outcome.putDataSetEntry to check this.configurationTemplateIdentifier (or
replace it with a non-null fallback) before invoking the method.
In
`@src/main/java/com/netgrif/application/engine/menu/service/MenuItemService.java`:
- Around line 140-149: The update flow in updateMenuItem currently deletes the
existing case via workflowService.deleteCase(itemCase) before creating the
replacement, making the operation non-atomic; change the flow to first validate
and create the replacement using createMenuItem(body), verify the new case and
its view/folder references were created successfully, then remove the old case
and update parent/view references (or swap identifiers) so the original is only
deleted after the new one is confirmed; ensure logging (log.debug) reflects
creation success prior to calling workflowService.deleteCase and handle failures
by rolling back or leaving the original intact.
- Around line 72-83: The current indexes created in ensureDatabaseIndexes
(CUSTOM_MENU_ITEM_INDEXES via mongoTemplate.indexOps(Case.class).ensureIndex)
are non-unique, so concurrent createMenuItem() calls can still insert
duplicates; update ensureDatabaseIndexes to create unique compound indexes for
(processIdentifier + identifier) and (processIdentifier + nodePath) by setting
the index definitions to unique (use the unique option on the
CompoundIndexDefinition/IndexDefinition used), and keep the existing index names
from CUSTOM_MENU_ITEM_INDEXES; additionally harden createMenuItem() by catching
the Mongo duplicate-key exception (e.g., DuplicateKeyException) around the
insert/persist and translate it into the same error behavior as existsMenuItem()
would indicate (reject/return conflict), ensuring
findMenuItem()/findFolderCase() cannot observe duplicates.
- Around line 96-103: Normalize the menu identifier once at the start of each
entry point: call the existing sanitize(...) helper (or add one if missing) and
use the resulting normalizedId for all lookups, uniqueness checks
(existsMenuItem), and persistence instead of raw body.getIdentifier(); for
example in createMenuItem, createOrUpdateMenuItem, and createOrIgnoreMenuItem
obtain String normalizedId = sanitize(body.getIdentifier()), replace uses of
body.getIdentifier() with normalizedId for existsMenuItem calls and when
creating/updating the Case, and optionally set body.setIdentifier(normalizedId)
after validation so all downstream code sees the normalized value; apply the
same change to the other affected blocks referenced (around the other line
ranges) to ensure consistent identifier handling.
- Around line 458-480: handleConfigurationTemplate currently calls createView
which immediately creates/persists initialized view cases (via
MenuItemBody.hasView -> createView) causing orphaned persisted cases if the
template selection is later cancelled or changed; change the flow so template
previewing does not persist: update createView to support a non-persisting mode
(e.g., an additional parameter or a new method createTransientView) that builds
and initializes the Case in-memory without saving, use that non-persisting
variant from handleConfigurationTemplate before calling
MenuItemBody.toDataSetByConfigTemplate(viewCase), and if any callers need
persisted behavior keep the original createView signature or add a separate
persistedCreateView while updating usages accordingly; ensure any code paths
that do persist now are deliberate and add cleanup logic for any edge-case
persistence if rollback is required.
In `@src/main/java/com/netgrif/application/engine/menu/utils/MenuItemUtils.java`:
- Around line 48-57: The code treats the URI separator as a regex: change
uri.split(uriService.getUriSeparator()) to use a literal split (e.g.
Pattern.quote(uriService.getUriSeparator())) so the separator is quoted, and
replace replaceAll("//", uriService.getUriSeparator()) with a literal
replacement (e.g. String.replace("//", uriService.getUriSeparator()) or use
Matcher.quoteReplacement(...) if keeping replaceAll) to avoid regex/replacement
metacharacter issues; update references in MenuItemUtils where uri.split(...)
and the final replaceAll(...) are used and add necessary imports
(Pattern/Matcher) if required.
In `@src/main/java/com/netgrif/application/engine/menu/web/MenuController.java`:
- Around line 33-34: The endpoint in MenuController annotated with
Operation("Get relevant data for the menu item") is missing authorization; add
an explicit access check before returning menu data by verifying the current
principal can access the target case id (or related resource) — either annotate
the controller method with a suitable Spring Security annotation (e.g.,
`@PreAuthorize`) AND/OR call an authorization helper (e.g.,
authorizationService.authorizeCaseAccess(caseId) or
searchAuthorizationService.authorizeSearch(user, criteria)) at the start of the
handler; return 403 when unauthorized and ensure the check uses the decoded case
id the method currently accepts to prevent exposing menu data without proper
permissions.
- Around line 37-44: The code in MenuController currently decodes encodedCaseId
with Base64.getDecoder().decode(...) and catches all Exceptions, causing client
errors to return 500; change the decoding to use new
String(Base64.getDecoder().decode(encodedCaseId), StandardCharsets.UTF_8) to fix
charset dependence, then add a specific catch for IllegalArgumentException
(thrown by Base64 decoding) that throws new
ResponseStatusException(HttpStatus.BAD_REQUEST, "Invalid encodedCaseId", e) and
keep the existing generic catch (Exception e) to rethrow 500 for true server
errors; reference Base64.getDecoder().decode, encodedCaseId, new String(...,
StandardCharsets.UTF_8), and the existing catch block in MenuController to
locate where to apply the changes.
In
`@src/main/java/com/netgrif/application/engine/menu/web/responsebodies/MenuItemDataResponse.java`:
- Around line 12-16: The Javadoc for the MenuItemDataResponse.data field
incorrectly describes a Map; update the comment to describe the actual type: a
List<DataGroup>. Edit the Javadoc above the private final List<DataGroup> data
field in class MenuItemDataResponse to state that it is a list of DataGroup
objects representing immediate fields (or groups of fields) for the menu item
(e.g., one DataGroup per view or section), ensuring the wording matches the
List<DataGroup> type and references DataGroup semantics.
In `@src/main/resources/petriNets/engine-processes/menu/menu_item.xml`:
- Around line 928-954: After applying the configuration template in the finish
event (where menuItemService.handleConfigurationTemplate(useCase) sets
outcome.mapping and values on useCase fields), re-run the tab-field visibility
logic so changes to use_tabbed_view/use_tab_icon take effect; specifically,
after the loop that applies outcome.mapping call the same initializer or
function used elsewhere (e.g., manageBehaviorOfTabFields() or the view_settings
initialization routine) to recompute visibility for view_settings, tabIcon,
tabName, etc., so templates that disable tabs actually hide tab-related fields;
apply the same fix to the other finish block at the 1425-1449 region.
- Around line 847-867: Compute the sanitized identifier up front using
com.netgrif.application.engine.menu.utils.MenuItemUtils.sanitize(menu_item_identifier.value)
and use that sanitizedIdentifier when calling
menuItemService.existsMenuItem(...) and when setting validations; if the
sanitizedIdentifier differs from the raw value, still update the field value via
the existing change menu_item_identifier value block (so the stored value is
normalized) but perform the uniqueness check against sanitizedIdentifier (and
update the validation logic around menu_item_identifier accordingly).
In
`@src/test/groovy/com/netgrif/application/engine/menu/MenuImportExportTest.groovy`:
- Around line 72-73: The field in MenuImportExportTest is misleadingly named
filterRunner while its type is MenuRunner and it's unused; either rename the
field to menuRunner to match the type (change the declaration from filterRunner
to menuRunner) and update any references in the test class to use menuRunner, or
remove the field entirely if there are no usages; ensure the `@Autowired`
annotation remains on the renamed field if kept.
In `@src/test/java/com/netgrif/application/engine/menu/MenuItemServiceTest.java`:
- Around line 724-733: Replace the fixed Thread.sleep(2000) with a polling wait
(e.g. Awaitility) that repeatedly reloads netgrifFolder via
workflowService.findOne(netgrifFolderId) and checks
MenuItemUtils.getCaseIdsFromCaseRef(netgrifFolder,
MenuItemConstants.FIELD_CHILD_ITEM_IDS) no longer contains
testFolder.getStringId(), and that workflowService.findOne(...) throws
IllegalArgumentException for testFolder.getStringId(),
leafItemCase.getStringId(), tabbedCaseViewId and tabbedTaskViewId; use
await().atMost(timeout).untilAsserted(() -> { assertNotNull(updatedChildIds);
assertFalse(updatedChildIds.contains(...));
assertThrows(IllegalArgumentException.class, () ->
workflowService.findOne(...)); ... }) so the test polls until the cascade
deletion completes instead of sleeping.
- Around line 560-566: The assertion in MenuItemServiceTest incorrectly compares
the settings form variable duplicatedFormValue to originAllFormValue; change the
assertion to compare duplicatedAllFormValue.get(0) to originAllFormValue.get(0)
so the duplicated all-data-form is validated, i.e., replace the use of
duplicatedFormValue with duplicatedAllFormValue in the failing assertNotEquals
that references duplicatedFormValue and originAllFormValue (the test variables
involved: duplicatedAllFormValue, duplicatedFormValue, originAllFormValue and
the constant MenuItemConstants.FIELD_VIEW_CONFIGURATION_ALL_DATA_FORM).
---
Outside diff comments:
In
`@src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/FieldType.groovy`:
- Around line 24-37: fromString(String name) currently calls
valueOf(name.toUpperCase()) which converts serialized names like
"caseFilter"/"taskFilter"/"processFilter" into
CASEFILTER/TASKFILTER/PROCESSFILTER and fails to match the enum constants;
update FieldType.fromString to lookup by the enum instance's serialized name
property instead (e.g., iterate FieldType.values() and return the entry whose
'name' field equals the input, with a case-insensitive comparison or sensible
fallback), and keep a fallback to valueOf(name.toUpperCase()) only if necessary
to preserve backwards compatibility.
In
`@src/main/java/com/netgrif/application/engine/menu/domain/configurations/ViewBody.java`:
- Around line 70-78: In ViewBody.toDataSet(Case) the code dereferences
this.getAssociatedViewBody() while only checking associatedViewCase != null;
change the guard to ensure the associated view body is non-null as well (e.g.,
if (associatedViewCase != null && this.getAssociatedViewBody() != null) or use
this.hasAssociatedView()) so you don't risk NPE when getAssociatedViewBody() can
be null (references: ViewBody.toDataSet(Case), getAssociatedViewBody(),
hasAssociatedView(), MenuItemService.createView(...)).
In
`@src/main/java/com/netgrif/application/engine/menu/domain/MenuItemViewType.java`:
- Around line 74-94: The tabbed/untabbed predicate is unclear and brittle;
update both findAllByIsTabbedAndIsPrimary and
findAllByIsTabbedAndParentIdentifier to explicitly match the intended semantics:
when isTabbed==true filter by view.isTabbed==true, otherwise filter by
view.isUntabbed==true; keep the other condition in each method (isPrimary match
or parentView.getAllowedAssociatedViews().contains(view.identifier)) unchanged
so only the tabbed check is simplified and made explicit using the view.isTabbed
and view.isUntabbed fields.
In `@src/main/resources/petriNets/engine-processes/menu/menu_item.xml`:
- Around line 203-216: Reject values for move_dest_uri_new_node that contain URI
separator characters instead of appending them verbatim: in the action that
reads newNodeName (symbol newNodeName / move_dest_uri_new_node) validate
newNodeName.value for occurrences of "/" and uriService.getUriSeparator() and if
found either return early (like existing empty check) or surface a validation
error; alternatively trim any leading/trailing separators before building
prefixUri so the code that constructs newUri (uses selectedUri,
uriService.getUriSeparator(), and uriService.getOrCreate) cannot create extra
path segments. Ensure the check/sanitization occurs before concatenating
prefixUri and calling uriService.getOrCreate.
In
`@src/test/groovy/com/netgrif/application/engine/menu/MenuImportExportTest.groovy`:
- Around line 183-184: The test calls
menuImportExportService.invokeMethod("loadFromXML", ...) but the test class
lacks a menuImportExportService collaborator; add a collaborator field or bean
for MenuImportExportService (e.g., declare and annotate a
MenuImportExportService menuImportExportService with `@Autowired` or initialize a
mock and inject it) so the invokeMethod calls on menuImportExportService (used
to loadFromXML) resolve at runtime; ensure the field name matches
menuImportExportService and that the service provides the loadFromXML behavior
expected by the test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 42646096-e6e5-4bbb-936d-5e1f70198cd5
📒 Files selected for processing (96)
docs/_sidebar.mddocs/search/filter.mddocs/search/filter_import_export.mdsrc/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/CaseFilterField.groovysrc/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/FieldType.groovysrc/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/FilterField.groovysrc/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/ProcessFilterField.groovysrc/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/TaskFilterField.groovysrc/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovysrc/main/groovy/com/netgrif/application/engine/startup/DefaultDashboardRunner.groovysrc/main/groovy/com/netgrif/application/engine/startup/DefaultFiltersRunner.groovysrc/main/groovy/com/netgrif/application/engine/startup/FilterRunner.groovysrc/main/groovy/com/netgrif/application/engine/startup/MenuRunner.groovysrc/main/groovy/com/netgrif/application/engine/startup/MongoDbRunner.groovysrc/main/groovy/com/netgrif/application/engine/startup/RunnerController.groovysrc/main/java/com/netgrif/application/engine/auth/service/UserService.javasrc/main/java/com/netgrif/application/engine/importer/service/FieldFactory.javasrc/main/java/com/netgrif/application/engine/importer/service/Importer.javasrc/main/java/com/netgrif/application/engine/menu/domain/ConfigurationTemplateOutcome.javasrc/main/java/com/netgrif/application/engine/menu/domain/FilterBody.javasrc/main/java/com/netgrif/application/engine/menu/domain/MenuItemBody.javasrc/main/java/com/netgrif/application/engine/menu/domain/MenuItemConstants.javasrc/main/java/com/netgrif/application/engine/menu/domain/MenuItemViewType.javasrc/main/java/com/netgrif/application/engine/menu/domain/configurations/CaseViewBody.javasrc/main/java/com/netgrif/application/engine/menu/domain/configurations/CaseViewConstants.javasrc/main/java/com/netgrif/application/engine/menu/domain/configurations/SingleTaskViewBody.javasrc/main/java/com/netgrif/application/engine/menu/domain/configurations/SingleTaskViewConstants.javasrc/main/java/com/netgrif/application/engine/menu/domain/configurations/TabbedCaseViewBody.javasrc/main/java/com/netgrif/application/engine/menu/domain/configurations/TabbedCaseViewConstants.javasrc/main/java/com/netgrif/application/engine/menu/domain/configurations/TabbedSingleTaskViewBody.javasrc/main/java/com/netgrif/application/engine/menu/domain/configurations/TabbedSingleTaskViewConstants.javasrc/main/java/com/netgrif/application/engine/menu/domain/configurations/TabbedTaskViewBody.javasrc/main/java/com/netgrif/application/engine/menu/domain/configurations/TabbedTaskViewConstants.javasrc/main/java/com/netgrif/application/engine/menu/domain/configurations/TabbedTicketViewBody.javasrc/main/java/com/netgrif/application/engine/menu/domain/configurations/TaskViewBody.javasrc/main/java/com/netgrif/application/engine/menu/domain/configurations/TaskViewConstants.javasrc/main/java/com/netgrif/application/engine/menu/domain/configurations/ViewBody.javasrc/main/java/com/netgrif/application/engine/menu/domain/configurations/ViewConstants.javasrc/main/java/com/netgrif/application/engine/menu/domain/templates/CustomViewTemplate.javasrc/main/java/com/netgrif/application/engine/menu/domain/templates/README.mdsrc/main/java/com/netgrif/application/engine/menu/domain/templates/SimpleCaseViewTemplate.javasrc/main/java/com/netgrif/application/engine/menu/domain/templates/SimpleTaskViewTemplate.javasrc/main/java/com/netgrif/application/engine/menu/domain/templates/SingleTaskViewTemplate.javasrc/main/java/com/netgrif/application/engine/menu/domain/templates/TabbedCaseViewTemplate.javasrc/main/java/com/netgrif/application/engine/menu/domain/templates/TabbedTaskViewTemplate.javasrc/main/java/com/netgrif/application/engine/menu/domain/templates/TabbedTicketViewTemplate.javasrc/main/java/com/netgrif/application/engine/menu/domain/templates/Template.javasrc/main/java/com/netgrif/application/engine/menu/service/DashboardItemServiceImpl.javasrc/main/java/com/netgrif/application/engine/menu/service/DashboardManagementServiceImpl.javasrc/main/java/com/netgrif/application/engine/menu/service/MenuItemService.javasrc/main/java/com/netgrif/application/engine/menu/service/MenuItemTemplateHolder.javasrc/main/java/com/netgrif/application/engine/menu/service/interfaces/DashboardItemService.javasrc/main/java/com/netgrif/application/engine/menu/service/interfaces/DashboardManagementService.javasrc/main/java/com/netgrif/application/engine/menu/service/interfaces/IMenuItemService.javasrc/main/java/com/netgrif/application/engine/menu/utils/MenuItemUtils.javasrc/main/java/com/netgrif/application/engine/menu/web/MenuController.javasrc/main/java/com/netgrif/application/engine/menu/web/responsebodies/MenuItemDataResponse.javasrc/main/java/com/netgrif/application/engine/workflow/domain/Case.javasrc/main/java/com/netgrif/application/engine/workflow/domain/DataField.javasrc/main/java/com/netgrif/application/engine/workflow/service/DataService.javasrc/main/java/com/netgrif/application/engine/workflow/service/FilterImportExportService.javasrc/main/java/com/netgrif/application/engine/workflow/service/MenuImportExportService.javasrc/main/java/com/netgrif/application/engine/workflow/service/UserFilterSearchService.javasrc/main/java/com/netgrif/application/engine/workflow/service/WorkflowService.javasrc/main/java/com/netgrif/application/engine/workflow/service/interfaces/IFilterImportExportService.javasrc/main/java/com/netgrif/application/engine/workflow/service/interfaces/IMenuImportExportService.javasrc/main/java/com/netgrif/application/engine/workflow/service/interfaces/IUserFilterSearchService.javasrc/main/java/com/netgrif/application/engine/workflow/web/responsebodies/LocalisedCaseFilterField.javasrc/main/java/com/netgrif/application/engine/workflow/web/responsebodies/LocalisedFieldFactory.javasrc/main/java/com/netgrif/application/engine/workflow/web/responsebodies/LocalisedFilterField.javasrc/main/java/com/netgrif/application/engine/workflow/web/responsebodies/LocalisedProcessFilterField.javasrc/main/java/com/netgrif/application/engine/workflow/web/responsebodies/LocalisedTaskFilterField.javasrc/main/resources/petriNets/engine-processes/export_filters.xmlsrc/main/resources/petriNets/engine-processes/filter.xmlsrc/main/resources/petriNets/engine-processes/import_filters.xmlsrc/main/resources/petriNets/engine-processes/menu/case_view_configuration.xmlsrc/main/resources/petriNets/engine-processes/menu/menu_item.xmlsrc/main/resources/petriNets/engine-processes/menu/single_task_view_configuration.xmlsrc/main/resources/petriNets/engine-processes/menu/tabbed_single_task_view_configuration.xmlsrc/main/resources/petriNets/engine-processes/menu/tabbed_task_view_configuration.xmlsrc/main/resources/petriNets/engine-processes/menu/tabbed_ticket_view_configuration.xmlsrc/main/resources/petriNets/engine-processes/menu/task_view_configuration.xmlsrc/main/resources/petriNets/engine-processes/org_group.xmlsrc/main/resources/petriNets/engine-processes/preference_filter_item.xmlsrc/test/groovy/com/netgrif/application/engine/TestHelper.groovysrc/test/groovy/com/netgrif/application/engine/action/ActionDelegateTest.groovysrc/test/groovy/com/netgrif/application/engine/action/FilterApiTest.groovysrc/test/groovy/com/netgrif/application/engine/action/MenuItemApiTest.groovysrc/test/groovy/com/netgrif/application/engine/export/service/XlsExportServiceTest.javasrc/test/groovy/com/netgrif/application/engine/filters/FilterImportExportTest.groovysrc/test/groovy/com/netgrif/application/engine/filters/FilterTest.groovysrc/test/groovy/com/netgrif/application/engine/menu/MenuImportExportTest.groovysrc/test/java/com/netgrif/application/engine/menu/MenuItemServiceTest.javasrc/test/resources/dashboard_management_test.xmlsrc/test/resources/petriNets/filter_api_test.xmlsrc/test/resources/petriNets/filter_test.xml
💤 Files with no reviewable changes (32)
- src/main/java/com/netgrif/application/engine/menu/domain/configurations/TabbedCaseViewConstants.java
- src/main/resources/petriNets/engine-processes/menu/tabbed_task_view_configuration.xml
- src/main/java/com/netgrif/application/engine/workflow/service/interfaces/IFilterImportExportService.java
- src/test/resources/petriNets/filter_api_test.xml
- src/main/resources/petriNets/engine-processes/menu/tabbed_single_task_view_configuration.xml
- src/main/java/com/netgrif/application/engine/workflow/service/interfaces/IMenuImportExportService.java
- src/main/resources/petriNets/engine-processes/import_filters.xml
- src/main/java/com/netgrif/application/engine/workflow/service/interfaces/IUserFilterSearchService.java
- src/test/groovy/com/netgrif/application/engine/action/FilterApiTest.groovy
- docs/search/filter_import_export.md
- src/main/java/com/netgrif/application/engine/workflow/web/responsebodies/LocalisedFilterField.java
- src/main/java/com/netgrif/application/engine/workflow/service/UserFilterSearchService.java
- src/main/java/com/netgrif/application/engine/menu/domain/configurations/TabbedTaskViewConstants.java
- docs/search/filter.md
- src/main/java/com/netgrif/application/engine/menu/domain/configurations/TabbedSingleTaskViewConstants.java
- src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/FilterField.groovy
- src/main/resources/petriNets/engine-processes/filter.xml
- src/main/java/com/netgrif/application/engine/menu/domain/configurations/TabbedSingleTaskViewBody.java
- src/main/resources/petriNets/engine-processes/preference_filter_item.xml
- src/main/groovy/com/netgrif/application/engine/startup/DefaultFiltersRunner.groovy
- src/main/java/com/netgrif/application/engine/auth/service/UserService.java
- src/main/resources/petriNets/engine-processes/export_filters.xml
- src/main/java/com/netgrif/application/engine/workflow/service/MenuImportExportService.java
- src/main/java/com/netgrif/application/engine/menu/domain/configurations/TabbedTaskViewBody.java
- src/main/groovy/com/netgrif/application/engine/startup/FilterRunner.groovy
- src/main/java/com/netgrif/application/engine/workflow/domain/DataField.java
- src/test/groovy/com/netgrif/application/engine/action/MenuItemApiTest.groovy
- docs/_sidebar.md
- src/main/java/com/netgrif/application/engine/workflow/service/FilterImportExportService.java
- src/test/groovy/com/netgrif/application/engine/filters/FilterImportExportTest.groovy
- src/main/java/com/netgrif/application/engine/menu/domain/configurations/TabbedCaseViewBody.java
- src/main/resources/petriNets/engine-processes/org_group.xml
|
|
||
| @Transactional | ||
| protected void resolveUserRef(CaseUserRef userRef) { | ||
| protected void resolveUserRef(CaseActorRef userRef) { |
There was a problem hiding this comment.
Restore the importer model types here; the file no longer compiles.
The new CaseActorRef/ActorRef signatures do not exist in com.netgrif.application.engine.importer.model, and the pipeline is already failing on these two lines. Revert to the existing generated model types or add the corresponding model/schema classes in the same PR so the callers and method signatures stay consistent.
Also applies to: 732-732
🧰 Tools
🪛 GitHub Actions: Pull Request / 1_Build.txt
[error] 243-243: Maven compilation failure (maven-compiler-plugin:3.10.1:compile). cannot find symbol: class CaseActorRef (Importer.java:[243,35]).
🪛 GitHub Actions: Pull Request / Build
[error] 243-243: Compilation error: cannot find symbol 'CaseActorRef' (referenced in class com.netgrif.application.engine.importer.service.Importer) during 'maven-compiler-plugin:3.10.1:compile (default-compile)'
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/com/netgrif/application/engine/importer/service/Importer.java`
at line 243, The method signature change to use CaseActorRef/ActorRef breaks
compilation because those types are not present in
com.netgrif.application.engine.importer.model; restore the original generated
model types (the previous importer model classes used by resolveUserRef and any
other affected methods such as the one at line ~732) or add the corresponding
model/schema classes to com.netgrif.application.engine.importer.model in this PR
so callers and method signatures remain consistent; update the signatures of
resolveUserRef and any other methods that reference CaseActorRef/ActorRef back
to the original model types (or ensure the newly added classes match the
expected names and packages) and re-run compilation to confirm the pipeline
passes.
Contributor
Author
There was a problem hiding this comment.
New schema must be first published. It is waiting on PR petriflow/petriflow#30. Then this is going to be ok
There was a problem hiding this comment.
@Retoocs, understood — this is the same intentional breakage tracked from PR #446. I already have a note in my long-term memory that CaseActorRef/ActorRef are not yet available in com.netgrif.application.engine.importer.model and the build will remain broken until petriflow/petriflow#30 is merged and the importer model is regenerated.
Would you like me to resolve this review comment since the failure is expected and will be addressed by the upstream schema release?
(=🐇=)
🧠 Learnings used
Learnt from: Retoocs
Repo: netgrif/application-engine PR: 446
File: src/main/java/com/netgrif/application/engine/importer/service/Importer.java:243-243
Timestamp: 2026-06-01T07:23:09.747Z
Learning: In `src/main/java/com/netgrif/application/engine/importer/service/Importer.java`, the PR NAE-2439 (PR `#446`) changes the `resolveUserRef` method signature from `CaseUserRef` to `CaseActorRef` and `addUserLogic` from `UserRef` to `ActorRef`. These types (`CaseActorRef`, `ActorRef`) are not yet available in `com.netgrif.application.engine.importer.model` — they will be generated once the upstream petriflow schema change (https://github.com/petriflow/petriflow/pull/30) is released. The build intentionally breaks until that schema lands and the importer model is regenerated.
Learnt from: Retoocs
Repo: netgrif/application-engine PR: 398
File: application-engine/src/main/java/com/netgrif/application/engine/importer/service/Importer.java:57-58
Timestamp: 2025-12-19T09:06:38.316Z
Learning: In the file `application-engine/src/main/java/com/netgrif/application/engine/importer/service/Importer.java`, the `definition` attribute of the `Action` class (from `com.netgrif.application.engine.objects.petrinet.domain.dataset.logic.action.Action`) is never null, so null checks for `action.getDefinition()` are not needed.
Learnt from: Retoocs
Repo: netgrif/application-engine PR: 397
File: application-engine/src/main/java/com/netgrif/application/engine/workflow/service/CaseSearchService.java:98-104
Timestamp: 2025-12-12T13:40:00.786Z
Learning: In application-engine/src/main/java/com/netgrif/application/engine/workflow/service/CaseSearchService.java, the permission constraint logic in buildQuery() intentionally applies negative actor constraints globally: ((roleConstraint AND NOT negRoleConstraint) OR actorConstraint) AND NOT negActorConstraint. This means users in negativeViewActors are excluded even if they have positive role permissions, which is the intended authorization behavior.
Learnt from: Retoocs
Repo: netgrif/application-engine PR: 397
File: application-engine/src/main/resources/petriNets/engine-processes/org_group.xml:36-39
Timestamp: 2025-12-12T12:36:06.651Z
Learning: In the org_group.xml file (application-engine/src/main/resources/petriNets/engine-processes/org_group.xml), variable names in action blocks like userField and user_selection should be kept as-is even when the field type changes from "user" to "actor".
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 348
File: application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/interfaces/IPetriNetService.java:4-4
Timestamp: 2026-01-13T16:33:27.510Z
Learning: In the netgrif/application-engine repository, wildcard imports are preferred for the com.netgrif.application.engine.objects.petrinet.domain package. Do not suggest replacing wildcard imports with explicit imports for this package.
Learnt from: Smotanka
Repo: netgrif/application-engine PR: 401
File: src/main/java/com/netgrif/application/engine/elastic/service/ElasticsearchQuerySanitizer.java:57-66
Timestamp: 2025-12-18T19:20:32.658Z
Learning: When performing multiple string replacements in Java files (e.g., sanitization/escaping logic like ElasticsearchQuerySanitizer), prefer using StringUtils.replaceEach() over chaining multiple StringUtils.replace() calls. This avoids cascading replacements where the result of one replacement triggers another replacement and potentially corrupts the final output. Apply this pattern to other similar sanitization code across the codebase as appropriate.
- resolve PR comments
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/test/groovy/com/netgrif/application/engine/menu/MenuImportExportTest.groovy (1)
119-187:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winThis disabled test still calls a removed collaborator.
Lines 183-184 dereference
menuImportExportService, but that field is no longer declared in this class. The test is disabled today, yet the first re-enable will fail immediately instead of validating import/export behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/groovy/com/netgrif/application/engine/menu/MenuImportExportTest.groovy` around lines 119 - 187, The test testMenuImportExport dereferences menuImportExportService.loadFromXML but the menuImportExportService collaborator was removed; restore a reference to the proper service (declare a field named menuImportExportService of the correct type used elsewhere in tests, e.g., MenuImportExportService or the class that exposes loadFromXML) and inject it (e.g., with `@Autowired` or the existing test setup method), or update the test to call the current service that provides loadFromXML/export functionality; ensure the symbol menuImportExportService and the method loadFromXML remain available to the test so the final assertions comparing MenuAndFilters work.src/main/java/com/netgrif/application/engine/menu/service/MenuItemService.java (1)
437-440:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAuthorize
getMenuItemData()before returning menu configuration data.
MenuController’s new/api/menu/{encodedCaseId}endpoint calls this method directly, and this implementation returns theall_menu_datatask payload for any decoded case id without checking whether the current user may view that menu item/task. That makes the new endpoint an IDOR-style read path for menu configuration data.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/netgrif/application/engine/menu/service/MenuItemService.java` around lines 437 - 440, The method getMenuItemData currently returns the all_menu_data payload for any decoded case id without permission checks; before calling dataService.getDataGroups you must authorize that the current principal may view the menu item/task. Locate getMenuItemData and, after obtaining menuItemCase and taskId via workflowService.findOne and MenuItemUtils.findTaskIdInCase (TRANS_ALL_MENU_DATA), invoke the existing authorization mechanism (e.g., authorizationService/permissionService or workflowService authorization helper) to verify read/view rights on the task or case and throw an access denied exception if unauthorized; only then call dataService.getDataGroups(taskId, locale). Ensure the check uses the same authority semantics as other endpoints (e.g., MenuController) so behavior is consistent.
♻️ Duplicate comments (3)
src/main/resources/petriNets/engine-processes/menu/menu_item.xml (1)
928-954:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRecompute tab-field behavior after applying template/system initialization.
These finish hooks update
use_tabbed_view/use_tab_icon, but they never re-runmanageBehaviorOfTabFields(...). With the current defaults in this file (use_tabbed_viewstarts astrue, and the tab refs inview_settingsare visible/editable by default), a non-tabbed template still leavestab_name,use_tab_icon,tab_icon, andtab_icon_previewexposed until the user toggles the checkbox manually.💡 Suggested fix
<action> trans: t.view_settings, + useTabbedView: f.use_tabbed_view, useTabIcon: f.use_tab_icon, view_configuration_form: f.view_configuration_form, view_configuration_type: f.view_configuration_type, @@ use_custom_view: f.use_custom_view, selector: f.custom_view_selector; + manageBehaviorOfTabFields(useTabIcon.value, useTabbedView.value) + if (use_custom_view.value) { make selector, editable on trans when { true } make [useTabIcon, tabIconPreview, tabName, tabIcon, view_configuration_form, view_configuration_type], hidden on trans when { true } } @@ <action> trans: t.view_settings, + useTabbedView: f.use_tabbed_view, useTabIcon: f.use_tab_icon, view_configuration_form: f.view_configuration_form, view_configuration_type: f.view_configuration_type, @@ use_custom_view: f.use_custom_view, selector: f.custom_view_selector; + manageBehaviorOfTabFields(useTabIcon.value, useTabbedView.value) + if (use_custom_view.value) { make selector, editable on trans when { true } make [useTabIcon, tabIconPreview, tabName, tabIcon, view_configuration_form, view_configuration_type], hidden on trans when { true } }Also applies to: 1425-1444
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/resources/petriNets/engine-processes/menu/menu_item.xml` around lines 928 - 954, After applying the configuration template in the finish event (where menuItemService.handleConfigurationTemplate(useCase) maps values into fields like use_tabbed_view/use_tab_icon/tab_name/tab_icon/tab_icon_preview and view_settings), explicitly invoke the function that enforces tab-field visibility/behavior (manageBehaviorOfTabFields or equivalent) so the visibility/editability of tab-related fields (use_tab_icon, tab_icon_preview, tab_name, tab_icon, view_configuration_form, view_configuration_type, custom_view_selector) is recomputed; do this immediately after the template-mapping action in the finish hook(s) (both at the shown block and the similar block around lines 1425-1444) so non-tabbed templates do not leave tab fields exposed.src/main/java/com/netgrif/application/engine/menu/service/MenuItemService.java (1)
162-170:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNormalize and validate the identifier once before lookup and persistence.
createOrUpdateMenuItem()andcreateOrIgnoreMenuItem()search with a sanitized identifier, butvalidateMenuItemBody()still accepts blank identifiers andcreateMenuItem()persists the raw value. That leaves the service inconsistent again: logically equivalent inputs can bypass each other’s uniqueness checks, and the stored identifier/node path can differ from the value used for lookup.💡 Suggested fix
public Case createOrUpdateMenuItem(MenuItemBody body) throws TransitionNotExecutableException { - if (body == null) { - throw new IllegalArgumentException("Menu item body cannot be null"); - } - Case itemCase = findMenuItem(MenuItemUtils.sanitize(body.getIdentifier())); + validateMenuItemBody(body); + Case itemCase = findMenuItem(body.getIdentifier()); if (itemCase != null) { return updateMenuItem(itemCase, body); } else { return createMenuItem(body); } } public Case createOrIgnoreMenuItem(MenuItemBody body) throws TransitionNotExecutableException { - if (body == null) { - throw new IllegalArgumentException("Menu item body cannot be null"); - } - String sanitizedIdentifier = MenuItemUtils.sanitize(body.getIdentifier()); - Case itemCase = findMenuItem(sanitizedIdentifier); + validateMenuItemBody(body); + Case itemCase = findMenuItem(body.getIdentifier()); if (itemCase != null) { log.debug("Ignored creation or update of menu item case [{}] with identifier [{}].", itemCase.getStringId(), - sanitizedIdentifier); + body.getIdentifier()); return itemCase; } else { return createMenuItem(body); } } protected void validateMenuItemBody(MenuItemBody body) { if (body == null) { throw new IllegalArgumentException("Input data cannot be null"); } - if (body.getIdentifier() == null) { + if (body.getIdentifier() == null || body.getIdentifier().isBlank()) { throw new IllegalArgumentException("Identifier cannot be null"); } + body.setIdentifier(MenuItemUtils.sanitize(body.getIdentifier())); if (body.getUri() == null || body.getUri().isBlank()) { throw new IllegalArgumentException("Uri cannot be null"); } else { body.setUri(MenuItemUtils.sanitizeUriSegments(body.getUri(), uriService)); List<String> uriSegments = List.of(body.getUri().split(uriService.getUriSeparator()));Also applies to: 183-195, 542-558
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/netgrif/application/engine/menu/service/MenuItemService.java` around lines 162 - 170, Normalize and validate the identifier once up front so lookup, validation and persistence use the same value: in createOrUpdateMenuItem and createOrIgnoreMenuItem call MenuItemUtils.sanitize(body.getIdentifier()) into a local variable, replace body.getIdentifier() with that sanitized value before calling findMenuItem/update/create, and pass the sanitized identifier into validateMenuItemBody; update validateMenuItemBody to reject blank/empty identifiers after sanitization; and ensure createMenuItem persists the sanitized identifier (i.e., set the sanitized id on the MenuItemBody or Case being saved) so stored node paths match the lookup key.src/main/java/com/netgrif/application/engine/menu/web/MenuController.java (1)
41-47: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winStack trace not preserved in ResponseStatusException.
Lines 43 and 46 throw
ResponseStatusExceptionwithout passing the caught exception as the cause parameter. While the exceptions are logged, Spring's error handling won't include the stack trace in monitoring/error responses, complicating debugging.🔧 Recommended fix to preserve stack traces
} catch (IllegalArgumentException e) { log.warn("Requested with invalid input", e); - throw new ResponseStatusException(HttpStatus.BAD_REQUEST, "Requested with invalid input"); + throw new ResponseStatusException(HttpStatus.BAD_REQUEST, "Requested with invalid input", e); } catch (Exception e) { log.error("Getting menu item data failed", e); - throw new ResponseStatusException(HttpStatus.INTERNAL_SERVER_ERROR, "Getting menu item data failed"); + throw new ResponseStatusException(HttpStatus.INTERNAL_SERVER_ERROR, "Getting menu item data failed", e); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/netgrif/application/engine/menu/web/MenuController.java` around lines 41 - 47, The catch blocks in MenuController catching IllegalArgumentException and Exception currently throw new ResponseStatusException without preserving the original cause; update both catch blocks (the ones handling IllegalArgumentException and the generic Exception) to pass the caught exception as the cause to the ResponseStatusException constructor (e.g., new ResponseStatusException(HttpStatus.BAD_REQUEST, "Requested with invalid input", e)) so the original stack traces are preserved for downstream error handling and monitoring.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@src/main/java/com/netgrif/application/engine/menu/service/MenuItemService.java`:
- Around line 437-440: The method getMenuItemData currently returns the
all_menu_data payload for any decoded case id without permission checks; before
calling dataService.getDataGroups you must authorize that the current principal
may view the menu item/task. Locate getMenuItemData and, after obtaining
menuItemCase and taskId via workflowService.findOne and
MenuItemUtils.findTaskIdInCase (TRANS_ALL_MENU_DATA), invoke the existing
authorization mechanism (e.g., authorizationService/permissionService or
workflowService authorization helper) to verify read/view rights on the task or
case and throw an access denied exception if unauthorized; only then call
dataService.getDataGroups(taskId, locale). Ensure the check uses the same
authority semantics as other endpoints (e.g., MenuController) so behavior is
consistent.
In
`@src/test/groovy/com/netgrif/application/engine/menu/MenuImportExportTest.groovy`:
- Around line 119-187: The test testMenuImportExport dereferences
menuImportExportService.loadFromXML but the menuImportExportService collaborator
was removed; restore a reference to the proper service (declare a field named
menuImportExportService of the correct type used elsewhere in tests, e.g.,
MenuImportExportService or the class that exposes loadFromXML) and inject it
(e.g., with `@Autowired` or the existing test setup method), or update the test to
call the current service that provides loadFromXML/export functionality; ensure
the symbol menuImportExportService and the method loadFromXML remain available
to the test so the final assertions comparing MenuAndFilters work.
---
Duplicate comments:
In
`@src/main/java/com/netgrif/application/engine/menu/service/MenuItemService.java`:
- Around line 162-170: Normalize and validate the identifier once up front so
lookup, validation and persistence use the same value: in createOrUpdateMenuItem
and createOrIgnoreMenuItem call MenuItemUtils.sanitize(body.getIdentifier())
into a local variable, replace body.getIdentifier() with that sanitized value
before calling findMenuItem/update/create, and pass the sanitized identifier
into validateMenuItemBody; update validateMenuItemBody to reject blank/empty
identifiers after sanitization; and ensure createMenuItem persists the sanitized
identifier (i.e., set the sanitized id on the MenuItemBody or Case being saved)
so stored node paths match the lookup key.
In `@src/main/java/com/netgrif/application/engine/menu/web/MenuController.java`:
- Around line 41-47: The catch blocks in MenuController catching
IllegalArgumentException and Exception currently throw new
ResponseStatusException without preserving the original cause; update both catch
blocks (the ones handling IllegalArgumentException and the generic Exception) to
pass the caught exception as the cause to the ResponseStatusException
constructor (e.g., new ResponseStatusException(HttpStatus.BAD_REQUEST,
"Requested with invalid input", e)) so the original stack traces are preserved
for downstream error handling and monitoring.
In `@src/main/resources/petriNets/engine-processes/menu/menu_item.xml`:
- Around line 928-954: After applying the configuration template in the finish
event (where menuItemService.handleConfigurationTemplate(useCase) maps values
into fields like use_tabbed_view/use_tab_icon/tab_name/tab_icon/tab_icon_preview
and view_settings), explicitly invoke the function that enforces tab-field
visibility/behavior (manageBehaviorOfTabFields or equivalent) so the
visibility/editability of tab-related fields (use_tab_icon, tab_icon_preview,
tab_name, tab_icon, view_configuration_form, view_configuration_type,
custom_view_selector) is recomputed; do this immediately after the
template-mapping action in the finish hook(s) (both at the shown block and the
similar block around lines 1425-1444) so non-tabbed templates do not leave tab
fields exposed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5bfb56b8-995b-4274-ae00-85ab82b3a012
📒 Files selected for processing (10)
src/main/groovy/com/netgrif/application/engine/startup/MenuRunner.groovysrc/main/java/com/netgrif/application/engine/menu/domain/ConfigurationTemplateOutcome.javasrc/main/java/com/netgrif/application/engine/menu/domain/FilterBody.javasrc/main/java/com/netgrif/application/engine/menu/domain/configurations/CaseViewBody.javasrc/main/java/com/netgrif/application/engine/menu/service/MenuItemService.javasrc/main/java/com/netgrif/application/engine/menu/web/MenuController.javasrc/main/java/com/netgrif/application/engine/menu/web/responsebodies/MenuItemDataResponse.javasrc/main/resources/petriNets/engine-processes/menu/menu_item.xmlsrc/test/groovy/com/netgrif/application/engine/menu/MenuImportExportTest.groovysrc/test/java/com/netgrif/application/engine/menu/MenuItemServiceTest.java
|
🎉 All dependencies have been resolved ! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixed:
MenuItemServiceMenuItemService(when creating folder items)MenuItemServiceparentIdswhen usingMenuItemService.moveItemNew features / Improvements:
simple-case-view,single-task-view, ...)case_view_configurationinstance and editablesingle_task_view_configurationheadersOther changes:
filterprocess with all the associated codeMenuItemService.updatemethod. Now the update is alias for: 1. remove, 2 createImplements ETASK-23
Dependencies
No new dependencies were introduced
Third party dependencies
No new dependencies were introduced
Blocking Pull requests
Depends on #446
How Has Been This Tested?
Manually and by unit tests
Test Configuration
Checklist:
Summary by CodeRabbit
New Features
Documentation
Refactor