[NAE-2393] Validation property on set data event#319
Merged
Kovy95 merged 4 commits intorelease/6.4.2from Apr 2, 2026
Merged
Conversation
- add a check of property validateData on setData event from frontend - if you want to validate all datafields, put property validateData on validating datafield
Kovy95
requested review from
Retoocs,
SamuelPalaj,
machacjozef,
mazarijuraj and
renczesstefan
WalkthroughAdds optional TaskContentService injection to file-field and file-list components and TaskDataService; introduces conditional task-data validation checks that can abort uploads and field value-change processing when Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as File Component
participant Abstract as Abstract File/List Component
participant TaskService as TaskContentService
participant TaskData as TaskDataService
UI->>Abstract: trigger upload()/field change
Abstract->>Abstract: checkAllowedTypes()
alt validateData === 'true' and TaskService present
Abstract->>TaskService: validateTaskData()
TaskService-->>Abstract: validation result (true/false)
alt validation false
Abstract-->>UI: abort (no upload / set waitingForResponse=false)
else validation true
Abstract->>TaskData: proceed with upload / update processing
TaskData-->>Abstract: response
Abstract-->>UI: update state
end
else no validation required
Abstract->>TaskData: proceed with upload / update processing
TaskData-->>Abstract: response
Abstract-->>UI: update state
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
projects/netgrif-components-core/src/lib/data-fields/file-field/file-default-field/abstract-file-default-field.component.spec.ts (1)
86-94: 🧹 Nitpick | 🔵 TrivialConsider adding test coverage for the new
validateDatabehavior.The current test only verifies component creation. Consider adding tests that verify:
upload()returns early whenvalidateData='true'andvalidateTaskData()returnsfalseupload()proceeds whenvalidateData='true'andvalidateTaskData()returnstrueupload()proceeds normally whenvalidateDatais not setThis would require providing a mock
TaskContentServiceand configuring the data field withcomponent.properties.validateData.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/netgrif-components-core/src/lib/data-fields/file-field/file-default-field/abstract-file-default-field.component.spec.ts` around lines 86 - 94, Add unit tests covering the new validateData behavior: mock or spy TaskContentService/validateTaskData and assert upload() returns early when component.properties.validateData === 'true' and validateTaskData() returns false, assert upload() proceeds when validateTaskData() returns true, and assert upload() proceeds when validateData is not set; locate tests around abstract-file-default-field.component.spec.ts using the component instance, the upload() method, and the component.properties.validateData flag to set up the scenarios and verify expected calls/side effects.projects/netgrif-components-core/src/lib/data-fields/file-list-field/file-list-default-field/abstract-file-list-default-field.component.spec.ts (1)
86-92: 🧹 Nitpick | 🔵 TrivialConsider adding test coverage for the
validateDatabehavior.Same as the file-field spec, consider adding tests that verify the
upload()method's behavior whenvalidateData='true'is configured on the data field.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/netgrif-components-core/src/lib/data-fields/file-list-field/file-list-default-field/abstract-file-list-default-field.component.spec.ts` around lines 86 - 92, Add unit tests for validateData behavior by mirroring the file-field spec: write two tests that set component.dataField.validateData = 'true', spy on the component.validateData() to return false in one test and true in the other, then call component.upload(); assert that when validateData() returns false the internal upload executor (spyOn component.uploadFiles or the method that actually performs the upload) is not called and that when validateData() returns true the upload executor is called; use the existing component variable and method names (validateData, upload, uploadFiles or the component's actual internal upload method) and add the tests to abstract-file-list-default-field.component.spec.ts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@projects/netgrif-components-core/src/lib/data-fields/file-list-field/file-list-default-field/abstract-file-list-default-field.component.ts`:
- Around line 158-162: The current placement calling
this._taskContentService.validateTaskData() can cause duplicate side-effectful
validations when multiple fields trigger nearly simultaneously; modify the call
site or the TaskContentService to deduplicate/debounce validations: either wrap
calls from abstract-file-list-default-field.component (check
dataField.component?.properties?.validateData) with a short debounce/throttle so
repeated invocations within, e.g., 300ms are collapsed, or add a guard in
_taskContentService (e.g., track lastValidationTimestamp or an in-flight
validation flag in validateTaskData()) so subsequent calls return early and do
not re-show snackbars; update references to validateTaskData(),
dataField.component?.properties?.validateData and _taskContentService
accordingly.
In `@projects/netgrif-components-core/src/lib/task/services/task-data.service.ts`:
- Around line 203-208: When validation fails inside the block checking
field.component?.properties?.validateData === 'true' and
this._taskContentService.validateTaskData() returns false, perform the same
cleanup as processUnsuccessfulSetDataRequest(): call revertToPreviousValue() on
the current field, clearWaitingForResponseFlag() for all affected fields (not
just field.waitingForResponse = false), and call updateStateInfo() so UI state
is consistent; alternatively, if preserving field.changed is intentional,
document that decision near this validation branch and ensure other
waitingForResponse flags are still cleared and updateStateInfo() is called to
keep the UI consistent.
---
Outside diff comments:
In
`@projects/netgrif-components-core/src/lib/data-fields/file-field/file-default-field/abstract-file-default-field.component.spec.ts`:
- Around line 86-94: Add unit tests covering the new validateData behavior: mock
or spy TaskContentService/validateTaskData and assert upload() returns early
when component.properties.validateData === 'true' and validateTaskData() returns
false, assert upload() proceeds when validateTaskData() returns true, and assert
upload() proceeds when validateData is not set; locate tests around
abstract-file-default-field.component.spec.ts using the component instance, the
upload() method, and the component.properties.validateData flag to set up the
scenarios and verify expected calls/side effects.
In
`@projects/netgrif-components-core/src/lib/data-fields/file-list-field/file-list-default-field/abstract-file-list-default-field.component.spec.ts`:
- Around line 86-92: Add unit tests for validateData behavior by mirroring the
file-field spec: write two tests that set component.dataField.validateData =
'true', spy on the component.validateData() to return false in one test and true
in the other, then call component.upload(); assert that when validateData()
returns false the internal upload executor (spyOn component.uploadFiles or the
method that actually performs the upload) is not called and that when
validateData() returns true the upload executor is called; use the existing
component variable and method names (validateData, upload, uploadFiles or the
component's actual internal upload method) and add the tests to
abstract-file-list-default-field.component.spec.ts.
🪄 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: 67247f01-3268-47be-a950-dde6bcc7263f
📒 Files selected for processing (7)
projects/netgrif-components-core/src/lib/data-fields/file-field/file-default-field/abstract-file-default-field.component.spec.tsprojects/netgrif-components-core/src/lib/data-fields/file-field/file-default-field/abstract-file-default-field.component.tsprojects/netgrif-components-core/src/lib/data-fields/file-list-field/file-list-default-field/abstract-file-list-default-field.component.spec.tsprojects/netgrif-components-core/src/lib/data-fields/file-list-field/file-list-default-field/abstract-file-list-default-field.component.tsprojects/netgrif-components-core/src/lib/task/services/task-data.service.tsprojects/netgrif-components/src/lib/data-fields/file-field/file-default-field/file-default-field.component.tsprojects/netgrif-components/src/lib/data-fields/file-list-field/file-list-default-field/file-list-default-field.component.ts
...fields/file-list-field/file-list-default-field/abstract-file-list-default-field.component.ts
Outdated
Show resolved
Hide resolved
projects/netgrif-components-core/src/lib/task/services/task-data.service.ts
Outdated
Show resolved
Hide resolved
Retoocs
previously approved these changes
Mar 31, 2026
mazarijuraj
previously approved these changes
Mar 31, 2026
Smotanka
previously approved these changes
Mar 31, 2026
machacjozef
reviewed
Apr 1, 2026
projects/netgrif-components-core/src/lib/task/services/task-data.service.ts
Outdated
Show resolved
Hide resolved
machacjozef
previously approved these changes
Apr 1, 2026
renczesstefan
previously approved these changes
Apr 1, 2026
tuplle
requested changes
Apr 2, 2026
...e/src/lib/data-fields/file-field/file-default-field/abstract-file-default-field.component.ts
Outdated
Show resolved
Hide resolved
projects/netgrif-components-core/src/lib/task/services/task-data.service.ts
Outdated
Show resolved
Hide resolved
- fix according to PR
Kovy95
dismissed stale reviews from renczesstefan, machacjozef, Smotanka, mazarijuraj, and Retoocs
via
ae4c982
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
projects/netgrif-components-core/src/lib/task/services/task-data.service.ts (1)
203-206:⚠️ Potential issue | 🟠 MajorValidation short-circuit still performs partial cleanup.
On Line 204, only the triggering field is reset before returning. If other changed fields are in
waitingForResponse, they can stay stuck in a pending state after validation aborts. Please clear waiting flags for all affected changed fields before this return path.Suggested fix
if (field.component?.properties?.validateData === 'true' && !this._taskContentService.validateTaskData()) { - field.waitingForResponse = false; + this._safeTask.dataGroups.forEach(group => { + group.fields.forEach(f => { + if (f.changed && f.waitingForResponse) { + f.waitingForResponse = false; + } + }); + }); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/netgrif-components-core/src/lib/task/services/task-data.service.ts` around lines 203 - 206, The validation short-circuit returns after resetting only the triggering field's waitingForResponse; update the early-return path (the block checking field.component?.properties?.validateData === 'true' && !this._taskContentService.validateTaskData()) to first clear waitingForResponse on all changed/pending fields before returning. Specifically, iterate over the service's changed fields collection (e.g., whatever holds the pending/edited fields in this task data handling code) and set each field.waitingForResponse = false, then perform the return; keep the existing reset of the triggering field as well.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@projects/netgrif-components-core/src/lib/data-fields/file-field/file-default-field/abstract-file-default-field.component.ts`:
- Around line 216-218: The early-return branch that checks
this.dataField.component?.properties?.validateData and
!this._taskContentService.validateTaskData() should clear the file input and any
internal selected-file state before returning so the user can re-select the same
file; locate the branch in abstract-file-default-field.component (the if using
validateData and validateTaskData()), reset the HTML file input element (e.g.
the file input reference or nativeElement's value) and null out any component
variables tracking the chosen File so a subsequent selection of the same
filename will fire change events and allow retrying the upload.
In
`@projects/netgrif-components-core/src/lib/data-fields/file-list-field/file-list-default-field/abstract-file-list-default-field.component.ts`:
- Around line 158-160: When validation blocks the upload in the early-return
inside the file picker handler (the branch that checks
this.dataField.component?.properties?.validateData === 'true' &&
this._taskContentService && !this._taskContentService.validateTaskData()),
clear/reset the file input picker before returning so the input has no selected
files (e.g., reset the element/value used for the picker) and guard for its
existence; this ensures a subsequent selection of the same files will fire
change. Locate the handler in abstract-file-list-default-field.component.ts and
reset the same file input reference used elsewhere in this component before the
return.
---
Duplicate comments:
In `@projects/netgrif-components-core/src/lib/task/services/task-data.service.ts`:
- Around line 203-206: The validation short-circuit returns after resetting only
the triggering field's waitingForResponse; update the early-return path (the
block checking field.component?.properties?.validateData === 'true' &&
!this._taskContentService.validateTaskData()) to first clear waitingForResponse
on all changed/pending fields before returning. Specifically, iterate over the
service's changed fields collection (e.g., whatever holds the pending/edited
fields in this task data handling code) and set each field.waitingForResponse =
false, then perform the return; keep the existing reset of the triggering field
as well.
🪄 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: 2b0c1d1c-c498-4bda-8350-d4d4b5a27040
📒 Files selected for processing (3)
projects/netgrif-components-core/src/lib/data-fields/file-field/file-default-field/abstract-file-default-field.component.tsprojects/netgrif-components-core/src/lib/data-fields/file-list-field/file-list-default-field/abstract-file-list-default-field.component.tsprojects/netgrif-components-core/src/lib/task/services/task-data.service.ts
...e/src/lib/data-fields/file-field/file-default-field/abstract-file-default-field.component.ts
Show resolved
Hide resolved
...fields/file-list-field/file-list-default-field/abstract-file-list-default-field.component.ts
Show resolved
Hide resolved
tuplle
approved these changes
Apr 2, 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
7 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-2393
Dependencies
none
Third party dependencies
Blocking Pull requests
There are no dependencies on other PR
How Has Been This Tested?
manually
Test Configuration
<Please describe configuration for tests to run if applicable, like program parameters, host OS, VM configuration etc. You can use >
Checklist:
Summary by CodeRabbit
New Features
Tests