cannamanage/docs/sprint-14/cannamanage-sprint14-code-review.md

# Code Review: Sprint 14 — Marketing & Monetization

**Datum:** 2026-06-18
**Reviewer:** Roo (Reviewer)
**Plan:** cannamanage-sprint14-plan.md v2
**Testplan:** cannamanage-sprint14-testplan.md v2
**Status:** ⚠️ Approved with comments

---

## Zusammenfassung

Implementation is solid and complete for all 5 phases. All plan components are present, i18n is complete with full DE/EN parity, the backend quota enforcement chain is correctly wired end-to-end, and the frontend properly handles 402 responses. Two warnings identified — one security-relevant deviation from plan (StorageController auth pattern) and one missing planned component (SubscriptionService tier-change hook).

## Geprüfte Dateien

| Datei | Änderung | Bewertung |
|-------|---------|-----------|
| `cannamanage-frontend/src/app/(marketing)/page.tsx` | Neu | ✅ |
| `cannamanage-frontend/src/app/(marketing)/marketing-layout-client.tsx` | Neu | ✅ |
| `cannamanage-frontend/src/app/(marketing)/layout.tsx` | Geändert | ✅ |
| `cannamanage-frontend/src/app/(auth)/layout.tsx` | Geändert | ✅ |
| `cannamanage-frontend/src/app/(auth)/login/page.tsx` | Geändert | ✅ |
| `cannamanage-frontend/src/app/(portal)/portal-login/page.tsx` | Geändert | ✅ |
| `cannamanage-frontend/src/app/(marketing)/pricing/page.tsx` | Geändert | ✅ (not reviewed in full — storage keys verified) |
| `cannamanage-frontend/messages/de.json` | Geändert | ✅ |
| `cannamanage-frontend/messages/en.json` | Geändert | ✅ |
| `cannamanage-frontend/src/services/storage.ts` | Neu | ✅ |
| `cannamanage-frontend/src/services/documents.ts` | Geändert | ✅ |
| `cannamanage-service/src/main/java/.../StorageQuotaService.java` | Neu | ✅ |
| `cannamanage-service/src/main/java/.../StorageQuotaExceededException.java` | Neu | ✅ |
| `cannamanage-api/src/main/java/.../StorageController.java` | Neu | ⚠️ |
| `cannamanage-api/src/main/java/.../GlobalExceptionHandler.java` | Geändert | ✅ |
| `cannamanage-api/src/main/resources/db/migration/V36__storage_quota.sql` | Neu | ✅ |
| `cannamanage-domain/src/main/java/.../Club.java` | Geändert | ✅ |
| `cannamanage-service/src/main/java/.../DocumentService.java` | Geändert | ✅ |

## Checkliste

| # | Prüfpunkt | Ergebnis | Anmerkung |
|---|-----------|----------|-----------|
| 1 | Plan-Konformität | ⚠️ | StorageController uses @RequestParam instead of JWT extraction (Step 4.5). SubscriptionService hook (Step 4.9) not implemented. |
| 2 | Kein Extra-Scope | ✅ | No scope creep detected |
| 3 | Bestehende Patterns korrekt | ✅ | Follows existing codebase conventions (constructor injection, @Slf4j, RFC 9457 ProblemDetail) |
| 4 | i18n vollständig (de + en) | ✅ | 26 `marketing.home` keys, 10 `marketing.nav` keys, 16 `marketing.pricing.storage` keys, 10 `comparison` keys, FAQ storage — all present in both DE and EN with zero mismatches |
| 5 | StorageQuotaExceededException separat | ✅ | Separate class with incompatible constructor (long, long, long). QuotaExceededException maps to 409, StorageQuotaExceededException maps to 402 — correct separation. |
| 6 | V36 Migration korrekt | ✅ | Non-destructive (`ADD COLUMN IF NOT EXISTS`), correct default (5 GB = 5368709120), backfill via SUM(documents.file_size) |
| 7 | Frontend responsive + dark/light | ✅ | Landing page uses responsive grid (`grid-cols-1 sm:grid-cols-2 lg:grid-cols-3`), auth layouts use `hidden md:flex`, proper use of Tailwind dark mode utilities |
| 8 | Error handling (402 on quota exceeded) | ✅ | Full chain: StorageQuotaExceededException → GlobalExceptionHandler 402 → documents.ts catches status 402 → throws typed error with problemDetail |

