Skip to content

Commit 49b3e4e

Browse files
actions-useractions-user
committed
fix(tests): resolve i18n mock issues in BulkDeleteCertificateDialog tests
Removed local i18n mock to allow global mock to function correctly, updated assertions to use resolved English translations for better consistency in test outcomes.
1 parent ca477c4 commit 49b3e4e

2 files changed

Lines changed: 153 additions & 175 deletions

File tree

‎docs/plans/current_spec.md‎

Lines changed: 148 additions & 163 deletions
Original file line numberDiff line numberDiff line change
@@ -1,219 +1,204 @@
1-
# Fix: Allow deletion of expiring_soon certificates not in use
1+
# Fix: Frontend Unit Test i18n Failures in BulkDeleteCertificateDialog
22

3-
> **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'`.
3+
> **Status:** Ready for implementation
4+
> **Severity:** CI-blocking (2 test failures)
5+
> **Scope:** Single test file change
46
57
---
68

7-
## 1. Bug Root Cause
9+
## 1. Introduction
810

9-
`frontend/src/components/CertificateList.tsx` — `isDeletable` (lines 26–34):
11+
Two frontend unit tests fail in CI because `BulkDeleteCertificateDialog.test.tsx` contains a local `vi.mock('react-i18next')` that overrides the global mock in the test setup. The local mock returns raw translation keys and JSON-serialized options instead of resolved English strings, causing assertion mismatches.
1012

11-
```ts
12-
export function isDeletable(cert: Certificate, hosts: ProxyHost[]): boolean {
13-
if (!cert.id) return false
14-
if (isInUse(cert, hosts)) return false
15-
return (
16-
cert.provider === 'custom' ||
17-
cert.provider === 'letsencrypt-staging' ||
18-
cert.status === 'expired'
19-
)
20-
}
21-
```
13+
### Objectives
2214

23-
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.
15+
- Fix the 2 failing tests in CI
16+
- Align `BulkDeleteCertificateDialog.test.tsx` with the project's established i18n test pattern
17+
- No behavioral or component changes required
2418

25-
**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`.
19+
---
2620

27-
Three secondary locations propagate the same status blind-spot:
21+
## 2. Research Findings
2822

29-
- `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.
30-
- `frontend/src/components/dialogs/DeleteCertificateDialog.tsx` — `getWarningKey` falls through to `'certificates.deleteConfirmCustom'` for expiring certs instead of a contextual message.
23+
### 2.1 Failing Tests (from CI log)
3124

32-
---
25+
| # | Test Name | Expected | Actual (DOM) |
26+
|---|-----------|----------|--------------|
27+
| 1 | `lists each certificate name in the scrollable list` | `"Custom"`, `"Staging"`, `"Expired LE"` | `certificates.providerCustom`, `certificates.providerStaging`, `certificates.providerExpiredLE` |
28+
| 2 | `renders "Expiring LE" label for a letsencrypt cert with status expiring` | `"Expiring LE"` | `certificates.providerExpiringLE` |
3329

34-
## 2. Frontend Fix
30+
Additional rendering artifacts visible in the DOM dump:
3531

36-
### 2a. `frontend/src/components/CertificateList.tsx` — `isDeletable`
32+
- Dialog title: `{"count":3}` instead of `"Delete 3 Certificate(s)"`
33+
- Button text: `{"count":3}` instead of `"Delete 3 Certificate(s)"`
34+
- Cancel button: `common.cancel` instead of `"Cancel"`
35+
- Warning text: `certificates.bulkDeleteConfirm` instead of translated string
36+
- Aria label: `certificates.bulkDeleteListAriaLabel` instead of translated string
3737

38-
**Before:**
39-
```ts
40-
return (
41-
cert.provider === 'custom' ||
42-
cert.provider === 'letsencrypt-staging' ||
43-
cert.status === 'expired'
44-
)
45-
```
38+
### 2.2 Relevant File Paths
4639

47-
**After:**
48-
```ts
49-
return (
50-
cert.provider === 'custom' ||
51-
cert.provider === 'letsencrypt-staging' ||
52-
cert.status === 'expired' ||
53-
cert.status === 'expiring'
54-
)
55-
```
40+
| File | Role |
41+
|------|------|
42+
| `frontend/src/components/dialogs/__tests__/BulkDeleteCertificateDialog.test.tsx` | **Failing test file** — contains the problematic local mock |
43+
| `frontend/src/components/dialogs/BulkDeleteCertificateDialog.tsx` | Component under test |
44+
| `frontend/src/test/setup.ts` | Global test setup with proper i18n mock (lines 20–60) |
45+
| `frontend/vitest.config.ts` | Vitest config — confirms `setupFiles: './src/test/setup.ts'` (line 24) |
46+
| `frontend/src/locales/en/translation.json` | English translations source |
5647

57-
### 2b. `frontend/src/components/dialogs/BulkDeleteCertificateDialog.tsx` — `providerLabel`
48+
### 2.3 i18n Mock Architecture
5849

59-
**Before:**
60-
```ts
61-
function providerLabel(cert: Certificate): string {
62-
if (cert.provider === 'letsencrypt-staging') return 'Staging'
63-
if (cert.provider === 'custom') return 'Custom'
64-
if (cert.status === 'expired') return 'Expired LE'
65-
return cert.provider
66-
}
67-
```
50+
**Global mock** (`frontend/src/test/setup.ts`, lines 20–60):
6851

69-
**After:**
70-
```ts
71-
function providerLabel(cert: Certificate): string {
72-
if (cert.provider === 'letsencrypt-staging') return 'Staging'
73-
if (cert.provider === 'custom') return 'Custom'
74-
if (cert.status === 'expired') return 'Expired LE'
75-
if (cert.status === 'expiring') return 'Expiring LE'
76-
return cert.provider
77-
}
78-
```
52+
- Dynamically imports `../locales/en/translation.json`
53+
- Implements `getTranslation(key)` that resolves dot-notation keys (e.g., `certificates.providerCustom` → `"Custom"`)
54+
- Handles `{{variable}}` interpolation via regex replacement
55+
- Applied automatically to all test files via `setupFiles` in vitest config
7956

80-
### 2c. `frontend/src/components/dialogs/DeleteCertificateDialog.tsx` — `getWarningKey`
57+
**Local mock** (`BulkDeleteCertificateDialog.test.tsx`, lines 9–14):
8158

82-
**Before:**
83-
```ts
84-
function getWarningKey(cert: Certificate): string {
85-
if (cert.status === 'expired') return 'certificates.deleteConfirmExpired'
86-
if (cert.provider === 'letsencrypt-staging') return 'certificates.deleteConfirmStaging'
87-
return 'certificates.deleteConfirmCustom'
88-
}
59+
```typescript
60+
vi.mock('react-i18next', () => ({
61+
useTranslation: () => ({
62+
t: (key: string, opts?: Record<string, unknown>) => (opts ? JSON.stringify(opts) : key),
63+
i18n: { language: 'en', changeLanguage: vi.fn() },
64+
}),
65+
}))
8966
```
9067

91-
**After:**
92-
```ts
93-
function getWarningKey(cert: Certificate): string {
94-
if (cert.status === 'expired') return 'certificates.deleteConfirmExpired'
95-
if (cert.status === 'expiring') return 'certificates.deleteConfirmExpiring'
96-
if (cert.provider === 'letsencrypt-staging') return 'certificates.deleteConfirmStaging'
97-
return 'certificates.deleteConfirmCustom'
98-
}
99-
```
68+
This local mock **overrides** the global mock because Vitest's `vi.mock()` at the file level takes precedence over the setup file's `vi.mock()`. It returns:
10069

101-
### 2d. `frontend/src/locales/en/translation.json` — two string updates
70+
- Raw key when no options: `t('certificates.providerCustom')` → `"certificates.providerCustom"`
71+
- JSON string when options present: `t('key', { count: 3 })` → `'{"count":3}'`
10272

103-
**Add** new key after `"deleteConfirmExpired"`:
104-
```json
105-
"deleteConfirmExpiring": "This certificate is expiring soon. It will be permanently removed and will not be auto-renewed.",
106-
```
73+
### 2.4 Translation Keys Required
10774

108-
**Update** `"noteText"` to reflect that expiring certs are now also deletable:
75+
From `frontend/src/locales/en/translation.json`:
10976

110-
**Before:**
111-
```json
112-
"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.",
113-
```
77+
| Key | English Value |
78+
|-----|---------------|
79+
| `certificates.bulkDeleteTitle` | `"Delete {{count}} Certificate(s)"` |
80+
| `certificates.bulkDeleteDescription` | `"Delete {{count}} certificate(s)"` |
81+
| `certificates.bulkDeleteConfirm` | `"The following certificates will be permanently deleted. The server creates a backup before each removal."` |
82+
| `certificates.bulkDeleteListAriaLabel` | `"Certificates to be deleted"` |
83+
| `certificates.bulkDeleteButton` | `"Delete {{count}} Certificate(s)"` |
84+
| `certificates.providerStaging` | `"Staging"` |
85+
| `certificates.providerCustom` | `"Custom"` |
86+
| `certificates.providerExpiredLE` | `"Expired LE"` |
87+
| `certificates.providerExpiringLE` | `"Expiring LE"` |
88+
| `common.cancel` | `"Cancel"` |
11489

115-
**After:**
116-
```json
117-
"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.",
118-
```
90+
All keys exist in the translation file. No missing translations.
91+
92+
### 2.5 Pattern Analysis — Other Test Files
93+
94+
20+ test files have local `vi.mock('react-i18next')` overrides. Most use `t: (key) => key` and assert against raw keys — this is internally consistent and **not failing**. The `BulkDeleteCertificateDialog.test.tsx` file is unique because its **assertions expect translated values** while its mock returns raw keys.
95+
96+
| File | Local Mock | Assertions | Status |
97+
|------|-----------|------------|--------|
98+
| `CertificateList.test.tsx` | `t: (key) => key` | Raw keys (`certificates.deleteTitle`) | Passing |
99+
| `Certificates.test.tsx` | Custom translations map | Translated values | Passing |
100+
| `AccessLists.test.tsx` | Custom translations map | Translated values | Passing |
101+
| **BulkDeleteCertificateDialog.test.tsx** | `t: (key, opts) => opts ? JSON.stringify(opts) : key` | **Mix of translated values AND raw keys** | **Failing** |
119102

120103
---
121104

122-
## 3. Unit Test Updates
105+
## 3. Root Cause Analysis
123106

124-
### 3a. `frontend/src/components/__tests__/CertificateList.test.tsx`
107+
**The local `vi.mock('react-i18next')` in `BulkDeleteCertificateDialog.test.tsx` returns raw translation keys, but the test assertions expect resolved English strings.**
125108

126-
**Existing test at line 152–155 — update assertion:**
109+
This is a mock/assertion mismatch introduced when the test was authored. The test expectations (`'Custom'`, `'Expiring LE'`) are correct for what the component should render, but the mock prevents translation resolution.
127110

128-
The test currently documents the wrong behavior:
111+
---
129112

130-
```ts
131-
it('returns false for expiring LE cert not in use', () => {
132-
const cert: Certificate = { id: 7, name: 'Exp', domain: 'd', issuer: 'LE', expires_at: '', status: 'expiring', provider: 'letsencrypt' }
133-
expect(isDeletable(cert, noHosts)).toBe(false) // wrong — must become true
134-
})
135-
```
113+
## 4. Technical Specification
136114

137-
**Changes:**
138-
- Rename description to `'returns true for expiring LE cert not in use'`
139-
- Change assertion to `toBe(true)`
115+
### 4.1 Fix: Remove Local Mock, Update Assertions
140116

141-
**Add** a new test case to guard the in-use path:
142-
```ts
143-
it('returns false for expiring LE cert in use', () => {
144-
const cert: Certificate = { id: 8, name: 'ExpUsed', domain: 'd', issuer: 'LE', expires_at: '', status: 'expiring', provider: 'letsencrypt' }
145-
expect(isDeletable(cert, withHost(8))).toBe(false)
146-
})
147-
```
117+
**File:** `frontend/src/components/dialogs/__tests__/BulkDeleteCertificateDialog.test.tsx`
148118

149-
### 3b. `frontend/src/components/dialogs/__tests__/BulkDeleteCertificateDialog.test.tsx`
150-
151-
The current `certs` fixture (lines 28–30) covers: `custom/valid`, `letsencrypt-staging/untrusted`, `letsencrypt/expired`. No `expiring` fixture exists.
152-
153-
**Add** a standalone test verifying `providerLabel` renders the correct label for an expiring cert:
154-
```ts
155-
it('renders "Expiring LE" label for expiring letsencrypt certificate', () => {
156-
render(
157-
<BulkDeleteCertificateDialog
158-
certificates={[makeCert({ id: 4, name: 'Cert Four', domain: 'four.example.com', provider: 'letsencrypt', status: 'expiring' })]}
159-
open={true}
160-
onConfirm={vi.fn()}
161-
onCancel={vi.fn()}
162-
isDeleting={false}
163-
/>
164-
)
165-
expect(screen.getByText('Expiring LE')).toBeInTheDocument()
166-
})
167-
```
119+
**Change 1 — Delete the local `vi.mock('react-i18next', ...)` block (lines 9–14)**
168120

169-
---
121+
Removing this allows the global mock from `setup.ts` to take effect, which properly resolves translation keys to English values with interpolation.
122+
123+
**Change 2 — Update assertions that relied on the local mock's behavior**
170124

171-
## 4. Backend Verification
125+
With the global mock active, translation calls resolve differently:
172126

173-
**No backend change required.**
127+
| Call in component | Local mock output | Global mock output |
128+
|-------------------|-------------------|--------------------|
129+
| `t('certificates.bulkDeleteTitle', { count: 3 })` | `'{"count":3}'` | `'Delete 3 Certificate(s)'` |
130+
| `t('certificates.bulkDeleteButton', { count: 3 })` | `'{"count":3}'` | `'Delete 3 Certificate(s)'` |
131+
| `t('certificates.bulkDeleteButton', { count: 1 })` | `'{"count":1}'` | `'Delete 1 Certificate(s)'` |
132+
| `t('common.cancel')` | `'common.cancel'` | `'Cancel'` |
133+
| `t('certificates.providerCustom')` | `'certificates.providerCustom'` | `'Custom'` |
134+
| `t('certificates.providerExpiringLE')` | `'certificates.providerExpiringLE'` | `'Expiring LE'` |
174135

175-
`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.
136+
Assertions to update:
176137

177-
`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.
138+
| Line | Old Assertion | New Assertion |
139+
|------|---------------|---------------|
140+
| ~48 | `getByRole('heading', { name: '{"count":3}' })` | `getByRole('heading', { name: 'Delete 3 Certificate(s)' })` |
141+
| ~82 | `getByRole('button', { name: '{"count":3}' })` | `getByRole('button', { name: 'Delete 3 Certificate(s)' })` |
142+
| ~95 | `getByRole('button', { name: 'common.cancel' })` | `getByRole('button', { name: 'Cancel' })` |
143+
| ~109 | `getByRole('button', { name: '{"count":3}' })` | `getByRole('button', { name: 'Delete 3 Certificate(s)' })` |
144+
| ~111 | `getByRole('button', { name: 'common.cancel' })` | `getByRole('button', { name: 'Cancel' })` |
178145

179-
A cert with `status = 'expiring'` that is not in use is deleted successfully by the backend today; the bug is frontend-only.
146+
The currently-failing assertions (`getByText('Custom')`, `getByText('Expiring LE')`, etc.) will pass without changes once the global mock is active.
147+
148+
### 4.2 Config File Review
149+
150+
| File | Finding |
151+
|------|---------|
152+
| `.gitignore` | No changes needed. Test artifacts, coverage outputs, and CI logs are properly excluded. |
153+
| `codecov.yml` | No changes needed. Test files (`**/__tests__/**`, `**/*.test.tsx`) and test setup (`**/vitest.config.ts`, `**/vitest.setup.ts`) are already excluded from coverage. |
154+
| `.dockerignore` | No changes needed. Test artifacts and coverage files are excluded from Docker builds. |
155+
| `Dockerfile` | No changes needed. No test files are copied into the production image. |
180156

181157
---
182158

183-
## 5. E2E Consideration
159+
## 5. Implementation Plan
184160

185-
**File:** `tests/certificate-bulk-delete.spec.ts`
161+
### Phase 1: Fix the Test File
186162

187-
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).
163+
**Single file edit:** `frontend/src/components/dialogs/__tests__/BulkDeleteCertificateDialog.test.tsx`
188164

189-
**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:
190-
1. Its row checkbox is enabled and its per-row delete button is present.
191-
2. It can be selected and bulk-deleted via `BulkDeleteCertificateDialog`.
165+
1. Remove the local `vi.mock('react-i18next', ...)` block (lines 9–14)
166+
2. Update 5 assertion strings to use resolved English translations (see table in §4.1)
167+
3. No other files need changes
192168

193-
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.
169+
### Phase 2: Validation
170+
171+
1. Run the specific test file: `cd /projects/Charon/frontend && npx vitest run src/components/dialogs/__tests__/BulkDeleteCertificateDialog.test.tsx`
172+
2. Run the full frontend test suite: `cd /projects/Charon/frontend && npx vitest run`
173+
3. Verify no regressions in other test files
194174

195175
---
196176

197-
## 6. Commit Slicing Strategy
177+
## 6. Acceptance Criteria
198178

199-
**Single PR.** This is a self-contained bug fix with no API contract changes and no migration.
179+
- [ ] Both failing tests pass: `lists each certificate name in the scrollable list` and `renders "Expiring LE" label for a letsencrypt cert with status expiring`
180+
- [ ] All 7 tests in `BulkDeleteCertificateDialog.test.tsx` pass
181+
- [ ] Full frontend test suite passes with no new failures
182+
- [ ] No local `vi.mock('react-i18next')` remains in `BulkDeleteCertificateDialog.test.tsx`
200183

201-
Suggested commit message:
202-
```
203-
fix(certificates): allow deletion of expiring certs not in use
204-
205-
Expiring Let's Encrypt certs not attached to any proxy host were
206-
undeletable because isDeletable only permitted deletion when
207-
status === 'expired'. Extends the condition to also allow
208-
status === 'expiring', updates providerLabel and getWarningKey for
209-
consistent UX, and corrects the existing unit test that was asserting
210-
the wrong behavior.
211-
```
184+
---
185+
186+
## 7. Commit Slicing Strategy
187+
188+
### Decision: Single PR
189+
190+
**Rationale:** This is a single-file fix with no cross-domain changes, no schema changes, no API changes, and no risk of affecting other components. The change is purely correcting assertion/mock alignment in one test file.
191+
192+
### PR-1: Fix BulkDeleteCertificateDialog i18n test mock
193+
194+
| Attribute | Value |
195+
|-----------|-------|
196+
| **Scope** | Remove local i18n mock override, update 5 assertions |
197+
| **Files** | `frontend/src/components/dialogs/__tests__/BulkDeleteCertificateDialog.test.tsx` |
198+
| **Dependencies** | None |
199+
| **Validation Gate** | All 7 tests in the file pass; full frontend suite green |
200+
| **Rollback** | Revert single commit |
201+
202+
### Contingency
212203

213-
**Files changed:**
214-
- `frontend/src/components/CertificateList.tsx`
215-
- `frontend/src/components/dialogs/BulkDeleteCertificateDialog.tsx`
216-
- `frontend/src/components/dialogs/DeleteCertificateDialog.tsx`
217-
- `frontend/src/locales/en/translation.json`
218-
- `frontend/src/components/__tests__/CertificateList.test.tsx`
219-
- `frontend/src/components/dialogs/__tests__/BulkDeleteCertificateDialog.test.tsx`
204+
If the global mock from `setup.ts` does not resolve all keys correctly (unlikely given the translation JSON analysis), the fallback is to replace the local mock with a custom translations map pattern (as used in `AccessLists.test.tsx` and `Certificates.test.tsx`) containing the exact keys needed by this component.

0 commit comments

Comments
 (0)