cannamanage/docs/sprint-12/cannamanage-sprint12-code-review.md

# Code Review: CannaManage Sprint 12

**Date:** 18.06.2026  
**Reviewer:** Roo (Reviewer)  
**Branch:** main (uncommitted working tree changes)  
**Status:** ⚠️ Approved with comments — 2 blockers, 4 warnings, 3 suggestions

---

## Summary

Sprint 12 delivers two phases: (1) Documents + Board page rewrites with React Query integration and dual-mode pattern, (2) Full integration test infrastructure with Docker, Playwright, and deterministic seed data. The architecture is solid and patterns are consistent. However, there are **2 blockers** that will prevent tests from running: a credential mismatch in global-setup.ts and missing `data-testid` attributes in the actual components.

## Changed Files

| File | Change | Rating |
|------|--------|--------|
| `cannamanage-frontend/src/services/documents.ts` | Modified | ✅ |
| `cannamanage-frontend/src/services/board.ts` | New | ✅ |
| `cannamanage-frontend/src/app/(dashboard-layout)/documents/page.tsx` | Modified | ⚠️ |
| `cannamanage-frontend/src/app/(dashboard-layout)/board/page.tsx` | Modified | ⚠️ |
| `cannamanage-api/.../TestResetController.java` | New | ✅ |
| `cannamanage-api/.../application-test.properties` | New | ⚠️ |
| `cannamanage-api/.../db/testdata/R__seed_test_data.sql` | New | ✅ |
| `docker-compose.test.yml` | Modified | ⚠️ |
| `docker-compose.test.local.yml` | New | ✅ |
| `cannamanage-frontend/Dockerfile.playwright` | New | ✅ |
| `cannamanage-frontend/playwright.config.ts` | Modified | ✅ |
| `cannamanage-frontend/e2e/global-setup.ts` | Modified | ❌ |
| `cannamanage-frontend/e2e/seed-constants.ts` | New | ✅ |
| `cannamanage-frontend/e2e/selectors.ts` | New | ⚠️ |
| `cannamanage-frontend/e2e/api-client.ts` | New | ✅ |
| `cannamanage-frontend/e2e/integration/*.spec.ts` (13 files) | New | ✅ |

## Checklist

| # | Check | Result | Notes |
|---|-------|--------|-------|
| 1 | Plan compliance | ✅ | All plan items implemented |
| 2 | React patterns | ✅ | Proper React Query mutations, `onSuccess`/`onError` handlers, query invalidation |
| 3 | TypeScript | ✅ | No `any` types in new code (except `api-client.ts` — see W-4), proper interfaces |
| 4 | Error handling | ✅ | try/catch on async ops, toast feedback consistent |
| 5 | Security | ✅ | TestResetController properly gated by `@ConditionalOnProperty` |
| 6 | SQL correctness | ✅ | `ON CONFLICT DO NOTHING` throughout, valid PostgreSQL |
| 7 | Docker | ⚠️ | Credential mismatches between properties and compose (W-1) |
| 8 | Test quality | ⚠️ | Good patterns overall, but `data-testid` attrs missing (B-2) |
| 9 | i18n | ⚠️ | Several hardcoded German strings in AlertDialogs (W-2) |
| 10 | Accessibility | ✅ | Buttons have proper labels, form inputs have Label components |
| 11 | Mock mode | ✅ | Dual-mode pattern is consistent across both pages |
| 12 | Build passes | ⏭️ | Not verified in review (no local run) |

---

## Findings

### ❌ Blockers (must fix before merge)

#### B-1: Login credentials mismatch between global-setup and seed data

**[`cannamanage-frontend/e2e/global-setup.ts`](cannamanage-frontend/e2e/global-setup.ts:46)**

The global-setup authenticates with:
```typescript
await page.fill('input[name="email"]...', "admin@test.de")
await page.fill('input[name="password"]...', "test123")
```

But the seed data creates the admin user with:
- Email: `admin@gruener-daumen.de` (in `R__seed_test_data.sql`)
- Password: `TestAdmin123!` (in `seed-constants.ts`)

This means **all integration tests will fail at the setup phase** because authentication will not succeed against the seeded database.

**Fix:** Update `global-setup.ts` to use `SEED.admin.email` and `SEED.admin.password`:
```typescript
import { SEED } from "./seed-constants"
await page.fill('...', SEED.admin.email)
await page.fill('...', SEED.admin.password)
```

---

#### B-2: `data-testid` attributes not present in components

**[`cannamanage-frontend/e2e/selectors.ts`](cannamanage-frontend/e2e/selectors.ts:8)**

The `SEL` object defines selectors like `[data-testid="documents-upload-button"]`, `[data-testid="documents-download-f1000000..."]`, etc. However, the actual components in `documents/page.tsx` and `board/page.tsx` **do not have any `data-testid` attributes**. 

All spec files using `page.locator(SEL.documents.uploadButton)` etc. will fail with element-not-found errors.

