[NAE-2393] Validation property on set data event#319
Open
Kovy95 wants to merge 2 commits intorelease/6.4.2from
Open
[NAE-2393] Validation property on set data event#319Kovy95 wants to merge 2 commits intorelease/6.4.2from
Kovy95 wants to merge 2 commits intorelease/6.4.2from
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
WalkthroughThis change introduces optional Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Possibly related PRs
Suggested labels
🚥 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
Comment on lines
+158
to
+162
| if (this.dataField.component?.properties?.validateData === 'true' && this._taskContentService) { | ||
| if (!this._taskContentService.validateTaskData()) { | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
Validation gate placement is correct, but beware of duplicate validateTaskData() calls.
The validation check is correctly placed after all file-specific validations and before the upload begins, so no state cleanup is needed on failure here.
However, validateTaskData() has side effects (shows error snackbars, marks all fields as touched). If multiple fields with validateData='true' trigger their actions in close succession (e.g., a text field change and a file upload), users may see duplicate error snackbars. Consider whether a debounce or deduplication mechanism would improve UX.
🤖 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.ts`
around lines 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.
Comment on lines
+203
to
+208
| if (field.component?.properties?.validateData === 'true') { | ||
| if (!this._taskContentService.validateTaskData()) { | ||
| field.waitingForResponse = false; | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
Incomplete state cleanup on validation failure may leave UI inconsistent.
When validation fails, only field.waitingForResponse = false is set before returning. Compare this to processUnsuccessfulSetDataRequest() which performs more thorough cleanup: revertToPreviousValue(), clearWaitingForResponseFlag() on all affected fields, and updateStateInfo().
The current implementation leaves:
field.changed = true, so the field remains marked as changed- Other fields with
waitingForResponse = trueare not cleared - No user feedback (snackbar is shown by
validateTaskData(), but the field state may appear inconsistent)
Consider whether additional cleanup is needed, or document that the field's changed state is intentionally preserved for retry.
🤖 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 - 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.
|
Retoocs
approved these changes
Mar 31, 2026
mazarijuraj
approved these changes
Mar 31, 2026
Smotanka
approved these changes
Mar 31, 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
4 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