Skip to content

NAS-142225 / 27.0.0-BETA.1 / Fix Force checkbox not enabling Save on the NTP Server form - #13944

Merged
AlexKarpov98 merged 8 commits into
masterfrom
NAS-142225
Aug 26, 2026
Merged

NAS-142225 / 27.0.0-BETA.1 / Fix Force checkbox not enabling Save on the NTP Server form#13944
AlexKarpov98 merged 8 commits into
masterfrom
NAS-142225

Conversation

@AlexKarpov98

@AlexKarpov98 AlexKarpov98 commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Changes:

Adding an unreachable NTP server via the documented Force escape hatch was a dead
end: the backend's "Server could not be reached. Check 'Force' to continue regardless."
was pinned onto the address control with setErrors(), which — unlike a validator
result — never re-evaluates. Angular drops such a pin when that control changes, but
an error the user is meant to answer from a different field has no way out, so the
form stayed invalid and Save stayed disabled. The only escape was retyping the Address.

Two fixes:

  1. form-error-handler.service.ts — the pin now retires itself. Each backend
    verdict describes one submitted payload, and the next edit anywhere in the form moves
    the payload on. So when the service pins an error it also subscribes one-shot to the
    control's root.valueChanges and, on the next edit, re-runs that control's
    validators. Re-validating rather than blanking is the point: the pinned set is
    replaced by the control's real validation state, so a field that is genuinely
    empty-but-required goes back to saying required instead of falling silently valid,
    and live client-side validators keep their say. If the verdict still stands, the next
    save pins it again.

    Putting this in the service rather than in <ix-form> means it covers all 67 callers
    of handleValidationErrors, including forms <ix-form> never wraps, and needs no
    per-field declaration — nothing to remember, nothing to forget. Unsubscribing before
    the clearing lets updateValueAndValidity() emit normally, which is what refreshes
    <ix-form>'s status signal and so the host's Save button.

    Behaviour change worth flagging: a pinned backend message now also disappears if
    the user edits an unrelated field. That is deliberate — the verdict describes a
    payload that no longer exists — and matches what the directory-services form already
    does by hand in clearFormControlErrors(). Validator-driven errors are untouched:
    image-validator and file-validator return ixManualValidateError from real
    validator functions, so they re-evaluate normally and never carry the
    manualValidateError flag this keys on.

  2. form-error-handler.service.ts — a failed element lookup no longer escalates to
    an error modal.
    The service pins the message on the control (rendered inline by
    tn-form-field) and then looks up the DOM element purely to scroll/focus it. Both
    lookup strategies missed for tn-* forms — IxFormService only knows ix-* controls,
    which register via RegisteredControlDirective, and [formControlName] is a
    property binding that leaves no attribute behind — so every <ix-form-renderer> form
    showed the message twice, inline and in a modal. The lookup now also matches the
    data-control-name the renderer already stamps on each control, so scroll and focus
    work there too, and focus lands on the native control inside the tn-* host rather
    than the host itself. The genuine fallback (no control found, nothing renderable
    inline) still opens the modal.

The three manualValidateError keys also get one home in
manual-validate-error.constants.ts, instead of being re-spelled at each reader.

Testing:

Automated — ntp-servers.form-config.spec.ts drives the reported path end to end
through <ix-form-renderer> with the real FormErrorHandlerService, so the error is
genuinely pinned by a rejected system.ntpserver.create:

  • backend rejects → message pinned on Address, Save blocked
  • tick Force → Save freed, Address value untouched (fails without the fix)
  • Max Poll 99 + Force → Save still blocked, live validators keep their say
  • tick Burst instead → also frees Save, documenting that no field is special-cased

Plus form-error-handler.service.spec.ts covers the retiring directly: re-validation vs
blanket-clearing, a sibling's live validator surviving, a nested control retired by an
edit in a sibling group, and re-pinning when the next save is rejected again — as well
as the message staying inline-only when the element is missing.

Manual — System → Advanced → NTP Servers → Add, Address 192.0.2.1 (RFC 5737,
never routable), Save. Expect the inline error and no error modal. Tick
Force → error clears and Save enables; saving adds the server. Untick Force and
save again → the error returns, confirming the verdict is retired, not suppressed.

Downstream

Affects Reasoning
Documentation No — the Force option already works as documented; this makes the UI match.
Testing Yes — NTP add/edit, plus any form surfacing backend field errors: those no longer show a duplicate error modal alongside the inline message, and a pinned message now clears on the next edit anywhere in the form rather than only on the flagged field.

…form

Replaces the form-wide clearing from the first commit, which dropped pinned
backend errors on any edit across all ix-form consumers. The clearing now
lives on the NTP form's own `force` field and touches nothing else.

Claude-Session: https://claude.ai/code/session_01PP236cd5LPtgSVz1aiL8pm
@AlexKarpov98 AlexKarpov98 self-assigned this Aug 19, 2026
@bugclerk bugclerk changed the title NAS-142225: Fix Force checkbox not enabling Save on the NTP Server form NAS-142225 / 27.0.0-BETA.1 / Fix Force checkbox not enabling Save on the NTP Server form Aug 19, 2026
@bugclerk

Copy link
Copy Markdown
Contributor

@codecov

codecov Bot commented Aug 19, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 91.66667% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.51%. Comparing base (1ba1031) to head (06cdea7).
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...ms/ix-forms/services/form-error-handler.service.ts 93.75% 2 Missing ⚠️
...-forms/components/ix-errors/ix-errors.component.ts 80.00% 1 Missing ⚠️
...-and-where/replication-what-and-where.component.ts 50.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master   #13944   +/-   ##
=======================================
  Coverage   88.51%   88.51%           
