diff --git a/docs/plans/current_spec.md b/docs/plans/current_spec.md index 27f10a89..e02b864e 100644 --- a/docs/plans/current_spec.md +++ b/docs/plans/current_spec.md @@ -1,44 +1,14 @@ -# Certificate Bulk Delete — Spec +# Fix: Allow deletion of expiring_soon certificates not in use -**Date**: 2026-03-22 -**Priority**: Medium -**Type**: User Requested Feature -**Status**: Planning — Awaiting Implementation +> **Status note:** The bug report refers to the status as `expiring_soon`. In this codebase the actual status string is `'expiring'` (defined in `frontend/src/api/certificates.ts` line 10 and `backend/internal/services/certificate_service.go` line 33). All references below use `'expiring'`. --- -## 1. Problem Statement +## 1. Bug Root Cause -The Certificates page now supports individual deletion of custom, staging, and expired -production Let's Encrypt certificates. Users who accumulate many such certificates must -click through a confirmation dialog for each one. There is no mechanism to select and -destroy multiple certificates in a single operation. - -This feature adds checkbox-based bulk selection and a single confirmation step to delete -N certificates at once, operating under exactly the same policy terms as the existing -individual delete affordance. - -### Non-Goals - -- Changes to the individual delete flow or `DeleteCertificateDialog`. -- A new backend batch endpoint (sequential per-cert calls are sufficient). -- Auto-cleanup / scheduled pruning. -- Migrating `CertificateList` from its current raw `` to the `DataTable` component. - ---- - -## 2. Existing Foundation - -### 2.1 Individual Delete Policy (Preserved Verbatim) - -The following logic lives in `frontend/src/components/CertificateList.tsx` and must be -preserved without modification. Bulk selection obeys it exactly: +`frontend/src/components/CertificateList.tsx` — `isDeletable` (lines 26–34): ```ts -export function isInUse(cert: Certificate, hosts: ProxyHost[]): boolean { - return hosts.some(h => (h.certificate_id ?? h.certificate?.id) === cert.id) -} - export function isDeletable(cert: Certificate, hosts: ProxyHost[]): boolean { if (!cert.id) return false if (isInUse(cert, hosts)) return false @@ -50,322 +20,43 @@ export function isDeletable(cert: Certificate, hosts: ProxyHost[]): boolean { } ``` -| Certificate Category | `isDeletable` | `isInUse` | Individual button | Checkbox | -|---|---|---|---|---| -| custom / staging — not in use | ✅ true | ❌ false | Active delete Trash2 | ✅ Enabled checkbox | -| custom / staging / expired LE — in use | ❌ false | ✅ true | `aria-disabled` Trash2 + tooltip | ✅ Checkbox rendered but `disabled` + tooltip | -| expired LE — not in use | ✅ true | ❌ false | Active delete Trash2 | ✅ Enabled checkbox | -| valid/expiring LE — not in use | ❌ false | ❌ false | No affordance at all | No checkbox, no column cell | -| valid/expiring LE — in use | ❌ false | ✅ true | No affordance at all | No checkbox, no column cell | +A cert with `provider === 'letsencrypt'` and `status === 'expiring'` that is not attached to any proxy host evaluates to `false` because `'expiring' !== 'expired'`. No delete button is rendered, and the cert cannot be selected for bulk delete. -### 2.2 Backend — No New Endpoint Required +**Additional bug:** `isInUse` has a false-positive when `cert.id` is falsy — `undefined === undefined` would match any proxy host with an unset certificate reference, incorrectly treating the cert as in-use. Fix by adding `if (!cert.id) return false` as the first line of `isInUse`. -`DELETE /api/v1/certificates/:id` is registered at `backend/internal/api/routes/routes.go:673` -and already: +Three secondary locations propagate the same status blind-spot: -- Guards against in-use certs (`IsCertificateInUse` → `409 Conflict`). -- Creates a server-side backup before deletion. -- Deletes the DB record and ACME files. -- Invalidates the cert cache and fires a notification. - -Bulk deletion will call this endpoint N times concurrently using `Promise.allSettled`, -exactly as the ProxyHosts bulk delete does for `deleteHost`. `ids.map(id => deleteCertificate(id))` -fires all promises concurrently; `Promise.allSettled` awaits all settlements before resolving. -No batch endpoint is warranted at this scale. - -### 2.3 CertificateList Rendering Architecture - -`CertificateList.tsx` renders a purpose-built raw `
` with a manual `sortedCertificates` -`useMemo`. It does **not** use the `DataTable` UI component. This plan does not migrate it — -the selection layer will be grafted directly onto the existing table. - -The `Checkbox` component at `frontend/src/components/ui/Checkbox.tsx` supports an -`indeterminate` prop backed by Radix UI `CheckboxPrimitive`. This component will be reused -for both the header "select all" checkbox and each row checkbox, matching the rendering -style in `DataTable.tsx`. - -### 2.4 Bulk Selection Precedent — ProxyHosts Page - -`frontend/src/pages/ProxyHosts.tsx` is the reference implementation for bulk operations: - -- `selectedHosts: Set` — `useState>(new Set())` -- The `DataTable` `selectable` prop handles the per-row checkbox column and the header - "select all" checkbox automatically, but `DataTable.handleSelectAll` selects **every** row. -- For certificates the "select all" must only select the `isDeletable && !isInUse` subset, - so we cannot delegate to `DataTable`'s built-in logic even if we migrated. -- The bulk action bar is a conditional `
` that appears only when - `selectedHosts.size > 0`, containing the count and action buttons. +- `frontend/src/components/dialogs/BulkDeleteCertificateDialog.tsx` — `providerLabel` falls through to `return cert.provider` (showing raw `"letsencrypt"`) for expiring certs instead of a human-readable label. +- `frontend/src/components/dialogs/DeleteCertificateDialog.tsx` — `getWarningKey` falls through to `'certificates.deleteConfirmCustom'` for expiring certs instead of a contextual message. --- -## 3. Technical Specification +## 2. Frontend Fix -### 3.1 State Changes — `CertificateList.tsx` - -Add two pieces of state alongside the existing `certToDelete` and `sortColumn` state: +### 2a. `frontend/src/components/CertificateList.tsx` — `isDeletable` +**Before:** ```ts -const [selectedIds, setSelectedIds] = useState>(new Set()) -const [showBulkDeleteDialog, setShowBulkDeleteDialog] = useState(false) +return ( + cert.provider === 'custom' || + cert.provider === 'letsencrypt-staging' || + cert.status === 'expired' +) ``` -Add a memoised derived set of all cert IDs that are eligible for selection: - +**After:** ```ts -const selectableCertIds = useMemo>(() => { - const ids = new Set() - for (const cert of sortedCertificates) { - if (isDeletable(cert, hosts) && cert.id) { - ids.add(cert.id) - } - } - return ids -}, [sortedCertificates, hosts]) +return ( + cert.provider === 'custom' || + cert.provider === 'letsencrypt-staging' || + cert.status === 'expired' || + cert.status === 'expiring' +) ``` -> Only `isDeletable` certs (not in use, correct provider/status) enter `selectableCertIds`. -> In-use-but-would-be-deletable certs (`isInUse && (custom || staging || expired)`) do NOT -> enter the selectable set — their row checkbox is rendered but is `disabled`. - -Add selection handlers: - -```ts -const handleSelectAll = () => { - if (selectedIds.size === selectableCertIds.size) { - setSelectedIds(new Set()) - } else { - setSelectedIds(new Set(selectableCertIds)) - } -} - -const handleSelectRow = (id: number) => { - const next = new Set(selectedIds) - next.has(id) ? next.delete(id) : next.add(id) - setSelectedIds(next) -} -``` - -Header checkbox state derived from selection: - -```ts -const allSelectableSelected = - selectableCertIds.size > 0 && selectedIds.size === selectableCertIds.size -const someSelected = - selectedIds.size > 0 && selectedIds.size < selectableCertIds.size -``` - -### 3.2 Table Column Changes — `CertificateList.tsx` - -#### 3.2.1 New Leftmost `
` row, before the existing -"Name" column: - -```tsx - -``` - -Also update the empty-state ` -``` - -**Case B — in-use-but-deletable category (`isInUse && (custom || staging || expired)`):** - -The individual delete button mirrors this with `aria-disabled`. The checkbox must match: -rendered but disabled, with the same tooltip text from `t('certificates.deleteInUse')`. - -```tsx - -``` - -> Radix `Checkbox` with `disabled` swallows pointer events. Wrapping in a `` restores -> hover targeting for the tooltip — the same technique used for the existing `aria-disabled` -> delete buttons where `TooltipTrigger asChild` wraps the ` diff --git a/frontend/src/components/__tests__/CertificateList.test.tsx b/frontend/src/components/__tests__/CertificateList.test.tsx index 43097b12..ea63b910 100644 --- a/frontend/src/components/__tests__/CertificateList.test.tsx +++ b/frontend/src/components/__tests__/CertificateList.test.tsx @@ -150,9 +150,14 @@ describe('CertificateList', () => { expect(isDeletable(cert, noHosts)).toBe(false) }) - it('returns false for expiring LE cert not in use', () => { + it('returns true for expiring LE cert not in use', () => { const cert: Certificate = { id: 7, name: 'Exp', domain: 'd', issuer: 'LE', expires_at: '', status: 'expiring', provider: 'letsencrypt' } - expect(isDeletable(cert, noHosts)).toBe(false) + expect(isDeletable(cert, noHosts)).toBe(true) + }) + + it('returns false for expiring LE cert that is in use', () => { + const cert: Certificate = { id: 7, name: 'Exp', domain: 'd', issuer: 'LE', expires_at: '', status: 'expiring', provider: 'letsencrypt' } + expect(isDeletable(cert, withHost(7))).toBe(false) }) }) @@ -172,6 +177,12 @@ describe('CertificateList', () => { const cert: Certificate = { id: 99, domain: 'd', issuer: 'X', expires_at: '', status: 'valid', provider: 'custom' } expect(isInUse(cert, [createProxyHost({ certificate_id: 3 })])).toBe(false) }) + + it('returns false when cert.id is undefined even if a host has certificate_id undefined', () => { + const cert: Certificate = { domain: 'd', issuer: 'X', expires_at: '', status: 'valid', provider: 'custom' } + const host = createProxyHost({ certificate_id: undefined }) + expect(isInUse(cert, [host])).toBe(false) + }) }) it('renders delete button for deletable certs', async () => { diff --git a/frontend/src/components/dialogs/BulkDeleteCertificateDialog.tsx b/frontend/src/components/dialogs/BulkDeleteCertificateDialog.tsx index 95ef012b..4db633ab 100644 --- a/frontend/src/components/dialogs/BulkDeleteCertificateDialog.tsx +++ b/frontend/src/components/dialogs/BulkDeleteCertificateDialog.tsx @@ -25,6 +25,7 @@ function providerLabel(cert: Certificate): string { if (cert.provider === 'letsencrypt-staging') return 'Staging' if (cert.provider === 'custom') return 'Custom' if (cert.status === 'expired') return 'Expired LE' + if (cert.status === 'expiring') return 'Expiring LE' return cert.provider } diff --git a/frontend/src/components/dialogs/DeleteCertificateDialog.tsx b/frontend/src/components/dialogs/DeleteCertificateDialog.tsx index 03fbf23f..68491eb6 100644 --- a/frontend/src/components/dialogs/DeleteCertificateDialog.tsx +++ b/frontend/src/components/dialogs/DeleteCertificateDialog.tsx @@ -23,6 +23,7 @@ interface DeleteCertificateDialogProps { function getWarningKey(cert: Certificate): string { if (cert.status === 'expired') return 'certificates.deleteConfirmExpired' + if (cert.status === 'expiring') return 'certificates.deleteConfirmExpiring' if (cert.provider === 'letsencrypt-staging') return 'certificates.deleteConfirmStaging' return 'certificates.deleteConfirmCustom' } diff --git a/frontend/src/components/dialogs/__tests__/BulkDeleteCertificateDialog.test.tsx b/frontend/src/components/dialogs/__tests__/BulkDeleteCertificateDialog.test.tsx index dd0a1991..65d4eac4 100644 --- a/frontend/src/components/dialogs/__tests__/BulkDeleteCertificateDialog.test.tsx +++ b/frontend/src/components/dialogs/__tests__/BulkDeleteCertificateDialog.test.tsx @@ -126,4 +126,18 @@ describe('BulkDeleteCertificateDialog', () => { ) expect(container.innerHTML).toBe('') }) + + it('renders "Expiring LE" label for a letsencrypt cert with status expiring', () => { + const expiringCert = makeCert({ id: 4, name: 'Expiring Cert', domain: 'expiring.example.com', provider: 'letsencrypt', status: 'expiring' }) + render( + + ) + expect(screen.getByText('Expiring LE')).toBeInTheDocument() + }) }) diff --git a/frontend/src/locales/de/translation.json b/frontend/src/locales/de/translation.json index 59c6e0b3..fde5acb9 100644 --- a/frontend/src/locales/de/translation.json +++ b/frontend/src/locales/de/translation.json @@ -179,6 +179,7 @@ "deleteConfirmCustom": "This will permanently delete this certificate. A backup will be created first.", "deleteConfirmStaging": "This staging certificate will be removed. It will be regenerated on next request.", "deleteConfirmExpired": "This expired certificate is no longer active and will be permanently removed.", + "deleteConfirmExpiring": "This certificate is expiring soon. It will be permanently removed and will not be auto-renewed.", "deleteSuccess": "Certificate deleted", "deleteFailed": "Failed to delete certificate", "deleteInUse": "Cannot delete — certificate is attached to a proxy host", diff --git a/frontend/src/locales/en/translation.json b/frontend/src/locales/en/translation.json index 181a97e3..5dbcda48 100644 --- a/frontend/src/locales/en/translation.json +++ b/frontend/src/locales/en/translation.json @@ -182,12 +182,13 @@ "uploadSuccess": "Certificate uploaded successfully", "uploadFailed": "Failed to upload certificate", "note": "Note", - "noteText": "You can delete custom certificates, staging certificates, and expired production certificates that are not attached to any proxy host. Active production certificates are automatically renewed by Caddy.", + "noteText": "You can delete custom certificates, staging certificates, and expired or expiring production certificates that are not attached to any proxy host. Active production certificates are automatically renewed by Caddy.", "provider": "Provider", "deleteTitle": "Delete Certificate", "deleteConfirmCustom": "This will permanently delete this certificate. A backup will be created first.", "deleteConfirmStaging": "This staging certificate will be removed. It will be regenerated on next request.", "deleteConfirmExpired": "This expired certificate is no longer active and will be permanently removed.", + "deleteConfirmExpiring": "This certificate is expiring soon. It will be permanently removed and will not be auto-renewed.", "deleteSuccess": "Certificate deleted", "deleteFailed": "Failed to delete certificate", "deleteInUse": "Cannot delete — certificate is attached to a proxy host", diff --git a/frontend/src/locales/es/translation.json b/frontend/src/locales/es/translation.json index 6452f8e0..6e525ffc 100644 --- a/frontend/src/locales/es/translation.json +++ b/frontend/src/locales/es/translation.json @@ -179,6 +179,7 @@ "deleteConfirmCustom": "This will permanently delete this certificate. A backup will be created first.", "deleteConfirmStaging": "This staging certificate will be removed. It will be regenerated on next request.", "deleteConfirmExpired": "This expired certificate is no longer active and will be permanently removed.", + "deleteConfirmExpiring": "This certificate is expiring soon. It will be permanently removed and will not be auto-renewed.", "deleteSuccess": "Certificate deleted", "deleteFailed": "Failed to delete certificate", "deleteInUse": "Cannot delete — certificate is attached to a proxy host", diff --git a/frontend/src/locales/fr/translation.json b/frontend/src/locales/fr/translation.json index b41448a2..3c927858 100644 --- a/frontend/src/locales/fr/translation.json +++ b/frontend/src/locales/fr/translation.json @@ -179,6 +179,7 @@ "deleteConfirmCustom": "This will permanently delete this certificate. A backup will be created first.", "deleteConfirmStaging": "This staging certificate will be removed. It will be regenerated on next request.", "deleteConfirmExpired": "This expired certificate is no longer active and will be permanently removed.", + "deleteConfirmExpiring": "This certificate is expiring soon. It will be permanently removed and will not be auto-renewed.", "deleteSuccess": "Certificate deleted", "deleteFailed": "Failed to delete certificate", "deleteInUse": "Cannot delete — certificate is attached to a proxy host", diff --git a/frontend/src/locales/zh/translation.json b/frontend/src/locales/zh/translation.json index 7e17a015..265f6f85 100644 --- a/frontend/src/locales/zh/translation.json +++ b/frontend/src/locales/zh/translation.json @@ -179,6 +179,7 @@ "deleteConfirmCustom": "This will permanently delete this certificate. A backup will be created first.", "deleteConfirmStaging": "This staging certificate will be removed. It will be regenerated on next request.", "deleteConfirmExpired": "This expired certificate is no longer active and will be permanently removed.", + "deleteConfirmExpiring": "This certificate is expiring soon. It will be permanently removed and will not be auto-renewed.", "deleteSuccess": "Certificate deleted", "deleteFailed": "Failed to delete certificate", "deleteInUse": "Cannot delete — certificate is attached to a proxy host",
` — Header Checkbox - -Insert a new `` as the **first column** in the `
- -` to `colSpan={7}`. - -#### 3.2.2 New Leftmost `` — Per-Row Checkbox - -For each row in the `sortedCertificates.map(...)`, insert a new `` as the first cell. -Three mutually exclusive cases: - -**Case A — `isDeletable && !isInUse` (enabled checkbox):** - -```tsx - - handleSelectRow(cert.id!)} - aria-label={t('certificates.selectCert', { name: cert.name || cert.domain })} - /> - - - - - - - - - {t('certificates.deleteInUse')} - - -` (§3.2.1); update empty-state `colSpan` 6 → 7. -9. Insert leftmost `` for each row with the three cases A/B/C (§3.2.2). -10. Mount `` at the end of the fragment (§3.4). - -**Invariant**: `isDeletable` and `isInUse` exported function signatures must not change. -All pre-existing assertions in `CertificateList.test.tsx` must continue to pass. - -### Phase 4 — Unit Tests - -#### 4.1 New file: `BulkDeleteCertificateDialog.test.tsx` - -**File**: `frontend/src/components/dialogs/__tests__/BulkDeleteCertificateDialog.test.tsx` - -Mock `react-i18next` using the `t: (key, opts?) => opts ? JSON.stringify(opts) : key` -pattern used throughout the test suite. - -| # | Test description | -|---|---| -| 1 | renders dialog with count in title when 3 certs supplied | -| 2 | lists each certificate name in the scrollable list | -| 3 | calls `onConfirm` when the Delete button is clicked | -| 4 | calls `onCancel` when the Cancel button is clicked | -| 5 | Delete button is loading/disabled when `isDeleting={true}` | -| 6 | returns null when `certificates` array is empty | - -#### 4.2 Additions to `CertificateList.test.tsx` - -Extend the existing `describe('CertificateList', ...)` in -`frontend/src/components/__tests__/CertificateList.test.tsx`. - -The existing fixture (`createCertificatesValue`) already supplies: -- `id: 1` custom expired, not in use → `isDeletable = true` → enabled checkbox -- `id: 2` letsencrypt-staging, not in use → `isDeletable = true` → enabled checkbox -- `id: 4` custom valid, not in use → `isDeletable = true` → enabled checkbox -- `id: 5` expired LE, not in use → `isDeletable = true` → enabled checkbox -- `id: 3` custom valid, in use (host has `certificate_id: 3`) → disabled checkbox -- `id: 6` valid LE, not in use → `isDeletable = false` → no checkbox - -| # | Test description | -|---|---| -| 1 | renders enabled checkboxes for ids 1, 2, 4, 5 (deletable, not in use) | -| 2 | renders disabled checkbox (with `aria-disabled`) for id 3 (in-use) | -| 3 | renders no checkbox in id 6's row (valid production LE) | -| 4 | selecting one cert makes the bulk action toolbar visible | -| 5 | header select-all selects only ids 1, 2, 4, 5 — not id 3 (in-use) | -| 6 | clicking the toolbar Delete button opens `BulkDeleteCertificateDialog` | -| 7 | confirming in the bulk dialog calls `deleteCertificate` for each selected ID | - -### Phase 5 — Playwright E2E Tests - -**File to create**: `tests/certificate-bulk-delete.spec.ts` - -Reuse `createCustomCertViaAPI` from `tests/certificate-delete.spec.ts`. Import shared -test helpers from: -- `tests/fixtures/auth-fixtures` — `test`, `expect`, `loginUser` -- `tests/utils/wait-helpers` — `waitForLoadingComplete`, `waitForDialog`, - `waitForAPIResponse` -- `tests/fixtures/test-data` — `generateUniqueId` -- `tests/constants` — `STORAGE_STATE` - -Seed three custom certs via `beforeAll`, clean up with `afterAll`. Each `test.beforeEach` -navigates to `/certificates` and calls `waitForLoadingComplete`. - -| # | Test scenario | -|---|---| -| 1 | **Checkbox column present**: checkboxes appear for each deletable cert | -| 2 | **No checkbox for valid LE**: valid production LE cert row has no checkbox | -| 3 | **Select one → toolbar appears**: checking one cert shows the count and Delete button | -| 4 | **Select-all**: header checkbox selects all three seeded certs; toolbar shows count 3 | -| 5 | **Dialog shows correct count**: opening bulk dialog shows "Delete 3 Certificate(s)" | -| 6 | **Cancel preserves certs**: cancelling the dialog leaves all three certs in the list | -| 7 | **Confirm deletes all selected**: confirming removes all selected certs from the table | - -The "confirm deletes" test awaits the success or failure toast appearance (toast appearance -confirms all requests have settled via `Promise.allSettled`) before asserting the cert -names are no longer visible in the table. - ---- - -## 5. Backend Considerations - -### 5.1 No New Endpoint - -The existing `DELETE /api/v1/certificates/:id` route at -`backend/internal/api/routes/routes.go:673` is the only backend touch point. Bulk deletion -is orchestrated entirely in the frontend using `Promise.allSettled`. This is intentional: - -- The volume of certificates eligible for bulk deletion is small in practice. -- Each deletion independently creates a server-side backup. A batch endpoint would need - N individual backups anyway, yielding no efficiency gain. -- Concurrent `Promise.allSettled` provides natural per-item error isolation — a 409 on - one cert (race: cert becomes in-use between checkbox selection and confirmation) surfaces - as a failed count in the toast rather than an unhandled rejection. - -### 5.2 No Backend Tests Required - -The handler tests added in the single-delete PR already cover: success, in-use 409, auth -guard, invalid ID, not-found, and backup-failure paths. Bulk deletion calls the same handler -N times with no new code paths. Nothing new at the backend layer warrants new tests. - ---- - -## 6. Security Considerations - -- Bulk delete inherits all security properties of the individual delete endpoint: - authentication required, in-use guard server-side, numeric ID validation in the handler. -- The client-side `isDeletable` check is a UX gate, not a security gate; the server is the - authoritative enforcer. -- `Promise.allSettled` does not short-circuit — a 409 on one cert becomes a failed count, - not an unhandled exception, preserving the remaining deletions. - ---- - -## 7. Commit Slicing Strategy - -**Single PR.** The entire feature — `BulkDeleteCertificateDialog`, i18n keys in 5 locales, -selection layer in `CertificateList.tsx`, unit tests, and E2E tests — is one cohesive -change. The diff is small (< 400 lines of production code across ~6 files) and all parts -are interdependent. Splitting would temporarily ship a broken feature mid-PR. - -Suggested commit title: -``` -feat(certificates): add bulk deletion with checkbox selection +"noteText": "You can delete custom certificates, staging certificates, and expired or expiring production certificates that are not attached to any proxy host. Active production certificates are automatically renewed by Caddy.", ``` --- -## 8. File Change Summary +## 3. Unit Test Updates -| File | Change | Description | -|------|--------|-------------| -| `frontend/src/components/CertificateList.tsx` | Modified | Selection state, checkbox column, bulk toolbar, bulk mutation | -| `frontend/src/components/dialogs/BulkDeleteCertificateDialog.tsx` | Created | New dialog for bulk confirmation | -| `frontend/src/locales/en/translation.json` | Modified | 10 new i18n keys under `certificates` | -| `frontend/src/locales/de/translation.json` | Modified | 10 new i18n keys (DE) | -| `frontend/src/locales/es/translation.json` | Modified | 10 new i18n keys (ES) | -| `frontend/src/locales/fr/translation.json` | Modified | 10 new i18n keys (FR) | -| `frontend/src/locales/zh/translation.json` | Modified | 10 new i18n keys (ZH) | -| `frontend/src/components/__tests__/CertificateList.test.tsx` | Modified | 7 new unit test cases | -| `frontend/src/components/dialogs/__tests__/BulkDeleteCertificateDialog.test.tsx` | Created | 6 unit test cases for the new dialog | -| `tests/certificate-bulk-delete.spec.ts` | Created | E2E test suite (7 scenarios) | +### 3a. `frontend/src/components/__tests__/CertificateList.test.tsx` -**Dependencies**: None — the backend API is already complete. No database migrations. +**Existing test at line 152–155 — update assertion:** -**Validation Gates**: -- `npx vitest run` — all existing and new unit tests pass -- Playwright E2E on Firefox — all 7 new scenarios pass -- `npx tsc --noEmit` — 0 errors -- `make lint-fast` — 0 new warnings +The test currently documents the wrong behavior: + +```ts +it('returns false for expiring LE cert not in use', () => { + const cert: Certificate = { id: 7, name: 'Exp', domain: 'd', issuer: 'LE', expires_at: '', status: 'expiring', provider: 'letsencrypt' } + expect(isDeletable(cert, noHosts)).toBe(false) // wrong — must become true +}) +``` + +**Changes:** +- Rename description to `'returns true for expiring LE cert not in use'` +- Change assertion to `toBe(true)` + +**Add** a new test case to guard the in-use path: +```ts +it('returns false for expiring LE cert in use', () => { + const cert: Certificate = { id: 8, name: 'ExpUsed', domain: 'd', issuer: 'LE', expires_at: '', status: 'expiring', provider: 'letsencrypt' } + expect(isDeletable(cert, withHost(8))).toBe(false) +}) +``` + +### 3b. `frontend/src/components/dialogs/__tests__/BulkDeleteCertificateDialog.test.tsx` + +The current `certs` fixture (lines 28–30) covers: `custom/valid`, `letsencrypt-staging/untrusted`, `letsencrypt/expired`. No `expiring` fixture exists. + +**Add** a standalone test verifying `providerLabel` renders the correct label for an expiring cert: +```ts +it('renders "Expiring LE" label for expiring letsencrypt certificate', () => { + render( + + ) + expect(screen.getByText('Expiring LE')).toBeInTheDocument() +}) +``` + +--- + +## 4. Backend Verification + +**No backend change required.** + +`DELETE /api/v1/certificates/:id` is handled by `CertificateHandler.Delete` in `backend/internal/api/handlers/certificate_handler.go` (line 140). The only guard is `IsCertificateInUse` (line 156). There is no status-based check — the handler invokes `service.DeleteCertificate(uint(id))` unconditionally once the in-use check passes. + +`CertificateService.DeleteCertificate` in `backend/internal/services/certificate_service.go` (line 396) likewise only inspects `IsCertificateInUse` before proceeding. The cert's `Status` field is never read or compared during deletion. + +A cert with `status = 'expiring'` that is not in use is deleted successfully by the backend today; the bug is frontend-only. + +--- + +## 5. E2E Consideration + +**File:** `tests/certificate-bulk-delete.spec.ts` + +The spec currently has no fixture that places a cert into `expiring` status, because status is computed server-side at query time from the actual expiry date. Manufacturing an `expiring` cert in Playwright requires inserting a certificate whose expiry falls within the renewal window (≈30 days out). + +**Recommended addition:** Add a test scenario that uploads a custom certificate with an expiry date 15 days from now and is not attached to any proxy host, then asserts: +1. Its row checkbox is enabled and its per-row delete button is present. +2. It can be selected and bulk-deleted via `BulkDeleteCertificateDialog`. + +If producing a near-expiry cert at the E2E layer is not feasible, coverage from §3a and §3b is sufficient for this fix and the E2E test may be deferred to a follow-up. + +--- + +## 6. Commit Slicing Strategy + +**Single PR.** This is a self-contained bug fix with no API contract changes and no migration. + +Suggested commit message: +``` +fix(certificates): allow deletion of expiring certs not in use + +Expiring Let's Encrypt certs not attached to any proxy host were +undeletable because isDeletable only permitted deletion when +status === 'expired'. Extends the condition to also allow +status === 'expiring', updates providerLabel and getWarningKey for +consistent UX, and corrects the existing unit test that was asserting +the wrong behavior. +``` + +**Files changed:** +- `frontend/src/components/CertificateList.tsx` +- `frontend/src/components/dialogs/BulkDeleteCertificateDialog.tsx` +- `frontend/src/components/dialogs/DeleteCertificateDialog.tsx` +- `frontend/src/locales/en/translation.json` +- `frontend/src/components/__tests__/CertificateList.test.tsx` +- `frontend/src/components/dialogs/__tests__/BulkDeleteCertificateDialog.test.tsx` diff --git a/frontend/src/components/CertificateList.tsx b/frontend/src/components/CertificateList.tsx index e4802324..cd35e821 100644 --- a/frontend/src/components/CertificateList.tsx +++ b/frontend/src/components/CertificateList.tsx @@ -20,6 +20,7 @@ type SortColumn = 'name' | 'expires' type SortDirection = 'asc' | 'desc' export function isInUse(cert: Certificate, hosts: ProxyHost[]): boolean { + if (!cert.id) return false return hosts.some(h => (h.certificate_id ?? h.certificate?.id) === cert.id) } @@ -29,7 +30,8 @@ export function isDeletable(cert: Certificate, hosts: ProxyHost[]): boolean { return ( cert.provider === 'custom' || cert.provider === 'letsencrypt-staging' || - cert.status === 'expired' + cert.status === 'expired' || + cert.status === 'expiring' ) } @@ -234,7 +236,7 @@ export default function CertificateList() { sortedCertificates.map((cert) => { const inUse = isInUse(cert, hosts) const deletable = isDeletable(cert, hosts) - const isInUseDeletableCategory = inUse && (cert.provider === 'custom' || cert.provider === 'letsencrypt-staging' || cert.status === 'expired') + const isInUseDeletableCategory = inUse && (cert.provider === 'custom' || cert.provider === 'letsencrypt-staging' || cert.status === 'expired' || cert.status === 'expiring') return (