Skip to content

Commit bb3f6cf

Browse files
dpwatrousdpwatrous
committed
Change overall form validation message logic
Prefer form-level errors to parameter level errors (which presumably will be shown next to their form control)
1 parent 544ad88 commit bb3f6cf

3 files changed

Lines changed: 183 additions & 12 deletions

File tree

‎packages/bonito-core/src/form/__tests__/form.spec.ts‎

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -347,7 +347,9 @@ describe("Form tests", () => {
347347
form.updateValue("parkName", "Yosemite");
348348
await form.validate();
349349
expect(form.validationStatus?.level).toEqual("error");
350-
expect(form.validationStatus?.message).toEqual("2 errors found");
350+
expect(form.validationStatus?.message).toEqual(
351+
"Invalid park/state combination"
352+
);
351353
expect(form.entryValidationStatus.parkName?.level).toEqual("error");
352354
expect(form.entryValidationStatus.parkName?.message).toEqual(
353355
"Cannot validate park name: no state selected"
@@ -359,15 +361,17 @@ describe("Form tests", () => {
359361

360362
form.updateValue("state", "California");
361363
await form.validate();
362-
// The state is now defined, but still invalid due to onValidate()
364+
// The state is now defined, but still invalid due to the state
365+
// parameter's validation function
363366
expect(form.validationStatus?.level).toEqual("error");
364367
expect(form.validationStatus?.message).toEqual(
365368
"State must be exactly 2 characters"
366369
);
367370

368-
form.updateValue("state", "CO");
371+
form.updateValue("state", "NV");
369372
await form.validate();
370-
// Park name parameter validation failure through dependency on state
373+
// Overall form validation passes, but park name parameter validation
374+
// fails through dependency on state
371375
expect(form.validationStatus?.level).toEqual("error");
372376
expect(form.validationStatus?.message).toEqual(
373377
"Invalid state selected. Valid options are: NY, New York, CA, California"
@@ -687,7 +691,13 @@ function createNationalParkForm() {
687691
const form = createForm<NationalParkFormValues>({
688692
values: {},
689693
onValidateSync: (values) => {
690-
if (values.parkName === "Yosemite" && values.state !== "CA") {
694+
if (
695+
values.parkName === "Yosemite" &&
696+
values.state !== "CA" &&
697+
values.state !== "California" &&
698+
// Note: Yosemite is not in Nevada, but we need another valid combo!
699+
values.state !== "NV"
700+
) {
691701
return new ValidationStatus(
692702
"error",
693703
"Invalid park/state combination"

‎packages/bonito-core/src/form/__tests__/validation-snapshot.spec.ts‎

Lines changed: 146 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,146 @@
1+
import { initMockEnvironment } from "../../environment";
2+
import { ValidationSnapshot } from "../validation-snapshot";
3+
import { ValidationStatus } from "../validation-status";
4+
5+
type FruitFormValues = {
6+
fruit?: string;
7+
color?: string;
8+
};
9+
10+
describe("ValidationSnapshot tests", () => {
11+
let snapshot: ValidationSnapshot<FruitFormValues>;
12+
13+
beforeEach(() => {
14+
initMockEnvironment();
15+
16+
snapshot = new ValidationSnapshot({
17+
fruit: "banana",
18+
color: "yellow",
19+
});
20+
21+
// Start out fully loaded and valid
22+
snapshot.onValidateSyncStatus = new ValidationStatus("ok");
23+
snapshot.onValidateAsyncStatus = new ValidationStatus("ok");
24+
snapshot.entryStatus.fruit = new ValidationStatus("ok");
25+
snapshot.entryStatus.color = new ValidationStatus("ok");
26+
snapshot.syncValidationComplete = true;
27+
snapshot.asyncValidationComplete = true;
28+
});
29+
30+
test("Valid", () => {
31+
expect(snapshot.overallStatus).toBeUndefined();
32+
33+
snapshot.updateOverallStatus();
34+
35+
expect(snapshot.overallStatus?.level).toBe("ok");
36+
expect(snapshot.overallStatus?.forced).toBe(false);
37+
expect(snapshot.overallStatus?.message).toBeUndefined();
38+
});
39+
40+
test("Single parameter validation error", () => {
41+
snapshot.entryStatus.fruit = new ValidationStatus(
42+
"error",
43+
"The fruit was rotten :("
44+
);
45+
snapshot.updateOverallStatus();
46+
47+
expect(snapshot.overallStatus?.level).toBe("error");
48+
expect(snapshot.overallStatus?.message).toBe("The fruit was rotten :(");
49+
});
50+
51+
test("More than one parameter validation error", () => {
52+
snapshot.entryStatus.fruit = new ValidationStatus(
53+
"error",
54+
"The fruit was rotten :("
55+
);
56+
snapshot.entryStatus.color = new ValidationStatus(
57+
"error",
58+
"And the color was brown"
59+
);
60+
snapshot.updateOverallStatus();
61+
62+
expect(snapshot.overallStatus?.level).toBe("error");
63+
expect(snapshot.overallStatus?.message).toBe("2 errors found");
64+
});
65+
66+
test("Sync form validation status overrides parameter status", () => {
67+
snapshot.entryStatus.fruit = new ValidationStatus(
68+
"error",
69+
"The fruit was rotten :("
70+
);
71+
snapshot.onValidateSyncStatus = new ValidationStatus(
72+
"error",
73+
"Sync validation wins"
74+
);
75+
snapshot.updateOverallStatus();
76+
77+
expect(snapshot.overallStatus?.level).toBe("error");
78+
expect(snapshot.overallStatus?.message).toBe("Sync validation wins");
79+
});
80+
81+
test("Async form validation status overrides parameter status", () => {
82+
snapshot.entryStatus.fruit = new ValidationStatus(
83+
"error",
84+
"The fruit was rotten :("
85+
);
86+
snapshot.onValidateAsyncStatus = new ValidationStatus(
87+
"error",
88+
"Async validation wins"
89+
);
90+
snapshot.updateOverallStatus();
91+
92+
expect(snapshot.overallStatus?.level).toBe("error");
93+
expect(snapshot.overallStatus?.message).toBe("Async validation wins");
94+
});
95+
96+
test("Sync form validation status overrides async form validation status", () => {
97+
snapshot.entryStatus.fruit = new ValidationStatus(
98+
"error",
99+
"The fruit was rotten :("
100+
);
101+
snapshot.onValidateSyncStatus = new ValidationStatus(
102+
"error",
103+
"Sync validation wins"
104+
);
105+
snapshot.onValidateAsyncStatus = new ValidationStatus(
106+
"error",
107+
"Async validation wins"
108+
);
109+
snapshot.updateOverallStatus();
110+
111+
expect(snapshot.overallStatus?.level).toBe("error");
112+
expect(snapshot.overallStatus?.message).toBe("Sync validation wins");
113+
});
114+
115+
test("Form validation warnings override parameter warnings", () => {
116+
snapshot.entryStatus.fruit = new ValidationStatus(
117+
"warn",
118+
"The fruit was just ok"
119+
);
120+
snapshot.onValidateSyncStatus = new ValidationStatus(
121+
"warn",
122+
"Sync validation warning wins"
123+
);
124+
snapshot.updateOverallStatus();
125+
126+
expect(snapshot.overallStatus?.level).toBe("warn");
127+
expect(snapshot.overallStatus?.message).toBe(
128+
"Sync validation warning wins"
129+
);
130+
});
131+
132+
test("Form validation warnings do not override parameter errors", () => {
133+
snapshot.entryStatus.fruit = new ValidationStatus(
134+
"error",
135+
"The fruit was rotten :("
136+
);
137+
snapshot.onValidateSyncStatus = new ValidationStatus(
138+
"warn",
139+
"Sync validation warning is ignored"
140+
);
141+
snapshot.updateOverallStatus();
142+
143+
expect(snapshot.overallStatus?.level).toBe("error");
144+
expect(snapshot.overallStatus?.message).toBe("The fruit was rotten :(");
145+
});
146+
});

‎packages/bonito-core/src/form/validation-snapshot.ts‎

Lines changed: 22 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -75,13 +75,28 @@ export class ValidationSnapshot<V extends FormValues> {
7575
overallStatus = new ValidationStatus("ok");
7676
}
7777

78-
if (overallStatus.level === "ok" && this.onValidateSyncStatus) {
79-
overallStatus = this.onValidateSyncStatus;
80-
}
81-
82-
if (overallStatus.level === "ok" && this.onValidateAsyncStatus) {
83-
overallStatus = this.onValidateAsyncStatus;
84-
}
78+
// Prefer parameter validation error, then async validation errors,
79+
// then sync validation errors in that order
80+
const computeOverallStatus = (validateStatus?: ValidationStatus) => {
81+
if (validateStatus) {
82+
if (
83+
overallStatus.level === "error" &&
84+
validateStatus.level === "error"
85+
) {
86+
overallStatus = validateStatus;
87+
} else if (
88+
overallStatus.level === "warn" &&
89+
(validateStatus.level === "warn" ||
90+
validateStatus.level === "error")
91+
) {
92+
overallStatus = validateStatus;
93+
} else if (overallStatus.level === "ok" && validateStatus) {
94+
overallStatus = validateStatus;
95+
}
96+
}
97+
};
98+
computeOverallStatus(this.onValidateAsyncStatus);
99+
computeOverallStatus(this.onValidateSyncStatus);
85100

86101
this.overallStatus = overallStatus;
87102
}

0 commit comments

Comments
 (0)