-
Notifications
You must be signed in to change notification settings - Fork 1
No-Jira - Disable Continue on Setup Step until values filled by user #1722
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,10 @@ | ||
| import React from 'react'; | ||
| import { SectionList } from 'src/components/Reports/GoalCalculator/SharedComponents/SectionList'; | ||
| import { DirectionButtons } from 'src/components/Reports/Shared/CalculationReports/DirectionButtons/DirectionButtons'; | ||
| import { | ||
| AutosaveForm, | ||
| useAutosaveForm, | ||
| } from 'src/components/Shared/Autosave/AutosaveForm'; | ||
| import { PdsGoalCalculatorStepEnum } from './PdsGoalCalculatorHelper'; | ||
| import { ReimbursableExpensesStep } from './ReimbursableExpenses/ReimbursableExpensesStep'; | ||
| import { SalaryStep } from './Salary/SalaryStep'; | ||
|
|
@@ -24,27 +28,38 @@ const CurrentStep: React.FC = () => { | |
| } | ||
| }; | ||
|
|
||
| export const PdsGoalCalculator: React.FC = () => { | ||
| const StepContent: React.FC = () => { | ||
| const { currentStep, stepIndex, steps, handleContinue, handlePreviousStep } = | ||
| usePdsGoalCalculator(); | ||
| const sections = currentStep.sections; | ||
|
|
||
| const { allValid } = useAutosaveForm(); | ||
| const isLastStep = stepIndex === steps.length - 1; | ||
|
|
||
| return ( | ||
| <> | ||
| <CurrentStep /> | ||
| {!isLastStep && ( | ||
| <DirectionButtons | ||
| formTitle={currentStep.title} | ||
| handleNextStep={handleContinue} | ||
| handlePreviousStep={handlePreviousStep} | ||
| disableNext={!allValid} | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
[Medium] **Loading-state flicker.** `useAutosaveForm()` starts with `invalidFields = []` → `allValid = true`. Child fields then mount, run their validity effects, and `allValid` flips to `false`. On first render (or slow Apollo load) Continue briefly shows enabled, then disabled.
Suggested: include the loading gate in the disabled check. const { currentStep, stepIndex, steps, handleContinue, handlePreviousStep, calculation } =
usePdsGoalCalculator();
...
disableNext={!calculation || !allValid}The Autocomplete at |
||
| /> | ||
| )} | ||
| </> | ||
| ); | ||
| }; | ||
|
|
||
| export const PdsGoalCalculator: React.FC = () => { | ||
| const { currentStep } = usePdsGoalCalculator(); | ||
| const sections = currentStep.sections; | ||
|
|
||
| return ( | ||
| <PdsGoalCalculatorLayout | ||
| sectionListPanel={<SectionList sections={sections} />} | ||
| mainContent={ | ||
| <> | ||
| <CurrentStep /> | ||
| {!isLastStep && ( | ||
| <DirectionButtons | ||
| formTitle={currentStep.title} | ||
| handleNextStep={handleContinue} | ||
| handlePreviousStep={handlePreviousStep} | ||
| /> | ||
| )} | ||
| </> | ||
| <AutosaveForm> | ||
| <StepContent /> | ||
| </AutosaveForm> | ||
| } | ||
| /> | ||
| ); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| import React, { useMemo } from 'react'; | ||
| import React, { useEffect, useMemo } from 'react'; | ||
| import CalculateIcon from '@mui/icons-material/Calculate'; | ||
| import { | ||
| Autocomplete, | ||
|
|
@@ -21,6 +21,7 @@ | |
| CurrencyAdornment, | ||
| PercentageAdornment, | ||
| } from 'src/components/Reports/GoalCalculator/Shared/Adornments'; | ||
| import { useOptionalAutosaveForm } from 'src/components/Shared/Autosave/AutosaveForm'; | ||
| import { useGetUserQuery } from 'src/components/User/GetUser.generated'; | ||
| import { | ||
| DesignationSupportSalaryType, | ||
|
|
@@ -47,31 +48,52 @@ | |
| () => | ||
| yup.object({ | ||
| name: yup.string().required(t('Goal Name is a required field')), | ||
| status: yup.string().nullable(), | ||
| salaryOrHourly: yup.string().nullable(), | ||
| status: yup | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
[Suggestion] Verify server-side parity: the new `required()` rules are client-only enforcement. If `DesignationSupportCalculationUpdateInput` (or the REST proxy) doesn't also reject missing values, a stale client or alternate entry path could still submit incomplete data. Not blocking — the same fields were `.nullable()` before this PR, so no new risk — but good to confirm.
|
||
| .string() | ||
| .required(t('Employment Status is a required field')), | ||
| salaryOrHourly: yup | ||
| .string() | ||
| .required(t('Pay Type is a required field')), | ||
| payRate: yup | ||
| .number() | ||
| .nullable() | ||
| .required(t('Pay Rate is a required field')) | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
[Suggestion] `yup.number().required().min(0)` accepts literal `0`. For a staff salary, `$0/yr` isn't meaningful. Consider `.moreThan(0, t('Pay Rate must be greater than zero'))` for `payRate` and also for `hoursWorkedPerWeek` (line 63). `benefits` can stay `.min(0)` since zero benefits is legitimate. Out of scope for the Continue-button work — file for follow-up if desired.
|
||
| .min(0, t('Pay Rate must be a positive number')), | ||
| hoursWorkedPerWeek: yup | ||
| .number() | ||
| .nullable() | ||
| .required(t('Hours Worked is a required field')) | ||
| .min(0, t('Hours Worked must be a positive number')), | ||
| benefits: yup | ||
| .number() | ||
| .nullable() | ||
| .required(t('Benefits is a required field')) | ||
| .min(0, t('Benefits must be a positive number')), | ||
| geographicLocation: yup | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
[Medium] **Dead schema rule + unprecedented pattern.** The `geographicLocation: yup.string().required(...)` rule you're adding here is never validated against — the runtime check is a manual `useEffect` at line 85 that only tests truthiness. And calling `markValid`/`markInvalid` from a component's useEffect is a new pattern (no other callers in the repo outside `useAutosave.ts`). SalaryCalculator has an `AutosaveAutocomplete` wrapper (`src/components/Reports/SalaryCalculator/Autosave/AutosaveAutocomplete.tsx`) that would be the cleaner precedent.
Options:
Flagged by Architecture and Standards agents. |
||
| .string() | ||
| .required(t('Geographic Multiplier is a required field')), | ||
| }), | ||
| [t], | ||
| ); | ||
| const { goalGeographicConstantMap } = useGoalCalculatorConstants(); | ||
| const saveField = useSaveField(); | ||
|
|
||
| const locations = useMemo( | ||
| () => Array.from(goalGeographicConstantMap.keys()), | ||
| [goalGeographicConstantMap], | ||
| ); | ||
|
|
||
| const autosaveForm = useOptionalAutosaveForm(); | ||
| const geographicLocationValue = calculation?.geographicLocation; | ||
| useEffect(() => { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
[Important] **Geographic Multiplier UX trap.** The Autocomplete (line 259 in this file) uses `value={calculation?.geographicLocation ?? 'None'}`, which displays `'None'` (a real option in `mpdGoalGeographicConstants`) whenever the underlying value is `null`. After this PR makes the field required, the user sees `'None'` as if selected, but this `useEffect` fires `markInvalid` and Continue is disabled — with no error state on the Autocomplete. Other fields show red helperText via AutosaveTextField, but this one bypasses that path.
Suggested fix: <Autocomplete
options={locations}
value={calculation?.geographicLocation ?? null}
onChange={(_, newValue) => saveField({ geographicLocation: newValue })}
disabled={!calculation}
size="small"
renderInput={(params) => (
<TextField
{...params}
label={t('Geographic Multiplier')}
required
error={!!calculation && !calculation.geographicLocation}
helperText={
!!calculation && !calculation.geographicLocation
? t('Geographic Multiplier is a required field')
: t('If not applicable, select "None"')
}
/>
)}
/>Passing |
||
| if (!autosaveForm) { | ||
| return; | ||
| } | ||
| if (geographicLocationValue) { | ||
| autosaveForm.markValid('geographicLocation'); | ||
| } else { | ||
| autosaveForm.markInvalid('geographicLocation'); | ||
| } | ||
| return () => autosaveForm.markValid('geographicLocation'); | ||
| }, [autosaveForm, geographicLocationValue]); | ||
|
|
||
|
Check warning on line 96 in src/components/Reports/PdsGoalCalculator/Setup/SetupStep.tsx
|
||
| const isSalaried = | ||
| calculation?.salaryOrHourly === DesignationSupportSalaryType.Salaried; | ||
| const isPartTime = calculation?.status === DesignationSupportStatus.PartTime; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,6 +25,7 @@ interface TestComponentProps { | |
| showBackButton?: boolean; | ||
| buttonTitle?: string; | ||
| isEdit?: boolean; | ||
| disableNext?: boolean; | ||
| } | ||
|
|
||
| const TestComponent: React.FC<TestComponentProps> = ({ | ||
|
|
@@ -33,6 +34,7 @@ const TestComponent: React.FC<TestComponentProps> = ({ | |
| showBackButton = false, | ||
| buttonTitle, | ||
| isEdit, | ||
| disableNext, | ||
| }) => ( | ||
| <ThemeProvider theme={theme}> | ||
| <TestRouter | ||
|
|
@@ -54,6 +56,7 @@ const TestComponent: React.FC<TestComponentProps> = ({ | |
| showBackButton={showBackButton} | ||
| buttonTitle={buttonTitle} | ||
| isEdit={isEdit} | ||
| disableNext={disableNext} | ||
| /> | ||
| </MinisterHousingAllowanceProvider> | ||
| </Formik> | ||
|
|
@@ -114,6 +117,24 @@ describe('DirectionButtons', () => { | |
| expect(await findByRole('button', { name: title })).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it('disables Continue when disableNext is true', async () => { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
[Medium] **Missing regression test for Submit branch.** All three new `disableNext` tests exercise the Continue branch (default `isSubmission=false`). The code is correct — `disabled={disableNext}` lives only in the `!isSubmission` branch — but a test would pin that contract. A refactor that accidentally applies `disabled={disableNext}` to the Submit branch wouldn't be caught here.
it('does not disable Submit even when disableNext is true', async () => {
const { findByRole } = render(
<TestComponent isSubmission={true} disableNext={true} />,
);
expect(await findByRole('button', { name: /submit/i })).toBeEnabled();
}); |
||
| const { findByRole } = render(<TestComponent disableNext={true} />); | ||
|
|
||
| expect(await findByRole('button', { name: 'Continue' })).toBeDisabled(); | ||
| }); | ||
|
|
||
| it('enables Continue when disableNext is false', async () => { | ||
| const { findByRole } = render(<TestComponent disableNext={false} />); | ||
|
|
||
| expect(await findByRole('button', { name: 'Continue' })).toBeEnabled(); | ||
| }); | ||
|
|
||
| it('leaves Continue enabled when disableNext is not provided', async () => { | ||
| const { findByRole } = render(<TestComponent />); | ||
|
|
||
| expect(await findByRole('button', { name: 'Continue' })).toBeEnabled(); | ||
| }); | ||
|
|
||
| it('renders Discard button', async () => { | ||
| const { findByRole } = render(<TestComponent />); | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,6 +21,7 @@ | |
| additionalApproval?: boolean; | ||
| splitAsr?: boolean; | ||
| disableSubmit?: boolean; | ||
| disableNext?: boolean; | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
[Medium] **Prop bloat.** This shared component now carries 24 props covering three distinct use cases (MHA formik submit, ASR formik submit, PDS autosave continue). Continue uses `disableNext`, Submit uses `submitCount ? !isValid : false`, and `disableSubmit` affects the SubmitModal. The next contributor who wants PDS to also show validity on a submission step, or ASR to disable Continue while invalid, will have to add yet another combination.
Follow-up refactor idea (not blocking this PR): consolidate to a single |
||
| //Formik validation for submit modal | ||
| isSubmission?: boolean; | ||
| submitForm?: () => Promise<void>; | ||
|
|
@@ -51,96 +52,98 @@ | |
| additionalApproval, | ||
| splitAsr, | ||
| disableSubmit, | ||
| disableNext, | ||
| }) => { | ||
| const { t } = useTranslation(); | ||
|
|
||
| const [openSubmitModal, setOpenSubmitModal] = useState(false); | ||
| const [openDiscardModal, setOpenDiscardModal] = useState(false); | ||
|
|
||
| const handleSubmit = async () => { | ||
| if (!submitForm || !validateForm) { | ||
| return; | ||
| } | ||
|
|
||
| const errors = await validateForm(); | ||
| if (Object.keys(errors).length === 0) { | ||
| setOpenSubmitModal(true); | ||
| } else { | ||
| submitForm(); | ||
| } | ||
| }; | ||
|
|
||
| const handleConfirm = async () => { | ||
| if (submitForm) { | ||
| try { | ||
| await submitForm(); | ||
| } catch { | ||
| return; | ||
| } | ||
| } | ||
| setOpenSubmitModal(false); | ||
| }; | ||
|
|
||
| const handleDiscardConfirm = () => { | ||
| if (handleDiscard) { | ||
| handleDiscard(); | ||
| } | ||
| setOpenDiscardModal(false); | ||
| }; | ||
|
|
||
| return ( | ||
| <Box | ||
| sx={{ | ||
| mt: 5, | ||
| display: 'flex', | ||
| justifyContent: | ||
| !handleDiscard && !showBackButton && !isSubmission | ||
| ? 'flex-end' | ||
| : 'space-between', | ||
| }} | ||
| > | ||
| {handleDiscard && ( | ||
| <Button | ||
| sx={{ color: 'error.light', px: 2, py: 1, fontWeight: 'bold' }} | ||
| onClick={() => setOpenDiscardModal(true)} | ||
| > | ||
| {isEdit ? t('Discard Changes') : t('Discard')} | ||
| </Button> | ||
| )} | ||
|
|
||
| <Box sx={{ display: 'flex', gap: 2 }}> | ||
| {showBackButton && ( | ||
| <Button | ||
| variant="contained" | ||
| sx={{ | ||
| bgcolor: 'grey.300', | ||
| color: 'text.primary', | ||
| '&:hover': { | ||
| bgcolor: 'grey.400', | ||
| }, | ||
| fontWeight: 'bold', | ||
| }} | ||
| onClick={handlePreviousStep} | ||
| > | ||
| <ChevronLeft sx={{ mr: 1 }} /> | ||
| {t('Back')} | ||
| </Button> | ||
| )} | ||
| {isSubmission ? ( | ||
| <Button | ||
| variant="contained" | ||
| color="primary" | ||
| onClick={handleSubmit} | ||
| disabled={submitCount ? !isValid : false} | ||
| > | ||
| {t('Submit')} | ||
| <ChevronRight sx={{ ml: 1 }} /> | ||
| </Button> | ||
| ) : ( | ||
| <Button | ||
| variant="contained" | ||
| color="primary" | ||
| onClick={overrideNext ?? handleNextStep} | ||
| disabled={disableNext} | ||
|
Check warning on line 146 in src/components/Reports/Shared/CalculationReports/DirectionButtons/DirectionButtons.tsx
|
||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
[Medium] **No feedback on why Continue is disabled.** `disabled={disableNext}` alone gives users no explanation once they've noticed they can't advance. After IMP-1 is fixed, field-level errors will explain most cases, but a tooltip on the button is still the clearest affordance.
Suggested: <Tooltip title={disableNext ? t('Complete all required fields to continue') : ''}>
<span>
<Button
variant="contained"
color="primary"
onClick={overrideNext ?? handleNextStep}
disabled={disableNext}
aria-describedby={disableNext ? 'continue-disabled-reason' : undefined}
>
{buttonTitle ?? t('Continue')}
<ChevronRight sx={{ ml: 1 }} />
</Button>
</span>
</Tooltip>(The Note: SalaryCalculator's ContinueButton has the same gap — fixing it here improves both. |
||
| > | ||
| {buttonTitle ?? t('Continue')} | ||
| <ChevronRight sx={{ ml: 1 }} /> | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.