# Code Review: User Preferences API

## Summary
**Overall Assessment: NEEDS_FIX**

The implementation is solid overall — clean hexagonal architecture, good test coverage, correct merge semantics, and proper error handling. However, there are two blockers related to the `UpdatedAt` time format (loses sub-second precision and timezone info) and the handler's manual JSON decoding approach bypassing the framework's `DecodeJSONStrict` for unknown-field rejection. There are also several warnings and suggestions.

## Findings

### Blockers

- [ ] `services/preferences-api/internal/api/handlers/preferences.go:81` **`UpdatedAt` formatted with hardcoded layout instead of `time.RFC3339Nano`** -- The format string `"2006-01-02T15:04:05Z"` always appends a literal `Z` regardless of the actual timezone, and truncates sub-second precision. Use `time.RFC3339Nano` (or at minimum `time.RFC3339`) which is the standard Go approach and matches the ISO 8601 format shown in the spec. While `UpdatedAt` is currently always set to UTC, this is a correctness issue that will bite when the code is copy-pasted or when the timestamp source changes.

- [ ] `services/preferences-api/internal/api/handlers/preferences.go:116-154` **Manual JSON decoding instead of using framework's `httpresponse.DecodeJSON` / `DecodeJSONStrict`** -- The handler manually decodes to `map[string]json.RawMessage`, then re-unmarshals the preferences payload. The framework provides `httpresponse.DecodeJSONStrict()` which handles unknown-field rejection, empty body, and malformed JSON in a consistent way. The current approach works but (a) duplicates framework logic, (b) doesn't use `app.BindAndValidate()` as specified in the design doc (AC/design says "Always use `app.Bind()` or `app.BindAndValidate()`"), and (c) the manual key-checking for the nested `preferences` object is fragile — adding a new preference key requires updating the `allowedKeys` map in the handler rather than leveraging struct-based validation. The CLAUDE.md critical rules state: "Always use `app.Bind()` or `app.BindAndValidate()`. Never use raw `json.NewDecoder`." While the handler uses `httpresponse.DecodeJSON` (not raw `json.NewDecoder`), it's still manually orchestrating decode steps the framework is designed to handle.

### Warnings

- [ ] `services/preferences-api/internal/adapter/memory/preferences.go:36-37` **Redundant copy of `Notifications` field** -- Lines `cp := *p` already copies all value-type fields including `Notifications` (which is a struct, not a pointer). The subsequent `cp.Notifications = p.Notifications` is a no-op. Same issue on lines 46-47 in `Upsert`. Not a bug, but misleading — suggests the author intended deep-copy logic but `NotificationSettings` contains no reference types.

- [ ] `services/preferences-api/internal/api/handlers/preferences.go:183-192` **`authorizeAccess` silently allows all access when auth is disabled** -- When `auth.GetUser(ctx)` returns `nil` (no auth context), the function returns `nil` (allow). This is the intended behavior per the design (auth is opt-in), but the logic means that with auth disabled, *any* user can access *any other user's* preferences without restriction. This matches the design doc ("Auth is gated by `AUTH_ENABLED` config") but should be explicitly noted as a known security trade-off in local dev vs. production.

- [ ] `services/preferences-api/internal/service/preferences.go:17` **Logger type is `*logging.Logger` not `*slog.Logger`** -- The task spec (T5) says `NewPreferencesService(repo port.PreferencesRepository, logger *slog.Logger)` but the implementation uses `*logging.Logger`. This follows the actual codebase pattern (the `logging` package wraps slog), so the implementation is correct and the task spec was slightly wrong. No action needed, but noting the deviation.

- [ ] `services/preferences-api/internal/api/handlers/preferences_test.go` **Handler tests don't validate response envelope `meta` field** -- The spec requires responses in `{data, meta}` format. Tests check `data` but never assert the presence or structure of `meta` (with `request_id`, `timestamp`). This should be verified in at least one test.

- [ ] `services/preferences-api/internal/api/handlers/preferences_test.go:106-141` **`TestPreferences_Update_CreateNew` doesn't set auth context** -- The test creates a PUT request without calling `withAuthUser()`. This works because `authorizeAccess` returns nil when no user is in context, but the test isn't testing the authenticated create path — it's testing the "auth disabled" path. For a feature that specifies auth is required, at least one PUT success test should include auth context.

### Suggestions

- [ ] `services/preferences-api/internal/api/handlers/preferences_test.go` **Add test for PUT with `{"preferences": null}` (ER-9 from QA plan)** -- The QA plan lists this as error case ER-9 but no handler test covers it explicitly. The current code handles it (line 126 checks `string(prefsRaw) == "null"`), but a test would ensure this remains covered.

