[NAE-2390] Action API Improvements#435
Open
SamuelPalaj wants to merge 4 commits intorelease/6.5.0from
Open
Conversation
- implemented new action API method to ActionDelegate - added getOption method to MapOptionsField - tests for new functionality added to ActionDelegateTest - test xml NAE-2390_action_api_improvements.xml added
SamuelPalaj
added
improvement
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 52 minutes and 0 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughExpands ActionDelegate with numerous case/task/petri‑net helper APIs, refactors task lifecycle methods to return Task/List (adds transition-based variants), adds MapOptionsField#getOption, and introduces comprehensive tests plus a new Petri net resource for those tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant ActionDelegate
participant PetriNetService as PetriNetSvc
participant TaskService
participant WorkflowService as WorkflowSvc
Caller->>ActionDelegate: assignTasksByTransitions(transitionIds, case, user, params)
ActionDelegate->>PetriNetSvc: resolve transition -> taskIds (getTaskIds)
ActionDelegate->>TaskService: findTasks(taskIds)
TaskService-->>ActionDelegate: List<Task>
ActionDelegate->>WorkflowSvc: assignTasks(List<Task>, assignee, params)
WorkflowSvc-->>ActionDelegate: updated Tasks
ActionDelegate-->>Caller: List<Task> (assigned)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
coderabbitai
Bot
added
the
breaking change
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy`:
- Around line 1017-1094: Summary: Javadoc for findCase(Field) and
findCase(DataField) incorrectly states they return lists; it should state they
return a single Case or null. Fix the Javadoc in both methods (findCase(Field)
and findCase(DataField)) by replacing the sentences that say "If the field value
is {`@code` null}, this method returns an empty list." and the `@return` that
mentions "list of matching cases" with text that accurately states the methods
return the first matching Case or {`@code` null} when the field value is {`@code`
null}, empty, or cannot be converted to a case ID; ensure other `@see` tags remain
unchanged and keep the field type list as-is.
- Around line 1165-1169: The helper methods like assignTasksByTransitions call
getTaskIds(transitionIds, aCase) and taskService.findAllById(taskIds) then
forward to assignTasks/cancelTasks/finishTasks without signaling when requested
transitionIds produced no tasks; add a debug (or warn) log in
assignTasksByTransitions (and the analogous methods around lines for
cancelTasksByTransitions and finishTasksByTransitions) that compares the
incoming transitionIds size and content to the resolved taskIds (from
getTaskIds) and logs a clear message including the requested transitionIds,
resolved task count/ids and any missing transitionIds so callers can detect
typos or unresolved transitions; use existing logger and reference the functions
getTaskIds, taskService.findAllById, assignTasks, cancelTasks, finishTasks to
locate where to insert the log.
- Around line 943-989: The ClassCastException handlers in findCases(Field),
findCases(DataField) (and the analogous findTasks(Field)/findTasks(DataField))
violate the declared List<Case>/List<Task> contract by returning null; change
each catch(ClassCastException e) block to log the error as before but return an
empty list ([]) instead of null so callers can safely chain collection
operations and the behavior matches the Javadoc null-case branch.
- Around line 1432-1448: The DataField overload findTask(DataField) directly
calls taskService.findOne(castValue[0]) which bypasses the logic in
findTask(String); change the method to delegate to the String overload by
calling this.findTask(castValue[0]) (after the same null/empty and
ClassCastException checks) so all task lookup, logging, validation and future
changes in findTask(String) are consistently applied; keep the existing error
handling in findTask(DataField) but replace the direct taskService.findOne call
with this.findTask(...)
- Around line 1538-1540: The getTaskId method can NPE when no TaskReference
matches; update getTaskId to guard the find result (e.g., use safe navigation or
check for null) before accessing .stringId and return a null/optional/empty
value consistently with getTaskIds behavior; locate the getTaskId method
(adjacent to getTaskIds) and change the expression refs.find { it.transitionId
== transitionId }.stringId to a null-safe form so it won't throw when nothing is
found.
- Around line 1261-1265: The multi-task finishTasks method currently drops the
params argument when calling taskService.finishTasks(tasks, finisher); update
the call in ActionDelegate.finishTasks to forward params to the service (use the
three-argument overload, e.g., taskService.finishTasks(tasks, finisher,
params)), keep collecting FinishTaskEventOutcome into this.outcomes and
returning outcomes.collect { it.task } unchanged, and ensure callers like
finishTasksByTransitions that pass params will now have them honored.
In
`@src/test/groovy/com/netgrif/application/engine/action/ActionDelegateTest.groovy`:
- Around line 345-378: Add assertions to ActionDelegateTest.testTaskEventActions
that verify params passed into finishTasksByTransitions/finishTasks (and
optionally assignTasksByTransitions/cancelTasks) are forwarded into the task
event; e.g., invoke actionDelegate.finishTasksByTransitions(transitionIds,
testCase, [myParam:'v']) or finishTasks with a params map, then either assert
the resulting Task(s) contain that param in a data/metadata field or spy/mock
taskService to verify finishTasks(...) was called with the same params object;
update assertions for assignTasksByTransitions and cancelTasks similarly to
prevent regression, referencing the methods finishTasksByTransitions,
finishTasks, assignTasksByTransitions, cancelTasks and the
ActionDelegate/taskService interactions in the test.
- Line 352: Fix the typo in the commented line in ActionDelegateTest.groovy:
change the comment string // testing "byTransition" variants should be
enough, as they call methods, that tak List<Task> instead of List<String> to use
correct wording — e.g. // testing "byTransition" variants should be enough, as
they call methods that take List<Task> instead of List<String> — updating the
phrase "tak" → "take" and removing the unnecessary comma after "methods".
- Around line 325-332: Replace the hard-coded literals inside the closures
passed to actionDelegate.casesToOptions with the declared constants
keyTransformationTestString and valueTransformationTestString so the
transformations remain consistent if the constants change; specifically update
the two closures in the call to casesToOptions (the value-mapping closure and
the key-mapping closure) to use valueTransformationTestString.concat(it.title)
and keyTransformationTestString.concat(it.stringId) respectively, and ensure
subsequent assertions that build expected keys/values still reference those
constants.
- Around line 269-285: The testPetriNetActions test incorrectly builds the
ObjectId searchTargets using the same objectId twice, so update the
searchTargets passed to actionDelegate.findPetriNetsByObjectIds to use both
distinct IDs (objectId and objectId3) instead of [objectId, objectId]; also
ensure the prior variables (objectId3 and netIdentifier2) created from filterNet
are actually used in the assertions (e.g., when verifying searchResults.collect
{ it.objectId } containsAll searchTargets) so the test exercises two distinct
PetriNet IDs and not a duplicate.
- Around line 389-454: Replace the "assert fieldId && <expr>" short-circuit
assertions in assertTaskSearchResults and assertCaseSearchResults with direct
assertions that include a descriptive message containing the fieldId; e.g.,
change checks like "assert fieldId && task != null" and "assert fieldId &&
task.stringId == firstTaskId" (and analogous checks for tasks list,
searchedCase, searchedCases, dataField lookups, sizes, and stringId comparisons)
to forms like "assert task != null : \"field [${fieldId}] findTask returned
null\"" so failures report which field failed and the real condition (repeat for
findTasks/list emptiness/size and case equivalents in both methods).
- Around line 380-387: The test uses assertTrue(expectedMessage ==
e.getMessage()) which yields poor diagnostics; update the assertion in
assertCaseDeletion to use assertEquals(expectedMessage, e.getMessage()) (keeping
the existing assertThrows and variables) so failures print both expected and
actual strings and help debug message mismatches in the assertCaseDeletion
method.
In `@src/test/resources/petriNets/NAE-2390_action_api_improvements.xml`:
- Around line 4-5: Replace the placeholder XML metadata values: change the
<initials> element from "NEW" to a descriptive identifier (e.g., "NAE-2390") and
update the <title> element from "New Model" to a clear test title like "Action
API Improvements Test" so test output identifies the model; modify the
<initials> and <title> elements in the XML header accordingly.
🪄 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: bbb7a788-0a7e-4532-a51a-1f819fd1aeb8
📒 Files selected for processing (4)
src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/MapOptionsField.groovysrc/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovysrc/test/groovy/com/netgrif/application/engine/action/ActionDelegateTest.groovysrc/test/resources/petriNets/NAE-2390_action_api_improvements.xml
Comment on lines
+943
to
+989
| List<Case> findCases(Field caseRef) { | ||
| if(caseRef.value == null) { | ||
| log.error("Value of field with id [${caseRef.importId}] is null, returning empty list.") | ||
| return [] | ||
| } | ||
| try { | ||
| return this.findCases([caseRef.value].flatten() as List<String>) | ||
| } catch (ClassCastException e) { | ||
| log.error("Method cannot be used with field with id [${caseRef.importId}].", e) | ||
| return null | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Finds cases referenced by a dataField in its value. | ||
| * | ||
| * Use this overload when working on a case not from the current action context. For working with fields from the current | ||
| * action context see other overloads of this action. | ||
| * | ||
| * <p>If the field value is {@code null}, this method returns an empty list.</p> | ||
| * <p>If the value cannot be converted to case IDs, this method returns {@code null}.</p> | ||
| * | ||
| * @param caseRef field whose value contains case IDs, may be of types | ||
| * {@link com.netgrif.application.engine.petrinet.domain.dataset.FieldType#CASE_REF}, | ||
| * {@link com.netgrif.application.engine.petrinet.domain.dataset.FieldType#MULTICHOICE}, | ||
| * {@link com.netgrif.application.engine.petrinet.domain.dataset.FieldType#MULTICHOICE_MAP}, | ||
| * {@link com.netgrif.application.engine.petrinet.domain.dataset.FieldType#ENUMERATION_MAP}, | ||
| * {@link com.netgrif.application.engine.petrinet.domain.dataset.FieldType#STRING_COLLECTION}, | ||
| * {@link com.netgrif.application.engine.petrinet.domain.dataset.FieldType#TEXT}, | ||
| * @return list of matching cases, or an empty list when the field value is {@code null} | ||
| * @see ActionDelegate#findCases(Field) | ||
| * @see ActionDelegate#findCases(List) | ||
| * @see ActionDelegate#findCases(Closure) | ||
| * @see ActionDelegate#findCases(Closure, Pageable) | ||
| */ | ||
| List<Case> findCases(DataField caseRef) { | ||
| if(caseRef.value == null) { | ||
| log.error("Value of field is null, returning empty list.") | ||
| return [] | ||
| } | ||
| try { | ||
| return this.findCases([caseRef.value].flatten() as List<String>) | ||
| } catch (ClassCastException e) { | ||
| log.error("Method cannot be used with field.", e) | ||
| return null | ||
| } | ||
| } |
There was a problem hiding this comment.
Return-type contract violation: findCases(Field/DataField) returns null on cast failure.
The method is declared List<Case> and the Javadoc states "returns an empty list when the field value is null". On a ClassCastException, however, it returns null. Callers chaining .size(), .each { }, .collect { } etc. on the result will NPE. Either:
- Return
[]on cast failure (and keep the error log), aligning with the null-value branch and the documented contract, or - Re-throw a typed exception so the caller can react explicitly.
The same pattern exists in findTasks(Field/DataField) (lines 1299-1343) — please fix consistently.
♻️ Proposed fix (option 1 — return empty list)
} catch (ClassCastException e) {
log.error("Method cannot be used with field with id [${caseRef.importId}].", e)
- return null
+ return []
}📝 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.
Suggested change
| List<Case> findCases(Field caseRef) { | |
| if(caseRef.value == null) { | |
| log.error("Value of field with id [${caseRef.importId}] is null, returning empty list.") | |
| return [] | |
| } | |
| try { | |
| return this.findCases([caseRef.value].flatten() as List<String>) | |
| } catch (ClassCastException e) { | |
| log.error("Method cannot be used with field with id [${caseRef.importId}].", e) | |
| return null | |
| } | |
| } | |
| /** | |
| * Finds cases referenced by a dataField in its value. | |
| * | |
| * Use this overload when working on a case not from the current action context. For working with fields from the current | |
| * action context see other overloads of this action. | |
| * | |
| * <p>If the field value is {@code null}, this method returns an empty list.</p> | |
| * <p>If the value cannot be converted to case IDs, this method returns {@code null}.</p> | |
| * | |
| * @param caseRef field whose value contains case IDs, may be of types | |
| * {@link com.netgrif.application.engine.petrinet.domain.dataset.FieldType#CASE_REF}, | |
| * {@link com.netgrif.application.engine.petrinet.domain.dataset.FieldType#MULTICHOICE}, | |
| * {@link com.netgrif.application.engine.petrinet.domain.dataset.FieldType#MULTICHOICE_MAP}, | |
| * {@link com.netgrif.application.engine.petrinet.domain.dataset.FieldType#ENUMERATION_MAP}, | |
| * {@link com.netgrif.application.engine.petrinet.domain.dataset.FieldType#STRING_COLLECTION}, | |
| * {@link com.netgrif.application.engine.petrinet.domain.dataset.FieldType#TEXT}, | |
| * @return list of matching cases, or an empty list when the field value is {@code null} | |
| * @see ActionDelegate#findCases(Field) | |
| * @see ActionDelegate#findCases(List) | |
| * @see ActionDelegate#findCases(Closure) | |
| * @see ActionDelegate#findCases(Closure, Pageable) | |
| */ | |
| List<Case> findCases(DataField caseRef) { | |
| if(caseRef.value == null) { | |
| log.error("Value of field is null, returning empty list.") | |
| return [] | |
| } | |
| try { | |
| return this.findCases([caseRef.value].flatten() as List<String>) | |
| } catch (ClassCastException e) { | |
| log.error("Method cannot be used with field.", e) | |
| return null | |
| } | |
| } | |
| List<Case> findCases(Field caseRef) { | |
| if(caseRef.value == null) { | |
| log.error("Value of field with id [${caseRef.importId}] is null, returning empty list.") | |
| return [] | |
| } | |
| try { | |
| return this.findCases([caseRef.value].flatten() as List<String>) | |
| } catch (ClassCastException e) { | |
| log.error("Method cannot be used with field with id [${caseRef.importId}].", e) | |
| return [] | |
| } | |
| } | |
| /** | |
| * Finds cases referenced by a dataField in its value. | |
| * | |
| * Use this overload when working on a case not from the current action context. For working with fields from the current | |
| * action context see other overloads of this action. | |
| * | |
| * <p>If the field value is {`@code` null}, this method returns an empty list.</p> | |
| * <p>If the value cannot be converted to case IDs, this method returns {`@code` null}.</p> | |
| * | |
| * `@param` caseRef field whose value contains case IDs, may be of types | |
| * {`@link` com.netgrif.application.engine.petrinet.domain.dataset.FieldType#CASE_REF}, | |
| * {`@link` com.netgrif.application.engine.petrinet.domain.dataset.FieldType#MULTICHOICE}, | |
| * {`@link` com.netgrif.application.engine.petrinet.domain.dataset.FieldType#MULTICHOICE_MAP}, | |
| * {`@link` com.netgrif.application.engine.petrinet.domain.dataset.FieldType#ENUMERATION_MAP}, | |
| * {`@link` com.netgrif.application.engine.petrinet.domain.dataset.FieldType#STRING_COLLECTION}, | |
| * {`@link` com.netgrif.application.engine.petrinet.domain.dataset.FieldType#TEXT}, | |
| * `@return` list of matching cases, or an empty list when the field value is {`@code` null} | |
| * `@see` ActionDelegate#findCases(Field) | |
| * `@see` ActionDelegate#findCases(List) | |
| * `@see` ActionDelegate#findCases(Closure) | |
| * `@see` ActionDelegate#findCases(Closure, Pageable) | |
| */ | |
| List<Case> findCases(DataField caseRef) { | |
| if(caseRef.value == null) { | |
| log.error("Value of field is null, returning empty list.") | |
| return [] | |
| } | |
| try { | |
| return this.findCases([caseRef.value].flatten() as List<String>) | |
| } catch (ClassCastException e) { | |
| log.error("Method cannot be used with field.", e) | |
| return [] | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy`
around lines 943 - 989, The ClassCastException handlers in findCases(Field),
findCases(DataField) (and the analogous findTasks(Field)/findTasks(DataField))
violate the declared List<Case>/List<Task> contract by returning null; change
each catch(ClassCastException e) block to log the error as before but return an
empty list ([]) instead of null so callers can safely chain collection
operations and the behavior matches the Javadoc null-case branch.
Comment on lines
+1165
to
+1169
| List<Task> assignTasksByTransitions(List<String> transitionIds, Case aCase = useCase, IUser user = userService.loggedOrSystem, Map<String, String> params = [:]) { | ||
| List<String> taskIds = getTaskIds(transitionIds, aCase) | ||
| List<Task> tasks = taskService.findAllById(taskIds) | ||
| return assignTasks(tasks, user, params) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Silent empty-input behavior in the three *TasksByTransitions helpers.
If transitionIds is empty (or none match), getTaskIds returns [] and taskService.findAllById([]) is called; the subsequent assignTasks/cancelTasks/finishTasks will be invoked on an empty task list. That's probably fine functionally, but it's worth logging at debug/warn when requested transitions can't be resolved — today the caller has no signal that their transition IDs were typos. Consider comparing requested vs resolved task count and logging the delta.
Also applies to: 1204-1208, 1247-1251
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy`
around lines 1165 - 1169, The helper methods like assignTasksByTransitions call
getTaskIds(transitionIds, aCase) and taskService.findAllById(taskIds) then
forward to assignTasks/cancelTasks/finishTasks without signaling when requested
transitionIds produced no tasks; add a debug (or warn) log in
assignTasksByTransitions (and the analogous methods around lines for
cancelTasksByTransitions and finishTasksByTransitions) that compares the
incoming transitionIds size and content to the resolved taskIds (from
getTaskIds) and logs a clear message including the requested transitionIds,
resolved task count/ids and any missing transitionIds so callers can detect
typos or unresolved transitions; use existing logger and reference the functions
getTaskIds, taskService.findAllById, assignTasks, cancelTasks, finishTasks to
locate where to insert the log.
Comment on lines
+345
to
+378
| @Test | ||
| void testTaskEventActions() { | ||
| importTestPetriNet() | ||
| Case testCase = actionDelegate.createCase(ACTION_API_NET_IDENTIFIER) | ||
| List<String> transitionIds = ["t1", "t2"] | ||
| IUser user = userService.getLoggedOrSystem() | ||
|
|
||
| // testing "byTransition" variants should be enough, as they call methods, that tak List<Task> instead of List<String> | ||
| List<Task> tasks = actionDelegate.assignTasksByTransitions(transitionIds, testCase) | ||
| assert tasks != null | ||
| assert tasks.size() == transitionIds.size() | ||
| assert tasks.stream().allMatch { it.user.email == user.email } | ||
| assert tasks.stream().allMatch { it.userId == user.stringId } | ||
| assert tasks.stream().allMatch { it.startDate != null } | ||
| tasks = null | ||
|
|
||
| tasks = actionDelegate.cancelTasksByTransitions(transitionIds, testCase) | ||
| assert tasks != null | ||
| assert tasks.size() == transitionIds.size() | ||
| assert tasks.stream().allMatch { it.userId == null } | ||
| assert tasks.stream().allMatch { it.startDate == null } | ||
| tasks = null | ||
|
|
||
| actionDelegate.assignTasksByTransitions(transitionIds, testCase) | ||
| tasks = actionDelegate.finishTasksByTransitions(transitionIds, testCase) | ||
| assert tasks != null | ||
| assert tasks.size() == transitionIds.size() | ||
| assert tasks.stream().allMatch { it.finishedBy == user.stringId } | ||
| assert tasks.stream().allMatch { it.finishDate != null } | ||
| assert tasks.stream().allMatch { it.userId == null } | ||
| tasks = actionDelegate.findTasks(tasks.collect { it.stringId }) | ||
| assert tasks.size() == 1 | ||
| assert tasks[0].transitionId == "t2" | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Missing coverage for finishTasks params pass-through.
Given the critical params-dropped bug flagged in ActionDelegate.finishTasks, please add an assertion that params provided to finishTasksByTransitions(...) / finishTasks(...) actually reach the task. The same would be useful for assignTasks/cancelTasks to prevent regression. A minimal assertion on a map-typed param (e.g. something that mutates a data field via the task event) or a spy on taskService would suffice.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/test/groovy/com/netgrif/application/engine/action/ActionDelegateTest.groovy`
around lines 345 - 378, Add assertions to
ActionDelegateTest.testTaskEventActions that verify params passed into
finishTasksByTransitions/finishTasks (and optionally
assignTasksByTransitions/cancelTasks) are forwarded into the task event; e.g.,
invoke actionDelegate.finishTasksByTransitions(transitionIds, testCase,
[myParam:'v']) or finishTasks with a params map, then either assert the
resulting Task(s) contain that param in a data/metadata field or spy/mock
taskService to verify finishTasks(...) was called with the same params object;
update assertions for assignTasksByTransitions and cancelTasks similarly to
prevent regression, referencing the methods finishTasksByTransitions,
finishTasks, assignTasksByTransitions, cancelTasks and the
ActionDelegate/taskService interactions in the test.
Comment on lines
+4
to
+5
| <initials>NEW</initials> | ||
| <title>New Model</title> |
There was a problem hiding this comment.
Cosmetic: replace placeholder <initials>NEW</initials> / <title>New Model</title>.
These are IDE defaults. For a test resource dedicated to NAE-2390 it's worth giving it a descriptive title (e.g. Action API Improvements Test) so failing test output identifies the model clearly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/test/resources/petriNets/NAE-2390_action_api_improvements.xml` around
lines 4 - 5, Replace the placeholder XML metadata values: change the <initials>
element from "NEW" to a descriptive identifier (e.g., "NAE-2390") and update the
<title> element from "New Model" to a clear test title like "Action API
Improvements Test" so test output identifies the model; modify the <initials>
and <title> elements in the XML header accordingly.
mazarijuraj
previously approved these changes
Apr 23, 2026
| * @see ActionDelegate#findCases(Closure, Pageable) | ||
| */ | ||
| List<Case> findCases(List<String> mongoIds) { | ||
| if(mongoIds == null) { |
- implemented new action API method to ActionDelegate - added getOption method to MapOptionsField - tests for new functionality added to ActionDelegateTest - test xml NAE-2390_action_api_improvements.xml added
- return value fixed
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy`:
- Around line 1345-1359: Correct the Javadoc typo in the
ActionDelegate.findTasks(List<String> mongoIds) method: change the phrase "or an
empty list when the input is {`@code` null} org {`@code` empty}" to "or an empty
list when the input is {`@code` null} or {`@code` empty}" so the documentation reads
correctly; update only the comment text associated with the findTasks method.
- Around line 921-989: Update the Javadoc for both findCases(Field) and
findCases(DataField) so the contract matches the implementation: state that if
the field value is null OR if the value cannot be converted to case IDs
(ClassCastException), the method returns an empty list (not null); reference the
methods findCases(Field) and findCases(DataField) and ensure the paragraphs that
currently say "this method returns {`@code` null}" are replaced with "this method
returns an empty list" to align with the catch blocks that return [].
🪄 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: 29541286-ded5-4a8b-aa84-83ea26a2c686
📒 Files selected for processing (3)
src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovysrc/test/groovy/com/netgrif/application/engine/action/ActionDelegateTest.groovysrc/test/resources/petriNets/NAE-2390_action_api_improvements.xml
- javadoc fixes
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
Implements NAE-2390
How Has Been This Tested?
Tests included in ActionDelegateTest.groovy file
Test Configuration
Checklist:
Summary by CodeRabbit
New Features
Refactor
Tests