[ETASK-23] Dynamic view configuration#447
Merged
Merged
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 MenuItemServiceTest
- set up headers for task lists
- fix uri change of menu item on creation
- add translation - update doc
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (10)
💤 Files with no reviewable changes (1)
WalkthroughThe PR replaces legacy filter-driven menu wiring with menu view templates and configuration forms, adds Mongo-backed menu item lookups and index creation, rewrites menu Petri nets, removes filter and import/export runtime paths, and updates startup, tests, and docs to the new menu flow. ChangesMenu and legacy filter refactor
Sequence Diagram(s)sequenceDiagram
participant Startup as MenuRunner
participant Importer as ImportHelper
participant Net as menu_item.xml
participant Service as MenuItemService
participant Mongo as MongoTemplate
Startup->>Importer: importProcess(view_configuration.xml)
Startup->>Importer: importProcess(menu_item.xml)
Net->>Service: handleConfigurationTemplate(menuItemCase)
Service->>Mongo: ensureIndex(...)
Service->>Mongo: findOne by processIdentifier and dataSet value
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
|
Retoocs
marked this pull request as ready for review
June 2, 2026 14:31
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 ! |
- remove MenuController - switch system role to default role on all data transition
- fix updating URI of menu item
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 (3)
src/main/resources/petriNets/engine-processes/menu/case_view_configuration.xml (1)
962-969:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winThe custom-default-headers toggle uses the visibility condition in the wrong direction in both view configs. In both files, the dependent headers field is explicitly made visible when the toggle is
false, so the form still exposes a field that the toggle just disabled.
src/main/resources/petriNets/engine-processes/menu/case_view_configuration.xml#L962-L969: makecase_default_headersvisible only whenuse_case_default_headersis true, and hide/optional it when false.src/main/resources/petriNets/engine-processes/menu/task_view_configuration.xml#L600-L607: maketask_default_headersvisible only whenuse_task_default_headersis true, and hide/optional it when false.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/resources/petriNets/engine-processes/menu/case_view_configuration.xml` around lines 962 - 969, The visibility conditions for the dependent headers fields are inverted in both configuration files. In src/main/resources/petriNets/engine-processes/menu/case_view_configuration.xml at lines 962-969, the case_default_headers field is currently made visible when use_case_default_headers is false (the !use.value condition), but it should be visible only when use_case_default_headers is true. Swap the visibility conditions so that the make headers,visible statement uses { use.value } instead of { !use.value }, and the make headers,editable statement uses { !use.value } instead of { use.value }. Apply the same fix in src/main/resources/petriNets/engine-processes/menu/task_view_configuration.xml at lines 600-607, swapping the visibility conditions for task_default_headers based on use_task_default_headers, so the field is only visible when the toggle is enabled.src/main/resources/petriNets/engine-processes/menu/menu_item.xml (2)
51-68:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRefresh
advanced_content_formafter changing the linked view.
initializeAdvancedForm()derives the task-list fromview_configuration_id, but it only runs duringinitialize/system_initialize.view_configuration_formstays editable inview_settings, so after switching the associated view,item_settings.advanced_content_formstill points at the old task set.Proposed fix
<event type="finish"> <id>finish_0</id> + <actions phase="post"> + <action> + initializeAdvancedForm() + </action> + </actions> <title/> </event>Also applies to: 1760-1785
🤖 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 51 - 68, The initializeAdvancedForm function only executes during initialization phases, so when view_configuration_id is changed via view_settings, the advanced_content_form field is not refreshed with the updated task list from the newly selected view. Add a trigger to call initializeAdvancedForm whenever view_configuration_id changes in the view_settings context to ensure advanced_content_form stays synchronized with the current linked view configuration.
1431-1468:⚠️ Potential issue | 🔴 Critical
system_initializetransition does not process configuration templates, leaving template-derived view configurations unapplied.When
MenuItemService.createMenuItem()is called with aMenuItemBodycontainingconfigurationTemplateIdentifier, the template ID is stored in the case but thesystem_initializetransition executes without callinghandleConfigurationTemplate(). This means the template-derived view configurations and field mappings are never materialized. Theinitializetransition includes this call (line 937 in menu_item.xml), butsystem_initialize(lines 1431–1468) skips it entirely.If menu items are created programmatically with templates populated, those templates will be inert.
🤖 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 1431 - 1468, The system_initialize transition is missing a call to handleConfigurationTemplate() in its post-phase actions within the finish event, which means configuration templates are never processed when menu items are created programmatically. Add a call to handleConfigurationTemplate() in the post-phase actions section of the system_initialize transition's finish event (similar to how the initialize transition handles it at line 937), ensuring that template-derived view configurations and field mappings are properly materialized when a MenuItemBody with configurationTemplateIdentifier is provided.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@src/main/resources/petriNets/engine-processes/menu/case_view_configuration.xml`:
- Around line 962-969: The visibility conditions for the dependent headers
fields are inverted in both configuration files. In
src/main/resources/petriNets/engine-processes/menu/case_view_configuration.xml
at lines 962-969, the case_default_headers field is currently made visible when
use_case_default_headers is false (the !use.value condition), but it should be
visible only when use_case_default_headers is true. Swap the visibility
conditions so that the make headers,visible statement uses { use.value } instead
of { !use.value }, and the make headers,editable statement uses { !use.value }
instead of { use.value }. Apply the same fix in
src/main/resources/petriNets/engine-processes/menu/task_view_configuration.xml
at lines 600-607, swapping the visibility conditions for task_default_headers
based on use_task_default_headers, so the field is only visible when the toggle
is enabled.
In `@src/main/resources/petriNets/engine-processes/menu/menu_item.xml`:
- Around line 51-68: The initializeAdvancedForm function only executes during
initialization phases, so when view_configuration_id is changed via
view_settings, the advanced_content_form field is not refreshed with the updated
task list from the newly selected view. Add a trigger to call
initializeAdvancedForm whenever view_configuration_id changes in the
view_settings context to ensure advanced_content_form stays synchronized with
the current linked view configuration.
- Around line 1431-1468: The system_initialize transition is missing a call to
handleConfigurationTemplate() in its post-phase actions within the finish event,
which means configuration templates are never processed when menu items are
created programmatically. Add a call to handleConfigurationTemplate() in the
post-phase actions section of the system_initialize transition's finish event
(similar to how the initialize transition handles it at line 937), ensuring that
template-derived view configurations and field mappings are properly
materialized when a MenuItemBody with configurationTemplateIdentifier is
provided.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 70214ad4-a64a-4aa4-bde5-dd59b4617ecb
📒 Files selected for processing (7)
src/main/java/com/netgrif/application/engine/menu/service/MenuItemService.javasrc/main/java/com/netgrif/application/engine/menu/service/interfaces/IMenuItemService.javasrc/main/resources/petriNets/engine-processes/menu/case_view_configuration.xmlsrc/main/resources/petriNets/engine-processes/menu/menu_item.xmlsrc/main/resources/petriNets/engine-processes/menu/single_task_view_configuration.xmlsrc/main/resources/petriNets/engine-processes/menu/tabbed_ticket_view_configuration.xmlsrc/main/resources/petriNets/engine-processes/menu/task_view_configuration.xml
💤 Files with no reviewable changes (2)
- src/main/java/com/netgrif/application/engine/menu/service/interfaces/IMenuItemService.java
- src/main/java/com/netgrif/application/engine/menu/service/MenuItemService.java
# Conflicts: # src/test/groovy/com/netgrif/application/engine/action/ActionDelegateTest.groovy
- fix tests after merge
- update data group for all data transition
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/main/resources/petriNets/engine-processes/menu/task_view_configuration.xml (1)
40-46: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueSame NPE risk pattern as other view configurations.
The
find()calls can return null, leading to NPE when accessing.task.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/resources/petriNets/engine-processes/menu/task_view_configuration.xml` around lines 40 - 46, The initializeAdvancedForm function contains two find() calls that search for task pairs by transition name ("filter_settings" and "header_settings"), but these calls lack null safety checks before accessing the .task property. Add null safety checks to both find() operations using the safe navigation operator or explicit null checks to prevent NullPointerException when a matching task pair is not found. Ensure the value assignment to advancedContentFormField only includes non-null task objects, filtering out any null results from the find() operations.src/main/resources/petriNets/engine-processes/menu/single_task_view_configuration.xml (1)
40-45: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueAdd null-safety for task lookup.
The
find()call can return null if no task matches the transition, which would cause an NPE when accessing.task. Whilefilter_settingsis defined in this net, defensive coding would be prudent.🤖 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/single_task_view_configuration.xml` around lines 40 - 45, The initializeAdvancedForm function uses a find() call on useCase.tasks to locate a task with transition == "filter_settings" and immediately accesses the .task property on the result. If no matching task exists, find() returns null and accessing .task will cause a NullPointerException. Add null-safety handling around the find() call using an elvis operator or explicit null check to safely handle cases where the expected task transition is not found, ensuring the advanced content form field is set to an appropriate default value when the lookup fails.src/main/resources/petriNets/engine-processes/menu/menu_item.xml (1)
51-69:⚠️ Potential issue | 🟠 Major | ⚡ Quick winIncorrect task extraction in
makestatement.At line 63, the expression
viewCase.tasks.findAll { it.transition == "settings" }?.taskis problematic:
findAllreturns a list of matching task pairs, not a single element- Accessing
.taskon a list doesn't make sense and will fail or produce unexpected results- The safe navigation
?.taskwon't help sincefindAllalways returns a list (possibly empty), never nullThe intent appears to be hiding the "settings" task form in the referenced view case. This should use
collectto extract task IDs:Proposed fix
- make ["advanced_content_form"], hidden on (viewCase.tasks.findAll { it.transition == "settings" }?.task as List) when { true } + def settingsTaskIds = viewCase.tasks.findAll { it.transition == "settings" }.collect { it.task } + if (!settingsTaskIds.isEmpty()) { + make ["advanced_content_form"], hidden on settingsTaskIds when { 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/resources/petriNets/engine-processes/menu/menu_item.xml` around lines 51 - 69, The make statement in the initializeAdvancedForm function incorrectly accesses the task property on the list returned by findAll. The expression viewCase.tasks.findAll { it.transition == "settings" }?.task attempts to get .task from a list, which doesn't work as intended. Replace .task with .collect { it.task } to properly extract the task ID from each element in the findAll result, ensuring the hidden condition receives a correctly formatted list of task IDs for the settings transition.src/main/resources/petriNets/engine-processes/menu/case_view_configuration.xml (1)
59-71:⚠️ Potential issue | 🟡 MinorApply safe navigation operator to all
find()calls on lines 62-64 for consistency and robustness.In
initializeAdvancedForm(), lines 62-64 use unsafe.taskaccess onfind()results without null-checking. Line 65 correctly uses the safe navigation operator?.taskfor the optionalopen_case_settingstransition. For consistency and defensive programming, apply the same safe pattern to all three required transitions:header_settings,create_case_btn_settings, andfilter_settings. While these transitions are currently defined in the petrinet, the unsafe pattern leaves the code vulnerable to NPE if the petrinet structure changes or this code is reused elsewhere.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/resources/petriNets/engine-processes/menu/case_view_configuration.xml` around lines 59 - 71, In the initializeAdvancedForm function, the three find() calls for header_settings, create_case_btn_settings, and filter_settings transitions (lines 62-64) use unsafe direct `.task` access without null-checking, while the open_case_settings transition on line 65 correctly uses the safe navigation operator `?.task`. Apply the safe navigation operator `?.task` to all three of the earlier find() calls to maintain consistency and prevent potential null pointer exceptions if the petrinet structure changes. This aligns all four transition lookups with the same defensive programming pattern.
🤖 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/petrinet/domain/dataset/logic/action/ActionDelegate.groovy`:
- Around line 1551-1554: The getTaskIds method lacks a null/empty guard for the
transitionIds parameter, causing a NullPointerException when called with null
values. Add a guard clause at the start of the getTaskIds method that checks if
transitionIds is null or empty, and returns an empty list in those cases before
proceeding with the findAll and collect operations on the transitionIds list.
- Around line 1539-1542: The getTaskId method calls .find() on the refs list and
immediately dereferences the result with .stringId without checking if a
matching task reference exists, which will throw a NullPointerException if no
task is found for the given transitionId. Add a null guard after the .find()
call to check if a matching task reference was found before accessing its
stringId property. If no task is found, return a controlled value (such as null)
or throw a meaningful exception instead of allowing the NullPointerException to
occur.
In
`@src/test/groovy/com/netgrif/application/engine/action/ActionDelegateTest.groovy`:
- Around line 257-259: The test code directly indexes the result of
petriNetService.getByIdentifier("menu_item") with [0] without verifying the list
is non-empty, which can result in an IndexOutOfBoundsException with poor
diagnostics. Add an assertion or verification before accessing the list element
to ensure the getByIdentifier call returns a non-empty list, making test
failures more diagnosable when the menu_item net cannot be found.
- Around line 453-455: The importTestPetriNet() helper method opens a
FileInputStream that is never closed, causing a resource leak. Refactor this
method to properly close the stream after the petriNetService.importPetriNet()
call completes by wrapping the stream in a try-with-resources statement or using
a try-finally block with an explicit close() call to ensure the stream is
released even if an exception occurs.
---
Outside diff comments:
In
`@src/main/resources/petriNets/engine-processes/menu/case_view_configuration.xml`:
- Around line 59-71: In the initializeAdvancedForm function, the three find()
calls for header_settings, create_case_btn_settings, and filter_settings
transitions (lines 62-64) use unsafe direct `.task` access without
null-checking, while the open_case_settings transition on line 65 correctly uses
the safe navigation operator `?.task`. Apply the safe navigation operator
`?.task` to all three of the earlier find() calls to maintain consistency and
prevent potential null pointer exceptions if the petrinet structure changes.
This aligns all four transition lookups with the same defensive programming
pattern.
In `@src/main/resources/petriNets/engine-processes/menu/menu_item.xml`:
- Around line 51-69: The make statement in the initializeAdvancedForm function
incorrectly accesses the task property on the list returned by findAll. The
expression viewCase.tasks.findAll { it.transition == "settings" }?.task attempts
to get .task from a list, which doesn't work as intended. Replace .task with
.collect { it.task } to properly extract the task ID from each element in the
findAll result, ensuring the hidden condition receives a correctly formatted
list of task IDs for the settings transition.
In
`@src/main/resources/petriNets/engine-processes/menu/single_task_view_configuration.xml`:
- Around line 40-45: The initializeAdvancedForm function uses a find() call on
useCase.tasks to locate a task with transition == "filter_settings" and
immediately accesses the .task property on the result. If no matching task
exists, find() returns null and accessing .task will cause a
NullPointerException. Add null-safety handling around the find() call using an
elvis operator or explicit null check to safely handle cases where the expected
task transition is not found, ensuring the advanced content form field is set to
an appropriate default value when the lookup fails.
In
`@src/main/resources/petriNets/engine-processes/menu/task_view_configuration.xml`:
- Around line 40-46: The initializeAdvancedForm function contains two find()
calls that search for task pairs by transition name ("filter_settings" and
"header_settings"), but these calls lack null safety checks before accessing the
.task property. Add null safety checks to both find() operations using the safe
navigation operator or explicit null checks to prevent NullPointerException when
a matching task pair is not found. Ensure the value assignment to
advancedContentFormField only includes non-null task objects, filtering out any
null results from the find() operations.
🪄 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: 0fc4b3f6-f29e-4288-9722-22e3742cfeac
📒 Files selected for processing (10)
src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovysrc/main/java/com/netgrif/application/engine/workflow/domain/Case.javasrc/main/java/com/netgrif/application/engine/workflow/service/WorkflowService.javasrc/main/resources/petriNets/engine-processes/menu/case_view_configuration.xmlsrc/main/resources/petriNets/engine-processes/menu/menu_item.xmlsrc/main/resources/petriNets/engine-processes/menu/single_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/test/groovy/com/netgrif/application/engine/action/ActionDelegateTest.groovysrc/test/java/com/netgrif/application/engine/menu/MenuItemServiceTest.java
💤 Files with no reviewable changes (1)
- src/test/java/com/netgrif/application/engine/menu/MenuItemServiceTest.java
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/main/resources/petriNets/engine-processes/menu/task_view_configuration.xml (1)
40-46: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueSame NPE risk pattern as other view configurations.
The
find()calls can return null, leading to NPE when accessing.task.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/resources/petriNets/engine-processes/menu/task_view_configuration.xml` around lines 40 - 46, The initializeAdvancedForm function contains two find() calls that search for task pairs by transition name ("filter_settings" and "header_settings"), but these calls lack null safety checks before accessing the .task property. Add null safety checks to both find() operations using the safe navigation operator or explicit null checks to prevent NullPointerException when a matching task pair is not found. Ensure the value assignment to advancedContentFormField only includes non-null task objects, filtering out any null results from the find() operations.src/main/resources/petriNets/engine-processes/menu/single_task_view_configuration.xml (1)
40-45: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueAdd null-safety for task lookup.
The
find()call can return null if no task matches the transition, which would cause an NPE when accessing.task. Whilefilter_settingsis defined in this net, defensive coding would be prudent.🤖 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/single_task_view_configuration.xml` around lines 40 - 45, The initializeAdvancedForm function uses a find() call on useCase.tasks to locate a task with transition == "filter_settings" and immediately accesses the .task property on the result. If no matching task exists, find() returns null and accessing .task will cause a NullPointerException. Add null-safety handling around the find() call using an elvis operator or explicit null check to safely handle cases where the expected task transition is not found, ensuring the advanced content form field is set to an appropriate default value when the lookup fails.src/main/resources/petriNets/engine-processes/menu/menu_item.xml (1)
51-69:⚠️ Potential issue | 🟠 Major | ⚡ Quick winIncorrect task extraction in
makestatement.At line 63, the expression
viewCase.tasks.findAll { it.transition == "settings" }?.taskis problematic:
findAllreturns a list of matching task pairs, not a single element- Accessing
.taskon a list doesn't make sense and will fail or produce unexpected results- The safe navigation
?.taskwon't help sincefindAllalways returns a list (possibly empty), never nullThe intent appears to be hiding the "settings" task form in the referenced view case. This should use
collectto extract task IDs:Proposed fix
- make ["advanced_content_form"], hidden on (viewCase.tasks.findAll { it.transition == "settings" }?.task as List) when { true } + def settingsTaskIds = viewCase.tasks.findAll { it.transition == "settings" }.collect { it.task } + if (!settingsTaskIds.isEmpty()) { + make ["advanced_content_form"], hidden on settingsTaskIds when { 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/resources/petriNets/engine-processes/menu/menu_item.xml` around lines 51 - 69, The make statement in the initializeAdvancedForm function incorrectly accesses the task property on the list returned by findAll. The expression viewCase.tasks.findAll { it.transition == "settings" }?.task attempts to get .task from a list, which doesn't work as intended. Replace .task with .collect { it.task } to properly extract the task ID from each element in the findAll result, ensuring the hidden condition receives a correctly formatted list of task IDs for the settings transition.src/main/resources/petriNets/engine-processes/menu/case_view_configuration.xml (1)
59-71:⚠️ Potential issue | 🟡 MinorApply safe navigation operator to all
find()calls on lines 62-64 for consistency and robustness.In
initializeAdvancedForm(), lines 62-64 use unsafe.taskaccess onfind()results without null-checking. Line 65 correctly uses the safe navigation operator?.taskfor the optionalopen_case_settingstransition. For consistency and defensive programming, apply the same safe pattern to all three required transitions:header_settings,create_case_btn_settings, andfilter_settings. While these transitions are currently defined in the petrinet, the unsafe pattern leaves the code vulnerable to NPE if the petrinet structure changes or this code is reused elsewhere.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/resources/petriNets/engine-processes/menu/case_view_configuration.xml` around lines 59 - 71, In the initializeAdvancedForm function, the three find() calls for header_settings, create_case_btn_settings, and filter_settings transitions (lines 62-64) use unsafe direct `.task` access without null-checking, while the open_case_settings transition on line 65 correctly uses the safe navigation operator `?.task`. Apply the safe navigation operator `?.task` to all three of the earlier find() calls to maintain consistency and prevent potential null pointer exceptions if the petrinet structure changes. This aligns all four transition lookups with the same defensive programming pattern.
🤖 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/petrinet/domain/dataset/logic/action/ActionDelegate.groovy`:
- Around line 1551-1554: The getTaskIds method lacks a null/empty guard for the
transitionIds parameter, causing a NullPointerException when called with null
values. Add a guard clause at the start of the getTaskIds method that checks if
transitionIds is null or empty, and returns an empty list in those cases before
proceeding with the findAll and collect operations on the transitionIds list.
- Around line 1539-1542: The getTaskId method calls .find() on the refs list and
immediately dereferences the result with .stringId without checking if a
matching task reference exists, which will throw a NullPointerException if no
task is found for the given transitionId. Add a null guard after the .find()
call to check if a matching task reference was found before accessing its
stringId property. If no task is found, return a controlled value (such as null)
or throw a meaningful exception instead of allowing the NullPointerException to
occur.
In
`@src/test/groovy/com/netgrif/application/engine/action/ActionDelegateTest.groovy`:
- Around line 257-259: The test code directly indexes the result of
petriNetService.getByIdentifier("menu_item") with [0] without verifying the list
is non-empty, which can result in an IndexOutOfBoundsException with poor
diagnostics. Add an assertion or verification before accessing the list element
to ensure the getByIdentifier call returns a non-empty list, making test
failures more diagnosable when the menu_item net cannot be found.
- Around line 453-455: The importTestPetriNet() helper method opens a
FileInputStream that is never closed, causing a resource leak. Refactor this
method to properly close the stream after the petriNetService.importPetriNet()
call completes by wrapping the stream in a try-with-resources statement or using
a try-finally block with an explicit close() call to ensure the stream is
released even if an exception occurs.
---
Outside diff comments:
In
`@src/main/resources/petriNets/engine-processes/menu/case_view_configuration.xml`:
- Around line 59-71: In the initializeAdvancedForm function, the three find()
calls for header_settings, create_case_btn_settings, and filter_settings
transitions (lines 62-64) use unsafe direct `.task` access without
null-checking, while the open_case_settings transition on line 65 correctly uses
the safe navigation operator `?.task`. Apply the safe navigation operator
`?.task` to all three of the earlier find() calls to maintain consistency and
prevent potential null pointer exceptions if the petrinet structure changes.
This aligns all four transition lookups with the same defensive programming
pattern.
In `@src/main/resources/petriNets/engine-processes/menu/menu_item.xml`:
- Around line 51-69: The make statement in the initializeAdvancedForm function
incorrectly accesses the task property on the list returned by findAll. The
expression viewCase.tasks.findAll { it.transition == "settings" }?.task attempts
to get .task from a list, which doesn't work as intended. Replace .task with
.collect { it.task } to properly extract the task ID from each element in the
findAll result, ensuring the hidden condition receives a correctly formatted
list of task IDs for the settings transition.
In
`@src/main/resources/petriNets/engine-processes/menu/single_task_view_configuration.xml`:
- Around line 40-45: The initializeAdvancedForm function uses a find() call on
useCase.tasks to locate a task with transition == "filter_settings" and
immediately accesses the .task property on the result. If no matching task
exists, find() returns null and accessing .task will cause a
NullPointerException. Add null-safety handling around the find() call using an
elvis operator or explicit null check to safely handle cases where the expected
task transition is not found, ensuring the advanced content form field is set to
an appropriate default value when the lookup fails.
In
`@src/main/resources/petriNets/engine-processes/menu/task_view_configuration.xml`:
- Around line 40-46: The initializeAdvancedForm function contains two find()
calls that search for task pairs by transition name ("filter_settings" and
"header_settings"), but these calls lack null safety checks before accessing the
.task property. Add null safety checks to both find() operations using the safe
navigation operator or explicit null checks to prevent NullPointerException when
a matching task pair is not found. Ensure the value assignment to
advancedContentFormField only includes non-null task objects, filtering out any
null results from the find() operations.
🪄 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: 0fc4b3f6-f29e-4288-9722-22e3742cfeac
📒 Files selected for processing (10)
src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovysrc/main/java/com/netgrif/application/engine/workflow/domain/Case.javasrc/main/java/com/netgrif/application/engine/workflow/service/WorkflowService.javasrc/main/resources/petriNets/engine-processes/menu/case_view_configuration.xmlsrc/main/resources/petriNets/engine-processes/menu/menu_item.xmlsrc/main/resources/petriNets/engine-processes/menu/single_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/test/groovy/com/netgrif/application/engine/action/ActionDelegateTest.groovysrc/test/java/com/netgrif/application/engine/menu/MenuItemServiceTest.java
💤 Files with no reviewable changes (1)
- src/test/java/com/netgrif/application/engine/menu/MenuItemServiceTest.java
🛑 Comments failed to post (4)
src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy (2)
1539-1542:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard missing task reference resolution in
getTaskId.If no task exists for the requested transition in the given case, this dereferences
nulland throws an NPE. Return a controlled error instead.💡 Proposed fix
String getTaskId(String transitionId, Case aCase = useCase) { List<TaskReference> refs = taskService.findAllByCase(aCase.stringId, null) - return refs.find { it.transitionId == transitionId }.stringId + TaskReference ref = refs.find { it.transitionId == transitionId } + if (ref == null) { + throw new IllegalArgumentException("Could not find task for transition [${transitionId}] in case [${aCase.stringId}]") + } + return ref.stringId }📝 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.String getTaskId(String transitionId, Case aCase = useCase) { List<TaskReference> refs = taskService.findAllByCase(aCase.stringId, null) TaskReference ref = refs.find { it.transitionId == transitionId } if (ref == null) { throw new IllegalArgumentException("Could not find task for transition [${transitionId}] in case [${aCase.stringId}]") } return ref.stringId }🤖 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/logic/action/ActionDelegate.groovy` around lines 1539 - 1542, The getTaskId method calls .find() on the refs list and immediately dereferences the result with .stringId without checking if a matching task reference exists, which will throw a NullPointerException if no task is found for the given transitionId. Add a null guard after the .find() call to check if a matching task reference was found before accessing its stringId property. If no task is found, return a controlled value (such as null) or throw a meaningful exception instead of allowing the NullPointerException to occur.
1551-1554:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd null/empty guard for
transitionIdsin bulk task lookup.
getTaskIdscurrently throws whentransitionIdsisnull, and this propagates into all new bulk task event methods.💡 Proposed fix
List<String> getTaskIds(List<String> transitionIds, Case aCase = useCase) { + if (transitionIds == null || transitionIds.empty) { + return [] + } List<TaskReference> refs = taskService.findAllByCase(aCase.stringId, null) return refs.findAll { transitionIds.contains(it.transitionId) }.collect { it.stringId} }📝 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.List<String> getTaskIds(List<String> transitionIds, Case aCase = useCase) { if (transitionIds == null || transitionIds.empty) { return [] } List<TaskReference> refs = taskService.findAllByCase(aCase.stringId, null) return refs.findAll { transitionIds.contains(it.transitionId) }.collect { it.stringId} }🤖 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/logic/action/ActionDelegate.groovy` around lines 1551 - 1554, The getTaskIds method lacks a null/empty guard for the transitionIds parameter, causing a NullPointerException when called with null values. Add a guard clause at the start of the getTaskIds method that checks if transitionIds is null or empty, and returns an empty list in those cases before proceeding with the findAll and collect operations on the transitionIds list.src/test/groovy/com/netgrif/application/engine/action/ActionDelegateTest.groovy (2)
257-259:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid unchecked list indexing when resolving
menu_itemnet in test.Indexing
[0]without asserting presence can throwIndexOutOfBoundsExceptionand make failures less diagnosable.💡 Proposed fix
- PetriNet menuItemNet = petriNetService.getByIdentifier("menu_item")[0] + List<PetriNet> menuItemNets = petriNetService.getByIdentifier("menu_item") + assertNotNull(menuItemNets) + assertFalse(menuItemNets.isEmpty()) + PetriNet menuItemNet = menuItemNets[0]📝 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.List<PetriNet> menuItemNets = petriNetService.getByIdentifier("menu_item") assertNotNull(menuItemNets) assertFalse(menuItemNets.isEmpty()) PetriNet menuItemNet = menuItemNets[0] String mongoId2 = menuItemNet.stringId ObjectId objectId3 = menuItemNet.objectId🤖 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/action/ActionDelegateTest.groovy` around lines 257 - 259, The test code directly indexes the result of petriNetService.getByIdentifier("menu_item") with [0] without verifying the list is non-empty, which can result in an IndexOutOfBoundsException with poor diagnostics. Add an assertion or verification before accessing the list element to ensure the getByIdentifier call returns a non-empty list, making test failures more diagnosable when the menu_item net cannot be found.
453-455:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClose the test Petri net input stream.
The helper opens a
FileInputStreamwithout closing it.💡 Proposed fix
private PetriNet importTestPetriNet() { - return petriNetService.importPetriNet(new FileInputStream("src/test/resources/petriNets/NAE-2390_action_api_improvements.xml"), VersionType.MAJOR, userService.getLoggedOrSystem().transformToLoggedUser()).getNet() + return new File("src/test/resources/petriNets/NAE-2390_action_api_improvements.xml").withInputStream { input -> + petriNetService.importPetriNet(input, VersionType.MAJOR, userService.getLoggedOrSystem().transformToLoggedUser()).getNet() + } }📝 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.private PetriNet importTestPetriNet() { return new File("src/test/resources/petriNets/NAE-2390_action_api_improvements.xml").withInputStream { input -> petriNetService.importPetriNet(input, VersionType.MAJOR, userService.getLoggedOrSystem().transformToLoggedUser()).getNet() } }🤖 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/action/ActionDelegateTest.groovy` around lines 453 - 455, The importTestPetriNet() helper method opens a FileInputStream that is never closed, causing a resource leak. Refactor this method to properly close the stream after the petriNetService.importPetriNet() call completes by wrapping the stream in a try-with-resources statement or using a try-finally block with an explicit close() call to ensure the stream is released even if an exception occurs.
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
2 participants
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
Release Notes
Bug Fixes
Removed Features
Improvements