diff --git a/context/code/command.md b/context/code/command.md index e4704362..f63eb334 100644 --- a/context/code/command.md +++ b/context/code/command.md @@ -3422,8 +3422,15 @@ The question it asks is the projection's own, never a second derivation of it: - A record found **in the store** is a record, whatever view anyone is on. `RecordHash` is document-scoped (gotcha 44). - A record found **only through the layout** is asked of `project::cell_id` at - the position where it was found. `Aggregate`, a fold or a sum over a None-axis - category, is refused. + the position of **its cell**: the one whose key is the whole key the write + names, record and measure together. `Aggregate`, a fold or a sum over a + None-axis category, is refused. It must be the whole key, not the first + position carrying the record (improvise-sjdb). `RecordHash` drops `_Measure`, + so every column of a row carries the same record, and with a collapsed + `_Measure` group the first of them can be the fold. That refused the empty + `Units` cell beside a `Money` fold, which the host itself projects as a + `Record`. A write whose whole key the host's grid does not draw is not + refused here, as it was not before this rule existed. **Why the `vec![]` arm stays.** A reviewer asked whether a `SetCell` whose cell does not resolve at all, which `model_effects` turns into `vec![]` and so diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index ac34354b..7e3ca124 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -5611,3 +5611,29 @@ it and when a hand-rolled basis omits it. The second half was expected to be `ChainBroken`, and the test said otherwise: the create exemption covers only commands after the first in a frame. That surprise is why it is asserted rather than argued. + +### an_empty_cell_beside_a_collapsed_measure_group_is_writable + +improvise-sjdb, a regression PR #217 introduced and its review found. `_Measure` +is on Column with Revenue and Cost in a collapsed group `Money` and `Units` +ungrouped, and the Feb row is empty. The host projects Feb × Units as a +`Record`. A caught-up `SetCell` on it was answered `Rejected(MutationRefused)` +at `bc060f67`. It was accepted before #217. `aggregate_refusal` found the +unstored record by `RecordHash` alone, which drops `_Measure`, so it judged the +record at the first column on Feb's row, the `Money` fold, which `cell_id` calls +`Aggregate`. The setup asserts that the fold comes before `Units` on the row and +that the target projects as `Record`, so a fixture where the first column is +harmless cannot pass. The fix looks the cell up by its whole key. The rival is +the old lookup by record, which is this test's red commit, `6e021699`. + +### a_stored_record_the_hosts_view_aggregates_is_writable + +improvise-sjdb, from the #217 review. South/Revenue is stored, then `Product` is +added on `Axis::None` in the host's view, so the host projects South's position +as `Aggregate`. A stored record is a record wherever it is drawn, because +`RecordHash` is document-scoped (gotcha 44). A records-mode client, or one on +another view, may write it. The setup asserts the host really does project +`Aggregate` there, so the test cannot pass on a view that draws South plainly. +Rival: stored records sent through `cell_id` like unstored ones. That rival +fails this test and nothing else in `improvise-command`. Before this test, +nothing in the suite saw it. diff --git a/crates/improvise-command/src/ui/app.rs b/crates/improvise-command/src/ui/app.rs index b2c04003..0113359c 100644 --- a/crates/improvise-command/src/ui/app.rs +++ b/crates/improvise-command/src/ui/app.rs @@ -583,24 +583,25 @@ impl App { f.target ) }); - derived.or_else(|| self.aggregate_refusal(cell.record)) + derived.or_else(|| self.aggregate_refusal(cell.record, &key)) } /// A record the store does not hold is known only by its layout position, and - /// is a record only where the projection there says `Record`, never `Aggregate`. - fn aggregate_refusal(&self, hash: RecordHash) -> Option { + /// is a record only where the projection at ITS CELL says so (improvise-sjdb). + fn aggregate_refusal(&self, hash: RecordHash, key: &CellKey) -> Option { let stored = self.model_state.workbook.model.data.resolve_record(hash); - let (row, col) = stored.map_or_else(|| self.record_position(hash), |_| None)?; + let (row, col) = stored.map_or_else(|| self.cell_position(key), |_| None)?; let projected = crate::ui::project::cell_id(self, &self.layout, row, col); matches!(projected, Some(CellId::Aggregate(_))).then(|| { "an aggregate combines several records, so there is no one cell to write".into() }) } - /// Where in the current layout a cell of record `hash` sits, if anywhere. - fn record_position(&self, hash: RecordHash) -> Option<(usize, usize)> { + /// Where in the current layout the cell `key` sits, if anywhere. By the whole + /// key: a `RecordHash` drops `_Measure`, so a row's columns all share one. + fn cell_position(&self, key: &CellKey) -> Option<(usize, usize)> { self.layout_cells() - .find(|(_, _, key)| RecordHash::of(key) == hash) + .find(|(_, _, at)| at == key) .map(|(row, col, _)| (row, col)) }