[OSDEV-2200] [Claims Process Update] Implement new claim intro page#772
[OSDEV-2200] [Claims Process Update] Implement new claim intro page#772roman-stolar merged 21 commits intomainfrom
Conversation
roman-stolar
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roman-stolar
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roman-stolar
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roman-stolar
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roman-stolar
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roman-stolar
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roman-stolar
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roman-stolar
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roman-stolar
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roman-stolar
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roman-stolar
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roman-stolar
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roman-stolar
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roman-stolar
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roman-stolar
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
React App | Jest test suite - Code coverage reportTotal: 35.32%Your code coverage diff: 0.23% ▴ ✅ All code changes are covered |
roman-stolar
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Dedupe Hub App | Unittest test suite - Code coverage reportTotal: 55.73%Your code coverage diff: 0.00% ▴ ✅ All code changes are covered |
roman-stolar
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Countries App | Unittest test suite - Code coverage reportTotal: 100%Your code coverage diff: 0.00% ▴ ✅ All code changes are covered |
roman-stolar
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roman-stolar
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roman-stolar
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
📝 WalkthroughWalkthroughThis PR adds Release 2.15.0 notes and implements a gated claim-introduction flow (OSDEV-2200) at Changes
Sequence DiagramsequenceDiagram
participant User
participant Router
participant FeatureFlag as FF
participant ClaimIntro
participant Auth
participant ClaimInfoSection as Info
User->>Router: GET /claim/:osId
Router->>FF: is ENABLE_V1_CLAIMS_FLOW enabled?
alt Flag disabled
FF->>Router: render Facilities route
else Flag enabled
FF->>ClaimIntro: render ClaimIntro
ClaimIntro->>Auth: check userHasSignedIn
alt Not signed in
Auth-->>ClaimIntro: false
ClaimIntro-->>User: show RequireAuthNotice (prompt to sign in)
else Signed in
Auth-->>ClaimIntro: true
ClaimIntro->>Info: render ClaimInfoSection
ClaimIntro-->>User: show actions (Back, Continue)
User->>ClaimIntro: Click Continue
ClaimIntro->>Router: navigate to /claim/:osId/details (makeClaimDetailsLink)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Reason: multiple new components, Redux and router integration, feature-flagged route, UI styles, expanded shared constants, and comprehensive tests — heterogeneous changes requiring cross-file validation. Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/react/src/util/constants.jsx (1)
153-153: Typo breaks profile field mapping (runtime bug).registrationFieldsEnum.wesbite is misspelled; this silently drops the website field key.
Apply this fix:
- [registrationFieldsEnum.wesbite]: registrationFieldsEnum.website, + [registrationFieldsEnum.website]: registrationFieldsEnum.website,
🧹 Nitpick comments (5)
src/react/src/util/constants.jsx (1)
21-22: External link looks good; consider centralizing path token.Hard-coding the path is fine, but consider adding an InfoPaths entry (e.g., claimFacility) and building this as
${InfoLink}/${InfoPaths.claimFacility}to avoid future drift.src/react/src/__tests__/components/ClaimIntro.test.js (1)
95-105: Make “GO BACK” assertion robust.Asserting pathname equals the pre-click path is brittle; verify goBack() was called instead.
Apply:
- test('navigates back when GO BACK button is clicked', () => { - history.push('/some-page'); - const previousPath = history.location.pathname; - - const { getByText } = renderComponent(); - const backButton = getByText('GO BACK'); - - fireEvent.click(backButton); - - expect(history.location.pathname).toBe(previousPath); - }); + test('navigates back when GO BACK button is clicked', () => { + history.push('/some-page'); + const goBackSpy = jest.spyOn(history, 'goBack'); + + const { getByText } = renderComponent(); + const backButton = getByText('GO BACK'); + + fireEvent.click(backButton); + + expect(goBackSpy).toHaveBeenCalledTimes(1); + goBackSpy.mockRestore(); + });src/react/src/__tests__/components/ClaimInfoSection.test.js (1)
179-189: Also verify rel on external link.Add a check for rel="noopener noreferrer" to harden the test against regressions.
Apply:
- expect(link).toHaveAttribute('target', '_blank'); + expect(link).toHaveAttribute('target', '_blank'); + expect(link).toHaveAttribute('rel', 'noopener noreferrer');src/react/src/components/V1Claim/ClaimIntro.jsx (1)
202-216: Defensive auth mapping.Avoid destructuring crashes when user is null; compute sign-in status safely.
Apply:
- userHasSignedIn: !user.isAnon, + userHasSignedIn: Boolean(user) && !user.isAnon,src/react/src/components/V1Claim/ClaimInfoSection.jsx (1)
221-228: Add accessible labels to the dialog and close button.Improve screen-reader support with aria labels.
Apply:
- <Dialog open={open} onClose={onClose} maxWidth="lg" fullWidth> - <IconButton className={classes.closeButton} onClick={onClose}> + <Dialog + open={open} + onClose={onClose} + maxWidth="lg" + fullWidth + aria-label="Example image preview" + > + <IconButton + aria-label="Close image preview" + className={classes.closeButton} + onClick={onClose} + > <CloseIcon /> </IconButton>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
src/react/src/images/business-card-example.jpgis excluded by!**/*.jpgsrc/react/src/images/business-license-example.jpgis excluded by!**/*.jpgsrc/react/src/images/business-registration-example.jpgis excluded by!**/*.jpgsrc/react/src/images/employee-id-example.jpgis excluded by!**/*.jpgsrc/react/src/images/employment-letter-example.jpgis excluded by!**/*.jpgsrc/react/src/images/utility-bill-example.jpgis excluded by!**/*.jpg
📒 Files selected for processing (9)
doc/release/RELEASE-NOTES.md(1 hunks)src/react/src/Routes.jsx(3 hunks)src/react/src/__tests__/components/ClaimInfoSection.test.js(1 hunks)src/react/src/__tests__/components/ClaimIntro.test.js(1 hunks)src/react/src/components/V1Claim/ClaimInfoSection.jsx(1 hunks)src/react/src/components/V1Claim/ClaimIntro.jsx(1 hunks)src/react/src/util/COLOURS.js(1 hunks)src/react/src/util/constants.jsx(2 hunks)src/react/src/util/util.js(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-01-17T16:12:18.285Z
Learnt from: Innavin369
PR: opensupplyhub/open-supply-hub#483
File: src/react/src/__tests__/components/SearchByNameAndAddressSuccessResult.test.js:0-0
Timestamp: 2025-01-17T16:12:18.285Z
Learning: In the SearchByNameAndAddressSuccessResult component's tests, attempting to test internal navigation logic through mocking useHistory is not feasible, and button presence/click events should be covered in the main rendering test case.
Applied to files:
src/react/src/__tests__/components/ClaimIntro.test.js
🧬 Code graph analysis (5)
src/react/src/Routes.jsx (2)
src/react/src/util/constants.jsx (4)
claimIntroRoute(343-343)claimIntroRoute(343-343)ENABLE_V1_CLAIMS_FLOW(579-579)ENABLE_V1_CLAIMS_FLOW(579-579)src/react/src/components/V1Claim/ClaimIntro.jsx (1)
ClaimIntro(122-190)
src/react/src/__tests__/components/ClaimIntro.test.js (2)
src/react/src/util/testUtils/renderWithProviders.jsx (1)
renderWithProviders(6-22)src/react/src/components/V1Claim/ClaimIntro.jsx (1)
ClaimIntro(122-190)
src/react/src/__tests__/components/ClaimInfoSection.test.js (1)
src/react/src/util/testUtils/renderWithProviders.jsx (1)
renderWithProviders(6-22)
src/react/src/components/V1Claim/ClaimInfoSection.jsx (1)
src/react/src/util/constants.jsx (2)
ClaimFacilityInfoLink(21-22)ClaimFacilityInfoLink(21-22)
src/react/src/components/V1Claim/ClaimIntro.jsx (3)
src/react/src/components/RequireAuthNotice.jsx (1)
RequireAuthNotice(9-29)src/react/src/util/util.js (2)
makeClaimDetailsLink(287-287)makeClaimDetailsLink(287-287)src/react/src/components/V1Claim/ClaimInfoSection.jsx (1)
ClaimInfoSection(294-517)
🪛 markdownlint-cli2 (0.18.1)
doc/release/RELEASE-NOTES.md
17-17: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
18-18: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: run-integration-test-code-quality
- GitHub Check: run-contricleaner-code-quality
- GitHub Check: run-countries-code-quality
- GitHub Check: run-dd-code-quality
🔇 Additional comments (6)
src/react/src/util/constants.jsx (1)
342-344: New claim routes LGTM.Names and shapes are clear and consistent with makeClaimDetailsLink. Ensure Routes.jsx wires both and the flag guards intro rendering.
src/react/src/__tests__/components/ClaimIntro.test.js (2)
19-36: Nice test harness.Preloaded auth state toggle is clear; injecting match.params osID keeps the component decoupled from route config.
107-115: Continue navigation assertion LGTM.Using makeClaimDetailsLink to derive the expected path is correct and keeps the test aligned with the helper.
src/react/src/__tests__/components/ClaimInfoSection.test.js (1)
148-177: Dialog interaction coverage is solid.Good verification of open state via role=dialog and image presence.
src/react/src/components/V1Claim/ClaimIntro.jsx (1)
140-189: Intro UI and handlers LGTM.Auth gate, layout, and navigation handlers are straightforward and readable.
src/react/src/components/V1Claim/ClaimInfoSection.jsx (1)
294-516: Content structure and styling read well.Clear steps, good alt text, secure external link; examples UX is nice.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
src/react/src/components/V1Claim/ClaimIntro.jsx (2)
170-178: Prefer declarative navigation for a11y and simplicity.Use a Link-backed Button instead of imperative history.push; removes a handler and improves semantics.
Apply:
-import { withRouter } from 'react-router-dom'; +import { withRouter, Link } from 'react-router-dom'; @@ - const handleContinue = () => { - history.push(makeClaimDetailsLink(osID)); - }; + // Declarative navigation preferred; no handler needed. @@ - <Button - variant="contained" - className={classes.continueButton} - onClick={handleContinue} - > + <Button + variant="contained" + className={classes.continueButton} + component={Link} + to={makeClaimDetailsLink(osID)} + disabled={!osID} + >Also applies to: 132-134, 5-7
142-151: Deprecated MUI v3 Typography variants present; optional refactor for future upgrades.The project uses @material-ui/core 3.1.0, which supports
variant="title"andvariant="subheading". These variants are deprecated in MUI v4+ and appear extensively throughout the codebase (48+ instances of "title", 30+ of "subheading"). While currently functional, they should be replaced withh5/h6/body1or variant mapping to prepare for potential future upgrades. Additionally, theme.spacing.unit is used 50+ times—another v3 pattern requiring updates for v4+ compatibility.src/react/src/components/V1Claim/ClaimInfoSection.jsx (2)
300-306: Review Typography variant usage for future compatibility."variant='title'" and "variant='subheading'" are specific to older MUI. If an upgrade is planned, adopt h5/h6/body1 or set a variant mapping.
Based on learnings.
Also applies to: 348-352, 400-406, 505-511
282-289: Tighten PropTypes to prevent invalid class keys; add lazy-loading.Restrict border/label class names and enable image lazy-loading to reduce initial cost.
Apply:
-ExampleImage.propTypes = { - src: PropTypes.string.isRequired, - alt: PropTypes.string.isRequired, - label: PropTypes.string.isRequired, - borderClass: PropTypes.string.isRequired, - labelColorClass: PropTypes.string.isRequired, - classes: PropTypes.object.isRequired, -}; +ExampleImage.propTypes = { + src: PropTypes.string.isRequired, + alt: PropTypes.string.isRequired, + label: PropTypes.string.isRequired, + borderClass: PropTypes.oneOf(['purpleBorder', 'greenBorder']).isRequired, + labelColorClass: PropTypes.oneOf(['purpleLabel', 'greenLabel']).isRequired, + classes: PropTypes.object.isRequired, +};And:
- <img + <img src={src} alt={alt} + loading="lazy" className={`${classes.exampleImage} ${classes[borderClass]}`} />Also applies to: 236-244, 262-267
src/react/src/util/COLOURS.js (2)
28-36: Confirm 8‑digit hex support or switch to rgba for wider compatibility.LIGHT_PURPLE uses #8428FA21 (with alpha). Modern browsers support it; if older browsers are in scope, prefer rgba(132, 40, 250, 0.13).
1-10: Consider mapping tokens into the MUI theme.For consistency (dark mode, contrast, theming), expose these via theme.palette and theme.custom rather than importing COLOURS directly. Keeps components palette‑aware.
Also applies to: 45-55
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/react/src/components/V1Claim/ClaimInfoSection.jsx(1 hunks)src/react/src/components/V1Claim/ClaimIntro.jsx(1 hunks)src/react/src/util/COLOURS.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/react/src/components/V1Claim/ClaimInfoSection.jsx (1)
src/react/src/util/constants.jsx (2)
ClaimFacilityInfoLink(21-22)ClaimFacilityInfoLink(21-22)
src/react/src/components/V1Claim/ClaimIntro.jsx (3)
src/react/src/components/RequireAuthNotice.jsx (1)
RequireAuthNotice(9-29)src/react/src/util/util.js (2)
makeClaimDetailsLink(287-287)makeClaimDetailsLink(287-287)src/react/src/components/V1Claim/ClaimInfoSection.jsx (1)
ClaimInfoSection(291-514)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: run-eslint-linter-and-prettier-formatter
- GitHub Check: run-flake8-linter
- GitHub Check: run-integration-test-code-quality
- GitHub Check: run-dd-code-quality
- GitHub Check: get-base-branch-contricleaner-cov
- GitHub Check: get-base-branch-dd-cov
- GitHub Check: run-contricleaner-code-quality
- GitHub Check: get-base-branch-fe-cov
- GitHub Check: run-django-code-quality
- GitHub Check: get-base-branch-countries-cov
- GitHub Check: get-base-branch-django-cov
- GitHub Check: run-countries-code-quality
- GitHub Check: run-fe-code-quality
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/react/src/components/V1Claim/ClaimInfoSection.jsx (1)
357-357: Use theme spacing instead of hard-coded pixel values.The inline styles use hard-coded values (
marginBottom: 16andmarginBottom: 8) instead oftheme.spacing.unit * 2andtheme.spacing.unit, which is inconsistent with the rest of the file and could cause issues with theme customization.Apply this pattern for consistency:
- style={{ color: COLOURS.PURPLE_TEXT, marginBottom: 16 }} + style={{ color: COLOURS.PURPLE_TEXT, marginBottom: theme.spacing.unit * 2 }}And:
- style={{ color: COLOURS.GREEN_TEXT, marginBottom: 8 }} + style={{ color: COLOURS.GREEN_TEXT, marginBottom: theme.spacing.unit }}However, this would require passing
themeas a prop or using a different styling approach. Given the codebase patterns, this is a minor consistency concern.Also applies to: 409-409
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/react/src/components/V1Claim/ClaimInfoSection.jsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/react/src/components/V1Claim/ClaimInfoSection.jsx (1)
src/react/src/util/constants.jsx (2)
ClaimFacilityInfoLink(21-22)ClaimFacilityInfoLink(21-22)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: run-flake8-linter
- GitHub Check: run-integration-test-code-quality
- GitHub Check: run-dd-code-quality
- GitHub Check: get-base-branch-countries-cov
- GitHub Check: run-contricleaner-code-quality
- GitHub Check: get-base-branch-contricleaner-cov
- GitHub Check: get-base-branch-dd-cov
- GitHub Check: run-countries-code-quality
- GitHub Check: run-eslint-linter-and-prettier-formatter
- GitHub Check: get-base-branch-django-cov
- GitHub Check: run-django-code-quality
- GitHub Check: get-base-branch-fe-cov
- GitHub Check: run-fe-code-quality
🔇 Additional comments (2)
src/react/src/components/V1Claim/ClaimInfoSection.jsx (2)
235-279: LGTM!The component correctly manages dialog visibility state and implements proper accessibility with an
aria-labelon the button. The use of inline styles for the unstyled button wrapper is acceptable here.
491-498: LGTM! Proper external link security.The external link correctly implements security best practices with
rel="noopener noreferrer"and uses the constantClaimFacilityInfoLinkfor the URL, ensuring consistency across the application.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/react/src/components/V1Claim/ClaimIntro.jsx (1)
24-24: Simplify redundant paddingTop.The expression
theme.spacing.unit * 0evaluates to0. Consider simplifying for clarity.Apply this diff:
- paddingTop: theme.spacing.unit * 0, + paddingTop: 0,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/react/src/components/V1Claim/ClaimIntro.jsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/react/src/components/V1Claim/ClaimIntro.jsx (3)
src/react/src/components/RequireAuthNotice.jsx (1)
RequireAuthNotice(9-29)src/react/src/util/util.js (2)
makeClaimDetailsLink(287-287)makeClaimDetailsLink(287-287)src/react/src/components/V1Claim/ClaimInfoSection.jsx (1)
ClaimInfoSection(290-513)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: get-base-branch-contricleaner-cov
- GitHub Check: get-base-branch-countries-cov
- GitHub Check: run-integration-test-code-quality
- GitHub Check: run-countries-code-quality
- GitHub Check: run-eslint-linter-and-prettier-formatter
- GitHub Check: get-base-branch-dd-cov
- GitHub Check: run-dd-code-quality
- GitHub Check: run-contricleaner-code-quality
- GitHub Check: run-flake8-linter
- GitHub Check: get-base-branch-django-cov
- GitHub Check: run-django-code-quality
- GitHub Check: get-base-branch-fe-cov
- GitHub Check: run-fe-code-quality
🔇 Additional comments (6)
src/react/src/components/V1Claim/ClaimIntro.jsx (6)
1-16: LGTM!All imports are used appropriately throughout the component. The dependencies are correctly structured with React core, Redux, Material-UI, routing, and internal components/utilities.
118-135: LGTM!The authentication gating is well-implemented with a clear fallback, and the navigation handlers are straightforward and correct. The use of
makeClaimDetailsLink(osID)maintains consistency with routing utilities.
136-186: LGTM!The JSX structure is clean and well-organized. The component composition appropriately uses existing layout components (
AppOverflow,AppGrid), integrates theClaimInfoSection, and provides clear user actions. The responsive layout considerations are good.
188-196: LGTM!PropTypes are comprehensive and correctly define all required props. The
historyshape properly specifies the required methods (goBackandpush).
214-216: LGTM!The component export correctly composes the necessary HOCs (
connect,withRouter,withStyles) in the appropriate order to provide Redux state, routing functionality, and styling to the component.
68-71: No issues found —gapproperty is appropriate for this codebase.The
gapproperty is fully supported in current major browsers (Chrome 84+, Edge 84+, Firefox 63+, Safari 14.1+, and most modern mobile browsers). The codebase contains no explicit legacy browser support configuration (no.browserslistrc, no browserslist in package.json), indicating it targets modern browsers. The use ofgapfor flexbox spacing at line 70 is correct and requires no changes.
src/react/src/components/InitialClaimFlow/ClaimIntro/ClaimInfoSection.jsx
Show resolved
Hide resolved
|
I have nothing to add. Pls, address all comments above and ping me to approve. |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
doc/release/RELEASE-NOTES.md (1)
16-18: Fix nested list indent (MD007) — use 2 spaces.Change nested bullets under “Release instructions” to two-space indents to satisfy markdownlint.
* Ensure that the following commands are included in the `post_deployment` command: - * `migrate` - * `reindex_database` + * `migrate` + * `reindex_database`src/react/src/components/InitialClaimFlow/ClaimIntro/ClaimIntro.jsx (1)
97-111: Fix HOC order (withRouter must wrap connect) and harden mapStateToProps.Current composition prevents mapStateToProps from receiving match, and deep destructuring will throw if state/props are missing. This breaks routing-derived osID and can crash. Tests mask it by passing match manually.
Apply:
-const mapStateToProps = ( - { - auth: { - user: { user }, - }, - }, - { - match: { - params: { osID }, - }, - }, -) => ({ - osID, - userHasSignedIn: !user.isAnon, -}); +const mapStateToProps = (state, ownProps) => { + const osID = ownProps && ownProps.match && ownProps.match.params + ? ownProps.match.params.osID + : ''; + const isAnon = + state && + state.auth && + state.auth.user && + state.auth.user.user && + typeof state.auth.user.user.isAnon === 'boolean' + ? state.auth.user.user.isAnon + : true; + return { + osID, + userHasSignedIn: !isAnon, + }; +}; @@ -export default connect(mapStateToProps)( - withRouter(withStyles(claimIntroStyles)(ClaimIntro)), -); +export default withRouter( + connect(mapStateToProps)(withStyles(claimIntroStyles)(ClaimIntro)), +);
- Outcome: osID reliably comes from router; component won’t crash if auth shape changes; PropTypes no longer warn on missing osID. Based on learnings.
Also applies to: 113-115
🧹 Nitpick comments (9)
src/react/src/components/InitialClaimFlow/ClaimIntro/styles.js (1)
10-11: Remove no‑op padding.
paddingTop: theme.spacing.unit * 0is redundant.- paddingTop: theme.spacing.unit * 0, paddingBottom: theme.spacing.unit * 4,src/react/src/Routes.jsx (1)
112-125: Avoid nested Route inside render; pass route props directly.Current nesting is harder to follow and mounts extra Route components. Use the render callback to feed props to both branches.
-<Route - exact - path={claimIntroRoute} - render={() => ( - <FeatureFlag - flag={ENABLE_V1_CLAIMS_FLOW} - alternative={<Route component={Facilities} />} - > - <Route component={ClaimIntro} /> - </FeatureFlag> - )} -/> +<Route + exact + path={claimIntroRoute} + render={routeProps => ( + <FeatureFlag + flag={ENABLE_V1_CLAIMS_FLOW} + alternative={<Facilities {...routeProps} />} + > + <ClaimIntro {...routeProps} /> + </FeatureFlag> + )} +/>src/react/src/components/InitialClaimFlow/ClaimIntro/ImageDialog.jsx (1)
10-12: Add accessible label to the close button.Improve a11y by labeling the control.
- <IconButton className={classes.closeButton} onClick={onClose}> + <IconButton + className={classes.closeButton} + onClick={onClose} + aria-label="Close dialog" + >src/react/src/components/InitialClaimFlow/ClaimIntro/ExampleImage.jsx (1)
18-27: Prefer MUI ButtonBase over a raw button for consistency.Using ButtonBase gives focus handling and integrates with theme; also lets you move inline styles to styles.js.
-import React, { useState } from 'react'; +import React, { useState } from 'react'; +import ButtonBase from '@material-ui/core/ButtonBase'; ... - <button - type="button" - onClick={() => setDialogOpen(true)} - style={{ - border: 'none', - background: 'none', - padding: 0, - cursor: 'pointer', - }} - aria-label={`View example: ${alt}`} - > + <ButtonBase + onClick={() => setDialogOpen(true)} + aria-label={`View example: ${alt}`} + focusRipple + > <img src={src} alt={alt} className={`${classes.exampleImage} ${classes[borderClass]}`} /> - </button> + </ButtonBase>src/react/src/__tests__/components/ClaimIntro.test.js (2)
30-36: Prefer MemoryRouter with initialEntries over a shared history singleton.Using a global history can make tests order‑dependent and brittle. MemoryRouter is simpler and deterministic.
Example:
- return renderWithProviders( - <Router history={history}> - <ClaimIntro match={mockMatch} /> - </Router>, - { preloadedState } - ); + return renderWithProviders( + <MemoryRouter initialEntries={[`/claim/${mockOsID}`]}> + <ClaimIntro match={mockMatch} /> + </MemoryRouter>, + { preloadedState } + );
118-124: Test doesn’t validate osID; it only checks render. Remove or assert something meaningful.Either drop this spec or assert behavior tied to osID (already covered by the “Continue” navigation test).
Apply:
- describe('Props validation', () => { - test('receives osID from route params', () => { - const { container } = renderComponent(); - - expect(container.firstChild).toBeInTheDocument(); - }); - }); + // Removed redundant props validation test (covered by navigation test).src/react/src/__tests__/components/ClaimInfoSection.test.js (2)
179-189: Also assert rel="noopener noreferrer" for the external link.We set it in the component; test it to prevent regressions.
Apply:
const link = getByText('Learn more about claiming your production location'); expect(link).toBeInTheDocument(); expect(link.tagName).toBe('A'); expect(link).toHaveAttribute('href', 'https://info.opensupplyhub.org/resources/claim-a-facility'); expect(link).toHaveAttribute('target', '_blank'); + expect(link).toHaveAttribute('rel', 'noopener noreferrer');
140-145: Brittle icon assertion (querying generic ). Expose a stable test id or role.Select a specific element (e.g., data-testid on the InfoIcon) instead of any SVG.
Test change:
- const svgIcons = container.querySelectorAll('svg'); - expect(svgIcons.length).toBeGreaterThan(0); + expect(getByTestId('warning-icon')).toBeInTheDocument();Component change is in ClaimInfoSection.jsx Lines 231-236 (see comment there).
src/react/src/components/InitialClaimFlow/ClaimIntro/ClaimInfoSection.jsx (1)
231-240: Expose a stable handle for tests and improve a11y on the warning icon.Add data-testid and an accessible label so tests don’t rely on generic SVG selection.
Apply:
- <div className={classes.warningBox}> - <InfoIcon className={classes.warningIcon} /> + <div className={classes.warningBox}> + <InfoIcon + className={classes.warningIcon} + data-testid="warning-icon" + titleAccess="Important information" + aria-label="Important information" + /> <Typography variant="subheading" style={{ fontSize: 16 }}>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
doc/release/RELEASE-NOTES.md(1 hunks)src/react/src/Routes.jsx(3 hunks)src/react/src/__tests__/components/ClaimInfoSection.test.js(1 hunks)src/react/src/__tests__/components/ClaimIntro.test.js(1 hunks)src/react/src/components/InitialClaimFlow/ClaimIntro/ClaimInfoSection.jsx(1 hunks)src/react/src/components/InitialClaimFlow/ClaimIntro/ClaimIntro.jsx(1 hunks)src/react/src/components/InitialClaimFlow/ClaimIntro/ExampleImage.jsx(1 hunks)src/react/src/components/InitialClaimFlow/ClaimIntro/ImageDialog.jsx(1 hunks)src/react/src/components/InitialClaimFlow/ClaimIntro/styles.js(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-01-17T16:12:18.285Z
Learnt from: Innavin369
PR: opensupplyhub/open-supply-hub#483
File: src/react/src/__tests__/components/SearchByNameAndAddressSuccessResult.test.js:0-0
Timestamp: 2025-01-17T16:12:18.285Z
Learning: In the SearchByNameAndAddressSuccessResult component's tests, attempting to test internal navigation logic through mocking useHistory is not feasible, and button presence/click events should be covered in the main rendering test case.
Applied to files:
src/react/src/__tests__/components/ClaimIntro.test.js
🧬 Code graph analysis (6)
src/react/src/components/InitialClaimFlow/ClaimIntro/ExampleImage.jsx (1)
src/react/src/components/InitialClaimFlow/ClaimIntro/ImageDialog.jsx (1)
ImageDialog(8-17)
src/react/src/Routes.jsx (2)
src/react/src/util/constants.jsx (4)
claimIntroRoute(343-343)claimIntroRoute(343-343)ENABLE_V1_CLAIMS_FLOW(579-579)ENABLE_V1_CLAIMS_FLOW(579-579)src/react/src/components/InitialClaimFlow/ClaimIntro/ClaimIntro.jsx (1)
ClaimIntro(17-85)
src/react/src/__tests__/components/ClaimIntro.test.js (2)
src/react/src/util/testUtils/renderWithProviders.jsx (1)
renderWithProviders(6-22)src/react/src/components/InitialClaimFlow/ClaimIntro/ClaimIntro.jsx (1)
ClaimIntro(17-85)
src/react/src/components/InitialClaimFlow/ClaimIntro/ClaimInfoSection.jsx (2)
src/react/src/components/InitialClaimFlow/ClaimIntro/ExampleImage.jsx (1)
ExampleImage(6-50)src/react/src/components/InitialClaimFlow/ClaimIntro/styles.js (2)
claimInfoStyles(104-297)claimInfoStyles(104-297)
src/react/src/components/InitialClaimFlow/ClaimIntro/ClaimIntro.jsx (3)
src/react/src/components/RequireAuthNotice.jsx (1)
RequireAuthNotice(9-29)src/react/src/components/InitialClaimFlow/ClaimIntro/ClaimInfoSection.jsx (1)
ClaimInfoSection(19-242)src/react/src/components/InitialClaimFlow/ClaimIntro/styles.js (2)
claimIntroStyles(3-102)claimIntroStyles(3-102)
src/react/src/__tests__/components/ClaimInfoSection.test.js (2)
src/react/src/util/testUtils/renderWithProviders.jsx (1)
renderWithProviders(6-22)src/react/src/components/InitialClaimFlow/ClaimIntro/ClaimInfoSection.jsx (1)
ClaimInfoSection(19-242)
🪛 markdownlint-cli2 (0.18.1)
doc/release/RELEASE-NOTES.md
17-17: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
18-18: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
src/react/src/components/InitialClaimFlow/ClaimIntro/ExampleImage.jsx (3)
20-30: Move inline styles to the styles file for consistency.The button styling is defined inline, which breaks the pattern established by using
withStylesfor the rest of the component. Consider moving these styles toclaimInfoStylesinstyles.js.Apply this diff to add a button style to your styles file:
In
styles.js, add:exampleButton: { border: 'none', background: 'none', padding: 0, cursor: 'pointer', },Then update the component:
<button type="button" onClick={() => setDialogOpen(true)} - style={{ - border: 'none', - background: 'none', - padding: 0, - cursor: 'pointer', - }} + className={classes.exampleButton} aria-label={`View example: ${alt}`} >
31-42: Consider using the classnames library for className composition.While the current template literal approach works, using a utility like
classnameswould make the code more maintainable and easier to read, especially if additional conditional classes are added in the future.If the project already includes
classnames, consider this refactor:+import classNames from 'classnames'; <img src={src} alt={alt} - className={`${classes.exampleImage} ${classes[borderClass]}`} + className={classNames(classes.exampleImage, classes[borderClass])} /> <Typography - className={`${classes.exampleLabel} ${classes[labelColorClass]}`} + className={classNames(classes.exampleLabel, classes[labelColorClass])} color="textSecondary" >
53-60: Use more specific PropTypes for the classes object.The
classesprop usesPropTypes.object, which is too generic and bypasses type checking. Consider usingPropTypes.shapeto define the expected class names, or add an ESLint disable comment if the generic object is intentional.Apply this diff:
ExampleImage.propTypes = { src: PropTypes.string.isRequired, alt: PropTypes.string.isRequired, label: PropTypes.string.isRequired, borderClass: PropTypes.string.isRequired, labelColorClass: PropTypes.string.isRequired, - classes: PropTypes.object.isRequired, + // eslint-disable-next-line react/forbid-prop-types + classes: PropTypes.object.isRequired, };Or define the shape explicitly:
- classes: PropTypes.object.isRequired, + classes: PropTypes.shape({ + exampleItem: PropTypes.string, + exampleImage: PropTypes.string, + exampleLabel: PropTypes.string, + }).isRequired,src/react/src/components/InitialClaimFlow/ClaimIntro/ClaimInfoSection.jsx (4)
27-33: Consider consolidating inline color styles into the styles file.Multiple inline
styleprops are used throughout the component for color customization (lines 30, 42, 55, 78, 86, 135, 145, 192, 208, etc.). While this approach works, it makes the component harder to maintain and bypasses the theming system provided bywithStyles.Consider creating specific style variants in
styles.jsfor each step's text colors:blueStepTitle: { color: COLOURS.DARK_BLUE, }, blueStepText: { color: COLOURS.LIGHT_MATERIAL_BLUE, }, purpleStepTitle: { color: COLOURS.DARK_PURPLE, }, purpleStepText: { color: COLOURS.PURPLE_TEXT, }, // ... and so onThis would make the component more maintainable and allow for easier theming adjustments.
Also applies to: 39-46, 52-61, 75-81, 83-92, 124-130, 132-140, 189-198, 205-222
19-236: Consider breaking down the component into smaller sub-components.The component is 235 lines long and contains five distinct step sections. While the current structure is functional, extracting each step into its own component (e.g.,
StepOne,StepTwo, etc.) would improve testability and maintainability.For example:
const StepOne = ({ classes }) => ( <div className={`${classes.stepBox} ${classes.blueStep}`}> {/* Step 1 content */} </div> ); const ClaimInfoSection = ({ classes }) => ( <div className={classes.root}> <StepOne classes={classes} /> <div className={classes.twoColumnGrid}> <StepTwo classes={classes} /> <StepThree classes={classes} /> </div> {/* ... */} </div> );This is optional since the component is primarily static content.
93-115: Refactor repetitive ExampleImage usage with array mapping.The component renders multiple
ExampleImagecomponents with very similar props (lines 94-114 and 153-173). This repetition violates the DRY principle and makes it harder to add or modify examples.Consider refactoring using an array and map:
const step2Examples = [ { src: employeeIdExample, alt: 'Example employee ID badge', label: 'Employee ID Badge' }, { src: employmentLetterExample, alt: 'Example employment letter', label: 'Employment Letter' }, { src: businessCardExample, alt: 'Example business card', label: 'Business Card' }, ]; const step3Examples = [ { src: businessRegistrationExample, alt: 'Example business registration certificate', label: 'Business Registration' }, { src: businessLicenseExample, alt: 'Example business license', label: 'Business License' }, { src: utilityBillExample, alt: 'Example utility bill', label: 'Utility Bill' }, ]; // Then in the render: <div className={classes.examplesContainer}> {step2Examples.map((example) => ( <ExampleImage key={example.label} src={example.src} alt={example.alt} label={example.label} borderClass="purpleBorder" labelColorClass="purpleLabel" /> ))} </div>Also applies to: 152-174
238-240: Use more specific PropTypes for the classes object.The
classesprop usesPropTypes.object, which is too generic. Consider usingPropTypes.shapeor adding an ESLint disable comment.Apply this diff:
ClaimInfoSection.propTypes = { - classes: PropTypes.object.isRequired, + // eslint-disable-next-line react/forbid-prop-types + classes: PropTypes.object.isRequired, };Or define the shape with the most commonly used class names (though this can be verbose given the number of classes used).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/react/src/components/InitialClaimFlow/ClaimIntro/ClaimInfoSection.jsx(1 hunks)src/react/src/components/InitialClaimFlow/ClaimIntro/ExampleImage.jsx(1 hunks)src/react/src/components/InitialClaimFlow/ClaimIntro/ImageDialog.jsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/react/src/components/InitialClaimFlow/ClaimIntro/ImageDialog.jsx
🧰 Additional context used
🧬 Code graph analysis (2)
src/react/src/components/InitialClaimFlow/ClaimIntro/ExampleImage.jsx (2)
src/react/src/components/InitialClaimFlow/ClaimIntro/ImageDialog.jsx (1)
ImageDialog(10-19)src/react/src/components/InitialClaimFlow/ClaimIntro/styles.js (2)
claimInfoStyles(104-297)claimInfoStyles(104-297)
src/react/src/components/InitialClaimFlow/ClaimIntro/ClaimInfoSection.jsx (2)
src/react/src/components/InitialClaimFlow/ClaimIntro/ExampleImage.jsx (1)
ExampleImage(8-51)src/react/src/components/InitialClaimFlow/ClaimIntro/styles.js (2)
claimInfoStyles(104-297)claimInfoStyles(104-297)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: run-dd-code-quality
- GitHub Check: run-flake8-linter
- GitHub Check: run-eslint-linter-and-prettier-formatter
- GitHub Check: get-base-branch-dd-cov
- GitHub Check: get-base-branch-contricleaner-cov
- GitHub Check: get-base-branch-countries-cov
- GitHub Check: run-contricleaner-code-quality
- GitHub Check: run-countries-code-quality
- GitHub Check: run-integration-test-code-quality
- GitHub Check: get-base-branch-django-cov
- GitHub Check: get-base-branch-fe-cov
- GitHub Check: run-fe-code-quality
- GitHub Check: run-django-code-quality
🔇 Additional comments (4)
src/react/src/components/InitialClaimFlow/ClaimIntro/ExampleImage.jsx (1)
1-62: Component structure looks good.The component is properly wrapped with
withStylesat line 62, which resolves the previous concern aboutclassesbeing null or undefined. The component follows React best practices with proper state management, accessibility attributes, and PropTypes validation.src/react/src/components/InitialClaimFlow/ClaimIntro/ClaimInfoSection.jsx (3)
214-221: External link follows security best practices.The external link properly includes
target="_blank"withrel="noopener noreferrer"attributes, which prevents potential security vulnerabilities and performance issues.
12-17: No issues found—all imported image assets exist in the repository.All six image files referenced in the imports (lines 12-17) are present and accessible at
src/react/src/images/. The imports will resolve correctly without runtime errors.
27-29: No issues found—Material-UI v3.1.0 correctly uses valid variant props.Verified that the project uses Material-UI v3.1.0 (confirmed in
src/react/package.json). Thevariant="title"andvariant="subheading"props are valid and not deprecated in this version. These variants only became deprecated in Material-UI v4+, so no changes are required for the current codebase. If a future upgrade to v4+ is planned, these would need to be updated then.
3 participants
OSDEV-2200 [Claims Process Update] Implement new claim intro page