[NAE-2390] Action API Improvements#435
Open
SamuelPalaj wants to merge 8 commits into
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 Review limit reached
More reviews will be available in 50 minutes and 39 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy 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 value-based case/task/petri‑net lookup and lifecycle helpers (assign/cancel/finish now return Task/List and add transition-based overloads), adds Elastic search entrypoints, introduces casesToOptions/getTaskIds utilities, adds MapOptionsField#getOption, and includes extensive tests plus a new Petri net resource. ChangesActionDelegate API & Tests
Sequence Diagram(s)sequenceDiagram
participant Caller
participant ActionDelegate
participant IPetriNetService as PetriNetSvc
participant ElasticTaskService as ElasticSvc
participant WorkflowService as WorkflowSvc
Caller->>ActionDelegate: assignTasksByTransitions(transitionIds, case, user, params)
ActionDelegate->>PetriNetSvc: getTaskIds(transitionIds, case)
ActionDelegate->>ElasticSvc: findTasks(taskIds) / findTasksElastic(...)
ElasticSvc-->>ActionDelegate: List<Task>
ActionDelegate->>WorkflowSvc: assignTasks(List<Task>, assignee, params)
WorkflowSvc-->>ActionDelegate: List<Task> (with AssignTaskEventOutcome)
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
+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
Retoocs
requested changes
May 22, 2026
- logs in ActionDelegate changed from error to warn level in newly added methods - ActionDelegateTest readability improved
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy (1)
1299-1343:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReturn-type inconsistency:
findTasks(Field/DataField)returnsnullon ClassCastException.When
findCases(Field/DataField)was fixed to return[]on ClassCastException, the same fix wasn't applied tofindTasks. The null-value branch returns[]but the ClassCastException branch returnsnull, creating inconsistent behavior and NPE risk for callers using collection operations.🐛 Proposed fix (align with findCases pattern)
} catch (ClassCastException e) { log.error("Method cannot be used with field with id [${taskRef.importId}].", e) - return null + return [] }Apply the same change to
findTasks(DataField)at line 1341.🤖 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 1299 - 1343, The two overloads findTasks(Field) and findTasks(DataField) currently catch ClassCastException and return null, causing inconsistent behavior with the null-value branch (which returns an empty list); change the catch blocks in both findTasks(Field) and findTasks(DataField) (the exception handlers around the this.findTasks([taskRef.value].flatten() as List<String>) calls) to return an empty List<Task> ([]) instead of null and keep the error logging as-is so callers always get a list on failure to convert.
🤖 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/test/groovy/com/netgrif/application/engine/action/ActionDelegateTest.groovy`:
- Line 328: The assertion in ActionDelegateTest is incorrect: replace
assertNotNull(options != null) with a proper null-check on the options object
(e.g., assertNotNull(options) or assertTrue(options != null)) so the test
actually fails when options is null; locate the assertion in the test method in
ActionDelegateTest and update the call to assertNotNull to pass the options
reference itself rather than a boolean expression.
---
Outside diff comments:
In
`@src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy`:
- Around line 1299-1343: The two overloads findTasks(Field) and
findTasks(DataField) currently catch ClassCastException and return null, causing
inconsistent behavior with the null-value branch (which returns an empty list);
change the catch blocks in both findTasks(Field) and findTasks(DataField) (the
exception handlers around the this.findTasks([taskRef.value].flatten() as
List<String>) calls) to return an empty List<Task> ([]) instead of null and keep
the error logging as-is so callers always get a list on failure to convert.
🪄 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: 00876271-f882-4de2-bfe6-1ba8242436ce
📒 Files selected for processing (2)
src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovysrc/test/groovy/com/netgrif/application/engine/action/ActionDelegateTest.groovy
- return type consistency fix
Retoocs
requested changes
May 26, 2026
- assertion bug fix
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy (2)
1312-1343:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSame Javadoc inconsistency: states
nullbut returns[]on cast failure.Line 1319 documents the return value as
nullon cast failure, but line 1341 returns[]. Please apply the same Javadoc fix as theFieldoverload above.✏️ Proposed fix
- * <p>If the value cannot be converted to task IDs, this method returns {`@code` null}.</p> + * <p>If the value cannot be converted to task IDs, this method returns an empty list.</p>🤖 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 1312 - 1343, The Javadoc for ActionDelegate.findTasks(DataField) incorrectly states the method returns {`@code` null} on cast failure while the implementation returns an empty list; update the Javadoc (method findTasks(DataField) and its `@return` description) to state that on conversion/cast failure the method returns an empty list (consistent with the Field overload), and ensure the descriptive sentences mention "returns an empty list" instead of "returns {`@code` null}" so docs match the implementation.
1279-1310:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winJavadoc contradicts the implementation: states
nullbut returns[]on cast failure.Line 1286 states "If the value cannot be converted to task IDs, this method returns
null", but the implementation at line 1308 correctly returns[]. This same inconsistency was fixed forfindCases(Field)andfindCases(DataField)in a prior commit. Please update the Javadoc to match the actual behavior.✏️ Proposed fix
- * <p>If the value cannot be converted to task IDs, this method returns {`@code` null}.</p> + * <p>If the value cannot be converted to task IDs, this method returns an empty list.</p>🤖 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 1279 - 1310, The Javadoc for ActionDelegate#findTasks(Field) is incorrect about error behavior: update the comment lines that say "If the value cannot be converted to task IDs, this method returns {`@code` null}." to state that it returns an empty list instead, and adjust the `@return` description to reflect returning an empty list when the field value is null or cannot be cast to task IDs so the doc matches the implementation which returns [] on ClassCastException.
🤖 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/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy`:
- Around line 1312-1343: The Javadoc for ActionDelegate.findTasks(DataField)
incorrectly states the method returns {`@code` null} on cast failure while the
implementation returns an empty list; update the Javadoc (method
findTasks(DataField) and its `@return` description) to state that on
conversion/cast failure the method returns an empty list (consistent with the
Field overload), and ensure the descriptive sentences mention "returns an empty
list" instead of "returns {`@code` null}" so docs match the implementation.
- Around line 1279-1310: The Javadoc for ActionDelegate#findTasks(Field) is
incorrect about error behavior: update the comment lines that say "If the value
cannot be converted to task IDs, this method returns {`@code` null}." to state
that it returns an empty list instead, and adjust the `@return` description to
reflect returning an empty list when the field value is null or cannot be cast
to task IDs so the doc matches the implementation which returns [] on
ClassCastException.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8de8b656-2f28-498a-80fb-f82273446b75
📒 Files selected for processing (2)
src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovysrc/test/groovy/com/netgrif/application/engine/action/ActionDelegateTest.groovy
- javadoc fix
Retoocs
approved these changes
May 26, 2026
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
3 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