Skip to content

Commit e1ff8b6

Browse files
committed
NAS-142225: Fix Force checkbox not enabling Save on the NTP Server form
Backport of 015ac71. Two files from the original do not apply here: tn-form-field-errors.provider.ts and ntp-servers.form-config.spec.ts both target the tn-*/<ix-form-renderer> NTP form, which this branch predates. The NTP-level regression coverage is kept, rewritten against the ix-* SlideIn form this branch actually ships. The one service spec asserting a nested dotted error path was retargeted too - that lookup comes from NAS-141758, not here. Claude-Session: https://claude.ai/code/session_015DNgeeCxxzRm7YJwr2uVvv
1 parent 31af476 commit e1ff8b6

9 files changed

Lines changed: 428 additions & 29 deletions

File tree

src/app/modules/forms/ix-forms/components/ix-errors/ix-errors.component.ts

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -10,12 +10,11 @@ import { Subscription } from 'rxjs';
1010
import { filter } from 'rxjs/operators';
1111
import { DefaultValidationError } from 'app/enums/default-validation-error.enum';
1212
import { IxSimpleChanges } from 'app/interfaces/simple-changes.interface';
13+
import { ixManualValidateErrorKey } from 'app/modules/forms/ix-forms/manual-validate-error.constants';
1314
import { ArrayLengthValidationError } from 'app/modules/forms/ix-forms/validators/array-length-validation';
1415

1516
type SomeError = Record<string, unknown>;
1617

17-
export const ixManualValidateError = 'ixManualValidateError';
18-
1918
@Component({
2019
selector: 'ix-errors',
2120
templateUrl: './ix-errors.component.html',
@@ -37,7 +36,7 @@ export class IxErrorsComponent implements OnChanges, OnDestroy {
3736
readonly control = input.required<AbstractControl>();
3837
readonly label = input<string>();
3938

40-
readonly ixManualValidateError = ixManualValidateError;
39+
readonly ixManualValidateError = ixManualValidateErrorKey;
4140

4241
private statusChangeSubscription: Subscription;
4342
messages: string[] = [];
@@ -158,7 +157,7 @@ export class IxErrorsComponent implements OnChanges, OnDestroy {
158157

159158
private handleErrors(options: { skipMarkAsTouched?: boolean } = {}): void {
160159
const newErrors: (string | null)[] = Object.keys(this.control().errors || []).map((error) => {
161-
if (error === ixManualValidateError) {
160+
if (error === ixManualValidateErrorKey) {
162161
return null;
163162
}
164163
const message = (this.control().errors?.[error] as SomeError)?.message as string;
@@ -263,7 +262,7 @@ export class IxErrorsComponent implements OnChanges, OnDestroy {
263262
removeManualError(): void {
264263
const errors = this.control().errors;
265264
if (errors) {
266-
delete errors[ixManualValidateError];
265+
delete errors[ixManualValidateErrorKey];
267266
delete errors.manualValidateError;
268267
delete errors.manualValidateErrorMsg;
269268
}
@@ -279,7 +278,7 @@ export class IxErrorsComponent implements OnChanges, OnDestroy {
279278
private announceErrors(): void {
280279
const messages = [...this.messages];
281280
const manualError = (
282-
this.control().errors?.[ixManualValidateError] as { message: string } | undefined
281+
this.control().errors?.[ixManualValidateErrorKey] as { message: string } | undefined
283282
)?.message;
284283
if (manualError) {
285284
messages.push(manualError);
Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
/**
2+
* Error keys `FormErrorHandlerService` writes onto a control whose error came from the backend
3+
* rather than from a validator. They travel as a set — the active key is the bare boolean
4+
* {@link manualValidateErrorKey}, while the human-readable text lives in the two siblings — so they
5+
* are declared together here rather than re-spelled at each reader (the error-message resolver, the
6+
* legacy `ix-errors` component, `<ix-form-renderer>`'s error clearing).
7+
*
8+
* Unlike a validator result these are pinned with `setErrors()` and never re-evaluate, so consumers
9+
* that need to tell a live validation failure from a stale server verdict key off
10+
* {@link manualValidateErrorKey}.
11+
*/
12+
export const manualValidateErrorKey = 'manualValidateError';
13+
export const manualValidateErrorMsgKey = 'manualValidateErrorMsg';
14+
export const ixManualValidateErrorKey = 'ixManualValidateError';

src/app/modules/forms/ix-forms/services/form-error-handler.service.spec.ts

Lines changed: 208 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import { DOCUMENT } from '@angular/common';
22
import { fakeAsync, tick } from '@angular/core/testing';
3+
import { Validators } from '@angular/forms';
34
import { FormControl, FormGroup } from '@ngneat/reactive-forms';
45
import { createServiceFactory, mockProvider, SpectatorService } from '@ngneat/spectator/jest';
56
import { ApiErrorName, JsonRpcErrorCode } from 'app/enums/api.enum';
@@ -57,6 +58,24 @@ const arrayFieldError = new ApiCallError({
5758
},
5859
});
5960

61+
const unreachableAddressError = new ApiCallError({
62+
code: JsonRpcErrorCode.CallError,
63+
message: 'Validation error',
64+
data: {
65+
error: 11,
66+
errname: ApiErrorName.Validation,
67+
extra: [
68+
[
69+
'ntp_server_create.address',
70+
'Server could not be reached. Check "Force" to continue regardless.',
71+
22,
72+
],
73+
],
74+
trace: { class: 'ValidationErrors', formatted: 'Formatted string', frames: [] as ApiTraceFrame[] },
75+
reason: 'Test reason',
76+
},
77+
});
78+
6079
const formGroup = new FormGroup({
6180
test_control_1: new FormControl(''),
6281
sudo_commands_no_passwd: new FormControl([]),
@@ -75,6 +94,8 @@ describe('FormErrorHandlerService', () => {
7594
const elementMock = {
7695
scrollIntoView: jest.fn() as HTMLElement['scrollIntoView'],
7796
focus: jest.fn() as HTMLElement['focus'],
97+
// The service looks for a native control inside the element first; an ix-* host wraps none.
98+
querySelector: jest.fn((): HTMLElement | null => null) as unknown as HTMLElement['querySelector'],
7899
} as HTMLElement;
79100

80101
const createService = createServiceFactory({
@@ -183,6 +204,21 @@ describe('FormErrorHandlerService', () => {
183204
expect(elementMock.focus).toHaveBeenCalled();
184205
}));
185206

207+
it('focuses the native control inside a tn-* component host', fakeAsync(() => {
208+
// `data-control-name` sits on the tn-* component host, which carries no tabindex and so
209+
// ignores focus() — the focusable element is the native control it wraps.
210+
const innerInput = { focus: jest.fn() } as unknown as HTMLElement;
211+
// `Once`: jest is configured to clear calls between tests, not implementations.
212+
(elementMock.querySelector as jest.Mock).mockReturnValueOnce(innerInput);
213+
214+
spectator.service.handleValidationErrors(callError, formGroup);
215+
tick();
216+
217+
expect(elementMock.scrollIntoView).toHaveBeenCalled();
218+
expect(innerInput.focus).toHaveBeenCalled();
219+
expect(elementMock.focus).not.toHaveBeenCalled();
220+
}));
221+
186222
it('notifies EditableComponents through secure service', () => {
187223
spectator.service.handleValidationErrors(callError, formGroup);
188224

@@ -207,6 +243,130 @@ describe('FormErrorHandlerService', () => {
207243
});
208244
});
209245

246+
describe('self-retiring pinned errors', () => {
247+
// NAS-142225. A backend verdict is pinned with `setErrors()` and never re-evaluates, so an
248+
// error the user is meant to answer from a DIFFERENT field would hold Save shut forever.
249+
const buildNtpForm = (): FormGroup<{ address: string; force: boolean }> => new FormGroup({
250+
address: new FormControl('192.0.2.1', [Validators.required]),
251+
force: new FormControl(false),
252+
});
253+
254+
it('retires the pinned error on the next edit anywhere in the form', () => {
255+
const form = buildNtpForm();
256+
spectator.service.handleValidationErrors(unreachableAddressError, form);
257+
expect(form.controls.address.errors).toEqual(expect.objectContaining({ manualValidateError: true }));
258+
expect(form.valid).toBe(false);
259+
260+
form.controls.force.setValue(true);
261+
262+
expect(form.controls.address.errors).toBeNull();
263+
expect(form.valid).toBe(true);
264+
});
265+
266+
it('restores the real validation state instead of blanket-clearing errors', () => {
267+
const form = buildNtpForm();
268+
form.controls.address.setValue('');
269+
spectator.service.handleValidationErrors(unreachableAddressError, form);
270+
271+
form.controls.force.setValue(true);
272+
273+
// The pinned verdict was masking a genuinely empty required field, which must say so again.
274+
expect(form.controls.address.errors).toEqual({ required: true });
275+
expect(form.valid).toBe(false);
276+
});
277+
278+
it('leaves a live client-side error on a sibling alone', () => {
279+
const form = new FormGroup({
280+
address: new FormControl('192.0.2.1', [Validators.required]),
281+
force: new FormControl(false),
282+
maxpoll: new FormControl(99, [Validators.max(17)]),
283+
});
284+
spectator.service.handleValidationErrors(unreachableAddressError, form);
285+
286+
form.controls.force.setValue(true);
287+
288+
expect(form.controls.address.errors).toBeNull();
289+
expect(form.controls.maxpoll.errors).toEqual(expect.objectContaining({ max: expect.anything() }));
290+
expect(form.valid).toBe(false);
291+
});
292+
293+
it('retires an error pinned on a nested control from an edit in a sibling group', () => {
294+
// The subscription listens on the control's root rather than its parent, so an edit anywhere
295+
// in the form counts — including a group the flagged control does not itself live in.
296+
const server = new FormGroup({ address: new FormControl('192.0.2.1') });
297+
const options = new FormGroup({ force: new FormControl(false) });
298+
// Both groups belong to one form, so `root` is the shared parent; they are handed over
299+
// separately because the leaf lookup resolves against each group it is given.
300+
const form = new FormGroup({ server, options });
301+
expect(server.controls.address.root).toBe(form);
302+
303+
spectator.service.handleValidationErrors(unreachableAddressError, [server, options]);
304+
expect(server.controls.address.errors).toEqual(expect.objectContaining({ manualValidateError: true }));
305+
306+
options.controls.force.setValue(true);
307+
308+
expect(server.controls.address.errors).toBeNull();
309+
});
310+
311+
// A verdict can flag several fields at once. Answering one must not wipe the messages for the
312+
// ones the user has not reached yet — those verdicts still stand.
313+
const twoFieldError = new ApiCallError({
314+
code: JsonRpcErrorCode.CallError,
315+
message: 'Validation error',
316+
data: {
317+
error: 11,
318+
errname: ApiErrorName.Validation,
319+
extra: [
320+
['ntp_server_create.address', 'Server could not be reached.', 22],
321+
['ntp_server_create.description', 'Description is already taken.', 22],
322+
],
323+
trace: { class: 'ValidationErrors', formatted: '', frames: [] as ApiTraceFrame[] },
324+
reason: 'Test reason',
325+
},
326+
});
327+
328+
const buildTwoFieldForm = (): FormGroup<{ address: string; description: string; force: boolean }> => new FormGroup({
329+
address: new FormControl('192.0.2.1'),
330+
description: new FormControl('Primary'),
331+
force: new FormControl(false),
332+
});
333+
334+
it('keeps the other fields of a multi-field verdict pinned while one of them is answered', () => {
335+
const form = buildTwoFieldForm();
336+
spectator.service.handleValidationErrors(twoFieldError, form);
337+
expect(form.controls.address.errors).toEqual(expect.objectContaining({ manualValidateError: true }));
338+
expect(form.controls.description.errors).toEqual(expect.objectContaining({ manualValidateError: true }));
339+
340+
form.controls.address.setValue('192.0.2.2');
341+
342+
expect(form.controls.address.errors).toBeNull();
343+
expect(form.controls.description.errors).toEqual(expect.objectContaining({ manualValidateError: true }));
344+
expect(form.valid).toBe(false);
345+
});
346+
347+
it('retires the still-pinned fields once the edit lands outside the flagged set', () => {
348+
const form = buildTwoFieldForm();
349+
spectator.service.handleValidationErrors(twoFieldError, form);
350+
form.controls.address.setValue('192.0.2.2');
351+
352+
form.controls.force.setValue(true);
353+
354+
expect(form.controls.description.errors).toBeNull();
355+
expect(form.valid).toBe(true);
356+
});
357+
358+
it('re-pins the verdict when the next save is rejected again', () => {
359+
const form = buildNtpForm();
360+
spectator.service.handleValidationErrors(unreachableAddressError, form);
361+
form.controls.force.setValue(true);
362+
expect(form.controls.address.errors).toBeNull();
363+
364+
spectator.service.handleValidationErrors(unreachableAddressError, form);
365+
366+
expect(form.controls.address.errors).toEqual(expect.objectContaining({ manualValidateError: true }));
367+
});
368+
});
369+
210370
describe('clearValidationErrorsForHiddenFields', () => {
211371
it('clears errors for hidden fields when provided as array', () => {
212372
const control1 = new FormControl('');
@@ -364,8 +524,11 @@ describe('FormErrorHandlerService', () => {
364524

365525
spectator.service.handleValidationErrors(callError, formGroup);
366526

527+
// Also matches `data-control-name`: tn-* controls built by `<ix-form-renderer>` register with
528+
// neither IxFormService nor a `formControlName` attribute (it is a property binding).
367529
// eslint-disable-next-line sonarjs/deprecation
368-
expect(doc.querySelector).toHaveBeenCalledWith('[formControlName="test_control_1"]');
530+
expect(doc.querySelector)
531+
.toHaveBeenCalledWith('[formControlName="test_control_1"], [data-control-name="test_control_1"]');
369532
});
370533

371534
it('warns when DOM element cannot be found', () => {
@@ -377,6 +540,50 @@ describe('FormErrorHandlerService', () => {
377540

378541
expect(console.warn).toHaveBeenCalledWith('Could not find DOM element for field test_control_1.');
379542
});
543+
544+
it('shows a rendered tn-* control inline only, without duplicating it in an error modal', () => {
545+
// NAS-142225: a tn-* form built by `<ix-form-renderer>` registers with neither IxFormService
546+
// nor a `formControlName` attribute, so its controls used to look unrendered and every
547+
// message the user could already read under the field was repeated in a modal. They are found
548+
// through `data-control-name` now, which keeps the report inline.
549+
jest.spyOn(spectator.inject(IxFormService), 'getElementByControlName').mockReturnValue(null);
550+
jest.spyOn(spectator.inject(DOCUMENT), 'querySelector').mockReturnValue(document.createElement('input'));
551+
const ntpForm = new FormGroup({ address: new FormControl('192.0.2.1') });
552+
553+
// The ErrorHandlerService mock is built once per factory, so call counts carry over
554+
// between tests in this file — clear them to assert on this call alone.
555+
const mockErrorHandler = spectator.inject(ErrorHandlerService);
556+
jest.clearAllMocks();
557+
558+
spectator.service.handleValidationErrors(unreachableAddressError, ntpForm);
559+
560+
expect(ntpForm.controls.address.errors).toEqual(expect.objectContaining({
561+
manualValidateError: true,
562+
manualValidateErrorMsg: 'Server could not be reached. Check "Force" to continue regardless.',
563+
}));
564+
expect(mockErrorHandler.showErrorModal).not.toHaveBeenCalled();
565+
});
566+
567+
it('still escalates to a modal for a control that is nowhere in the DOM', () => {
568+
// A control in the form group but not rendered (behind an `@if`, or payload-only) has nowhere
569+
// to show its pinned message, so the modal is the only signal the user gets.
570+
jest.spyOn(spectator.inject(IxFormService), 'getElementByControlName').mockReturnValue(null);
571+
jest.spyOn(spectator.inject(DOCUMENT), 'querySelector').mockReturnValue(null);
572+
const ntpForm = new FormGroup({ address: new FormControl('192.0.2.1') });
573+
574+
const mockErrorHandler = spectator.inject(ErrorHandlerService);
575+
jest.clearAllMocks();
576+
577+
spectator.service.handleValidationErrors(unreachableAddressError, ntpForm);
578+
579+
expect(mockErrorHandler.showErrorModal).toHaveBeenCalledWith(
580+
expect.objectContaining({
581+
message: expect.stringContaining(
582+
'address: Server could not be reached. Check "Force" to continue regardless.',
583+
),
584+
}),
585+
);
586+
});
380587
});
381588

382589
describe('field path extraction edge cases', () => {

0 commit comments

Comments
 (0)