[OSDEV-1678] Highlight the mandatory fields on the Production Location Info page.#522
Conversation
React App | Jest test suite - Code coverage reportTotal: 31.36%Your code coverage diff: 1.80% ▴ ✅ All code changes are covered |
Dedupe Hub App | Unittest test suite - Code coverage reportTotal: 56.14%Your code coverage diff: 0.00% ▴ ✅ All code changes are covered |
Contricleaner App | Unittest test suite - Code coverage reportTotal: 98.91%Your code coverage diff: 0.00% ▴ ✅ All code changes are covered |
Countries App | Unittest test suite - Code coverage reportTotal: 100%Your code coverage diff: 0.00% ▴ ✅ All code changes are covered |
VadimKovalenkoSNF
left a comment
There was a problem hiding this comment.
Approved with minor suggestion.
…rs-but-no-field-is-highlighted
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/react/src/components/Contribute/ProductionLocationInfo.jsx (1)
821-821: Consider adding aria-required attribute for better accessibility.While the asterisk visually indicates required fields, adding
aria-required="true"to the input fields would improve accessibility for screen readers.<Button color="secondary" variant="contained" onClick={() => { handleProductionLocation(inputData, osID); }} className={classes.submitButtonStyles} - disabled={!isFormValid} + disabled={!isFormValid} + aria-required="true" >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
doc/release/RELEASE-NOTES.md(1 hunks)src/react/src/__tests__/utils.tests.js(3 hunks)src/react/src/components/Contribute/ProductionLocationInfo.jsx(19 hunks)src/react/src/util/util.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- doc/release/RELEASE-NOTES.md
🧰 Additional context used
🧠 Learnings (1)
src/react/src/components/Contribute/ProductionLocationInfo.jsx (1)
Learnt from: VadimKovalenkoSNF
PR: opensupplyhub/open-supply-hub#470
File: src/react/src/components/Contribute/ProductionLocationInfo.jsx:680-692
Timestamp: 2025-01-23T11:10:17.929Z
Learning: The ProductionLocationInfo component in Open Supply Hub implements field-level validation for name, address and country fields using Material-UI's TextField validation patterns and touch states, providing real-time feedback to users.
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: run-integration-test-code-quality
- GitHub Check: run-flake8-linter
- GitHub Check: run-fe-code-quality
- GitHub Check: run-eslint-linter-and-prettier-formatter
- GitHub Check: run-django-code-quality
- GitHub Check: run-dd-code-quality
- GitHub Check: run-contricleaner-code-quality
- GitHub Check: run-countries-code-quality
- GitHub Check: get-base-branch-fe-cov
- GitHub Check: get-base-branch-django-cov
- GitHub Check: get-base-branch-countries-cov
- GitHub Check: get-base-branch-dd-cov
- GitHub Check: get-base-branch-contricleaner-cov
🔇 Additional comments (8)
src/react/src/components/Contribute/ProductionLocationInfo.jsx (4)
96-96: LGTM! State management for country field validation.The addition of
countryTouchedstate andisCountryErrorvalidation check aligns with the existing validation pattern for name and address fields.Also applies to: 104-104
153-161: LGTM! Consistent blur handlers for form fields.The new blur handlers follow the same pattern for all required fields, setting their touched state when they lose focus.
163-167: LGTM! Form validation logic.The
isFormValidcheck ensures all required fields are valid before enabling the submit button, using the newisRequiredFieldValidutility function.
415-418: LGTM! Required field indicators.The addition of asterisks to required field labels clearly indicates which fields are mandatory, directly addressing the PR's objective.
Also applies to: 470-473, 525-528
src/react/src/util/util.js (2)
1491-1491: LGTM! Simple and reusable field validation.The
isRequiredFieldValidfunction is concise and handles all edge cases (empty strings, whitespace, null, undefined).
1493-1525: LGTM! Comprehensive select component styling.The
getSelectStylesfunction provides consistent styling for select components, including error states, focus states, and hover effects.src/react/src/__tests__/utils.tests.js (2)
2174-2187: LGTM! Thorough validation function tests.The test suite for
isRequiredFieldValidcovers all edge cases including empty strings, whitespace, null, and undefined values.
2189-2243: LGTM! Comprehensive select styles tests.The test suite for
getSelectStylesthoroughly verifies all styling scenarios including focus states, error states, and hover effects.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/react/src/util/util.js (1)
1493-1525: Consider extracting hardcoded values into constants.The implementation is solid and handles all states correctly. However, consider extracting the hardcoded style values into constants for better maintainability:
+const SELECT_MIN_HEIGHT = '56px'; +const SELECT_BORDER_RADIUS = '0'; + export const getSelectStyles = (isErrorState = false) => ({ control: (provided, state) => ({ ...provided, - minHeight: '56px', - borderRadius: '0', + minHeight: SELECT_MIN_HEIGHT, + borderRadius: SELECT_BORDER_RADIUS,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/react/src/util/util.js(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: run-integration-test-code-quality
- GitHub Check: run-flake8-linter
- GitHub Check: run-fe-code-quality
- GitHub Check: run-eslint-linter-and-prettier-formatter
- GitHub Check: run-dd-code-quality
- GitHub Check: run-django-code-quality
- GitHub Check: run-countries-code-quality
- GitHub Check: run-contricleaner-code-quality
- GitHub Check: get-base-branch-fe-cov
- GitHub Check: get-base-branch-countries-cov
- GitHub Check: get-base-branch-contricleaner-cov
- GitHub Check: get-base-branch-dd-cov
- GitHub Check: get-base-branch-django-cov
🔇 Additional comments (2)
src/react/src/util/util.js (2)
26-26: LGTM!The new lodash import is correctly placed and necessary for the
isRequiredFieldValidfunction.
1491-1491: LGTM! Simple and effective validation function.The function is well-named, concise, and correctly handles whitespace validation for required fields.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/react/src/components/Contribute/ProductionLocationInfo.jsx (3)
153-161: Consider debouncing blur handlers for better performance.The blur handlers are simple state setters now, but if validation logic grows more complex, consider debouncing them to prevent excessive re-renders.
-const handleNameBlur = () => { - setNameTouched(true); -}; -const handleAddressBlur = () => { - setAddressTouched(true); -}; -const handleCountryBlur = () => { - setCountryTouched(true); -}; +const handleBlur = useCallback( + debounce((setter) => { + setter(true); + }, 150), + [] +); + +const handleNameBlur = () => handleBlur(setNameTouched); +const handleAddressBlur = () => handleBlur(setAddressTouched); +const handleCountryBlur = () => handleBlur(setCountryTouched);
430-462: Consider adding aria-required attribute for better accessibility.While the visual indicators for required fields are present, adding aria-required would improve accessibility for screen readers.
<TextField id="name" className={classes.textInputStyles} value={inputName} onChange={handleNameChange} onBlur={handleNameBlur} placeholder="Enter the name" variant="outlined" + aria-required="true" aria-label="Enter the name" // ... rest of the props />Also applies to: 483-517, 538-555
104-104: Consider showing error state immediately for prefilled invalid fields.When fields are prefilled via URL parameters (lines 87-89), the error state is only shown after blur. Consider showing errors immediately for prefilled invalid fields.
const [countryTouched, setCountryTouched] = useState(false); + +useEffect(() => { + if (nameInQuery) setNameTouched(true); + if (addressInQuery) setAddressTouched(true); + if (countryInQuery) setCountryTouched(true); +}, [nameInQuery, addressInQuery, countryInQuery]);Also applies to: 551-555
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/react/src/components/Contribute/ProductionLocationInfo.jsx(19 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/react/src/components/Contribute/ProductionLocationInfo.jsx (1)
Learnt from: VadimKovalenkoSNF
PR: opensupplyhub/open-supply-hub#470
File: src/react/src/components/Contribute/ProductionLocationInfo.jsx:680-692
Timestamp: 2025-01-23T11:10:17.929Z
Learning: The ProductionLocationInfo component in Open Supply Hub implements field-level validation for name, address and country fields using Material-UI's TextField validation patterns and touch states, providing real-time feedback to users.
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: run-flake8-linter
- GitHub Check: run-eslint-linter-and-prettier-formatter
- GitHub Check: run-dd-code-quality
- GitHub Check: run-integration-test-code-quality
- GitHub Check: run-countries-code-quality
- GitHub Check: run-contricleaner-code-quality
- GitHub Check: get-base-branch-dd-cov
- GitHub Check: get-base-branch-countries-cov
- GitHub Check: get-base-branch-contricleaner-cov
- GitHub Check: get-base-branch-django-cov
- GitHub Check: run-django-code-quality
- GitHub Check: run-fe-code-quality
- GitHub Check: get-base-branch-fe-cov
🔇 Additional comments (3)
src/react/src/components/Contribute/ProductionLocationInfo.jsx (3)
45-46: LGTM! Form validation logic is well-implemented.The validation logic correctly checks all required fields and number of workers before enabling form submission.
Also applies to: 163-168
416-420: LGTM! Required field indicators are consistently implemented.The asterisk indicators are properly added to all mandatory fields (Name, Address, Country) as specified in the PR objectives.
Also applies to: 471-475, 526-530
808-826: LGTM! Submit button state is properly managed.The submit button's disabled state is correctly tied to form validation, preventing submission of invalid data.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/react/src/__tests__/components/ProductionLocationInfo.test.js (3)
8-30: Consider enhancing the StyledSelect mock implementation.While the mock implementation is functional, consider these improvements for better maintainability and accessibility:
- Move the mock to a shared test utilities file for reuse across test suites
- Add aria-label support for better accessibility testing
jest.mock("../../components/Filters/StyledSelect", () => (props) => { - const { options = [], value, onChange, onBlur, placeholder } = props; + const { options = [], value, onChange, onBlur, placeholder, "aria-label": ariaLabel } = props; return ( <select data-testid="mocked-select" value={value ? value.value : ""} + aria-label={ariaLabel} onChange={(e) => { const selectedOption = options.find( (opt) => opt.value === e.target.value,
32-59: LGTM! Well-structured test setup.The test setup with default state and renderComponent utility provides a solid foundation for the test suite.
Consider adding more test data to defaultState.filterOptions for comprehensive testing:
const defaultState = { filterOptions: { countries: { - data: [{ value: "US", label: "United States" }], + data: [ + { value: "US", label: "United States" }, + { value: "GB", label: "United Kingdom" }, + { value: "FR", label: "France" } + ], error: null, fetching: false, },
92-111: Make error message assertions more specific.While the test correctly verifies the presence of error messages, consider making the assertions more specific to each field:
- const nameError = getAllByText("This field is required."); - expect(nameError).toHaveLength(3); + const requiredErrors = getAllByText("This field is required."); + expect(requiredErrors).toHaveLength(3); + + // Verify error messages are associated with correct fields + const nameLabel = getByText("Location Name"); + const addressLabel = getByText("Address"); + const countryLabel = getByText("Country"); + + expect(nameLabel.nextSibling).toHaveTextContent("This field is required."); + expect(addressLabel.nextSibling).toHaveTextContent("This field is required."); + expect(countryLabel.nextSibling).toHaveTextContent("This field is required.");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/react/src/__tests__/components/ProductionLocationInfo.test.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: run-integration-test-code-quality
- GitHub Check: run-flake8-linter
- GitHub Check: run-fe-code-quality
- GitHub Check: run-eslint-linter-and-prettier-formatter
- GitHub Check: run-django-code-quality
- GitHub Check: run-dd-code-quality
- GitHub Check: run-countries-code-quality
- GitHub Check: run-contricleaner-code-quality
- GitHub Check: get-base-branch-fe-cov
- GitHub Check: get-base-branch-django-cov
- GitHub Check: get-base-branch-countries-cov
- GitHub Check: get-base-branch-dd-cov
- GitHub Check: get-base-branch-contricleaner-cov
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/react/src/components/Contribute/SearchByNameAndAddressTab.jsx (1)
121-164: Consider extracting repeated validation logic and standardizing error handling structure.While the validation implementation is correct, there's some inconsistency and repetition in the error handling structure. Consider these improvements:
- Extract the validation check into a memoized variable for each field
- Standardize the error handling structure between name and address fields (remove extra parentheses in the address field's helper text)
Example refactor for the name field (apply similar pattern for address):
+ const isNameInvalid = nameTouched && !isRequiredFieldValid(inputName); + <TextField // ... other props ... - helperText={ - nameTouched && - !isRequiredFieldValid(inputName) && <InputErrorText /> - } - error={nameTouched && !isRequiredFieldValid(inputName)} + helperText={isNameInvalid && <InputErrorText />} + error={isNameInvalid} InputProps={{ classes: { input: `${classes.searchInputStyles} ${ - nameTouched && - !isRequiredFieldValid(inputName) && + isNameInvalid && classes.errorStyle }`,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/react/src/components/Contribute/SearchByNameAndAddressTab.jsx(4 hunks)src/react/src/util/styles.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/react/src/util/styles.js
🧰 Additional context used
🧠 Learnings (1)
src/react/src/components/Contribute/SearchByNameAndAddressTab.jsx (1)
Learnt from: mazursasha1990
PR: opensupplyhub/open-supply-hub#509
File: src/react/src/components/Contribute/SearchByNameAndAddressTab.jsx:78-79
Timestamp: 2025-02-13T17:37:02.545Z
Learning: The SearchByNameAndAddressTab component in Open Supply Hub does not require special character validation for input fields. Simple non-empty string validation after trimming is sufficient.
🔇 Additional comments (2)
src/react/src/components/Contribute/SearchByNameAndAddressTab.jsx (2)
13-14: LGTM! Consistent implementation of required field indicators.The addition of the RequiredAsterisk component and its integration into FormFieldTitle provides a standardized way to highlight mandatory fields, which directly addresses the PR's objective.
Also applies to: 17-18, 20-25
76-80: LGTM! Validation logic is correctly implemented.The form validation logic appropriately uses the new
isRequiredFieldValidutility function while maintaining component-specific validation requirements. This aligns with our previous discussions and the established validation approach.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/react/src/components/Contribute/ProductionLocationInfo.jsx (1)
154-169: Consider enhancing form validation UX.While the current validation on blur works, consider showing all validation errors when the user attempts to submit the form. This would provide a better user experience by showing all issues at once rather than discovering them one by one.
const isFormValid = !!( isRequiredFieldValid(inputName) && isRequiredFieldValid(inputAddress) && inputCountry?.value && isValidNumberOfWorkers(numberOfWorkers) ); +const handleSubmit = () => { + setNameTouched(true); + setAddressTouched(true); + setCountryTouched(true); + + if (isFormValid) { + handleProductionLocation(inputData, osID); + } +};
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/react/src/components/Contribute/ProductionLocationInfo.jsx(19 hunks)src/react/src/util/styles.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/react/src/util/styles.js
🧰 Additional context used
🧠 Learnings (1)
src/react/src/components/Contribute/ProductionLocationInfo.jsx (1)
Learnt from: VadimKovalenkoSNF
PR: opensupplyhub/open-supply-hub#470
File: src/react/src/components/Contribute/ProductionLocationInfo.jsx:680-692
Timestamp: 2025-01-23T11:10:17.929Z
Learning: The ProductionLocationInfo component in Open Supply Hub implements field-level validation for name, address and country fields using Material-UI's TextField validation patterns and touch states, providing real-time feedback to users.
🔇 Additional comments (3)
src/react/src/components/Contribute/ProductionLocationInfo.jsx (3)
45-45: LGTM! Well-structured form validation setup.The implementation follows React form validation best practices by tracking field touch states and computing error states.
Also applies to: 97-97, 105-105
417-547: LGTM! Clear and consistent required field indicators.The addition of asterisks and error messages provides clear visual feedback to users about required fields and validation states.
814-814: LGTM! Proper form submission control.The submit button is correctly disabled when required fields are invalid, preventing submission of incomplete data.
…rs-but-no-field-is-highlighted
|



OSDEV-1678 - SLC. new location - "The request body is invalid" appears but no field is highlighted.
In this PR, the following changes were implemented: