Skip to content

Commit 01834e0

Browse files
zubronzubron
committed
Address review comments
Validation functions do not modify state, instead makes it the parent component's responsibility.
1 parent 47c9992 commit 01834e0

File tree

6 files changed

+273
-73
lines changed

6 files changed

+273
-73
lines changed

‎webui/src/lib/components/repository/compareBranchesActionBar.jsx‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,8 @@ import React, {useCallback, useRef, useState} from "react";
22
import {refs as refsAPI} from "../../../lib/api";
33
import {RefTypeBranch} from "../../../constants";
44
import {ActionGroup, ActionsBar, AlertError, RefreshButton} from "../controls";
5-
import {MetadataFields, validateMetadataKeys} from "./metadata";
5+
import {MetadataFields} from "./metadata";
6+
import {getMetadataIfValid, touchInvalidFields} from "./metadataHelpers";
67
import {GitMergeIcon} from "@primer/octicons-react";
78
import Button from "react-bootstrap/Button";
89
import Modal from "react-bootstrap/Modal";
@@ -72,12 +73,13 @@ const MergeButton = ({repo, onDone, source, dest, disabled = false}) => {
7273
}
7374

7475
const onSubmit = async () => {
75-
if (!validateMetadataKeys(metadataFields, setMetadataFields)) {
76+
const metadata = getMetadataIfValid(metadataFields);
77+
if (!metadata) {
78+
setMetadataFields(touchInvalidFields(metadataFields));
7679
return;
7780
}
81+
7882
const message = textRef.current.value;
79-
const metadata = {};
80-
metadataFields.forEach(pair => metadata[pair.key] = pair.value)
8183

8284
let strategy = mergeState.strategy;
8385
if (strategy === "none") {

‎webui/src/lib/components/repository/metadata.jsx‎

Lines changed: 4 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import Button from "react-bootstrap/Button";
55
import Form from "react-bootstrap/Form";
66
import Row from "react-bootstrap/Row";
77
import Col from "react-bootstrap/Col";
8+
import { getFieldError } from './metadataHelpers'
89

910
/**
1011
* MetadataFields is a component that allows the user to add/remove key-value pairs of metadata.
@@ -42,7 +43,7 @@ export const MetadataFields = ({ metadataFields, setMetadataFields, ...rest}) =>
4243
return (
4344
<div className="mt-3 mb-3" {...rest}>
4445
{metadataFields.map((f, i) => {
45-
const showError = isEmptyKey(f.key) && f.touched;
46+
const fieldError = getFieldError(f)
4647
return (
4748
<Form.Group key={`commit-metadata-field-${i}`} className="mb-3">
4849
<Row>
@@ -53,13 +54,9 @@ export const MetadataFields = ({ metadataFields, setMetadataFields, ...rest}) =>
5354
value={f.key}
5455
onChange={onChangeKey(i)}
5556
onBlur={onBlurKey(i)}
56-
isInvalid={showError}
57+
isInvalid={fieldError}
5758
/>
58-
{showError && (
59-
<Form.Control.Feedback type="invalid">
60-
Key is required
61-
</Form.Control.Feedback>
62-
)}
59+
{fieldError && <Form.Control.Feedback type="invalid">{fieldError}</Form.Control.Feedback>}
6360
</Col>
6461
<Col md={{span: 5}}>
6562
<Form.Control type="text" placeholder="Value" value={f.value} onChange={onChangeValue(i)}/>
@@ -82,23 +79,3 @@ export const MetadataFields = ({ metadataFields, setMetadataFields, ...rest}) =>
8279
</div>
8380
)
8481
}
85-
86-
const isEmptyKey = (key) => !key || key.trim() === "";
87-
88-
/**
89-
* Validates metadata fields and marks empty keys as touched to show validation errors.
90-
* Use this before submitting to ensure all keys are filled in.
91-
*
92-
* @param {Array<{key: string, value: string, touched: boolean}>} metadataFields - Array of metadata field objects
93-
* @param {Function} setMetadataFields - Setter function to update metadata fields
94-
* @returns {boolean} True if validation passed (no empty keys), false if validation failed
95-
*/
96-
export const validateMetadataKeys = (metadataFields, setMetadataFields) => {
97-
const hasEmptyKeys = metadataFields.some(f => isEmptyKey(f.key));
98-
if (!hasEmptyKeys) return true;
99-
100-
setMetadataFields(prev =>
101-
prev.map(f => isEmptyKey(f.key) ? {...f, touched: true} : f)
102-
);
103-
return false;
104-
};

‎webui/src/lib/components/repository/metadata.test.jsx‎

Lines changed: 2 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,8 @@
11
import React from "react";
2-
import { describe, it, expect, vi } from 'vitest';
2+
import { describe, it, expect } from 'vitest';
33
import { render, screen } from '@testing-library/react';
44
import userEvent from '@testing-library/user-event';
5-
import { MetadataFields, validateMetadataKeys } from './metadata';
5+
import { MetadataFields } from './metadata';
66

77
/**
88
* MetadataFieldsWrapper is a component wrapper used for testing the MetadataFields component.
@@ -98,32 +98,3 @@ describe('MetadataFields validation flow', () => {
9898
});
9999
});
100100

101-
describe('validateMetadataKeys', () => {
102-
it('returns true for empty array', () => {
103-
const setMetadataFields = vi.fn();
104-
expect(validateMetadataKeys([], setMetadataFields)).toBe(true);
105-
expect(setMetadataFields).not.toHaveBeenCalled();
106-
});
107-
108-
it('returns true when all keys are valid', () => {
109-
const setMetadataFields = vi.fn();
110-
const fields = [
111-
{ key: "key1", value: "value1", touched: false },
112-
{ key: "key2", value: "", touched: false },
113-
];
114-
115-
expect(validateMetadataKeys(fields, setMetadataFields)).toBe(true);
116-
expect(setMetadataFields).not.toHaveBeenCalled();
117-
});
118-
119-
it('returns false when any key is empty or whitespace', () => {
120-
const setMetadataFields = vi.fn();
121-
const fields = [
122-
{ key: "", value: "value", touched: false },
123-
{ key: " ", value: "value", touched: false },
124-
];
125-
126-
expect(validateMetadataKeys(fields, setMetadataFields)).toBe(false);
127-
expect(setMetadataFields).toHaveBeenCalledTimes(1);
128-
});
129-
});

‎webui/src/lib/components/repository/metadataHelpers.js‎

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,73 @@
1+
const isEmptyKey = (key) => !key || key.trim() === "";
2+
3+
const isInvalidKey = (key) => {
4+
// TODO: Add more validation checks, e.g. duplicate keys, invalid characters
5+
return isEmptyKey(key);
6+
};
7+
8+
/**
9+
* Checks if there are any invalid keys in the metadata fields.
10+
* @param {Array<{key: string, value: string, touched: boolean}>} metadataFields
11+
* @returns {boolean} True if there are any invalid keys
12+
*/
13+
export const hasInvalidKeys = (metadataFields) => {
14+
return metadataFields.some(f => isInvalidKey(f.key));
15+
};
16+
17+
/**
18+
* Converts metadata fields array to a plain key-value object.
19+
* @param {Array<{key: string, value: string, touched: boolean}>} metadataFields
20+
* @returns {Object} Metadata object with key-value pairs
21+
*/
22+
export const fieldsToMetadata = (metadataFields) => {
23+
const metadata = {};
24+
metadataFields.forEach(pair => metadata[pair.key] = pair.value);
25+
return metadata;
26+
};
27+
28+
/**
29+
* Returns a new array with invalid fields marked as touched.
30+
* Does not mutate the input array.
31+
* @param {Array<{key: string, value: string, touched: boolean}>} metadataFields
32+
* @returns {Array<{key: string, value: string, touched: boolean}>} New array with invalid fields touched
33+
*/
34+
export const touchInvalidFields = (metadataFields) => {
35+
return metadataFields.map(field =>
36+
isInvalidKey(field.key) ? { ...field, touched: true } : field
37+
);
38+
};
39+
40+
/**
41+
* Validates metadata fields and returns the metadata object if valid, or null if invalid.
42+
*
43+
* Use this in form submission handlers. If null is returned, call touchInvalidFields()
44+
* to mark invalid fields and show errors to the user.
45+
*
46+
* @param {Array<{key: string, value: string, touched: boolean}>} metadataFields
47+
* @returns {Object|null} Metadata object if valid, null if validation failed
48+
*/
49+
export const getMetadataIfValid = (metadataFields) => {
50+
if (hasInvalidKeys(metadataFields)) {
51+
return null;
52+
}
53+
return fieldsToMetadata(metadataFields);
54+
};
55+
56+
/**
57+
* Returns the validation error message for a metadata field, if any.
58+
* Only returns an error if the field has been touched (interacted with by the user).
59+
*
60+
* Use this function in the MetadataFields component to display field-level errors.
61+
*
62+
* @param {Object} field - Metadata field object with key, value, and touched properties
63+
* @returns {string|null} Error message if field is invalid and touched, null otherwise
64+
*/
65+
export const getFieldError = (field) => {
66+
if (!field.touched) return null;
67+
68+
if (isEmptyKey(field.key)) {
69+
return "Key is required";
70+
}
71+
72+
return null;
73+
};

‎webui/src/lib/components/repository/metadataHelpers.test.js‎

Lines changed: 175 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,175 @@
1+
import { describe, it, expect } from 'vitest';
2+
import { hasInvalidKeys, fieldsToMetadata, touchInvalidFields, getFieldError, getMetadataIfValid } from './metadataHelpers';
3+
4+
describe('hasInvalidKeys', () => {
5+
it('returns false for empty array', () => {
6+
expect(hasInvalidKeys([])).toBe(false);
7+
});
8+
9+
it('returns false when all keys are valid', () => {
10+
const fields = [
11+
{ key: "key1", value: "value1", touched: false },
12+
{ key: "key2", value: "", touched: false },
13+
];
14+
15+
expect(hasInvalidKeys(fields)).toBe(false);
16+
});
17+
18+
it('returns true when any key is empty', () => {
19+
const fields = [
20+
{ key: "", value: "value", touched: false },
21+
{ key: "valid", value: "value", touched: false },
22+
];
23+
24+
expect(hasInvalidKeys(fields)).toBe(true);
25+
});
26+
27+
it('returns true when any key is whitespace only', () => {
28+
const fields = [
29+
{ key: " ", value: "value", touched: false },
30+
{ key: "valid", value: "value", touched: false },
31+
];
32+
33+
expect(hasInvalidKeys(fields)).toBe(true);
34+
});
35+
});
36+
37+
describe('fieldsToMetadata', () => {
38+
it('converts empty array to empty object', () => {
39+
expect(fieldsToMetadata([])).toEqual({});
40+
});
41+
42+
it('converts fields to key-value object', () => {
43+
const fields = [
44+
{ key: "env", value: "prod", touched: false },
45+
{ key: "region", value: "us-east-1", touched: true },
46+
];
47+
48+
expect(fieldsToMetadata(fields)).toEqual({
49+
env: "prod",
50+
region: "us-east-1",
51+
});
52+
});
53+
54+
it('handles empty values', () => {
55+
const fields = [
56+
{ key: "key1", value: "", touched: false },
57+
];
58+
59+
expect(fieldsToMetadata(fields)).toEqual({ key1: "" });
60+
});
61+
});
62+
63+
describe('touchInvalidFields', () => {
64+
it('returns empty array for empty input', () => {
65+
expect(touchInvalidFields([])).toEqual([]);
66+
});
67+
68+
it('marks fields with empty keys as touched', () => {
69+
const fields = [
70+
{ key: "", value: "value", touched: false },
71+
{ key: "valid", value: "value", touched: false },
72+
];
73+
74+
const result = touchInvalidFields(fields);
75+
76+
expect(result[0].touched).toBe(true);
77+
expect(result[1].touched).toBe(false);
78+
});
79+
80+
it('marks fields with whitespace keys as touched', () => {
81+
const fields = [
82+
{ key: " ", value: "value", touched: false },
83+
];
84+
85+
const result = touchInvalidFields(fields);
86+
87+
expect(result[0].touched).toBe(true);
88+
});
89+
90+
it('preserves already touched state', () => {
91+
const fields = [
92+
{ key: "valid", value: "value", touched: true },
93+
];
94+
95+
const result = touchInvalidFields(fields);
96+
97+
expect(result[0].touched).toBe(true);
98+
});
99+
100+
it('does not mutate original array', () => {
101+
const fields = [
102+
{ key: "", value: "value", touched: false },
103+
];
104+
105+
const result = touchInvalidFields(fields);
106+
107+
expect(fields[0].touched).toBe(false); // Original unchanged
108+
expect(result[0].touched).toBe(true); // New array modified
109+
});
110+
});
111+
112+
describe('getFieldError', () => {
113+
it('returns null for untouched field', () => {
114+
const field = { key: "", value: "", touched: false };
115+
expect(getFieldError(field)).toBeNull();
116+
});
117+
118+
it('returns error message for empty key when touched', () => {
119+
const field = { key: "", value: "", touched: true };
120+
expect(getFieldError(field)).toBe("Key is required");
121+
});
122+
123+
it('returns error message for whitespace key when touched', () => {
124+
const field = { key: " ", value: "", touched: true };
125+
expect(getFieldError(field)).toBe("Key is required");
126+
});
127+
128+
it('returns null for valid key', () => {
129+
const field = { key: "valid", value: "", touched: true };
130+
expect(getFieldError(field)).toBeNull();
131+
});
132+
});
133+
134+
describe('getMetadataIfValid', () => {
135+
it('returns metadata object for valid fields', () => {
136+
const fields = [
137+
{ key: "env", value: "prod", touched: false },
138+
{ key: "region", value: "us-east-1", touched: false },
139+
];
140+
141+
const result = getMetadataIfValid(fields);
142+
143+
expect(result).toEqual({
144+
env: "prod",
145+
region: "us-east-1",
146+
});
147+
});
148+
149+
it('returns null for fields with empty keys', () => {
150+
const fields = [
151+
{ key: "", value: "value", touched: false },
152+
{ key: "valid", value: "value", touched: false },
153+
];
154+
155+
const result = getMetadataIfValid(fields);
156+
157+
expect(result).toBeNull();
158+
});
159+
160+
it('returns empty object for empty array', () => {
161+
const result = getMetadataIfValid([]);
162+
163+
expect(result).toEqual({});
164+
});
165+
166+
it('returns null for whitespace-only keys', () => {
167+
const fields = [
168+
{ key: " ", value: "value", touched: false },
169+
];
170+
171+
const result = getMetadataIfValid(fields);
172+
173+
expect(result).toBeNull();
174+
});
175+
});

0 commit comments

Comments
 (0)