Review follow-ups: PR #638 — fix(analytics): honest sales/profit numbers — losses, $0 cost is unknown, no double-counting, no voided, date-based horizons #881

New Issue

2026-09-29T09:45:10Z

gambit-admin commented

2026-09-29 09:45:10 +00:00

Non-blocking findings from the auto code review of PR #638 (fix(analytics): honest sales/profit numbers — losses, $0 cost is unknown, no double-counting, no voided, date-based horizons) — captured for batch cleanup rather than blocking the PR.
Commit 9c4e2e3 · https://github.com/Gambit-Inc/gambit/pull/638

SHOULD-FIX:

frontend/app/gambit/GambitLiveScreens.jsx:970 (and :35-41, formatCurrencyFromCents) — the DashboardPage's "Profit" tile renders sales30.estimated_profit_cents through formatCurrencyFromCents, which is `$${(value/100).toLocaleString(...)}` — for a negative value this produces $-12.34 instead of -$12.34. Before this PR estimated_profit_cents was floored at 0 (max(0, price - cost)), so this path never carried a negative number; this diff's backend change (backend/app/routes/inventory_items.py:2982/3003, dropping the floor so real losses show) now feeds negatives straight into a formatter that was never fixed for sign placement (unlike compactCents in the same file, and formatCompactCents in mobile, which this diff did fix). A shop running a net loss over 30 days sees a malformed dollar figure on the live dashboard.
backend/app/routes/inventory_items.py:2770-2781 — the market_trends_by_game fix assumes EbaySale.estimated_profit_cents is stored per unit ("see the summary/list endpoints"), aligning it with inventory_ebay_sales_summary's pre-existing * qty handling (line 2974, unchanged by this diff). There is no production code path that ever writes this column (only backend/scripts/seed_showcase_vendor.py, and there qty is always 1, so it can't distinguish per-unit vs. total). The two endpoints are now internally consistent with each other, but that consistency is unverified against the column's actual real-world semantics — if a future writer (or a legacy value already sitting in prod) populates it as a line-total, both endpoints will silently overcount by the sale's qty.

NITS:

backend/app/routes/inventory_items.py:2065 / 2136 — "ask AND cost known" only checks cost_each_cents > 0 but merely ask_each_cents is not None (an ask of exactly $0 is treated as "known" and still contributes (0 - cost) * qty as a negative potential-profit with 0 basis). Consistent between the Python and SQL paths, so no parity bug, but it's an asymmetric application of the "honest numbers" rule the rest of this PR establishes for cost.

Filed from a Claude Code session

Non-blocking findings from the auto code review of **PR #638** (fix(analytics): honest sales/profit numbers — losses, $0 cost is unknown, no double-counting, no voided, date-based horizons) — captured for batch cleanup rather than blocking the PR. Commit `9c4e2e3` · https://github.com/Gambit-Inc/gambit/pull/638 SHOULD-FIX: - `frontend/app/gambit/GambitLiveScreens.jsx:970` (and `:35-41`, `formatCurrencyFromCents`) — the `DashboardPage`'s "Profit" tile renders `sales30.estimated_profit_cents` through `formatCurrencyFromCents`, which is `` `$${(value/100).toLocaleString(...)}` `` — for a negative value this produces `$-12.34` instead of `-$12.34`. Before this PR `estimated_profit_cents` was floored at 0 (`max(0, price - cost)`), so this path never carried a negative number; this diff's backend change (`backend/app/routes/inventory_items.py:2982`/`3003`, dropping the floor so real losses show) now feeds negatives straight into a formatter that was never fixed for sign placement (unlike `compactCents` in the same file, and `formatCompactCents` in mobile, which this diff did fix). A shop running a net loss over 30 days sees a malformed dollar figure on the live dashboard. - `backend/app/routes/inventory_items.py:2770-2781` — the `market_trends_by_game` fix assumes `EbaySale.estimated_profit_cents` is stored **per unit** ("see the summary/list endpoints"), aligning it with `inventory_ebay_sales_summary`'s pre-existing `* qty` handling (line 2974, unchanged by this diff). There is no production code path that ever writes this column (only `backend/scripts/seed_showcase_vendor.py`, and there `qty` is always 1, so it can't distinguish per-unit vs. total). The two endpoints are now internally consistent with each other, but that consistency is unverified against the column's actual real-world semantics — if a future writer (or a legacy value already sitting in prod) populates it as a line-total, both endpoints will silently overcount by the sale's qty. NITS: - `backend/app/routes/inventory_items.py:2065` / `2136` — "ask AND cost known" only checks `cost_each_cents > 0` but merely `ask_each_cents is not None` (an ask of exactly `$0` is treated as "known" and still contributes `(0 - cost) * qty` as a negative potential-profit with `0` basis). Consistent between the Python and SQL paths, so no parity bug, but it's an asymmetric application of the "honest numbers" rule the rest of this PR establishes for cost. --- Filed from a Claude Code session

gambit-admin added the enhancement label 2026-09-29 09:45:10 +00:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: gambit/gambit#881