=======================================
  Files        1889     1890    +1     
  Lines       71294    71322   +28     
  Branches     9204     9209    +5     
=======================================
+ Hits        63103    63129   +26     
- Misses       8191     8193    +2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Comment thread src/app/modules/forms/ix-forms/services/form-error-handler.service.ts Outdated
@github-actions

github-actions Bot commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Four findings, all LOW. Nothing here blocks. ✅

The move from clearsServerErrorsFor to a default in FormErrorHandlerService is the right call — the pin is set there, so retiring it there is where the invariant actually lives, and it reaches the <ix-form>-less hosts a per-field declaration never would. Re-running validators instead of blanking setErrors(null) is the detail that makes it safe: a genuinely-empty required field goes back to saying required rather than falling silently valid, and the spec pins that down. The multi-field refinement is neat too — inferring "which control moved" from which pin Angular already dropped, rather than from valueChanges (which can't say), is a legitimate read of the framework's ordering, and form-error-handler.service.spec.ts covers both halves.

I traced the emission ordering for the claim in the doc comment on retirePinnedErrorsOnNextEdit, since it's load-bearing for the whole fix: updateValueAndValidity() runs re-entrantly inside root's own valueChanges.emit(), and the outer frame's statusChanges.emit(this.status) reads status after that returns, so <ix-form>'s signal gets the fresh VALID rather than a stale INVALID. The comment is accurate. 👍

The findings, in full inline:

  • LOWntp-servers.form-config.spec.ts:111: the one test spanning the renderer and the real service doesn't assert the absence of the duplicate modal, so nothing anywhere ties findControlElement's [data-control-name="…"] to the attribute the renderer template stamps. One expect(…showErrorModal).not.toHaveBeenCalled() closes it.
  • LOWform-error-handler.service.ts:236: root.valueChanges can't tell a user edit from a programmatic patchValue, so a form that patches itself from an async source after a rejected save retires the pin before the message is read.
  • LOWix-errors.component.ts:265-267: the new constants are used for one of the three keys and literals for the other two, here and in directory-service-validation.service.ts:121-123 / additional-details-section.component.ts:165 — so the "one home" the constants file documents isn't established yet.
  • LOWmanual-validate-error.constants.ts:8: the doc lists <ix-form-renderer>'s error clearing among the readers; the renderer never references these keys (the readers are the tn-* resolver, ix-errors, and the directory-services service). No inline thread, since a resolved one already sits on that comment block.

Focusing the native control inside the tn-* host is a real a11y improvement over focus()-ing a host with no tabindex — worth noting it also changes ix-* behaviour, where the same call was previously a no-op.

Restores the error-modal escalation for a control that is nowhere in the DOM,
replaces the side-effecting NTP validator with a declarative
`clearsServerErrorsFor` hook on FormDefinition, gives the three
manualValidateError keys one home, focuses the native control inside a tn-*
host, and asserts the pinned message in the NTP spec.

Claude-Session: https://claude.ai/code/session_015DNgeeCxxzRm7YJwr2uVvv
Comment thread src/app/modules/forms/ix-forms/manual-validate-error.constants.ts
@AlexKarpov98
AlexKarpov98 marked this pull request as ready for review August 19, 2026 11:11
@AlexKarpov98
AlexKarpov98 requested a review from a team as a code owner August 19, 2026 11:11
@AlexKarpov98
AlexKarpov98 requested review from william-gr and removed request for a team August 19, 2026 11:11
Replaces `clearsServerErrorsFor` with the same semantics applied by default.
`FormErrorHandlerService` now owns retiring the pin it sets: a one-shot
subscription on the control's root drops the verdict on the next edit anywhere
in the form, by re-running the control's validators so its real validation
state comes back.

This covers every caller of `handleValidationErrors`, including legacy
SidePanelForm hosts that `<ix-form>` can never reach, and needs no per-field
declaration — nothing to remember, nothing to forget. Unsubscribing before the
clearing lets `updateValueAndValidity()` emit normally, so `<ix-form>`'s status
signal refreshes on its own and the manual `formStatus.set()` from the first
attempt is not needed.

Claude-Session: https://claude.ai/code/session_01PP236cd5LPtgSVz1aiL8pm
Comment thread src/app/modules/forms/ix-forms/services/form-error-handler.service.ts Outdated
…eIn host

The NTP spec provided a null SlideInRef so `<ix-form-renderer>` would fall back
to its panel-only path. Master removed the legacy host (#13955) along with the
token, so the import no longer resolves and the suite fails to run. The
renderer no longer needs the provider at all.

Claude-Session: https://claude.ai/code/session_01PP236cd5LPtgSVz1aiL8pm
Comment thread src/app/modules/forms/ix-forms/services/form-error-handler.service.ts Outdated
Comment thread src/app/modules/forms/ix-forms/components/ix-errors/ix-errors.component.ts Outdated
@AlexKarpov98
AlexKarpov98 merged commit 015ac71 into master Aug 26, 2026
16 checks passed
@AlexKarpov98
AlexKarpov98 deleted the NAS-142225 branch August 26, 2026 13:00
@bugclerk

Copy link
Copy Markdown
Contributor

This PR has been merged and conversations have been locked.
If you would like to discuss more about this issue please use our forums or raise a Jira ticket.

@truenas truenas locked as resolved and limited conversation to collaborators Aug 26, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants