diff --git a/context/code/command.md b/context/code/command.md index efd1995e..7c042044 100644 --- a/context/code/command.md +++ b/context/code/command.md @@ -997,7 +997,9 @@ coordinates nothing owns. The fix has two parts. the plainer case. A **measure edit alone** on such a row stages no `AddItem`, so nothing refuses, and it still writes an orphan cell keyed under the removed category. `RecordRewrite::of` therefore asks the model whether every category - the row names still exists, both its coordinates and its renames. If any is + the row names still exists, both its coordinates and its renames (a sparse + row with no coordinate for a category it renames reaches the check only + through the renames; improvise-6r2y). If any is gone, the row is `unwritable`: it refuses with `Unknown category '…'` and carries no writes. This is ADR 001's "force never creates structure", applied inside the drill. @@ -1005,8 +1007,17 @@ coordinates nothing owns. The fix has two parts. (`design-principles.md` §4: "`ApplyAndClearDrill` writes them all atomically … No partial writes reach the model"). A per-row skip would commit the other rows' edits and drop this one's, a partial write of exactly the kind that - contract rules out. `merged` therefore keeps only the refusals when any row is - unwritable. + contract rules out. So when any row is unwritable, `merged` writes nothing. +- **What the user is told (improvise-o9q7).** `merged` first kept every row's + refusals. Refusals are applied in row order and each one overwrites the + status line, so a later writable row's own refusal, such as a blank + coordinate, replaced the one that mattered. The user read "Record coordinates + cannot be empty" when in fact every staged edit had been thrown away. So when + any row is unwritable, `merged` keeps only the unwritable rows' reasons and + combines them into one refusal that says so: + `Unknown category 'Type': every staged edit was discarded`. Sibling refusals + are dropped because nothing those rows staged is written, so their reasons + describe edits that are no longer pending. - **`refuse`, not `refuse_and_stop`.** This follows #181's rule (gotcha 21): abort only where a later effect in the batch assumes the failed one happened. The effect after `ApplyAndClearDrill` in `view-back`'s batch is `ViewBack`, diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index 4ccf6b91..c9f8f2dd 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -5629,3 +5629,44 @@ no `Type` coordinate at all, so it stays writable after `Type` is removed. Both rows stage a `Cost`. The expected store is row 1's untouched `-/Jan/Cost = 17`. Rival A skips only the unwritable row, and it fails this test and nothing else in the workspace, writing row 1's `999`. Rival B fails here too. + +## `ui/effect.rs` — the renames half and the refusal wording of the qz9p check (improvise-6r2y, improvise-o9q7) + +### a_sparse_drill_row_renamed_into_a_removed_category_is_refused_and_writes_nothing + +improvise-6r2y. The qz9p check asks whether every category a row names still +exists, and it collects those names from **two** sources: the row's coordinates +and the categories it renames. Before this test, deleting the renames half +(`.chain(renames.keys().copied())`) left the lib green. The rename tests all +use rows that already carry a `Type` coordinate, so the coordinates half caught +them anyway. + +This test uses the one row that reaches the renames half only: a sparse record +`Jan/Cost = 17` with no `Type` coordinate, which stages `Type → Drink` after +`Type` is removed. + +Fault injected: the `.chain(...)` line commented out. The test failed on the +store assertion, because the commit wrote an orphan `Drink/Jan/Cost = 17`. The +refusal assertion alone would not have caught it: `AddItem` into the missing +category refuses by itself, so `refused` is true under the fault. The store +assertion is what makes the test work. + +### an_unwritable_drill_commit_reports_the_discard_not_a_later_rows_refusal + +improvise-o9q7. Row 0 names the removed `Type` and stages `Cost = 5`. Row 1, the +sparse record, stages a blank `Month`. `merged` used to keep both rows' +refusals. They were applied in row order, so the status line ended on row 1's +"Record coordinates cannot be empty". That hid the fact that the whole commit +had been discarded. + +This test was red before the fix: the status line was exactly +`Record coordinates cannot be empty`. It asserts three things: +- the status names `Unknown category 'Type'`; +- the status says `every staged edit was discarded`; +- the status does **not** carry the blank-coordinate refusal. + +Reading from those three assertions, not by injecting them, each of these +partial fixes would fail the test: +- one that only rewords the message; +- one that only filters out the writable rows' refusals; +- one that emits the combined refusal first and the siblings' refusals after it. diff --git a/crates/improvise-command/src/ui/effect.rs b/crates/improvise-command/src/ui/effect.rs index 1e069ecb..fd7662bf 100644 --- a/crates/improvise-command/src/ui/effect.rs +++ b/crates/improvise-command/src/ui/effect.rs @@ -803,21 +803,32 @@ impl RecordRewrite { /// Several rows' rewrites as one: every clear before any write, the later row winning /// a shared cell, nothing written if any row is unwritable. `context/code/command.md`. pub(crate) fn merged(rewrites: impl IntoIterator) -> Self { - let all = rewrites.into_iter().fold(Self::default(), |mut all, one| { - all.refusals.extend(one.refusals); - all.unwritable |= one.unwritable; - all.items.extend(one.items); - all.clears.extend(one.clears); - all.sets.extend(one.sets); - all - }); - match all.unwritable { - true => Self { - refusals: all.refusals, - unwritable: true, - ..Self::default() - }, - false => all, + let rewrites: Vec = rewrites.into_iter().collect(); + match rewrites.iter().any(|one| one.unwritable) { + true => Self::discarded(rewrites.into_iter().filter(|one| one.unwritable)), + false => rewrites.into_iter().fold(Self::default(), |mut all, one| { + all.refusals.extend(one.refusals); + all.items.extend(one.items); + all.clears.extend(one.clears); + all.sets.extend(one.sets); + all + }), + } + } + + /// One refusal naming every gone category and saying the whole commit was dropped, + /// so no writable row's refusal can overwrite it (improvise-o9q7). + fn discarded(unwritable: impl Iterator) -> Self { + let why: std::collections::BTreeSet = + unwritable.flat_map(|one| one.refusals).collect(); + let why: Vec = why.into_iter().collect(); + Self { + refusals: vec![format!( + "{}: every staged edit was discarded", + why.join("; ") + )], + unwritable: true, + ..Self::default() } } @@ -3514,8 +3525,9 @@ mod tests { assert_eq!(store, std::collections::BTreeSet::new(), "no orphan cell"); } - #[test] - fn a_drill_commit_with_one_unwritable_row_writes_none_of_its_rows() { + /// A drill over `Food/Jan/Cost = 2` (row 0) and a sparse `Jan/Cost = 17` naming no + /// Type (row 1). + fn typed_and_untyped_drill() -> App { let mut app = test_app(); let typed = drill_cell("Food", "Jan", "Cost"); let untyped = CellKey::new(vec![ @@ -3533,9 +3545,43 @@ mod tests { assert_eq!(records.len(), 2); assert_eq!(records[1].coords.get("Type"), None, "row 1 names no Type"); StartDrill(records).apply(&mut app); + app + } + #[test] + fn a_sparse_drill_row_renamed_into_a_removed_category_is_refused_and_writes_nothing() { let (store, refused) = commit_after_removing_type( - app, + typed_and_untyped_drill(), + &[(1, category("Type"), "Drink")], + ); + assert!(refused, "the commit must say it could not be applied"); + assert_eq!( + store, + ["-/Jan/Cost = 17".to_string()].into_iter().collect(), + "row 1 names Type only through its rename; no orphan Drink cell" + ); + } + + #[test] + fn an_unwritable_drill_commit_reports_the_discard_not_a_later_rows_refusal() { + let mut app = typed_and_untyped_drill(); + app.model_state.workbook.remove_category("Type"); + stage(&mut app, 0, measure("Cost"), "5"); + stage(&mut app, 1, category("Month"), ""); + app.apply_effects(vec![Box::new(ApplyAndClearDrill)]); + let status = &app.view_state.status_msg; + assert!(status.contains("Unknown category 'Type'"), "{status}"); + assert!( + status.contains("every staged edit was discarded"), + "{status}" + ); + assert!(!status.contains(RECORD_COORDS_CANNOT_BE_EMPTY), "{status}"); + } + + #[test] + fn a_drill_commit_with_one_unwritable_row_writes_none_of_its_rows() { + let (store, refused) = commit_after_removing_type( + typed_and_untyped_drill(), &[(0, measure("Cost"), "5"), (1, measure("Cost"), "999")], ); assert!(refused, "the commit must say it could not be applied");