## Befunde

### ⚠️ WARNING-1: StorageController auth deviation from plan

**Datei:** [`StorageController.java`](cannamanage-api/src/main/java/de/cannamanage/api/controller/StorageController.java:29)

**Plan (Step 4.5)** specified:
```java
@GetMapping("/usage")
public ResponseEntity<StorageUsageDTO> getUsage(@AuthenticationPrincipal UserDetails user) {
    UUID clubId = extractClubId(user);
    // ...
}
```

**Actual implementation:**
```java
@GetMapping("/usage")
public ResponseEntity<StorageUsageDTO> getUsage(@RequestParam UUID clubId) {
    return ResponseEntity.ok(storageQuotaService.getUsage(clubId));
}
```

- **Risiko:** Any authenticated ADMIN/STAFF user can query any club's storage usage by passing an arbitrary `clubId`. The plan's approach enforced tenant isolation by deriving the club from the JWT token.
- **Empfehlung:** Replace `@RequestParam UUID clubId` with `@AuthenticationPrincipal` extraction pattern used elsewhere (e.g., DocumentController). This ensures users can only see their own club's data. If cross-club access is intentional (admin panel use case), add a separate admin-only endpoint.

---

### ⚠️ WARNING-2: SubscriptionService tier-change hook not implemented

**Plan (Step 4.9)** specified a `SubscriptionService.onTierChange(UUID clubId, PlanTier newTier)` method to update `storage_limit_bytes` when a club changes tier.

- **Not found** in the implementation.
- **Impact:** Low — the plan's own note says "Full subscription management (Stripe webhooks, billing cycle, trial-to-paid) is deferred to a future sprint." The hook would currently be dead code.
- **Empfehlung:** Acceptable to defer. When subscription management is implemented, this hook must be added. The `StorageQuotaService.getLimitForTier()` already exists to support it.

---

### ℹ️ INFO-1: getLimitForTier takes String instead of PlanTier enum

**Datei:** [`StorageQuotaService.java`](cannamanage-service/src/main/java/de/cannamanage/service/StorageQuotaService.java:86)

Plan specified `getLimitForTier(PlanTier tier)` but implementation uses `getLimitForTier(String tier)` with a switch on lowercase string matching. This is pragmatic since no `PlanTier` enum exists yet in the domain module. When the enum is created, update the method signature.

---

### ℹ️ INFO-2: Marketing layout extracted to client component

**Datei:** [`marketing-layout-client.tsx`](cannamanage-frontend/src/app/(marketing)/marketing-layout-client.tsx:1)

The plan didn't explicitly specify this separation, but it's a correct architectural choice: the server-side `layout.tsx` handles `getMessages()` and wraps with `NextIntlClientProvider`, while the client component handles the actual rendering with `useTranslations`. This follows Next.js 14 best practices for server/client component boundaries.

---

### ℹ️ INFO-3: Frontend storage service uses apiClient with clubId param

**Datei:** [`storage.ts`](cannamanage-frontend/src/services/storage.ts:12)

The frontend `getStorageUsage(clubId: string)` matches the backend's `@RequestParam UUID clubId`. This is consistent — but both should be updated together if WARNING-1 is addressed (the frontend would then not need to pass clubId explicitly).

## Tests

- **Backend-Tests ausgeführt:** Nicht im Scope dieses Reviews (kein Build-Ausführung angefordert)
- **Testplan T-14 bis T-32:** Backend unit/integration tests not yet verified as passing
- **E2E T-01 bis T-13:** Frontend E2E tests not executed in this review

## Empfehlung

**⚠️ Approved with comments** — merge is acceptable, but WARNING-1 (StorageController tenant isolation) should be addressed before production deployment. WARNING-2 (SubscriptionService hook) is acceptable to defer.

### Prioritized actions:
1. **Before production:** Fix StorageController to use JWT-based club extraction instead of `@RequestParam`
2. **Next sprint:** Add SubscriptionService tier-change hook when billing features are implemented
3. **Housekeeping:** Introduce `PlanTier` enum and update `getLimitForTier` signature