[NAE-2231] Unable to change behavior of taskRef on finish event witho… #371
Conversation
…ut error message - implement FinishTaskEventOutcome flag, which signals if the finished task still exists
Retoocs
requested review from
Kovy95,
machacjozef,
mazarijuraj,
renczesstefan and
tuplle
WalkthroughAdds a boolean flag isTaskStillExecutable to FinishTaskEventOutcome and exposes it in LocalisedFinishTaskEventOutcome. The flag is computed during construction via a helper that checks if the task still exists in the case’s tasks. Localised response now includes this value via a getter. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
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]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/main/java/com/netgrif/application/engine/workflow/domain/eventoutcomes/taskoutcomes/FinishTaskEventOutcome.java(1 hunks)src/main/java/com/netgrif/application/engine/workflow/web/responsebodies/eventoutcomes/LocalisedFinishTaskEventOutcome.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
🔇 Additional comments (2)
src/main/java/com/netgrif/application/engine/workflow/domain/eventoutcomes/taskoutcomes/FinishTaskEventOutcome.java (2)
13-16: LGTM!The field declaration is clear and well-documented.
24-24: LGTM!The field initialization correctly delegates to the helper method.
| protected boolean isTaskStillExecutable(Case useCase, Task task) { | ||
| return useCase.getTasks().stream() | ||
| .anyMatch(taskPair -> taskPair.getTask().equals(task.getStringId())); | ||
| } |
There was a problem hiding this comment.
Guard against potential NPE and consider method naming.
The method has two concerns:
- NPE risk: If
taskPair.getTask()returnsnull, theequalscall will throw aNullPointerException. - Naming clarity: The method name
isTaskStillExecutablesuggests checking executability, but the logic only verifies existence in the case's tasks collection. Consider a name likeisTaskStillPresentordoesTaskStillExistfor clarity.
Apply this diff to add null safety:
protected boolean isTaskStillExecutable(Case useCase, Task task) {
return useCase.getTasks().stream()
- .anyMatch(taskPair -> taskPair.getTask().equals(task.getStringId()));
+ .anyMatch(taskPair -> task.getStringId().equals(taskPair.getTask()));
}Or with explicit null check:
protected boolean isTaskStillExecutable(Case useCase, Task task) {
return useCase.getTasks().stream()
- .anyMatch(taskPair -> taskPair.getTask().equals(task.getStringId()));
+ .anyMatch(taskPair -> {
+ String taskId = taskPair.getTask();
+ return taskId != null && taskId.equals(task.getStringId());
+ });
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| protected boolean isTaskStillExecutable(Case useCase, Task task) { | |
| return useCase.getTasks().stream() | |
| .anyMatch(taskPair -> taskPair.getTask().equals(task.getStringId())); | |
| } | |
| protected boolean isTaskStillExecutable(Case useCase, Task task) { | |
| return useCase.getTasks().stream() | |
| .anyMatch(taskPair -> { | |
| String taskId = taskPair.getTask(); | |
| return taskId != null && taskId.equals(task.getStringId()); | |
| }); | |
| } |
🤖 Prompt for AI Agents
In
src/main/java/com/netgrif/application/engine/workflow/domain/eventoutcomes/taskoutcomes/FinishTaskEventOutcome.java
around lines 32 to 35, the method risks an NPE by calling
taskPair.getTask().equals(... ) and the name misleads about intent; change the
name to something like doesTaskStillExist or isTaskStillPresent, make the
existence check null-safe (e.g., use Objects.equals(task.getStringId(),
taskPair.getTask()) or invert the equals to compare
task.getStringId().equals(taskPair.getTask()) only after null-check of
task.getStringId()), and update all call sites and imports accordingly so the
method reflects its purpose and no longer throws on null task IDs.
| @Getter | ||
| protected Boolean isTaskStillExecutable; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider field naming for cleaner getter.
The @Getter annotation generates getIsTaskStillExecutable(), which deviates from the standard JavaBean convention of isXxx() for boolean properties. While not incorrect, renaming the field to taskStillExecutable would result in a more conventional getter getTaskStillExecutable().
🤖 Prompt for AI Agents
In
src/main/java/com/netgrif/application/engine/workflow/web/responsebodies/eventoutcomes/LocalisedFinishTaskEventOutcome.java
around lines 11-12, the protected Boolean field named isTaskStillExecutable
yields a non-idiomatic getter getIsTaskStillExecutable(); rename the field to
taskStillExecutable to produce a conventional getter getTaskStillExecutable(),
update all references/usages (including tests, serialization names or builders)
to the new field name, and ensure any JSON/property mappings or reflection
usages are adjusted (or add @JsonProperty if name must remain externally) so
behavior stays identical.
| if (outcome != null) { | ||
| this.isTaskStillExecutable = outcome.isTaskStillExecutable() ; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remove extra whitespace.
Minor formatting issue: there's an extra space before the semicolon on line 17.
Apply this diff:
if (outcome != null) {
- this.isTaskStillExecutable = outcome.isTaskStillExecutable() ;
+ this.isTaskStillExecutable = outcome.isTaskStillExecutable();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (outcome != null) { | |
| this.isTaskStillExecutable = outcome.isTaskStillExecutable() ; | |
| } | |
| if (outcome != null) { | |
| this.isTaskStillExecutable = outcome.isTaskStillExecutable(); | |
| } |
🤖 Prompt for AI Agents
In
src/main/java/com/netgrif/application/engine/workflow/web/responsebodies/eventoutcomes/LocalisedFinishTaskEventOutcome.java
around lines 16 to 18, there's an extra space before the semicolon on line 17;
remove the stray space so the assignment reads without a space before the
semicolon (i.e., ensure there is no space between the method call and the
semicolon), and save formatting so no other whitespace changes are introduced.
Description
I have added a flag to the FinishTaskEventOutcome. The boolean flag is true if the task, that was finished, is still executable. This information needs frontend to prohibit reloading on non-existing tasks
Fixes NAE-2231
Dependencies
No new dependencies were introduced
Third party dependencies
No new dependencies were introduced
Blocking Pull requests
There are no dependencies on other PR
How Has Been This Tested?
Manually. I tested multiple situations for the updated code conditions
Test Configuration
Checklist:
Summary by CodeRabbit