[NAE-2231] Unable to change behavior of taskRef on finish event witho… #307
Conversation
…ut error message - fix reloading a non-existing task
Retoocs
requested review from
Kovy95,
machacjozef,
mazarijuraj,
renczesstefan and
tuplle
WalkthroughAdds optional executable-state flags to event outcome and task interfaces. Updates finish-task flow to pass this flag into task state updates. TaskContentService stores the flag and gates TASK_REF-triggered reloads based on it. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant C as Client UI
participant F as FinishTaskService
participant B as Backend
participant T as TaskContentService
U->>C: Finish task action
C->>F: performFinishRequest(taskId)
F->>B: POST /tasks/{id}/finish
B-->>F: outcomeResource { outcome, success }
alt success
F->>T: updateStateData(outcome, outcome.isTaskStillExecutable)
T->>T: this._task.isStillExecutable = flag
else error
F-->>C: Emit error
end
sequenceDiagram
participant UI as UI Field Update
participant T as TaskContentService
UI->>T: updateField(field)
alt field.type == TASK_REF and (task.isStillExecutable === true or undefined)
T->>T: Trigger task data reload
else
T->>T: Skip reload
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
projects/netgrif-components-core/src/lib/event/model/event-outcomes/task-outcomes/finish-task-event-outcome.ts(1 hunks)projects/netgrif-components-core/src/lib/resources/interface/task.ts(1 hunks)projects/netgrif-components-core/src/lib/task-content/services/task-content.service.ts(2 hunks)projects/netgrif-components-core/src/lib/task/services/finish-task.service.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
projects/netgrif-components-core/src/lib/task/services/finish-task.service.ts (1)
projects/netgrif-components-core/src/lib/event/model/event-outcomes/task-outcomes/finish-task-event-outcome.ts (1)
FinishTaskEventOutcome(4-6)
⏰ 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). (4)
- GitHub Check: Matrix Test (18)
- GitHub Check: Matrix Test (14)
- GitHub Check: Matrix Test (16)
- GitHub Check: Test with SonarCloud
🔇 Additional comments (3)
projects/netgrif-components-core/src/lib/event/model/event-outcomes/task-outcomes/finish-task-event-outcome.ts (1)
5-5: LGTM!The property addition aligns with the PR objective to track task executability through event outcomes.
projects/netgrif-components-core/src/lib/task-content/services/task-content.service.ts (1)
209-216: LGTM! Default value ensures backward compatibility.The
isStillExecutableparameter defaults totrue, which means existing code paths that don't explicitly pass this flag will assume tasks remain executable. This is appropriate for backward compatibility.projects/netgrif-components-core/src/lib/task/services/finish-task.service.ts (1)
136-137: LGTM! Correctly propagates executable state.The changes properly extract the
isTaskStillExecutableflag from the finish response and pass it toupdateStateData, completing the flow to prevent reloading finished tasks.
|
|
||
| isStillExecutable?: boolean; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
Verify naming consistency across interfaces.
The property is named isStillExecutable here, but isTaskStillExecutable in FinishTaskEventOutcome. While the difference might be intentional (Task context vs Event context), consider whether consistent naming would improve clarity.
Run the following script to check for any other references to these properties:
🏁 Script executed:
#!/bin/bash
# Description: Check usage of the executable state properties across the codebase
echo "=== Checking for isStillExecutable references ==="
rg -n "isStillExecutable" --type=ts
echo -e "\n=== Checking for isTaskStillExecutable references ==="
rg -n "isTaskStillExecutable" --type=tsLength of output: 1153
Align naming of executable flag across interfaces.
Task interface uses isStillExecutable but FinishTaskEventOutcome defines isTaskStillExecutable. Consider unifying to a single property name (e.g. isStillExecutable) for clarity.
🤖 Prompt for AI Agents
In projects/netgrif-components-core/src/lib/resources/interface/task.ts around
lines 73-74, the Task interface declares the flag as isStillExecutable while
related FinishTaskEventOutcome uses isTaskStillExecutable; rename the latter to
isStillExecutable so both interfaces use the exact same property name, then
update all references/usages, type imports, serializers/deserializers, and tests
to the new name to ensure consistency and no unresolved type or runtime errors.
|
|
||
| protected updateField(chFields: ChangedFields, field: DataField<any>, frontendActions: Change, referenced: boolean = false): void { | ||
| if (this._fieldConverterService.resolveType(field) === FieldTypeResource.TASK_REF) { | ||
| if (this._fieldConverterService.resolveType(field) === FieldTypeResource.TASK_REF && (!!this._task?.isStillExecutable || this._task?.isStillExecutable === undefined)) { |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Simplify the conditional logic.
The double negation !! is redundant in this boolean context. The condition can be simplified for better readability.
Apply this diff to simplify the condition:
- if (this._fieldConverterService.resolveType(field) === FieldTypeResource.TASK_REF && (!!this._task?.isStillExecutable || this._task?.isStillExecutable === undefined)) {
+ if (this._fieldConverterService.resolveType(field) === FieldTypeResource.TASK_REF && this._task?.isStillExecutable !== false) {This alternative reads more clearly: "reload task data unless explicitly marked as not executable."
📝 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 (this._fieldConverterService.resolveType(field) === FieldTypeResource.TASK_REF && (!!this._task?.isStillExecutable || this._task?.isStillExecutable === undefined)) { | |
| if (this._fieldConverterService.resolveType(field) === FieldTypeResource.TASK_REF | |
| && this._task?.isStillExecutable !== false) { |
🤖 Prompt for AI Agents
In
projects/netgrif-components-core/src/lib/task-content/services/task-content.service.ts
around line 239, simplify the conditional by removing the redundant double
negation; change the right-hand check to test that the task is not explicitly
false (e.g., this._task?.isStillExecutable !== false) so the whole condition
becomes: ensure the field type is TASK_REF and the task is not explicitly marked
as not executable, which reads as "reload task data unless explicitly marked as
not executable."
|
Description
The problem was, that if you finish the task, which updates any taskRef, that is referenced in the task you are finishing, frontend tries to do reload. I have updated the implementation, that backend sends information, that the task which is being finished, does not exist and the frontend will not reload the task itself.
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
Test Configuration
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes