fix: add SC validation and update error message #4693
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates validation messages and adds Services Component (SC) validation for budget line items in the agreement review process. The changes improve error messaging clarity and enhance validation logic for budget line item fields.
- Updated error messages from "This is required information" to "This information is required to submit for approval"
- Refactored budget line item validation to group by field type rather than checking all fields per item
- Added dynamic tooltip messages for non-actionable budget lines based on action type and status
Reviewed changes
Copilot reviewed 13 out of 15 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/pages/agreements/review/suite.js | Updated validation error messages and restructured budget line validation to be field-specific |
| frontend/src/pages/agreements/review/ReviewAgreement.jsx | Removed bold styling from validation errors and added action prop to BLI table |
| frontend/src/pages/agreements/review/ReviewAgreement.hooks.js | Updated filter to match new validation test names |
| frontend/src/helpers/utils.js | Updated JSDoc to indicate dateNeeded parameter can be null |
| frontend/src/components/UI/USWDS/Tooltip/Tooltip.jsx | Improved tooltip reinitialization logic and added cleanup for empty labels |
| frontend/src/components/UI/Term/Term.jsx | Added text-normal class to error messages |
| frontend/src/components/BudgetLineItems/CreateBLIsAndSCs/CreateBLIsAndSCs.hooks.js | Removed unused workflow variable from dependency array |
| frontend/src/components/BudgetLineItems/BudgetLinesTable/BLIRow.helpers.js | Updated JSDoc to indicate item parameter can be null |
| frontend/src/components/BudgetLineItems/BLIReviewTable/BLIReviewTable.jsx | Added action prop and fixed typo in class name |
| frontend/src/components/BudgetLineItems/BLIReviewTable/BLIReviewTable.constants.js | Added BUDGET_LINE_STATUSES constant |
| frontend/src/components/BudgetLineItems/BLIReviewTable/BLIReviewRow.jsx | Refactored checkbox rendering with dynamic tooltips and improved null handling |
| frontend/sass/uswds/styles.scss | Added styling for disabled checkbox labels |
| frontend/cypress/e2e/createAgreementWithValidations.cy.js | Updated test to expect 4 validation errors instead of 2 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
| test("Budget Line Amount", () => { | ||
| enforce(item.amount).greaterThan(0); | ||
| }); | ||
|
|
||
| test(`Budget line item (${item.id}) must be in the future`, () => { | ||
| test("Budget Line CAN", () => { | ||
| enforce(item.can_id).isNotBlank(); | ||
| }); | ||
|
|
||
| test("Budget Line Services Component", () => { | ||
| enforce(item.services_component_id).isNotBlank(); | ||
| }); | ||
|
|
||
| test("Budget Line Obligate By Date", () => { | ||
| enforce(item.date_needed).isNotBlank(); | ||
| }); |
There was a problem hiding this comment.
The new validation test names don't include clear error messages. Unlike other tests in the file that include helpful messages like "This information is required to submit for approval", these tests have no second parameter to provide user-facing error messages.
| formatDateNeeded(budgetLine?.date_needed ?? null) === NO_DATA | ||
| ? null | ||
| : formatDateNeeded(budgetLine?.date_needed ?? null), |
There was a problem hiding this comment.
The formatDateNeeded function is called twice with the same arguments. Store the result in a variable to avoid redundant computation.
|
|
||
| const labelContent = ( | ||
| <label | ||
| className={`usa-checkbox__label ${isDisabled ? 'text-gray-50 checkbox-disabled' : ''}`} |
There was a problem hiding this comment.
String literals should use double quotes consistently with the rest of the codebase. Replace single quotes with double quotes for 'text-gray-50 checkbox-disabled' and the empty string.
| className={`usa-checkbox__label ${isDisabled ? 'text-gray-50 checkbox-disabled' : ''}`} | |
| className={`usa-checkbox__label ${isDisabled ? "text-gray-50 checkbox-disabled" : ""}`} |
| <label | ||
| className={`usa-checkbox__label ${isDisabled ? 'text-gray-50 checkbox-disabled' : ''}`} | ||
| htmlFor={checkboxId} | ||
| style={isDisabled ? { cursor: 'not-allowed' } : undefined} |
There was a problem hiding this comment.
String literal should use double quotes to match codebase conventions. Replace 'not-allowed' with "not-allowed".
| style={isDisabled ? { cursor: 'not-allowed' } : undefined} | |
| style={isDisabled ? { cursor: "not-allowed" } : undefined} |
What changed
Described what changes in this PR, at a high level
Issue
This PR closes #4689
How to test
Write out steps for how someone could test this PR against the acceptance criteria
Screenshots
If relevant, e.g. for a front-end feature
Definition of Done Checklist
Links
If relevant, e.g. for a link to a piece of markdown documentation