**Note:** The `selectors.ts` file itself comments (line 7): _"The actual data-testid attributes will be added incrementally to frontend components during Phase 2E as tests are written."_ — but this Phase 2E work was not done yet. Either:
1. Add the `data-testid` attributes to the components now, or
2. Rewrite specs to use accessible selectors (role, label, text) instead

---

### ⚠️ Warnings (should fix)

#### W-1: Credential/secret mismatch between application-test.properties and docker-compose.test.yml

**[`cannamanage-api/src/main/resources/application-test.properties`](cannamanage-api/src/main/resources/application-test.properties:9)** vs **[`docker-compose.test.yml`](docker-compose.test.yml:26)**

| Setting | application-test.properties | docker-compose.test.yml |
|---------|---------------------------|------------------------|
| DB username | `cannamanage_test` | `cannamanage` |
| DB password | `test_password` | `cannamanage_test` |
| JWT secret | `...LXRlc3RzLW9ubHktMzJi` | `...LXRlc3RzLW9ubHktMzJjaGFycw==` |

In Docker, env vars override properties, so Docker-based tests will work. But anyone running `./mvnw test -Ptest` locally against the Docker-compose DB will hit authentication failures.

**Fix:** Align the properties file with the Docker compose values, or add a comment explaining the intentional override.

---

#### W-2: Hardcoded German strings in AlertDialogs (i18n gap)

**[`cannamanage-frontend/src/app/(dashboard-layout)/documents/page.tsx`](cannamanage-frontend/src/app/(dashboard-layout)/documents/page.tsx:568)**

Multiple hardcoded strings bypass the i18n system:
- Line 447: `"Wird hochgeladen..."`
- Line 568: `"Dokument löschen?"`
- Line 570: `"Möchtest du...wirklich löschen?"`
- Line 575: `"Abbrechen"`
- Line 580: `"Löschen..."` / `"Löschen"`

Same pattern in `board/page.tsx`:
- Line 400: `"Wird gespeichert..."`
- Line 587: `"Vorstandsmitglied entfernen?"`
- Lines 593–599: `"Abbrechen"`, `"Entfernen..."`

**Fix:** Use `t("confirmDelete")`, `t("cancel")`, etc. from the `useTranslations` hook already imported.

---

#### W-3: Board page member select shows hardcoded mock options in API mode

**[`cannamanage-frontend/src/app/(dashboard-layout)/board/page.tsx`](cannamanage-frontend/src/app/(dashboard-layout)/board/page.tsx:439)**

The "Elect Member" dialog always shows only 3 hardcoded `<option>` elements:
```tsx
<option value="m1">Max Mustermann</option>
<option value="m2">Anna Schmidt</option>
<option value="m3">Peter Weber</option>
```

This is acceptable for mock mode, but should load real members from the API when not in mock mode.

---

#### W-4: `any` return types in api-client.ts

**[`cannamanage-frontend/e2e/api-client.ts`](cannamanage-frontend/e2e/api-client.ts:29)**

All public methods return `Promise<any>`. Since this is test infrastructure, it's less critical, but typed responses would make spec assertions more robust.

---

### ℹ️ Suggestions (nice to have)

#### S-1: global-setup.ts uses `waitForTimeout(1000)` 

**[`cannamanage-frontend/e2e/global-setup.ts`](cannamanage-frontend/e2e/global-setup.ts:57)**

While generally acceptable for auth state setup, consider replacing with `waitForLoadState("networkidle")` for more deterministic behavior.

---

#### S-2: Docker compose frontend service missing healthcheck

**[`docker-compose.test.yml`](docker-compose.test.yml:40)**

The `frontend` service uses `condition: service_started` but has no healthcheck defined. The Playwright container compensates with a wget loop (line 76–77), but a proper healthcheck would be cleaner and allow `condition: service_healthy`.

---

#### S-3: Consider extracting shared mock data to a constants file

Both `documents/page.tsx` (lines 69–135) and `board/page.tsx` (lines 42–158) contain substantial mock data inline. Consider moving these to `src/data/mock/documents.ts` and `src/data/mock/board.ts` to match the existing pattern (`src/data/mock/dashboard.ts`, `src/data/mock/members.ts`, etc.).

---

## Architecture Highlights (Positive)

- **Dual-mode pattern** is well-executed: `const isMockMode = !data` → graceful fallback, operations work locally without backend
- **TestResetController** uses `@ConditionalOnProperty` — correct Spring Boot pattern, impossible to accidentally activate in prod
- **Repeatable migration** (`R__seed_test_data.sql`) with `ON CONFLICT DO NOTHING` — properly idempotent
- **Playwright 3-project config** (setup → authenticated → integration) is a solid pattern
- **Seed constants as single source of truth** — avoids magic strings in specs
- **Docker tmpfs for test DB** — fast, ephemeral, no disk pollution
- **ApiClient with DB reset in `beforeEach`** — ensures test isolation

## Recommendation

**⚠️ Approved with comments** — Fix the 2 blockers (B-1 credential mismatch, B-2 missing data-testid attributes) before running integration tests. The i18n warnings (W-2) and credential alignment (W-1) should be addressed before final merge but are not blocking functionality.