- Landing page with hero, feature grid, trust signals - Split-layout login redesign (admin + portal) - Pricing page with storage tiers (5GB/50GB/unlimited) - StorageQuotaService backend (V36 migration, 402 on exceeded) - Frontend storage integration + 402 error handling - StorageController uses TenantContext for tenant isolation - onTierChange() hook for subscription tier updates
This commit is contained in:
Patrick Plate
@@ -0,0 +1,124 @@
|
||||
# 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
|
||||
Reference in New Issue
Block a user