- [ ] `services/preferences-api/internal/api/handlers/preferences_test.go` **Add test for PUT with extra top-level fields (ER-20 from QA plan)** -- The QA plan lists `{"preferences": {"theme": "dark"}, "extra": true}` as an error case. The handler code does check for unknown top-level keys (lines 131-135), but there's no test exercising this path.

- [ ] `services/preferences-api/internal/domain/preferences_test.go` **Add test for `MergeFrom` with nil notifications but other fields set** -- QA plan UT-D9 specifies this case. The existing "nil update does nothing" test covers `nil` update entirely, but doesn't cover the case where `Theme` and `Language` are set while `Notifications` is `nil`.

- [ ] `services/preferences-api/internal/service/preferences_test.go` **Service tests use in-memory adapter instead of a mock** -- The task spec (T5) says "Tests use mock repository implementing `port.PreferencesRepository`" but the implementation uses the real in-memory adapter. This is a pragmatic choice (the adapter is simple), but coupling service tests to the adapter means adapter bugs could mask service bugs. For a feature this small it's acceptable.

## Spec Alignment

The implementation largely matches the spec. Key alignment points:

| Spec Requirement | Status |
|-----------------|--------|
| GET returns 200 with preferences | Implemented and tested |
| GET returns 404 when none exist | Implemented and tested |
| PUT creates on first call (upsert) | Implemented and tested |
| PUT merges with shallow merge | Implemented and tested |
| PUT returns full merged result | Implemented and tested |
| PUT validates `preferences` field present | Implemented and tested |
| PUT rejects unknown top-level preference keys | Implemented and tested |
| Both endpoints require auth | Implemented (conditional on `AUTH_ENABLED`) |
| Own-user access + admin override | Implemented and tested |
| `user_id` validated as UUID | Implemented and tested |
| In-memory persistence | Implemented and tested |
| OpenAPI spec documents both endpoints | Implemented |
| Domain validation for allowed values | Implemented and tested |
| Example scaffold removed | Confirmed (no example references remain) |

**Gaps:**
- The handler uses manual JSON decoding rather than `app.BindAndValidate()` as specified in both the design doc and CLAUDE.md critical rules.
- The time format for `updated_at` doesn't use standard Go time constants.

## Test Coverage Assessment

**23 tests total, all passing.** Coverage by acceptance criterion:

| AC | Criterion | Tests | Coverage |
|----|-----------|-------|----------|
| AC-1 | GET 200 | `TestPreferences_GetAfterUpdate` | Covered |
| AC-2 | GET 404 | `TestPreferences_Get_NotFound` | Covered |
| AC-3 | PUT upsert | `TestPreferences_Update_CreateNew` | Covered |
| AC-4 | PUT merge | `TestPreferences_Update_MergeExisting`, domain merge tests | Well covered |
| AC-5 | PUT returns full set | `TestPreferences_Update_CreateNew` (checks response) | Covered |
| AC-6 | PUT validates preferences present | `TestPreferences_Update_MissingPreferencesField`, `TestPreferences_Update_EmptyBody` | Covered |
| AC-7 | PUT rejects unknown keys | `TestPreferences_Update_UnknownPreferenceKey` | Covered |
| AC-8 | Auth required | Not directly tested (auth disabled in tests) | **Partial** |
| AC-9 | Own-user + admin | `TestPreferences_Get_Forbidden`, `TestPreferences_Get_AdminAccess`, `TestPreferences_Update_Forbidden` | Covered |
| AC-10 | UUID validation | `TestPreferences_Get_InvalidUUID`, `TestPreferences_Update_InvalidUUID` | Covered |
| AC-11 | In-memory persistence | Adapter tests + `TestPreferences_GetAfterUpdate` | Covered |
| AC-12 | OpenAPI spec | No automated test | **Not tested** |
| AC-13 | Domain validation rules | Domain tests (7 validation subtests) + `TestPreferences_Update_InvalidTheme` | Covered |
| AC-14 | Handler test coverage | 13 handler tests | Covered |
| AC-15 | Service test coverage | 9 service subtests | Covered |
| AC-16 | Example scaffold removed | Verified via grep | Confirmed |

**Missing test coverage:**
- AC-8: No test verifies that requests without auth tokens are rejected when auth is enabled
- AC-12: No test verifies OpenAPI spec renders correctly
- QA plan ER-9: No test for `{"preferences": null}`
- QA plan ER-20: No test for extra top-level fields outside `preferences`