Remove front end name validation for add certificate modal - #47242
Conversation
There was a problem hiding this comment.
Pull request overview
This PR removes the Add certificate modal’s client-side “duplicate name” validation (which only worked for certificates in the current paginated page) so that duplicate detection relies on server-side validation and the server error is surfaced inline.
Changes:
- Removed
existingCerts/ICertificateplumbing and the client-side “unique name” validation rule from the modal. - Updated the modal to build validations without needing the certificates list.
- Updated tests and certificates card wiring to match the new modal API.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| frontend/pages/ManageControlsPage/OSSettings/cards/Certificates/components/AddCertificateModal/helpers.ts | Removes client-side uniqueness validation and the existingCerts dependency from validation generation. |
| frontend/pages/ManageControlsPage/OSSettings/cards/Certificates/components/AddCertificateModal/AddCertificateModal.tsx | Removes existingCerts prop usage and updates memoized validation generation accordingly. |
| frontend/pages/ManageControlsPage/OSSettings/cards/Certificates/components/AddCertificateModal/AddCertificateModal.tests.tsx | Removes the “duplicate name as you type” test and updates rendering helper to the new modal props. |
| frontend/pages/ManageControlsPage/OSSettings/cards/Certificates/Certificates.tsx | Stops passing existingCerts into the Add certificate modal. |
Comments suppressed due to low confidence (1)
frontend/pages/ManageControlsPage/OSSettings/cards/Certificates/components/AddCertificateModal/AddCertificateModal.tests.tsx:146
- Now that client-side uniqueness validation was removed, the name-conflict path is only exercised on submit via server response parsing. There’s no test asserting that a 409/"already exists" error is surfaced inline on the Name field (and cleared on edit), so regressions would likely reintroduce the original UX (generic flash only).
it("shows inline error for Name longer than 255 characters as user types", async () => {
const { user } = await renderModal();
// Paste rather than type to keep the test fast (256 simulated keypresses is slow).
await user.click(screen.getByPlaceholderText(NAME_PLACEHOLDER));
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #47242 +/- ##
==========================================
- Coverage 67.14% 67.14% -0.01%
==========================================
Files 3045 3045
Lines 226606 226600 -6
Branches 11831 11830 -1
==========================================
- Hits 152150 152144 -6
Misses 60717 60717
Partials 13739 13739
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
WalkthroughThis PR removes client-side duplicate certificate name validation from the AddCertificateModal component. The Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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.
🧹 Nitpick comments (1)
frontend/pages/ManageControlsPage/OSSettings/cards/Certificates/components/AddCertificateModal/AddCertificateModal.tests.tsx (1)
201-243: ⚡ Quick winConsider adding a test for server-side duplicate name error handling.
The existing test verifies that server-side SAN validation errors are properly surfaced inline. Since this PR shifts duplicate name detection to server-side validation (the core fix for issue
#44821), adding a similar test for duplicate name errors would document the expected behavior and prevent regression.The test would follow the same pattern: mock a 422 response with an "already exists" error, submit the form, verify the error appears inline against the Name field, then verify that editing the Name field clears the error.
🧪 Example test to add
it("surfaces a 422 duplicate name error against the Name input inline", async () => { const DUPLICATE_NAME_ERR = "Certificate name already exists"; mockServer.use( http.post(baseUrl("/certificates"), () => { return HttpResponse.json( { message: "Validation Failed", errors: [ { name: "name", reason: `Certificate with name "Test Cert" already exists` }, ], }, { status: 422 } ); }) ); const { user } = await renderModal(); const nameInput = screen.getByPlaceholderText(NAME_PLACEHOLDER); await user.type(nameInput, "Test Cert"); await user.type( screen.getByPlaceholderText(SUBJECT_NAME_PLACEHOLDER), "/CN=test/O=Org" ); await selectScepCa(user); await user.click(screen.getByRole("button", { name: /Add/i })); await waitFor(() => { expect(screen.getByText("Name is already used by another certificate.")).toBeInTheDocument(); }); expect(mockOnSuccess).not.toHaveBeenCalled(); // Editing the Name clears the server error. await user.type(nameInput, " Modified"); await waitFor(() => { expect(screen.queryByText("Name is already used by another certificate.")).not.toBeInTheDocument(); }); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/pages/ManageControlsPage/OSSettings/cards/Certificates/components/AddCertificateModal/AddCertificateModal.tests.tsx` around lines 201 - 243, Add a new test in AddCertificateModal.tests.tsx mirroring the SAN test: mockServer.post to return a 422 JSON with errors: [{ name: "name", reason: 'Certificate with name "Test Cert" already exists' }], then use renderModal(), fill the NAME_PLACEHOLDER input (use NAME_PLACEHOLDER to locate the field), fill SUBJECT_NAME_PLACEHOLDER and selectScepCa(user), click the Add button, waitFor the inline message "Name is already used by another certificate." to appear and assert mockOnSuccess was not called, then type into the name input (e.g. " Modified") and waitFor the server error to be cleared (queryByText returns null).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@frontend/pages/ManageControlsPage/OSSettings/cards/Certificates/components/AddCertificateModal/AddCertificateModal.tests.tsx`:
- Around line 201-243: Add a new test in AddCertificateModal.tests.tsx mirroring
the SAN test: mockServer.post to return a 422 JSON with errors: [{ name: "name",
reason: 'Certificate with name "Test Cert" already exists' }], then use
renderModal(), fill the NAME_PLACEHOLDER input (use NAME_PLACEHOLDER to locate
the field), fill SUBJECT_NAME_PLACEHOLDER and selectScepCa(user), click the Add
button, waitFor the inline message "Name is already used by another
certificate." to appear and assert mockOnSuccess was not called, then type into
the name input (e.g. " Modified") and waitFor the server error to be cleared
(queryByText returns null).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 917c78cd-388a-4cae-9799-07027eb1e0c0
📒 Files selected for processing (4)
frontend/pages/ManageControlsPage/OSSettings/cards/Certificates/Certificates.tsxfrontend/pages/ManageControlsPage/OSSettings/cards/Certificates/components/AddCertificateModal/AddCertificateModal.tests.tsxfrontend/pages/ManageControlsPage/OSSettings/cards/Certificates/components/AddCertificateModal/AddCertificateModal.tsxfrontend/pages/ManageControlsPage/OSSettings/cards/Certificates/components/AddCertificateModal/helpers.ts
💤 Files with no reviewable changes (1)
- frontend/pages/ManageControlsPage/OSSettings/cards/Certificates/Certificates.tsx
Related issue: Resolves #44821
Related PR: #46414
key to note that the server side validation is now the only validation that will happen to detect duplicate names.
Testing
For unreleased bug fixes in a release candidate, one of:
Summary by CodeRabbit
Refactor
Tests