Claude-assisted improvements (untested)
- dev auth flow (side-step OAuth) - db event processing integration tests - dev scripts (eg Makefile) - db / test db migration setup scripts.
This commit is contained in:
@@ -0,0 +1,24 @@
|
||||
# Work Summary — 2026-08-03 20:09
|
||||
|
||||
## Task
|
||||
Move hardcoded secrets (Auth0 client secret, Etsy API credentials, Postgres DSN) out of source and into environment configuration, as the first step toward a faster local dev/test loop.
|
||||
|
||||
## Context
|
||||
Audited the repo for iteration/testing friction. Found no `.env`/config layer at all — Auth0 client secret, Etsy API keystring/shared secret, and the Postgres connection string (including its password) were literal constants in `domains/authentication/auth.go`, `main.go`, and `database.go`, committed to git. This also blocked adding a dev-only auth bypass cleanly, since `authentication.New` had no way to accept alternate config.
|
||||
|
||||
## Changes
|
||||
- Added `config/config.go`: loads `.env` via `github.com/joho/godotenv`, reads required env vars, fails fast with a clear error naming any that are missing.
|
||||
- Added `.env` (gitignored, holds real local values so nothing broke) and `.env.example` (committed template).
|
||||
- `.gitignore`: added `.env`.
|
||||
- `database.go`: `newPool` now takes `databaseURL` as a parameter instead of a hardcoded DSN.
|
||||
- `domains/authentication/auth.go`: removed the `AUTH0_*` constants; `authentication.New` now takes `domain, clientID, clientSecret, callbackURL` as parameters; `Authenticator` gained a `domain` field used by `GetLogoutURL`.
|
||||
- `main.go`: calls `config.Load()` up front and threads values into `newPool`, `authentication.New`, and the Etsy `NewPlatform` call (replacing the hardcoded `etsyAPIKeystring`/`etsyAPISharedSecret` consts).
|
||||
- `go.mod`/`go.sum`: added `github.com/joho/godotenv`; `go mod tidy` also dropped a few unrelated stale indirect deps.
|
||||
|
||||
## Verification
|
||||
- `go build ./...` — clean.
|
||||
- `go run .` — boots against `.env`, registers all routes identically to before the change.
|
||||
|
||||
## Follow-ups / not done here
|
||||
- Secrets are still present in old git history (pre-existing commits) — not rotated or scrubbed. Worth rotating the Auth0 client secret and Etsy credentials at some point since repo history still exposes them.
|
||||
- Next planned step: add an env-gated dev-only auth bypass (mint a local session without going through real Auth0), now that config is externalized enough to support it cleanly.
|
||||
@@ -0,0 +1,25 @@
|
||||
# Work Summary — 2026-08-03 23:42
|
||||
|
||||
## Task
|
||||
Add a dev-only auth bypass so local testing doesn't require a real Auth0 login round-trip, per the dev-iteration plan from the previous session (see `work-summary-Claude-2026-08-03-2009.md`).
|
||||
|
||||
## Context
|
||||
The only way to get an authenticated session locally was to log into the real Auth0 tenant in a browser and copy the resulting `access_token` JWT cookie into `curl` commands by hand — evidenced by several one-off curl-with-pasted-cookie entries in `.claude/settings.local.json`. Auth is driven entirely by DB state: `Identify` middleware (`server/auth/auth.go`) looks up `access_token` in `oauth_tokens`, joined to `oauth_users` and `accounts` — there's no in-process session logic to fake, just rows to write.
|
||||
|
||||
Also discovered along the way: the `database_migrations/` `.sql` files are stale relative to the live schema — migration `000016` (still on disk) references a `claims JSONB NOT NULL` column and a `TEXT`-typed `id_token_custom_claims_updated_at`, but the live `oauth_tokens` table has no `claims` column at all and that column is actually `timestamptz`. Something changed the schema by hand at some point without updating the migration files. Didn't touch this — just noted it and built against the real live schema (confirmed via `psql \d oauth_tokens`).
|
||||
|
||||
## Changes
|
||||
- `config/config.go`: added optional `DevAuthEnabled bool`, parsed from `DEV_AUTH_ENABLED` (`strconv.ParseBool`; unset = false; invalid value = fail fast).
|
||||
- `domains/authentication/dev.go` (new): `Authenticator.DevLogin(ctx, userID, name)` generates a random `dev_`-prefixed token and writes real `oauth_users`/`oauth_tokens` rows (expiry set 1 year out, specifically to stay clear of the near-expiry auto-refresh path in `server/auth`, since a dev token has no real Auth0 refresh token behind it). Reuses the exact same tables real login writes to, so every downstream code path (identity lookup, account linking/creation, cookie handling) treats it identically to a real session — no special-cased "is this dev" branches anywhere else in the app.
|
||||
- `server/api/auth/router.go`: `Routes` takes a new `devAuthEnabled bool`. When true, logs a loud startup warning and registers `GET /api/auth/dev-login?user_id=...&name=...&target=...` (all params optional; different `user_id` values let you test multiple accounts side by side).
|
||||
- Threaded `devAuthEnabled` through `server/api/apis.go` → `server/server.go` (`NewRouter`) → `main.go` (`runServer`), sourced from `cfg.DevAuthEnabled`.
|
||||
- `.env` / `.env.example`: added `DEV_AUTH_ENABLED` (true in local `.env`, documented in `.env.example`).
|
||||
|
||||
## Verification
|
||||
- `go build ./...` clean.
|
||||
- Full manual end-to-end run against the real local Postgres: hit `/api/auth/dev-login?user_id=dev-smoke-test&target=/ui`, confirmed `Set-Cookie` on the response, followed up with `/ui` using that cookie and got a 200 with account-appropriate content (the "no account yet" state, matching what a real first-time login produces). Verified `oauth_users`/`oauth_tokens` rows landed correctly via `psql`, then deleted the test rows.
|
||||
- `go vet ./...` shows only two pre-existing unreachable-code warnings unrelated to this change (`domains/accounts/accounts.go:1472`, `server/sse/publisher.go:141`).
|
||||
|
||||
## Follow-ups / not done here
|
||||
- `database_migrations/*.sql` are out of sync with the live DB schema (see Context above) — worth reconciling at some point (either a migration that documents the drift, or regenerating migrations from the live schema) so `go:generate`'d diagrams and any fresh-DB setup aren't misleading.
|
||||
- Next planned step per the dev-iteration plan: a dev script/Makefile to bring up Postgres, run migrations, build Tailwind, and start the server in one command.
|
||||
@@ -0,0 +1,25 @@
|
||||
# Work Summary — 2026-08-03 23:58
|
||||
|
||||
## Task
|
||||
Add a `Makefile` with a `make dev` target: apply pending DB migrations, start the Tailwind watcher, and run the server, all from one command.
|
||||
|
||||
## Context
|
||||
User pushed back on the initial framing ("I can already start the server with `go run .`") — fair, since that part was never the friction. The real friction, confirmed via `.zsh_history`, is the *other* two steps done by hand around it: the `migrate` CLI invoked with its full DSN spelled out literally ~10 times over recent weeks (`migrate -path database_migrations -database "postgres://app_client:app_password@localhost:5432/inventory_2?sslmode=disable" up 1`), and `tailwind.sh` (which already runs `--watch`) needing to be started manually in a separate terminal — easy to forget, leading to template/CSS changes silently not showing up. Framed the Makefile's value around removing those two specific manual steps, not around wrapping `go run .`.
|
||||
|
||||
## Changes
|
||||
- New `Makefile` at repo root:
|
||||
- `-include .env` + `export` so `DATABASE_URL` (and everything else in `.env`) is available to recipes without hand-typing it — fixes the DSN-drift risk where the hand-typed migrate command could silently diverge from what's actually in `.env`.
|
||||
- `dev` (default goal): depends on `migrate-up`, then backgrounds `./tailwind.sh`, captures its PID, sets a trap to kill it on `EXIT`/`INT`/`TERM`, then runs `go run .` in the foreground. Ctrl-C (or any termination) stops both cleanly.
|
||||
- `migrate-up` / `migrate-down` / `migrate-version`: thin wrappers around the `migrate` CLI using `$(DATABASE_URL)`, so the DSN is typed once (in `.env`) instead of per-invocation.
|
||||
- `tailwind`: one-shot alias for `./tailwind.sh` (still watch mode, matching existing script behavior).
|
||||
- `run`: plain `go run .`, for when you don't want migrations/CSS touched.
|
||||
|
||||
## Verification
|
||||
- `make migrate-version` / `make migrate-up` — ran cleanly against the real local DB (idempotent: reported "no change" since already at the latest migration).
|
||||
- `make dev` under a `timeout` — confirmed via log output that migrations ran, Tailwind's watcher started (`tailwindcss v4.1.18` banner), the server bound and served a 200 on `/ui`, and on SIGTERM both the server and the Tailwind watcher shut down (verified no leftover `tailwindcss`/`npx` process survived — first check was a `pgrep` self-match false positive on the search string appearing in the invoking shell's own command line, re-verified cleanly with `ps aux`).
|
||||
|
||||
## Incident during verification (self-caused, fixed)
|
||||
Killing `make dev` mid-run once truncated the *committed* `styles/index.css` to empty (1370 lines → 0) — the Tailwind watcher was killed while mid-write on its first build. Caught it in the post-test `git status`/`git diff` review before finishing; restored via `git checkout -- styles/index.css`. Worth knowing for next time: killing the watcher while it's actively writing that file is a real (if narrow) way to corrupt a tracked file — not something `make dev` itself introduces (same risk exists running `tailwind.sh` directly), but noting it here since it's the kind of thing to double check after using this target.
|
||||
|
||||
## Follow-ups / not done here
|
||||
- Next planned step per the dev-iteration plan: broaden test coverage beyond `server/ui/charts/*_test.go` (nothing currently covers `domains/accounts`, `domains/reports`, `domains/raw_events`, or the Amazon mock pipeline).
|
||||
@@ -0,0 +1,34 @@
|
||||
# Work Summary — 2026-08-04 19:14
|
||||
|
||||
## Task
|
||||
Broaden test coverage beyond `server/ui/charts/*_test.go` (which was the only tested package). Final step in the dev-iteration plan.
|
||||
|
||||
## Context
|
||||
Surveyed `domains/accounts`, `domains/reports`, `domains/raw_events`, `domains/amazon` and found the codebase is almost entirely thin `Store` methods wrapping SQL - there's very little pure logic to unit test in isolation. The real risk lives in the queries themselves (already proved this by finding a live bug: `RefreshAccessToken` in `domains/authentication/auth.go:270` passes the empty named return `accessToken` instead of the `oldAccessToken` parameter to `getRefreshTokenForAccessToken` - token refresh is silently broken. Not fixed here, flagging for a decision - see Follow-ups.).
|
||||
|
||||
Asked the user how to test DB-heavy code; they chose a dedicated test database with the explicit ability to point the same suite at the real dev DB when wanted.
|
||||
|
||||
## Infra changes
|
||||
- Created `inventory_2_test` Postgres database (owned by `app_client`, matching `inventory_2`'s setup). The `angel` OS-peer-auth Postgres role has `CREATEDB`; `app_client` does not, so this had to be created out-of-band, not from app code.
|
||||
- **Found and fixed a real migration bug while doing this**: `database_migrations/000010_oauth_login_states.up.sql` had invalid SQL (`NOW() + 10 'minute'`) that fails on any fresh database - a hard blocker for setting up the test DB, and for anyone else spinning up this project from scratch. Fixed to `NOW() + '10 minutes'::interval`, matching what the live dev DB actually runs (confirmed via `psql \d`).
|
||||
- **Found and fixed schema drift**: migrating fresh revealed `oauth_tokens` in the migration files still has a `claims JSONB NOT NULL` column and a `TEXT`-typed `id_token_custom_claims_updated_at` - neither matches the live dev DB (no `claims` column at all; that column is `timestamptz`), and no application code reads/writes `claims`. Someone patched the dev DB by hand at some point without ever committing the migration. Added `database_migrations/000030_fix_oauth_tokens_schema_drift.{up,down}.sql` to close the gap, then `migrate force 30` on the *dev* DB (schema already matched, just needed the migration bookkeeping to catch up) and a normal `migrate up` on the new test DB. Verified both DBs now have an identical `oauth_tokens` shape (column ordering differs cosmetically, doesn't matter - the app scans by column name).
|
||||
- New `internal/testdb` package: `Pool(t)` connects via `TEST_DATABASE_URL` (skips the test if unset, so `go test ./...` doesn't hard-require Postgres), `Logger()` for a discard-output logger, `NewUserID(t)` for collision-safe fixture IDs, `SeedOAuthUser`/`SeedOAuthSession` for tests that need a valid `oauth_users`/`oauth_tokens` row. All seed helpers register `t.Cleanup` in FK-safe order.
|
||||
- `Makefile`: added `test` (migrates the test DB, then `go test ./...`), `test-against-dev-db` (same suite, `TEST_DATABASE_URL` overridden to `DATABASE_URL` for one run - the "option to run against the real database" the user asked for), and `migrate-test-up`/`down`/`version`.
|
||||
- `.env` / `.env.example`: added `TEST_DATABASE_URL`.
|
||||
|
||||
## Test coverage added
|
||||
- `domains/authentication/dev_test.go`: `DevLogin` round-trips through `GetAccessTokenClaimsAndExpiration` correctly, is safe to call twice for the same `user_id` (mints a new token each time), and `GetAccessTokenClaimsAndExpiration` returns `ErrNotFound` for an unknown token.
|
||||
- `domains/accounts/accounts_test.go`: `CreateAccount` happy path + round-trip through `GetAccount`; duplicate `user_id` returns `ErrConflict`; `GetAccount` on a missing ID returns `ErrNotFound`; `GetUserAndAccountByAccessToken` correctly resolves the user-but-no-account state and the user-with-account state (the same join logic the auth middleware depends on for every request).
|
||||
- `domains/raw_events/events_test.go`: `Save` + `LoadEventsForStore` round-trip, ordering (newest first), and the empty-store case.
|
||||
|
||||
## Verification
|
||||
- `make test`: all new tests pass against `inventory_2_test`. `make test-against-dev-db`: same suite, same results, against the real dev DB - confirmed zero leftover rows afterward (`SELECT count(*) FROM oauth_users WHERE user_id LIKE 'test-%'` → 0, same for `raw_store_events`).
|
||||
- `go build ./...` and `go vet ./...` clean except two pre-existing unreachable-code warnings unrelated to this work.
|
||||
- The only test failures are `ExampleBar`/`ExampleLineChart` in `server/ui/charts` - confirmed pre-existing and unrelated (verified via `git stash` that they fail identically on the pre-session code; see Incident below). Their hardcoded `// Output:` expectations are stale relative to the current chart-rendering code (missing padding/border/label styling that's since been added).
|
||||
|
||||
## Incident during verification (self-caused, recovered cleanly)
|
||||
Used `git stash` / `git stash pop` to check whether the chart test failures were pre-existing, and the pop conflicted on `server/ui/charts/test.svg` (a file the chart tests overwrite as a side effect of running - it was already dirty before this session even started). Resolved by discarding the working-tree's post-test-run `test.svg` (disposable scratch content) and re-running `git stash pop`, which then applied cleanly. Verified afterward with `go build ./...` and a full `git status` review that every change from this session (and the prior two) was intact. No work was lost, but noting it: don't reach for `git stash` casually in a working tree with substantial uncommitted work when a narrower check (e.g. `git worktree` or just re-reading the file) would do.
|
||||
|
||||
## Follow-ups / not done here
|
||||
- **`RefreshAccessToken` bug** (`domains/authentication/auth.go:270`): passes the empty named-return `accessToken` instead of the `oldAccessToken` parameter to `getRefreshTokenForAccessToken`, so token refresh looks up an empty string instead of the real token and will always fail to find a refresh token. This means the "refresh when access token is old enough" path in `server/auth/auth.go` (`AuthenticateHandler`) is currently broken for real (non-dev) sessions - users would get bounced to `/` with a "failed to refresh access token" body once their 48-hour ID token crosses the refresh floor, instead of transparently refreshing. Did not fix it in this pass since it's a production auth-flow behavior change, not a test-coverage change - flagging for an explicit decision. It's a one-line fix (`accessToken` → `oldAccessToken` on that call).
|
||||
- No coverage added for `domains/reports` or `domains/amazon` - `reports.GetRawShopEvents` needs a mock-shop fixture (depends on `accts.GetMockShop`, which lives in the much larger `mocks.go`/`sync_groups.go` surface) and `domains/amazon`'s mock event pipeline is stateful/background-process-shaped, both meaningfully bigger lifts than what fit in this pass.
|
||||
@@ -0,0 +1,20 @@
|
||||
# Work Summary — 2026-08-04 19:17
|
||||
|
||||
## Task
|
||||
Fix the `RefreshAccessToken` bug surfaced while adding test coverage (see `work-summaries/work-summary-Claude-2026-08-04-1914.md`), per explicit go-ahead.
|
||||
|
||||
## Change
|
||||
`domains/authentication/auth.go:257`: `RefreshAccessToken` was calling `a.getRefreshTokenForAccessToken(ctx, accessToken)`, where `accessToken` is the function's *empty named return value*, not the `oldAccessToken` parameter it clearly meant to use (both are `string`, so the compiler had nothing to catch). This meant every real (non-dev) token refresh looked up a refresh token for `""` instead of the actual expiring token, always failed, and sent the user back to `/` with a "failed to refresh access token" error instead of transparently refreshing their session.
|
||||
|
||||
Fixed by passing `oldAccessToken` instead:
|
||||
```go
|
||||
refreshToken, tokenType, err := a.getRefreshTokenForAccessToken(ctx, oldAccessToken)
|
||||
```
|
||||
|
||||
## Verification
|
||||
- `go build ./...` clean.
|
||||
- `go test ./domains/authentication/... ./domains/accounts/... ./domains/raw_events/...` — all pass, no regressions.
|
||||
- No new automated regression test for this specific bug: `RefreshAccessToken` calls `a.TokenSource(...).Token()`, which makes a real network call to Auth0 to redeem the refresh token - not mockable without adding an interface seam around `oauth2.Config`/`oidc.Provider`, which is a larger refactor than this fix warranted. The underlying query helper (`getRefreshTokenForAccessToken`) is already covered indirectly via `domains/authentication/dev_test.go`.
|
||||
|
||||
## Follow-ups / not done here
|
||||
- If this path matters enough to regression-test end-to-end, it'd need `oauth2.Config`'s token source made injectable/mockable - flagging as a possible future task, not doing it now.
|
||||
@@ -0,0 +1,28 @@
|
||||
# Work Summary — 2026-08-04 22:53
|
||||
|
||||
## Task
|
||||
Investigate and, if fixable, fix the two pre-existing failing tests (`ExampleBar`, `ExampleLineChart` in `server/ui/charts`) flagged during the test-coverage work.
|
||||
|
||||
## Investigation
|
||||
Read `bar.go`/`line.go` (the renderers) and their tests. Both the source files and their tests were introduced in a single commit, `c51ad80 "prototyped svg reports"` (`git log` shows no other commits touching either) - so this wasn't drift accumulated over time, it was the renderer being finished after (or without) the test's expected output ever being filled in, then committed as-is (the commit message itself says "prototyped").
|
||||
|
||||
The renderer code itself looks intentional and coherent, not buggy:
|
||||
- `Bar.SVG()` draws value labels above each bar and a rotated per-bar `Label` text, plus `padding`/`border-width` styling and `rounded-lg border-border` classes - all deliberate, readable code, not something that looks like an accident.
|
||||
- `LineChart.SVG()` draws a `polyline`, an `ellipse` marker at each point, and a (here, empty, since the test fixture sets no `Label`) text element per point via `upsideDownCenteredText` - also coherent.
|
||||
|
||||
The old `// Output:` expectations were a bare, unstyled rect list (`bar_test.go`) and a completely empty `<svg>...</svg>` shell with no children at all (`line_test.go`) - clearly placeholders from before the labeling/styling/point-rendering features existed, not a description of intended behavior that the code regressed from.
|
||||
|
||||
Conclusion: fixable, and the fix is "update the stale expected output to match the current, correct renderer" - not a renderer bug to chase.
|
||||
|
||||
## Change
|
||||
Regenerated both expectations from the renderers' actual current output (captured via `go test -v`, substituted into the test files with a small Python script rather than hand-typing ~3KB single-line SVG strings, to avoid transcription errors) and replaced the `// Output:` line in each file. One line changed per file.
|
||||
|
||||
## Verification
|
||||
- `go build ./...` clean.
|
||||
- `go test ./server/ui/charts/... -v`: both `ExampleBar` and `ExampleLineChart` pass.
|
||||
- `gofmt -l` on both changed files: clean.
|
||||
- `make test`: full suite now exits 0 (previously failed only on these two).
|
||||
- `go vet ./...`: same two pre-existing, unrelated `unreachable code` warnings as before (`domains/accounts/accounts.go:1472`, `server/sse/publisher.go:141`) - untouched by this change.
|
||||
|
||||
## Follow-ups / not done here
|
||||
- None specific to this fix. The dev-iteration plan (secrets, dev auth, `make dev`, test coverage, refresh-token bug, and now these) is fully closed out as of this session.
|
||||
@@ -0,0 +1,15 @@
|
||||
# Work Summary — 2026-08-04 22:56
|
||||
|
||||
## Task
|
||||
Address the two `go vet` "unreachable code" warnings that have been showing up alongside test runs since the test-coverage work started surfacing them.
|
||||
|
||||
## Changes
|
||||
Both were leftover dead `return` statements after the surrounding logic was later changed to already return on every path - simple deletions, no behavior change:
|
||||
|
||||
- `domains/accounts/accounts.go:1472` (in `SetListingInListingInMockSyncGroupBeingEdited`'s per-schema update loop): a trailing `return nil` after a `switch` whose three cases (`continue`/`return nil`/`return fmt.Errorf(...)`) already cover every value of `RowsAffected()`. Removed the dead line.
|
||||
- `server/sse/publisher.go:141` (`Push`): a `return nil` sitting after `return errors.Join(errs...)`, which already unconditionally returns. Removed the dead line - `errors.Join(errs...)` was the intended return value all along (returns `nil` itself when `errs` has no non-nil entries, so behavior is unchanged).
|
||||
|
||||
## Verification
|
||||
- `go build ./...` clean.
|
||||
- `go vet ./...` now fully clean (previously exactly these two warnings).
|
||||
- `make test`: full suite still green, no regressions.
|
||||
@@ -0,0 +1,42 @@
|
||||
# Work Summary — 2026-08-04 23:18
|
||||
|
||||
## Task
|
||||
Build test coverage for `domains/amazon`, the mock Amazon event processor - explicitly framed by the user as wanting tests around the *stateful background-processing* part specifically, both for long-term regression protection and to surface design insights, not just a coverage checkbox.
|
||||
|
||||
## What the code actually does
|
||||
Traced the full pipeline before writing anything, since `domains/amazon/mock.go` is the consumer end of a chain that starts in SQL:
|
||||
|
||||
1. A row is inserted into `mock.raw_shop_events` (platform-agnostic staging table; this is what the "simulate a sale/refund/inventory change" UI writes to, via `domains/accounts/accounts.go`).
|
||||
2. A Postgres trigger (`amazon_store_events`, `WHEN NEW.platform = 'amazon'`, defined across migrations `000026`/`000028`) copies the row into `mock.shop_amazon_events` and fires `pg_notify('mock_shop_amazon_event_inserted', null)`.
|
||||
3. `(*Mocks).ProcessEvents` - the code under test - registers `LISTEN mock_shop_amazon_event_inserted` on a dedicated pooled connection, then loops: drain all unprocessed rows from `mock.shop_amazon_events` (paginated 100 at a time via `processUnprocessedEvents`), then block on whichever comes first: a notification, an error, context cancellation, or a 1-minute timeout (poll fallback). Each processed event marks the row `processed=true` and fires an async, fire-and-forget notification to an app-level `MockEventListener` (wired to SSE in `main.go`, so the UI updates live).
|
||||
|
||||
## Test approach
|
||||
Rather than seeding `mock.shop_amazon_events` directly, tests insert into `mock.raw_shop_events` (the real entry point) and let the trigger do its job - so the tests exercise the exact same DB-side path production traffic does, not a hand-rolled approximation of it.
|
||||
|
||||
New file: `domains/amazon/mock_test.go`.
|
||||
- `notifySpy`: a `MockEventListener` that records calls and exposes a channel-based `waitForCount`, since the production code notifies from an un-awaited goroutine - there's no way to assert on it without a synchronization point.
|
||||
- `TestProcessUnprocessedEvents_ProcessesAllEventsAcrossBatches`: inserts 150 events (the batch loop's `LIMIT` is 100) and confirms every single one gets processed and notified - this is a real correctness test of the pagination loop, not just "does one row work."
|
||||
- `TestProcessUnprocessedEvents_NoListenerConfigured`: confirms processing doesn't depend on / crash without a listener being set.
|
||||
- `TestProcessEvents_ReactsToNotification`: the main event - runs the actual `ProcessEvents` loop in a goroutine, inserts a real event, and waits for the full reactive path (LISTEN → trigger → NOTIFY → WaitForNotification → reprocess → listener callback) to complete, then cancels and confirms clean shutdown.
|
||||
- `TestProcessEvents_ShutsDownOnContextCancel`: isolates lifecycle/shutdown behavior from NOTIFY timing entirely, so a shutdown regression doesn't hide behind notification flakiness or vice versa.
|
||||
|
||||
## Verification
|
||||
- `go build ./...` / `go vet ./...` clean.
|
||||
- `go test ./domains/amazon/... -v`: all 4 pass; also ran 10x in a row (`TestProcessEvents_ReactsToNotification` specifically, since it's the timing-sensitive one) with consistent ~0.25s runs, no flakes.
|
||||
- `go test ./domains/amazon/... -race`: clean, no data races detected under this exercise.
|
||||
- `make test` and `make test-against-dev-db`: full suite green both ways; confirmed zero leftover rows in the dev DB afterward.
|
||||
|
||||
## Design insights surfaced while building these tests
|
||||
This is the part the user specifically asked for - not fixed, just documented, since these are real behavior/architecture calls, not bugs:
|
||||
|
||||
1. **No "now listening" readiness signal.** `listenForNotifications` issues `LISTEN` synchronously, but from outside `ProcessEvents` there's no way to know when that's happened - a caller (or a test) that inserts an event immediately after starting `ProcessEvents` in a goroutine is racing against Go's scheduler getting around to running it. The test above works around this with a flat 200ms sleep before inserting, which is reliable in practice (10/10 clean runs) but is inherently a "hope the goroutine got scheduled by then" workaround, not a real guarantee. If this loop is ever driven by something less forgiving than a local dev machine (heavier load, slower CI), a small readiness channel returned from `listenForNotifications` (closed once `LISTEN` succeeds) would remove the guesswork entirely, for tests and for any other caller that cares about "is it actually listening yet."
|
||||
|
||||
2. **Fire-and-forget listener notification.** `processEvent` spawns `go func() { listener.Notify(ctx, e) }()` and only logs a failure - the event is marked `processed = true` in the DB regardless of whether the SSE listener actually received it. That's a reasonable tradeoff (a flaky UI push shouldn't block or retry core event processing), but it does mean there's currently no compensating mechanism if a notification is dropped - the UI just silently misses that one live update, permanently, with no re-send. Worth a deliberate decision on whether that's acceptable long-term, since it isn't caught by anything short of a user noticing stale data.
|
||||
|
||||
3. **The 1-minute poll fallback is a hardcoded constant** (`time.After(time.Minute)` inline in `ProcessEvents`), not a field/parameter. That's fine for production but means the fallback-poll path itself is essentially untestable without either waiting a full minute per test run or refactoring the interval to be injectable - the tests above only exercise the NOTIFY-driven reactive path, not the poll fallback, for exactly this reason. If the fallback path's correctness ever needs its own regression test, that constant would need to become configurable first.
|
||||
|
||||
4. **A LISTEN-connection error is fatal to the whole application, not just this subsystem.** If `WaitForNotification` ever errors for a reason other than context cancellation (dropped connection, Postgres restart, pool churn), `listenForNotifications`'s goroutine sends that error on `errCh`, `ProcessEvents` returns it, and in `main.go`, `runApp`'s top-level `select` treats `eventErrCh` firing as cause to call `shutdown()` - which cancels the shared context and tears down the HTTP server too. There's no reconnect/retry loop around the LISTEN connection specifically. In other words: a transient hiccup on one background Postgres connection currently brings down the entire server, not just the mock Amazon event processor. This is probably the single most consequential finding here if this pattern gets reused for the other nine `mock_shop_*_event_inserted` channels already defined in the migrations (`big_cartel`, `ebay`, `ecwid`, `etsy`, `shopify`, `square_online`, `squarespace`, `tiktok`, `walmart_marketplace`, `wix`, `woo_commerce`, `zoho` all have the same trigger+notify shape already migrated, just no Go-side processor yet) - multiplying this fragility by twelve without addressing it first would mean any one platform's listen-connection blip can take the whole app down.
|
||||
|
||||
## Follow-ups / not done here
|
||||
- None of the four insights above were acted on - flagging for a decision, same pattern as the `RefreshAccessToken` bug from the previous session. #4 in particular seems worth prioritizing before this pattern is replicated across the other mock platforms, given the "reports potentially later" plan implies more processors like this one are coming.
|
||||
- `domains/reports` still has no coverage (explicitly deferred by the user to later).
|
||||
Reference in New Issue
Block a user