From 7b397fd4ec10bd1e080997c89652a533f2bf8c3a Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Fri, 4 Sep 2026 10:00:44 -0400 Subject: [PATCH 1/2] feat(web): status tabs and the rule that placed each order (#700) Most of #700 was already shipped. The issue's "Context (verified)" says there is no `/api/orders` endpoint and no Orders view; both have existed since #659, along with expected-vs-actual fill, the divergence and whether it went against keel, fee with a charged/modelled flag, the placement record, `venue_order_id`, and #626's submit-time book. What was actually missing was two things, and this is those two. THE STATUS FILTER. `gather_orders(status=)`, applied in the same pass as the scope so `get_orders()` is still called with no arguments at all -- the load-bearing pin in that function is unchanged, and its reason was always about MODE rather than about filtering in general. Server-side because a client filtering the page it received under `?limit=` would be filtering fifty rows and presenting the result as every canceled order: the same argument that already puts `?sort=` here. An unknown status is applied, not refused. `?sort=`'s columns are a closed set this code declares, so an unknown one is a client bug worth a 400; a status is written by whatever placed the order, and `_ORDER_STATUS_STATES` already carries `cancelled` beside `canceled` because spellings differ. A refusal would reject a status some book genuinely holds. An unmatched filter comes back empty saying which emptiness it is, which shows a typo as plainly as a 400 and cannot be wrong about a real word. Three new report fields, each because a number already on the page would otherwise be quietly wrong: - `statuses` -- every status in the WHOLE book, which is what a tab bar is built from. Scoped, the Canceled tab would disappear on a quiet day and return tomorrow, and a control that comes and goes is worse than one that is sometimes empty. Same reasoning as `modes`. - `filtered_count` -- `scoped_count` keeps its meaning (rows in the WINDOW), so a filtered view needs its own denominator. Without it "2 of 3 filled" counts rows from other tabs: every number true, the sentence false. - `empty_reason: "status"` -- a fourth way to render zero rows, and a fourth sentence. Reusing the empty-book wording would tell a reader keel has never traded on a deployment whose book is full. Checked widest-cause-first, so an empty book is never blamed on the open tab. The note is a FIXED string that does not echo the requested status. `?status=` is caller-controlled text; it crosses in `status`, which the client places as a value. THE RULE THAT PLACED IT. `rule_id` alone is a foreign key -- a number a reader cannot act on. The name is `rules.kind`, the same string `build_rule_track_record` calls `rule_name`, so the Orders table and the track-record table cannot name one rule two ways. Resolved from ONE read of the rules table, passed down as a map: a per-row lookup would be one query per order against a book capped at `MAX_ORDERS_LIMIT`, and would be invisible on any fixture small enough to read, so there is a pin on the read count. `rule_name_detail` separates two absences that both render as an empty cell: no rule was recorded against this order, or a rule was and that rule has left the book. Only the second is worth investigating. That second state cannot be written through keel today -- `orders.rule_id` carries a FOREIGN KEY and `db.connect` enables `PRAGMA foreign_keys = ON` -- so its test drives `_row_from_dict` directly rather than being refused by SQLite instead of by the code under test. The branch stays: the map can also miss for a row from an older keel, an external sqlite3 session with the pragma off, or a partial map. WHAT IS NOT HERE, AND WHY. #700 also asked for per-order quote provenance and `client_order_id`. Neither is recorded anywhere: the provenance strings exist only as confirm-gate banners (`confirm.py:33-36`) describing a preview shown to a human, and `client_order_id` is not a column. Both are split into #715 to be captured at placement first -- deriving either here would put a figure on the page that no record supports. The third omission is a refusal, not a gap: #700 asked for a disclosure exposing `raw_response`, and `payload.py` now records why it will not. The column holds whatever the venue chose to send -- session metadata, internal identifiers, error schemas nobody here has read -- and the service's reduction to `venue_order_id` exists so that emitting the rest is impossible here rather than one line away. What would reverse it is a curated, named set of venue fields, which is a different feature with its own review. Co-Authored-By: Claude Opus 5 (1M context) --- keel/commands/orders.py | 128 +++++++++++++++++-- keel/web/api.py | 11 +- keel/web/payload.py | 29 +++++ keel/web/static/js/main.js | 20 ++- keel/web/static/js/render.js | 61 +++++++++- tests/commands/test_orders.py | 223 ++++++++++++++++++++++++++++++++++ tests/web/test_orders_view.py | 163 ++++++++++++++++++++++++- 7 files changed, 615 insertions(+), 20 deletions(-) diff --git a/keel/commands/orders.py b/keel/commands/orders.py index 93159b5..846108d 100644 --- a/keel/commands/orders.py +++ b/keel/commands/orders.py @@ -50,6 +50,7 @@ import datetime import json import time +from collections.abc import Mapping from dataclasses import dataclass from decimal import Decimal, InvalidOperation from typing import Any @@ -294,6 +295,18 @@ class OrderRow: #: Why it is absent, when it is: `""` when there is an id to show. venue_order_id_detail: str + #: The NAME of the rule that placed this order -- `rules.kind`, the same string + #: `build_rule_track_record` calls `rule_name`, so the Orders view and the track-record + #: table name one rule one way. `""` when there is no name to show, for either of the two + #: reasons `rule_name_detail` tells apart. + rule_name: str + + #: Why the name is absent, when it is. Two states that both render as an empty cell and are + #: NOT the same fact: no rule was recorded against this order at all, or a rule WAS and that + #: rule is no longer in the book. The second is the one worth investigating, so it names the + #: id. `""` when there is a name. + rule_name_detail: str + rule_id: int | None created_at: int | None updated_at: int | None @@ -320,9 +333,17 @@ class OrdersReport: #: "nothing here" into a fact rather than a guess. total_count: int - #: Rows inside the scope, before `limit`. + #: Rows inside the scope, before `status` and before `limit`. Its meaning is unchanged by + #: the status filter on purpose: it is the denominator that says whether the SCOPE is what + #: emptied the table, and a count that moved with the open tab could not answer that. scoped_count: int + #: Rows inside the scope AND matching `status`, before `limit` -- what `rows` is a page of. + #: Equal to `scoped_count` when no status filter is applied. It exists because "1 of 3" on + #: a Canceled tab must count canceled orders; counting the scope there would be a true + #: number answering a question nobody asked. + filtered_count: int + #: Rows in `rows`. Held on the report rather than measured by a front-end: Rule 6e of #: `test_console_thinness.py` bans `len()` in the serialiser precisely so a count on the #: wire is one the report already carries. @@ -333,15 +354,38 @@ class OrdersReport: #: it from an empty live section. modes: tuple[str, ...] + #: The resolved status filter, lowercased, or `""` for "every status". Echoed back for the + #: reason `scope` is: a client rendering the active tab reads what was APPLIED rather than + #: trusting what it asked for. + status: str + + #: Every distinct `status` present in the whole book, sorted -- what a tab bar is built + #: from. From the WHOLE book and not the scope, deliberately: scoped, the Canceled tab + #: would vanish on a quiet day and return tomorrow, and a control that comes and goes is + #: worse than one that is sometimes empty. Same reasoning as `modes`. + statuses: tuple[str, ...] + #: `""` when there are rows; `"book"` when the book has never held an order; `"scope"` - #: when it holds orders and this window excluded all of them. Decided here, where the - #: counts are. + #: when it holds orders and this window excluded all of them; `"status"` when the window + #: holds orders and none of them wear the filtered status. Decided here, where the counts + #: are -- four different facts that all render as zero rows, and a reader who cannot tell + #: them apart learns the wrong one. empty_reason: str #: NEWEST FIRST. Reversed here, never in a renderer. rows: tuple[OrderRow, ...] +#: What an order with no `rule_id` says. A real state, not a defect: a manual order, or one +#: placed before the column was written. +NO_RULE_DETAIL = "no rule recorded against this order" + +#: What an order whose rule has left the book says. Deliberately different wording AND the id, +#: because "nothing placed this" and "rule 7 placed this and rule 7 is gone" are different facts +#: and only the second is worth chasing. +MISSING_RULE_DETAIL = "rule {rule_id} placed this order and is no longer in the book" + + def _venue_order_id(raw_response: Any) -> str: """The venue's own order id, and nothing else, out of `raw_response`. @@ -422,12 +466,27 @@ def _adverse(side: str, difference: Decimal | None) -> bool | None: return None -def _row_from_dict(row: dict[str, Any]) -> OrderRow: +def _row_from_dict(row: dict[str, Any], rule_names: Mapping[int, str] | None = None) -> OrderRow: """One repository dict, projected. Every judgement this report makes about a row is made - here, once, so neither renderer has to make it twice.""" + here, once, so neither renderer has to make it twice. + + `rule_names` is `{rules.id: rules.kind}`, read ONCE by `gather_orders` and passed down. A + lookup per row would be one query per order against a book capped at `MAX_ORDERS_LIMIT`, on + a route with no proxy in front of it -- and would be invisible on any fixture small enough + to read. Omitted (the default), every row reports its rule as unresolved rather than + claiming there is none: a caller that did not supply the map has not established that. + """ mode = str(row.get("mode") or "") side = str(row.get("side") or "") confirmation = str(row.get("confirmation") or "") + rule_id = None if row.get("rule_id") is None else int(row["rule_id"]) + rule_name = "" if rule_id is None else (rule_names or {}).get(rule_id, "") + if rule_name: + rule_detail = "" + elif rule_id is None: + rule_detail = NO_RULE_DETAIL + else: + rule_detail = MISSING_RULE_DETAIL.format(rule_id=rule_id) expected = row.get("expected_fill") actual = row.get("actual_fill") difference, _ = _divergence(expected, actual) @@ -477,6 +536,8 @@ def _row_from_dict(row: dict[str, Any]) -> OrderRow: submit_book_detail=book_detail, venue_order_id=venue_id, venue_order_id_detail=venue_detail, + rule_name=rule_name, + rule_name_detail=rule_detail, rule_id=None if row.get("rule_id") is None else int(row["rule_id"]), created_at=None if row.get("created_at") is None else int(row["created_at"]), updated_at=None if row.get("updated_at") is None else int(row["updated_at"]), @@ -488,15 +549,31 @@ def gather_orders( *, now_ts: int, scope: str = DEFAULT_ORDERS_SCOPE, + status: str = "", limit: int | None = DEFAULT_ORDERS_LIMIT, ) -> OrdersReport: """Every order in the book, newest first, scoped and capped. - `repo.get_orders()` is called with NO arguments: no `mode`, no `product_id`, no `status`. - That is the load-bearing choice in this function. Each deployment book holds exactly one - mode, so any mode filter renders empty on the books that hold the other one, and a report - that showed nothing while the book held fifteen rows would be a surface asserting a fact it - had not established. The mode is carried per ROW instead. + `repo.get_orders()` is still called with NO arguments: no `mode`, no `product_id`, no + `status`. That is the load-bearing choice in this function, and the reason is about MODE -- + each deployment book holds exactly one, so any mode filter renders empty on the books that + hold the other one, and a report that showed nothing while the book held fifteen rows would + be a surface asserting a fact it had not established. The mode is carried per ROW instead. + + `status` (#700) narrows the report WITHOUT touching that call: the filter runs in the same + pass as the scope, over rows the repository handed back whole, so `total_count` and + `statuses` still describe the entire book and the empty states below can still tell "keel + has never traded" from "nothing wears this status". Pushing it into the query would trade + those for nothing -- the cap is applied after both narrowings either way. + + Filtering server-side rather than in the browser is the same argument `?sort=` makes: a + client filtering a CAPPED page would be filtering the fifty rows it happened to receive and + presenting the result as every rejected order. + + An unrecognised status is not refused. It is applied, matches nothing, and comes back with + `empty_reason == "status"` naming what was asked for -- which shows a typo plainly, and + cannot wrongly refuse a status some venue or some older row spells differently. `statuses` + is the list a client should build tabs from. A row with no `created_at` cannot be placed in time, so it is never excluded by a scope: excluding it would hide a row on the strength of a timestamp that does not exist. It counts @@ -504,29 +581,53 @@ def gather_orders( """ resolved_scope = normalise_scope(scope) resolved_limit = normalise_limit(limit) + resolved_status = (status or "").strip().lower() start_ts = scope_start_ts(resolved_scope, now_ts) raw = repo.get_orders() + # ONE read of a small table, before the row loop -- see `_row_from_dict`'s note on why this + # is not a per-row lookup. `kind` IS the rule's name here (`build_rule_track_record` reads + # the same column under the same meaning); there is no separate name column to prefer. + rule_names: dict[int, str] = { + int(rule["id"]): str(rule.get("kind") or "") + for rule in repo.get_rules() + if rule.get("id") is not None + } total = 0 modes: set[str] = set() + statuses: set[str] = set() scoped: list[dict[str, Any]] = [] + filtered: list[dict[str, Any]] = [] for row in raw: total += 1 modes.add(str(row.get("mode") or "")) + # From every row in the BOOK, before either narrowing -- see `statuses`' own note. + statuses.add(str(row.get("status") or "")) created = row.get("created_at") if start_ts is not None and created is not None and int(created) < start_ts: continue scoped.append(row) + # Lowercased on BOTH sides. The stored word is lowercase today, but matching the + # caller's casing against a raw column would make a hand-typed `?status=FILLED` fail + # while the tab that sends `filled` works -- one control, two behaviours. + if resolved_status and str(row.get("status") or "").strip().lower() != resolved_status: + continue + filtered.append(row) # Newest first, HERE. `get_orders` orders by `id` ascending; reversing in each renderer # would be the same decision made twice and the second copy is the one that drifts. - scoped.reverse() - shown = tuple(_row_from_dict(row) for row in scoped[:resolved_limit]) + filtered.reverse() + shown = tuple(_row_from_dict(row, rule_names) for row in filtered[:resolved_limit]) + # Ordered widest cause first: a book with nothing in it is not a scope problem, and a scope + # that excluded everything is not the tab's doing. Reporting the narrowest true cause would + # send a reader to change the filter when the book is simply empty. if total == 0: empty_reason = "book" elif not scoped: empty_reason = "scope" + elif not filtered: + empty_reason = "status" else: empty_reason = "" @@ -535,10 +636,13 @@ def gather_orders( scope=resolved_scope, scope_start_ts=start_ts, limit=resolved_limit, + status=resolved_status, total_count=total, scoped_count=len(scoped), + filtered_count=len(filtered), shown_count=len(shown), modes=tuple(sorted(modes)), + statuses=tuple(sorted(s for s in statuses if s)), empty_reason=empty_reason, rows=shown, ) diff --git a/keel/web/api.py b/keel/web/api.py index 6a07fd6..668974b 100644 --- a/keel/web/api.py +++ b/keel/web/api.py @@ -285,11 +285,20 @@ def read_orders(cfg: ServeConfig, query: Query, _state: Any, _now_ts: int) -> di ) scope = normalise_scope(_first(query, "scope")) + # `?status=` is applied, never refused (#700). `?sort=`'s columns are a closed set this + # module declares, so an unknown one is a client bug worth a 400; a status is written by + # whatever placed the order -- a venue's own word, or an older spelling like `cancelled` + # already in `_ORDER_STATUS_STATES` -- so a refusal here would reject a status some book + # genuinely holds. An unmatched filter comes back empty with `empty_reason: "status"`, + # which shows a typo as plainly as a 400 would and cannot be wrong about a real one. + status = _first(query, "status") or "" raw_limit = _first(query, "limit") limit = DEFAULT_ORDERS_LIMIT if not raw_limit else _whole_number(raw_limit, MAX_ORDERS_LIMIT) repo = open_repo(cfg.db_path) try: - report = gather_orders(repo, now_ts=int(time.time()), scope=scope, limit=limit) + report = gather_orders( + repo, now_ts=int(time.time()), scope=scope, status=status, limit=limit + ) finally: close_repo(repo) return payload.orders_payload(report) diff --git a/keel/web/payload.py b/keel/web/payload.py index eacc623..e69d9fc 100644 --- a/keel/web/payload.py +++ b/keel/web/payload.py @@ -1342,6 +1342,13 @@ def activity_payload(feed: ActivityFeed) -> dict[str, Any]: "no orders in this window. The book holds orders, all of them outside this scope -- " "widen it to see them." ), + # A FIXED sentence that does not name the status asked for (#700). `?status=` is + # caller-controlled text; it crosses in `status`, which the client places as a value, and + # keeping it out of the prose means an unrecognised word cannot arrive as a sentence. + "status": ( + "no orders with this status in this window. The book holds orders with other statuses " + "-- switch tabs to see them." + ), } #: The two `mode` words, styled. `live` is `warn` not because live trading is wrong but because @@ -1374,6 +1381,15 @@ def _order_row_payload(row: OrderRow) -> dict[str, Any]: read out of it. A reader wanting the blob opens SQLite, which is the right amount of friction for unbounded venue JSON. + **#700 asked for a per-row disclosure exposing that blob, and it was REFUSED.** Not on + layout grounds: the column is whatever the venue chose to send, so it can carry session + metadata, internal identifiers, and error schemas nobody here has read, and a page that + renders it wholesale publishes all of that to anyone who can open the console. The + reduction to `venue_order_id` happens in the service precisely so that emitting the rest is + an impossible mistake here rather than a one-line one. What would reverse this: a curated, + named set of venue fields worth showing -- which is a different feature, with its own + review of what each field contains, and not "render the blob". + **`fee` is a figure, never a rate.** No percentage appears in this payload. Deriving one would be arithmetic Rule 3 forbids here, and would put a number on the page that the report does not hold. @@ -1460,6 +1476,12 @@ def _order_row_payload(row: OrderRow) -> dict[str, Any]: # a paper row shows a sentence rather than a blank that reads as missing data. "venue_order_id": row.venue_order_id, "venue_order_id_note": row.venue_order_id_detail, + # The NAME first, the id after it (#700). A foreign key is not something a reader can + # act on; `rules.kind` is the same string the track-record table names the rule by, so + # the two views cannot call one rule two things. `rule_note` tells the two absences + # apart -- no rule recorded, versus a rule that has left the book. + "rule_name": label(row.rule_name or "unattributed", state=NEUTRAL), + "rule_note": row.rule_name_detail, "rule_id": count(row.rule_id), "created_at": moment(row.created_at), "updated_at": moment(row.updated_at), @@ -1487,8 +1509,15 @@ def orders_payload(report: OrdersReport) -> dict[str, Any]: "scope": report.scope, "scope_start_at": moment(report.scope_start_ts), "limit": count(report.limit), + # The status FILTER as applied, and the tab bar to build (#700). Both bare strings, like + # `scope` and `modes` beside them: a status is an enum word with no precision hazard and + # no judgement -- the judgement is `_ORDER_STATUS_STATES`, and it is made per ROW, where + # a reader meets it. + "status": report.status, + "statuses": [str(status) for status in report.statuses], "total_count": count(report.total_count), "scoped_count": count(report.scoped_count), + "filtered_count": count(report.filtered_count), "shown_count": count(report.shown_count), "modes": [str(mode) for mode in report.modes], "empty_reason": report.empty_reason, diff --git a/keel/web/static/js/main.js b/keel/web/static/js/main.js index 9d89f5f..108b2d8 100644 --- a/keel/web/static/js/main.js +++ b/keel/web/static/js/main.js @@ -392,10 +392,22 @@ function mount(route, readings) { // The same server-side scope switch Activity uses, and deliberately the same control: two // views over one deployment whose "how far back" behaved differently would be a thing an // operator has to learn twice. - return ordersView(data, primary.sort, onSort, (scope) => { - paramsFor(route.endpoints[0]).scope = scope; - void paint(route, true, true); - }); + return ordersView( + data, + primary.sort, + onSort, + (scope) => { + paramsFor(route.endpoints[0]).scope = scope; + void paint(route, true, true); + }, + // #700: the status tab re-asks the SERVER. Filtering the received page here would filter + // the rows that happened to arrive under `?limit=` and present the result as every + // canceled order -- the same argument that puts `?sort=` on the server. + (status) => { + paramsFor(route.endpoints[0]).status = status; + void paint(route, true, true); + }, + ); } if (route.name === "insights") { const journal = readings[1]; diff --git a/keel/web/static/js/render.js b/keel/web/static/js/render.js index a4dd719..986ec02 100644 --- a/keel/web/static/js/render.js +++ b/keel/web/static/js/render.js @@ -1168,7 +1168,7 @@ function jobPanel(job) { * @param {(scope: string) => void} onScope * @returns {DocumentFragment} */ -export function ordersView(data, sort, onSort, onScope) { +export function ordersView(data, sort, onSort, onScope, onStatus) { const fragment = document.createDocumentFragment(); fragment.append(el("h1", undefined, "Orders")); @@ -1177,10 +1177,19 @@ export function ordersView(data, sort, onSort, onScope) { fragment.append(sub); fragment.append(scopeSwitch(plain(data.scope), onScope, "Orders scope")); + // Guarded, so a caller that has not wired the tabs renders the view unchanged rather + // than a bar whose buttons do nothing. + if (onStatus) { + fragment.append(statusSwitch(plain(data.status), data.statuses || [], onStatus)); + } fragment.append( gridCard([ kv("shown", data.shown_count), + // `filtered_count`, never `shown` against `in scope`: with a tab open, the + // denominator of "shown" has to count that tab, and the report is the only place + // allowed to work it out (Rule 2 keeps the subtraction out of the browser). + kv("in this status", data.filtered_count), kv("in scope", data.scoped_count), kv("in this book", data.total_count), // Which modes this book actually holds. A deployment book holds one, and saying which @@ -1202,6 +1211,10 @@ export function ordersView(data, sort, onSort, onScope) { { label: "side", numeric: false, key: "side" }, { label: "product", numeric: false, key: "product_id" }, { label: "status", numeric: false, key: "status" }, + // The rule's NAME, beside the status rather than buried in the disclosure: on a + // book with several rules live, which one placed an order is a scanning + // question, and a foreign key in a detail panel does not answer it. + { label: "rule", numeric: false, key: "rule_name" }, { label: "qty", numeric: true, key: "qty" }, { label: "filled", numeric: true, key: "filled_quantity" }, { label: "expected", numeric: true, key: "expected_fill" }, @@ -1301,7 +1314,11 @@ function orderDetail(row) { "venue order id", plain(row.venue_order_id) || plain(row.venue_order_id_note) || "—", ), - kv("rule", row.rule_id), + // The name is in the table; this is the id it resolved from, plus the sentence + // that tells the two absences apart -- no rule recorded at all, versus a rule + // that has left the book. A blank cell would collapse them. + kv("rule", plain(row.rule_note) || field(row.rule_name)), + kv("rule id", row.rule_id), kv("last updated", row.updated_at), ]), ); @@ -1407,6 +1424,46 @@ export function activityView(data, sort, onSort, onScope) { * @param {string} [label] how the control announces itself. Defaults to Activity's wording. * @returns {HTMLElement} */ +/** + * The Orders status tabs (#700). + * + * **Built from `statuses`, never from a constant.** A tab bar listing every status keel CAN + * write would invite a reader to click into four empty tabs and conclude something about the + * engine from what is really a list of possibilities. `statuses` is what this book actually + * recorded, which is why the service carries it and why it comes from the whole book rather than + * the open scope -- a tab that vanishes on a quiet day is worse than one that is empty. + * + * **`current` is what the report APPLIED**, not what the client last asked for. If the two ever + * disagree the report is right, and a bar drawn from the request would show a filter that is not + * in force. + * + * The leading "all" tab is not decoration: without it, a reader who has clicked into a status has + * no way back short of knowing that the empty string means every status. + * + * @param {string} current `data.status` -- `""` when unfiltered. + * @param {string[]} statuses `data.statuses`. + * @param {(status: string) => void} onStatus + * @returns {HTMLElement} + */ +function statusSwitch(current, statuses, onStatus) { + const wrap = el("nav", "scopes"); + wrap.setAttribute("aria-label", "Order status"); + wrap.append(el("span", "k", "status")); + const all = el("button", "scopekey", "all"); + all.setAttribute("type", "button"); + if (!current) all.setAttribute("aria-current", "true"); + all.addEventListener("click", () => onStatus("")); + wrap.append(all); + for (const name of statuses) { + const button = el("button", "scopekey", name); + button.setAttribute("type", "button"); + if (name === current) button.setAttribute("aria-current", "true"); + button.addEventListener("click", () => onStatus(name)); + wrap.append(button); + } + return wrap; +} + function scopeSwitch(current, onScope, label) { const wrap = el("nav", "scopes"); wrap.setAttribute("aria-label", label || "Activity scope"); diff --git a/tests/commands/test_orders.py b/tests/commands/test_orders.py index 2682c6f..e2e46e0 100644 --- a/tests/commands/test_orders.py +++ b/tests/commands/test_orders.py @@ -99,6 +99,11 @@ def get_orders(self, *args: Any, **kwargs: Any) -> list[dict[str, Any]]: self.calls.append((args, kwargs)) return list(self.rows) + def get_rules(self, *args: Any, **kwargs: Any) -> list[dict[str, Any]]: + # `gather_orders` resolves rule NAMES off this (#700). Recorded nowhere: this stub + # exists to pin how `get_orders` is called, and that pin is unchanged. + return [] + def test_gather_passes_no_filter_of_any_kind_to_get_orders() -> None: """THE pin the operator's scope note asked for. @@ -664,12 +669,230 @@ def _report_with(**overrides: Any) -> OrdersReport: "scope": "all", "scope_start_ts": None, "limit": DEFAULT_ORDERS_LIMIT, + "status": "", "total_count": len(rows), "scoped_count": len(rows), + "filtered_count": len(rows), "shown_count": len(rows), "modes": ("live",), + "statuses": ("filled",), "empty_reason": "", "rows": rows, } base.update(overrides) return OrdersReport(**base) + + +# -- the status filter, server-side (#700) ---------------------------------------------------- +# +# The Orders view's tabs. Filtered HERE and not in the browser, for the reason `?sort=` is: a +# client that filtered a capped page would be filtering the 50 rows it happened to receive and +# calling the result "every rejected order", which is a different and false claim. +# +# It does NOT reach `get_orders()`. The unfiltered-read pin above is unchanged and still the +# load-bearing one -- the status narrowing happens in the same pass as the scope, over rows the +# repository handed back whole. + + +def test_a_status_filter_keeps_only_that_status(tmp_path: Path) -> None: + repo = _repo(tmp_path) + repo.insert_order(_order(status="filled")) + repo.insert_order(_order(status="canceled")) + repo.insert_order(_order(status="pending")) + + report = gather_orders(repo, now_ts=NOW_TS, status="canceled") + + assert [row.status for row in report.rows] == ["canceled"] + + +def test_no_status_filter_keeps_every_row(tmp_path: Path) -> None: + """The default is the whole book. A tabbed view opens on "All", and a report that quietly + defaulted to one status would answer a question nobody asked.""" + repo = _repo(tmp_path) + repo.insert_order(_order(status="filled")) + repo.insert_order(_order(status="canceled")) + + assert len(gather_orders(repo, now_ts=NOW_TS).rows) == 2 + assert len(gather_orders(repo, now_ts=NOW_TS, status="").rows) == 2 + + +def test_the_status_filter_is_matched_without_case(tmp_path: Path) -> None: + """`?status=FILLED` from a hand-typed URL is the same question as `?status=filled`. The + stored word is lowercase; matching on the caller's casing would make the tab work from the + UI and fail from the address bar.""" + repo = _repo(tmp_path) + repo.insert_order(_order(status="filled")) + + assert len(gather_orders(repo, now_ts=NOW_TS, status="FILLED").rows) == 1 + + +def test_the_status_filter_composes_with_the_scope(tmp_path: Path) -> None: + """Both narrowings apply, and the scope still comes first: a filled order from last month is + not in today's book, whichever tab is open.""" + repo = _repo(tmp_path) + repo.insert_order(_order(status="filled", created_at=TODAY_START + 60)) + repo.insert_order(_order(status="filled", created_at=TODAY_START - 86_400)) + repo.insert_order(_order(status="canceled", created_at=TODAY_START + 60)) + + report = gather_orders(repo, now_ts=NOW_TS, scope="today", status="filled") + + assert report.shown_count == 1 + + +def test_the_report_names_every_status_in_the_whole_book(tmp_path: Path) -> None: + """What the tabs are built from -- the statuses this deployment ACTUALLY recorded, not a + hardcoded list of the ones keel can write. `modes` is carried for the same reason: a reader + must never have to conclude which tabs exist from which ones came back empty. + + From the WHOLE book, deliberately: scoped to today, a tab bar would lose the Canceled tab on + a quiet day and reappear it tomorrow, and a control that comes and goes is worse than one + that is sometimes empty.""" + repo = _repo(tmp_path) + repo.insert_order(_order(status="filled", created_at=TODAY_START - 86_400)) + repo.insert_order(_order(status="canceled", created_at=TODAY_START + 60)) + repo.insert_order(_order(status="filled", created_at=TODAY_START + 60)) + + report = gather_orders(repo, now_ts=NOW_TS, scope="today") + + assert report.statuses == ("canceled", "filled") + + +def test_the_resolved_status_is_echoed_back(tmp_path: Path) -> None: + """Same contract as `scope`: what the report actually applied, so a client rendering the + active tab reads it back rather than trusting what it asked for.""" + repo = _repo(tmp_path) + repo.insert_order(_order(status="filled")) + + assert gather_orders(repo, now_ts=NOW_TS, status="FILLED").status == "filled" + assert gather_orders(repo, now_ts=NOW_TS).status == "" + + +def test_an_empty_status_tab_is_not_an_empty_book(tmp_path: Path) -> None: + """The three ways this table comes back empty are three different facts, and a reader who + cannot tell them apart learns the wrong one. "keel has never traded", "nothing in this + window", and "nothing with this status" all render as zero rows.""" + repo = _repo(tmp_path) + repo.insert_order(_order(status="filled", created_at=TODAY_START + 60)) + + assert gather_orders(repo, now_ts=NOW_TS, status="rejected").empty_reason == "status" + assert gather_orders(repo, now_ts=NOW_TS, scope="today", status="filled").empty_reason == "" + + +def test_an_out_of_scope_row_still_reports_the_scope_as_the_reason(tmp_path: Path) -> None: + """Scope is checked before status, so a book whose only row is outside the window says so + rather than blaming the tab -- the scope is the narrowing the reader chose first.""" + repo = _repo(tmp_path) + repo.insert_order(_order(status="filled", created_at=TODAY_START - 86_400)) + + report = gather_orders(repo, now_ts=NOW_TS, scope="today", status="filled") + assert report.empty_reason == "scope" + + +def test_the_filtered_count_is_what_the_shown_rows_are_a_page_of(tmp_path: Path) -> None: + """`scoped_count` keeps its meaning -- rows inside the SCOPE -- so a filtered view needs its + own denominator. Without it a tab showing 1 of 3 would be counting rows from other tabs, and + "1 of 3 canceled orders" would be false while every number in it was true.""" + repo = _repo(tmp_path) + repo.insert_order(_order(status="filled")) + repo.insert_order(_order(status="filled")) + repo.insert_order(_order(status="canceled")) + + report = gather_orders(repo, now_ts=NOW_TS, status="filled") + + assert report.total_count == 3 + assert report.scoped_count == 3 + assert report.filtered_count == 2 + assert report.shown_count == 2 + + +def test_the_filtered_count_reflects_the_cap_being_a_cap(tmp_path: Path) -> None: + repo = _repo(tmp_path) + for _ in range(4): + repo.insert_order(_order(status="filled")) + + report = gather_orders(repo, now_ts=NOW_TS, status="filled", limit=2) + + assert report.filtered_count == 4 + assert report.shown_count == 2 + +# -- the rule that placed it (#700) ----------------------------------------------------------- +# +# `orders.rule_id` is a foreign key, and a foreign key on a page is a number a reader cannot act +# on. The NAME is `rules.kind` -- the same string `build_rule_track_record` calls `rule_name`, so +# the Orders view and the track-record table name the same rule the same way. + + +class _RuleCountingRepo: + """Records how often each read is made, so the resolution cannot be N+1 unnoticed.""" + + def __init__(self, orders: list[dict[str, Any]], rules: list[dict[str, Any]]) -> None: + self._orders = orders + self._rules = rules + self.order_reads = 0 + self.rule_reads = 0 + + def get_orders(self, *args: Any, **kwargs: Any) -> list[dict[str, Any]]: + self.order_reads += 1 + return list(self._orders) + + def get_rules(self, *args: Any, **kwargs: Any) -> list[dict[str, Any]]: + self.rule_reads += 1 + return list(self._rules) + + +def test_the_rule_that_placed_an_order_is_named(tmp_path: Path) -> None: + repo = _repo(tmp_path) + rule_id = repo.insert_rule("turtle_breakout", {"product_id": "BTC-USD"}) + repo.insert_order(_order(rule_id=rule_id)) + + report = gather_orders(repo, now_ts=NOW_TS) + + assert report.rows[0].rule_name == "turtle_breakout" + + +def test_an_order_with_no_rule_says_so_rather_than_showing_a_blank(tmp_path: Path) -> None: + """A `NULL` rule_id is a real state -- a manual order, or one placed before the column was + written. It is not the same as a rule that has gone missing, and the sentence is what keeps + the two apart on a page where both render as no name.""" + repo = _repo(tmp_path) + repo.insert_order(_order(rule_id=None)) + + row = gather_orders(repo, now_ts=NOW_TS).rows[0] + + assert row.rule_name == "" + assert "no rule" in row.rule_name_detail.lower() + + +def test_a_rule_that_is_no_longer_in_the_book_is_named_as_missing() -> None: + """The distinction that matters for an audit trail: "nothing placed this" and "rule 7 placed + this and rule 7 is gone" are different facts, and only the second is worth chasing. Rendering + both as an empty cell would hide it. + + Driven through `_row_from_dict` rather than a repository, because `orders.rule_id` carries a + FOREIGN KEY to `rules(id)` and `db.connect` enables `PRAGMA foreign_keys = ON` -- so keel + cannot write this row, and a test that tried would be refused by SQLite rather than by the + code under test. The branch is defensive and stays: the map can also miss for a row written + by an older keel, by an external sqlite3 session with the pragma off, or by a caller passing + a partial map.""" + row = orders_service._row_from_dict(_order(id=1, rule_id=7), {}) + + assert row.rule_name == "" + assert "7" in row.rule_name_detail + assert row.rule_name_detail != orders_service._row_from_dict( + _order(id=2, rule_id=None), {} + ).rule_name_detail + + +def test_naming_the_rules_costs_one_read_however_many_orders(tmp_path: Path) -> None: + """The pin against an N+1. A per-row lookup would be invisible on a fixture of two and would + be one query per row on a real book -- `MAX_ORDERS_LIMIT` of them, on a route with no proxy + in front of it (`api.py`'s own note).""" + repo = _RuleCountingRepo( + [_order(id=i, rule_id=1) for i in range(1, 26)], + [{"id": 1, "kind": "turtle_breakout", "params": {}, "status": "live"}], + ) + + report = gather_orders(repo, now_ts=NOW_TS) # type: ignore[arg-type] + + assert repo.rule_reads == 1 + assert {row.rule_name for row in report.rows} == {"turtle_breakout"} diff --git a/tests/web/test_orders_view.py b/tests/web/test_orders_view.py index 5aaefa6..6686936 100644 --- a/tests/web/test_orders_view.py +++ b/tests/web/test_orders_view.py @@ -431,6 +431,45 @@ def test_the_scope_is_normalised_and_echoed_rather_than_refused( assert scoped["data"]["total_count"]["value"] == "2" +def test_the_status_filter_reaches_the_service_from_the_query( + book: web_server.ServeConfig, +) -> None: + """The tabs, end to end. Filtered by the SERVICE off the whole book -- not by the client + over the capped page it received, which would present "every canceled order" while meaning + "the canceled ones among the fifty rows I was sent".""" + _status, unfiltered = _json_get(book, "/api/orders") + assert len(unfiltered["data"]["rows"]) == 2, "the fixture must hold both rows" + assert unfiltered["data"]["statuses"] == ["filled"], "both fixture rows are filled" + + status, document = _json_get(book, "/api/orders?status=filled") + assert status == 200 + data = document["data"] + assert data["status"] == "filled" + assert [row["status"]["value"] for row in data["rows"]] == ["filled", "filled"] + assert data["filtered_count"]["value"] == "2" + + # The narrowing itself, against a status this book does not hold. Asserted through the + # ENDPOINT rather than only on the service, because a filter applied in the client would + # answer this with both rows -- the two cases are indistinguishable on the matching status. + _status, none_match = _json_get(book, "/api/orders?status=pending") + assert none_match["data"]["rows"] == [] + # The book is still described whole: the tab narrowed the rows, not the denominators. + assert none_match["data"]["total_count"]["value"] == "2" + assert none_match["data"]["filtered_count"]["value"] == "0" + + +def test_an_unknown_status_returns_an_honest_empty_rather_than_a_400( + book: web_server.ServeConfig, +) -> None: + """Unlike `?sort=`, whose columns are a closed set this code declares, a status is written + by whatever placed the order -- a venue word or an older spelling would be refused wrongly. + So it filters, matches nothing, and says which emptiness that is.""" + status, document = _json_get(book, "/api/orders?status=rejected") + assert status == 200 + assert document["data"]["rows"] == [] + assert document["data"]["empty_reason"] == "status" + + @pytest.mark.parametrize("bad", ["0", "-1", "abc", "99999"]) def test_a_bad_limit_is_refused_rather_than_silently_substituted( book: web_server.ServeConfig, bad: str @@ -522,7 +561,11 @@ def test_the_orders_view_is_wired_into_the_client_router() -> None: source = _source("main.js") assert "ordersView," in source assert 'route.name === "orders"' in source - assert "ordersView(data, primary.sort, onSort," in source + # The ARGUMENTS, not one line's worth of whitespace: #700 added a fifth (the status tab + # callback), which reflowed the call across lines and broke the literal without changing + # anything this test is here to protect. + call = re.search(r"ordersView\(\s*data,\s*primary\.sort,\s*onSort,", source) + assert call is not None, "ordersView must still be called with (data, sort, onSort, ...)" def test_the_scope_switch_names_itself_per_view() -> None: @@ -541,3 +584,121 @@ def test_the_client_added_no_new_asset_to_precache() -> None: precache = (_STATIC / "sw.js").read_text(encoding="utf-8") assert "orders.js" not in precache assert not (_STATIC / "js" / "orders.js").exists() + + +# -- the status tabs, on the wire (#700) ------------------------------------------------------ +# +# The service decides; this pins that the browser gets the decision unaltered. The tabs come +# from what this book RECORDED (`statuses`), the active one from what the report APPLIED +# (`status`), and the caption's denominator from the report's own `filtered_count` -- so no +# client ever subtracts one count from another to caption a tab. + + +def test_the_orders_payload_carries_the_tabs_and_the_active_one(tmp_path: Path) -> None: + report = _report( + tmp_path, + _order(status="filled"), + _order(status="canceled"), + status="canceled", + ) + built = web_payload.orders_payload(report) + + assert built["status"] == "canceled" + assert built["statuses"] == ["canceled", "filled"] + + +def test_the_tabs_list_every_status_in_the_book_not_only_the_open_one(tmp_path: Path) -> None: + """A tab bar that lost its other tabs the moment one was chosen would be a control that + deletes its own alternatives.""" + built = web_payload.orders_payload( + _report(tmp_path, _order(status="filled"), _order(status="pending"), status="filled") + ) + + assert built["statuses"] == ["filled", "pending"] + + +def test_the_filtered_count_crosses_as_its_own_field(tmp_path: Path) -> None: + """`scoped_count` counts the window and `filtered_count` counts the open tab. A client that + had to compare them to caption "2 of 3 filled" would be deriving a figure in the browser.""" + built = web_payload.orders_payload( + _report( + tmp_path, + _order(status="filled"), + _order(status="filled"), + _order(status="canceled"), + status="filled", + ) + ) + + assert built["scoped_count"]["value"] == "3" + assert built["filtered_count"]["value"] == "2" + assert built["shown_count"]["value"] == "2" + + +def test_an_empty_status_tab_reads_differently_from_an_empty_book(tmp_path: Path) -> None: + """Four ways to render zero rows, four sentences. Reusing the book's wording here would tell + a reader keel has never traded on a deployment whose book is full.""" + filtered = web_payload.orders_payload( + _report(tmp_path, _order(status="filled"), status="rejected") + ) + empty_book = web_payload.orders_payload(_report(tmp_path)) + + assert filtered["empty_reason"] == "status" + assert filtered["empty_note"] + assert filtered["empty_note"] != empty_book["empty_note"] + + +def test_the_status_note_does_not_echo_the_requested_word_back(tmp_path: Path) -> None: + """`?status=` is caller-controlled text. It is applied as a filter and named on the wire in + `status`, which the client places as a value -- but the prose sentence is a fixed string, so + an unrecognised word cannot reach the page through the note.""" + built = web_payload.orders_payload( + _report(tmp_path, _order(status="filled"), status="") + ) + + assert "".lower() + + +# -- the tabs and the rule column, in the client (#700) --------------------------------------- +# +# Source-text assertions, per this module's own header: there is no JavaScript runtime here, so +# what is pinned is that the declarations exist and read from the right keys -- not that the DOM +# they describe behaves. + + +def test_the_status_tabs_are_built_from_the_book_not_from_a_hardcoded_list() -> None: + """A tab bar listing statuses this deployment never recorded invites a reader to click into + four empty tabs and conclude something about the engine. `data.statuses` is what the book + actually holds, which is why the service carries it.""" + code = _code("render.js") + assert "data.statuses" in code, "the tabs must come from the payload, not from a constant" + assert "statusSwitch" in code, "the tab bar needs a control of its own" + + +def test_the_tab_bar_offers_a_way_back_to_every_status() -> None: + """A filter with no way to clear it is a trap: having clicked Canceled, a reader who wants + the whole book again has to know that the empty string means "all".""" + code = _code("render.js") + body = code.split("function statusSwitch")[1][:900] + assert '"all"' in body, "the tab bar needs an All tab" + assert 'onStatus("")' in body, "the All tab must clear the filter, not set a word" + + +def test_the_active_tab_is_read_from_what_the_report_applied() -> None: + """`data.status`, never the value the client last sent. If the two ever disagree the report + is right, and a tab bar drawn from the request would show a filter that is not in force.""" + assert "data.status" in _code("render.js") + + +def test_the_orders_table_names_the_rule_that_placed_each_order() -> None: + """`rule_id` alone is a foreign key -- a number a reader cannot act on.""" + code = _code("render.js") + assert '"rule_name"' in code, "the orders table must carry the rule NAME column" + + +def test_the_status_tab_reaches_the_query_string_not_a_client_side_filter() -> None: + """The tabs must re-ask the SERVER. Filtering the received page in the browser would filter + the fifty rows that arrived and label the result "every canceled order".""" + code = _code("main.js") + assert ".status = status" in code, "the tab must set the endpoint's status param" From 808131d73a328035d8204acc31ffcf2454f4022b Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Fri, 4 Sep 2026 11:54:01 -0400 Subject: [PATCH 2/2] fix(web): the rule column shifted every money column on the Orders table (#700) Review findings on this branch. The first one is the reason the rest are worth reading. THE COLUMN SHIFT. `table()` pairs `columns[index]` with `row.entries()` BY POSITION. The "rule" header was added at index 6 and no cell was added with it, so the Orders table declared fourteen headers over thirteen values: every column from index 6 rendered one place left, and the last rendered nothing. "rule" showed the quantity, "expected" showed the actual fill, "divergence" showed the fee, "fee" showed the PLACEMENT TIMESTAMP, and "placed (UTC)" had no cell at all. Every value on the page was individually true and every column named the wrong one -- on the view whose entire purpose is per-order honesty. The test that was supposed to cover this asserted `'"rule_name"' in code`, which the header declaration alone satisfies. It was written after the code and pinned the wrong thing. Replaced by a scan that compares HEADER COUNT to CELL COUNT for every `table()` call in `render.js` -- it found exactly one mismatch, `(14, 13)`, and it covers the other six tables for free. Nothing in the suite compared those two lists before. FOCUS. `statusSwitch`'s buttons carried no `data-focus`. Pressing a tab replaces the view, which destroys the button that was pressed, and `main.js` puts focus back by that attribute -- without it a keyboard user is returned to the top of the document on every tab press. `scopeSwitch`, twenty-five lines below and otherwise copied, has always set it. A CLAIM THE CODE COULD NOT SUPPORT. `MISSING_RULE_DETAIL` -- "rule 7 placed this order and is no longer in the book" -- also fired when the caller passed NO rule map, and when the rule was found with an empty `kind`. The first states the result of a search nobody ran, and would send a reader hunting for a deleted rule that is sitting in the table; the second calls a rule absent while it is present. `_row_from_dict`'s own docstring already said the right thing ("Omitted, every row reports its rule as unresolved rather than claiming there is none") -- the code disagreed with it. `UNRESOLVED_RULE_DETAIL` is the third sentence those two cases needed. TABS THAT MATCH NOTHING. `statuses` stored the column verbatim while `status` was lowercased, and the client marks the active tab with `name === current`. A book row spelled `Filled` produced a tab that sent `?status=Filled`, got `filled` echoed back, and highlighted neither itself nor "all" -- and a book holding both spellings showed two tabs returning identical rows. Folded at the source. This is not hypothetical by this code's own reasoning: `api.py` declines to refuse unknown statuses precisely because venue-written and older spellings exist in real books, so the case it anticipates was the case it mishandled. THE TERMINAL WAS LEFT BEHIND. The service resolves the rule name for BOTH front-ends and only the browser rendered it, so `keel orders` could not answer a question the report already held. And `render_orders` had no branch for `empty_reason == "status"`: it printed its header, "showing 0 of N", and then nothing -- the silent blank the other two branches exist to prevent, and the one most likely to read as "keel has never traded". Unreachable from `keel orders` today (no `--status` flag), so it is pinned on the renderer rather than on the command. It now names the statuses the book does hold, because the next thing a reader wants is which filter would have worked. Also: "in this status" is captioned only when a status is actually on -- unfiltered it repeated the count below it under a label naming a filter that was not in force -- and `scopeSwitch`'s doc block, which the new function had been inserted underneath, is back above `scopeSwitch`. Co-Authored-By: Claude Opus 5 (1M context) --- keel/commands/orders.py | 39 ++++++++++++++- keel/web/static/js/render.js | 65 ++++++++++++++---------- tests/commands/test_orders.py | 87 +++++++++++++++++++++++++++++++++ tests/web/test_client_assets.py | 81 ++++++++++++++++++++++++++++++ 4 files changed, 243 insertions(+), 29 deletions(-) diff --git a/keel/commands/orders.py b/keel/commands/orders.py index 846108d..6847d36 100644 --- a/keel/commands/orders.py +++ b/keel/commands/orders.py @@ -385,6 +385,13 @@ class OrdersReport: #: and only the second is worth chasing. MISSING_RULE_DETAIL = "rule {rule_id} placed this order and is no longer in the book" +#: What a row says when nobody LOOKED. A caller that passed no rule map has not searched the +#: book, so reporting the rule as gone would state the result of a search that never ran -- and +#: would send a reader hunting for a deleted rule that is sitting in the table. The same sentence +#: covers a rule that WAS found and has no `kind` to show: it is present, so "no longer in the +#: book" would be false, and the only honest report is that no name is available. +UNRESOLVED_RULE_DETAIL = "rule {rule_id} placed this order; its name is unresolved here" + def _venue_order_id(raw_response: Any) -> str: """The venue's own order id, and nothing else, out of `raw_response`. @@ -480,11 +487,15 @@ def _row_from_dict(row: dict[str, Any], rule_names: Mapping[int, str] | None = N side = str(row.get("side") or "") confirmation = str(row.get("confirmation") or "") rule_id = None if row.get("rule_id") is None else int(row["rule_id"]) - rule_name = "" if rule_id is None else (rule_names or {}).get(rule_id, "") + rule_name = "" if rule_id is None or rule_names is None else rule_names.get(rule_id, "") if rule_name: rule_detail = "" elif rule_id is None: rule_detail = NO_RULE_DETAIL + elif rule_names is None or rule_id in rule_names: + # Either nobody looked, or the rule was found and carries no name. Both are "present, or + # at least not shown to be absent" -- and neither supports the claim below. + rule_detail = UNRESOLVED_RULE_DETAIL.format(rule_id=rule_id) else: rule_detail = MISSING_RULE_DETAIL.format(rule_id=rule_id) expected = row.get("expected_fill") @@ -602,7 +613,13 @@ def gather_orders( total += 1 modes.add(str(row.get("mode") or "")) # From every row in the BOOK, before either narrowing -- see `statuses`' own note. - statuses.add(str(row.get("status") or "")) + # LOWERCASED, like `resolved_status` below and for the same reason: the client marks the + # active tab with `name === current`, so a raw `Filled` tab would send `?status=Filled`, + # get `filled` echoed back, and highlight neither itself nor "all" -- while a book + # holding both spellings would show two tabs returning identical rows. `api.py` declines + # to refuse unknown statuses precisely because such spellings exist in real books, so + # they have to be folded here rather than assumed away. + statuses.add(str(row.get("status") or "").strip().lower()) created = row.get("created_at") if start_ts is not None and created is not None and int(created) < start_ts: continue @@ -719,6 +736,18 @@ def render_orders(report: OrdersReport) -> list[str]: f"outside scope={report.scope}. Widen it (--scope all) to see them." ) return lines + if report.empty_reason == "status": + # The fourth empty state needs the fourth sentence. Without it this renderer prints its + # header, "showing 0 of N", and then nothing -- the silent blank the two branches above + # exist to prevent, and the one a reader is most likely to read as "keel never traded". + # Names the statuses the book DOES hold, because the next thing a reader wants is which + # filter would have worked. + held = ", ".join(report.statuses) if report.statuses else "none" + lines.append( + f"no orders with status={report.status} in this window -- the book holds " + f"{report.total_count}, with statuses: {held}." + ) + return lines for row in report.rows: placement = "AUTONOMOUS" if row.confirmation_is_autonomous else (row.confirmation or "--") @@ -758,6 +787,12 @@ def render_orders(report: OrdersReport) -> list[str]: lines.append(f" venue order id: {row.venue_order_id}") else: lines.append(f" {row.venue_order_id_detail}") + # The service resolves this for BOTH front-ends; rendering it only in the browser would + # leave `keel orders` unable to answer a question the report already holds. + if row.rule_name: + lines.append(f" rule: {row.rule_name}") + else: + lines.append(f" {row.rule_name_detail}") rule = "--" if row.rule_id is None else str(row.rule_id) lines.append( f" rule={rule} placed={_utc(row.created_at)} updated={_utc(row.updated_at)}" diff --git a/keel/web/static/js/render.js b/keel/web/static/js/render.js index 986ec02..a9e089d 100644 --- a/keel/web/static/js/render.js +++ b/keel/web/static/js/render.js @@ -1186,10 +1186,12 @@ export function ordersView(data, sort, onSort, onScope, onStatus) { fragment.append( gridCard([ kv("shown", data.shown_count), - // `filtered_count`, never `shown` against `in scope`: with a tab open, the - // denominator of "shown" has to count that tab, and the report is the only place - // allowed to work it out (Rule 2 keeps the subtraction out of the browser). - kv("in this status", data.filtered_count), + // Only when a status is actually on. Unfiltered, `filtered_count` equals `scoped_count` + // by construction, so the row would repeat the number below it under a label naming a + // filter that is not in force. With a tab open it is the denominator "shown" is a page + // of -- and the report is the only place allowed to work that out (Rule 2 keeps the + // subtraction out of the browser). + ...(plain(data.status) ? [kv("in this status", data.filtered_count)] : []), kv("in scope", data.scoped_count), kv("in this book", data.total_count), // Which modes this book actually holds. A deployment book holds one, and saying which @@ -1231,6 +1233,9 @@ export function ordersView(data, sort, onSort, onScope, onStatus) { row.side, plain(row.product_id) || "—", row.status, + // Positional: `table()` pairs `columns[index]` with this array by index, so this cell + // sits where the "rule" header does and every later one shifts if it is missing. + row.rule_name, row.qty, row.filled_quantity, row.expected_fill, @@ -1401,29 +1406,6 @@ export function activityView(data, sort, onSort, onScope) { return fragment; } -/** - * The three scopes, and the one that is on. - * - * The order is `keel.commands.activity.ACTIVITY_SCOPES`', which is the order the TUI's `t` key - * cycles them in. - * - * **All three stay buttons, including the current one**, where the rendered page makes the - * current scope a bare ``. The rendered page is right for a LINK -- a link to the page - * you are on is a keyboard stop that goes nowhere -- and wrong for this: pressing the current - * scope re-reads it, which is a refresh, and taking the control away is what makes focus vanish - * when the view is rebuilt underneath a keyboard user who just pressed it. `aria-current` and the - * underline carry which one is on, from one attribute, so nothing is lost by keeping it pressable. - * - * The `label` argument arrived with #659's second caller. Two scope switches on one site - * announcing themselves identically would leave a screen-reader user unable to tell which view's - * window they had just landed in, and hard-coding "Activity scope" onto the Orders view would be - * worse than no name at all. - * - * @param {string} current - * @param {(scope: string) => void} onScope - * @param {string} [label] how the control announces itself. Defaults to Activity's wording. - * @returns {HTMLElement} - */ /** * The Orders status tabs (#700). * @@ -1451,12 +1433,18 @@ function statusSwitch(current, statuses, onStatus) { wrap.append(el("span", "k", "status")); const all = el("button", "scopekey", "all"); all.setAttribute("type", "button"); + // Pressing a tab replaces the whole view, which destroys the button that was pressed. + // `data-focus` is how `main.js` puts focus back on its replacement -- without it a keyboard + // user is returned to the top of the document on every tab press, which is exactly what the + // note above `sortTrigger` says this attribute exists to prevent. + all.setAttribute("data-focus", "status:"); if (!current) all.setAttribute("aria-current", "true"); all.addEventListener("click", () => onStatus("")); wrap.append(all); for (const name of statuses) { const button = el("button", "scopekey", name); button.setAttribute("type", "button"); + button.setAttribute("data-focus", "status:".concat(name)); if (name === current) button.setAttribute("aria-current", "true"); button.addEventListener("click", () => onStatus(name)); wrap.append(button); @@ -1464,6 +1452,29 @@ function statusSwitch(current, statuses, onStatus) { return wrap; } +/** + * The three scopes, and the one that is on. + * + * The order is `keel.commands.activity.ACTIVITY_SCOPES`', which is the order the TUI's `t` key + * cycles them in. + * + * **All three stay buttons, including the current one**, where the rendered page makes the + * current scope a bare ``. The rendered page is right for a LINK -- a link to the page + * you are on is a keyboard stop that goes nowhere -- and wrong for this: pressing the current + * scope re-reads it, which is a refresh, and taking the control away is what makes focus vanish + * when the view is rebuilt underneath a keyboard user who just pressed it. `aria-current` and the + * underline carry which one is on, from one attribute, so nothing is lost by keeping it pressable. + * + * The `label` argument arrived with #659's second caller. Two scope switches on one site + * announcing themselves identically would leave a screen-reader user unable to tell which view's + * window they had just landed in, and hard-coding "Activity scope" onto the Orders view would be + * worse than no name at all. + * + * @param {string} current + * @param {(scope: string) => void} onScope + * @param {string} [label] how the control announces itself. Defaults to Activity's wording. + * @returns {HTMLElement} + */ function scopeSwitch(current, onScope, label) { const wrap = el("nav", "scopes"); wrap.setAttribute("aria-label", label || "Activity scope"); diff --git a/tests/commands/test_orders.py b/tests/commands/test_orders.py index e2e46e0..1cf211a 100644 --- a/tests/commands/test_orders.py +++ b/tests/commands/test_orders.py @@ -896,3 +896,90 @@ def test_naming_the_rules_costs_one_read_however_many_orders(tmp_path: Path) -> assert repo.rule_reads == 1 assert {row.rule_name for row in report.rows} == {"turtle_breakout"} + + +# -- review findings on the rule name and the status tabs (#700) ------------------------------ + + +def test_an_unresolved_map_does_not_claim_the_rule_is_gone() -> None: + """The docstring and the code disagreed, and the docstring was right. With no map supplied, + the row has not been LOOKED UP -- saying "rule 7 is no longer in the book" states the result + of a search nobody performed, and sends a reader hunting for a deleted rule that is sitting + in the table.""" + unlooked = orders_service._row_from_dict(_order(id=1, rule_id=7)) + searched = orders_service._row_from_dict(_order(id=1, rule_id=7), {}) + + assert unlooked.rule_name == "" + assert "no longer" not in unlooked.rule_name_detail + assert "unresolved" in unlooked.rule_name_detail.lower() + # And the genuine miss still says what it found. + assert "no longer" in searched.rule_name_detail + + +def test_a_rule_with_no_name_is_not_reported_as_a_missing_rule() -> None: + """A rule row whose `kind` is empty was found -- it just has nothing to show. Reporting it as + absent would blame the book for a blank column.""" + row = orders_service._row_from_dict(_order(id=1, rule_id=7), {7: ""}) + + assert row.rule_name == "" + assert "no longer" not in row.rule_name_detail + + +def test_the_tab_list_is_lowercased_like_the_filter_it_is_compared_against( + tmp_path: Path, +) -> None: + """The client highlights the active tab with `name === current`. `status` is lowercased on + the way in, so a tab carrying the raw column would match neither itself nor "all" -- and a + book holding both `Filled` and `filled` would show two tabs returning identical rows. + + Not hypothetical by this module's own reasoning: `api.py` declines to refuse unknown + statuses precisely because venue-written and older spellings exist in real books.""" + repo = _repo(tmp_path) + repo.insert_order(_order(status="Filled")) + repo.insert_order(_order(status="filled")) + + report = gather_orders(repo, now_ts=NOW_TS, status="Filled") + + assert report.statuses == ("filled",) + assert report.status == "filled" + assert report.filtered_count == 2, "both spellings are the same status" + + +def test_the_cli_renderer_says_which_rule_placed_an_order(tmp_path: Path) -> None: + """The service resolves the name for BOTH front-ends. Rendering it only in the browser would + leave `keel orders` unable to answer a question the report already holds -- the asymmetry + this module's header exists to prevent.""" + repo = _repo(tmp_path) + rule_id = repo.insert_rule("turtle_breakout", {"product_id": "BTC-USD"}) + repo.insert_order(_order(rule_id=rule_id)) + + text = "\n".join(render_orders(gather_orders(repo, now_ts=NOW_TS))) + + assert "turtle_breakout" in text + + +def test_the_cli_renderer_explains_a_status_tab_that_matched_nothing() -> None: + """The fourth empty reason needs the fourth sentence here too. Without it the CLI prints its + header, "showing 0 of 3", and then nothing at all -- the silent blank the other two branches + exist to prevent. Unreachable from `keel orders` today (it has no `--status`), which is why + it is pinned on the renderer rather than on the command.""" + report = OrdersReport( + now_ts=NOW_TS, + scope="all", + scope_start_ts=None, + limit=50, + status="rejected", + total_count=3, + scoped_count=3, + filtered_count=0, + shown_count=0, + modes=("live",), + statuses=("filled",), + empty_reason="status", + rows=(), + ) + + text = "\n".join(render_orders(report)) + + assert "rejected" in text + assert "filled" in text, "a reader must be told which statuses this book does hold" diff --git a/tests/web/test_client_assets.py b/tests/web/test_client_assets.py index 639ba10..d5565a2 100644 --- a/tests/web/test_client_assets.py +++ b/tests/web/test_client_assets.py @@ -1414,3 +1414,84 @@ def test_an_unknown_drawdown_ceiling_draws_no_floor_line() -> None: assert "point.dd_floor_y !== null" in chart_code, ( "points with no recorded drawdown floor must be filtered before the floor is drawn" ) + + +# -- every table declares as many cells as it declares headers --------------------------------- + + +def _balanced_block(text: str, start: int) -> tuple[str, int]: + """The bracketed block beginning at `start` (which must be a `[`), and the index after it.""" + depth = 0 + for index in range(start, len(text)): + char = text[index] + if char in "([{": + depth += 1 + elif char in ")]}": + depth -= 1 + if depth == 0: + return text[start : index + 1], index + 1 + raise AssertionError("unbalanced block") + + +def _top_level_items(block: str) -> int: + """How many elements a `[...]` literal declares, counting commas at depth 1 only.""" + inner = block[1:-1] + depth = 0 + items = 0 + seen_content = False + for char in inner: + if char in "([{": + depth += 1 + elif char in ")]}": + depth -= 1 + elif char == "," and depth == 0: + items += 1 + seen_content = False + continue + if not char.isspace(): + seen_content = True + return items + (1 if seen_content else 0) + + +def _table_calls(code: str) -> list[tuple[int, int]]: + """`(headers, cells)` for every `table(` call in `render.js` that inlines both.""" + pairs: list[tuple[int, int]] = [] + cursor = 0 + while True: + found = code.find("table(", cursor) + if found == -1: + return pairs + cursor = found + 6 + # The columns array is the first `[` after the id argument. + columns_at = code.find("[", cursor) + if columns_at == -1: + continue + columns_block, after = _balanced_block(code, columns_at) + if "label:" not in columns_block: + continue # not a table() call -- some other identifier ending in `table(` + headers = columns_block.count("label:") + # The cells come from a `.map(` whose arrow body is an array literal. + map_at = code.find(".map(", after) + if map_at == -1 or map_at > after + 400: + continue # rows passed as a variable; nothing inline to compare + cells_at = code.find("[", map_at) + if cells_at == -1: + continue + cells_block, _ = _balanced_block(code, cells_at) + pairs.append((headers, _top_level_items(cells_block))) + + +def test_every_table_emits_one_cell_per_declared_header() -> None: + """`table()` pairs `columns[index]` with `row.entries()` BY POSITION (render.js:400), so a + column added to the header list without a matching value silently shifts every later cell + one place left -- and the last column renders no `` at all. + + That is not a cosmetic failure. On the Orders table it put the placement timestamp under + "fee" and a quantity under "rule", so every money column named the wrong figure while each + value stayed individually true. Nothing else in this suite compares the two lists: the + per-view tests assert that a key is *declared*, which a header alone satisfies.""" + pairs = _table_calls(_code_only(_source("render.js"))) + + assert len(pairs) >= 6, f"the table scan found only {len(pairs)} tables; it has stopped working" + mismatched = [(headers, cells) for headers, cells in pairs if headers != cells] + assert mismatched == [], f"(headers, cells) mismatches: {mismatched}"