Merge pull request #235 from fiddlerwoaroof/collapse-fu/b114
improvise-b114/xur5: no coordinate of a wire write may name a group
This commit is contained in:
@@ -3607,6 +3607,18 @@ the refusal marks the batch, `Host::mutating_frame` undoes the frame, the asker
|
||||
gets `Rejected(MutationRefused)`, and the reason is on the host's status line.
|
||||
The question it asks is the projection's own, never a second derivation of it:
|
||||
|
||||
- A coordinate that names a **group** rather than an item is refused, in any
|
||||
view (improvise-b114). A group has no cell of its own, so writing one stores a
|
||||
non-item coordinate. This rule asks the document, not the layout, and runs
|
||||
first, because a fold in the measure half hides from both rules below. Take
|
||||
`{Month=Feb, _Measure=Money}`, where `Money` groups Revenue and Cost. Its
|
||||
record `{Month=Feb}` is stored, so the second rule below accepts it and never
|
||||
asks the projection. And with `Money` expanded the host draws no such cell at
|
||||
all, so the third rule has no position to ask. A fold on the row axis is
|
||||
refused here too, so the `Aggregate` arm below now guards only the None-axis
|
||||
aggregate. It also closes improvise-xur5: a fold's record `{Month=Q2}` written
|
||||
with a measure the host draws nowhere (hidden, unknown or absent) has no
|
||||
position for the third rule to ask, and was accepted.
|
||||
- A cell a formula owns is refused. That is `cell_id`'s own `Derived` rule,
|
||||
`formula_for(&key)`. It is asked directly because a stored record may sit on
|
||||
no position in the host's current view.
|
||||
|
||||
@@ -5862,8 +5862,10 @@ client sends `SetCell` (then, separately, `ClearCell`) for `{Month=Q2, _Measure=
|
||||
with a caught-up basis. At `ea119bd9` both were answered `Accepted`, and the
|
||||
`SetCell` stored `Month=Q2, _Measure=Cost = 7`, the non-item coordinate
|
||||
improvise-gw18 exists to prevent. The test asserts `Rejected(MutationRefused)`
|
||||
and an unchanged document for both. Rival: the aggregate refusal disabled. This
|
||||
test and the None-axis test fail. The control is
|
||||
and an unchanged document for both. Rival: the aggregate refusal disabled. At
|
||||
`46e75c41` that failed this test and the None-axis test. Since improvise-b114 a
|
||||
fold is refused first by the group rule, so this rival fails only the None-axis
|
||||
test. Disabling both rules fails this test. The control is
|
||||
`a_wire_write_to_a_stored_cell_beside_a_fold_is_accepted`, so a host that refused
|
||||
every wire write cannot pass.
|
||||
|
||||
@@ -5906,6 +5908,39 @@ 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_wire_write_naming_a_measure_group_is_rejected_in_any_view
|
||||
|
||||
improvise-b114, found in the review of PR #217 at `bc060f67` (the reviewer's r3).
|
||||
`_Measure` is on Column, Revenue and Cost are grouped as `Money`, and `Units` is
|
||||
ungrouped. Every month is funded. A caught-up `SetCell` on
|
||||
`{Month=Feb, _Measure=Money}` was answered `Accepted` and stored
|
||||
`Month=Feb, _Measure=Money = 7`, a non-item coordinate. Its record `{Month=Feb}`
|
||||
is stored, so the stored arm accepted it without asking the projection. The test
|
||||
runs with `Money` collapsed, where the host draws that cell as the fold, and
|
||||
expanded, where the host draws no such cell. It asserts `MutationRefused` and an
|
||||
unchanged document in both, so a rule that asks the host's layout cannot pass
|
||||
the expanded half. Red at `5eaafbae`. The fix is `App::group_refusal`, which asks
|
||||
the document whether any coordinate names a group rather than an item. Rival:
|
||||
the group rule removed. Injected into a full workspace run, it fails this test
|
||||
and nothing else.
|
||||
|
||||
### a_wire_write_to_a_fold_record_with_an_undrawn_measure_is_rejected
|
||||
|
||||
improvise-xur5, found in the delta re-review of PR #217 at `6026768c`. It is the
|
||||
reviewer's `review_217b::probe_fold_hidden_measure`, widened to all three rows
|
||||
of the reviewer's table. Q2 is collapsed and every month is stored, so the
|
||||
record `{Month=Q2}` is unstored and resolves only through the layout, at the
|
||||
fold row. A caught-up write names that record with a measure the host draws
|
||||
nowhere: `Cost` hidden in the host's view, `Bogus`, which is no measure at all,
|
||||
or no measure. At `6026768c` all three were **accepted** and stored `Month=Q2`,
|
||||
a group name, as a coordinate. The sjdb fix finds the position by the whole key,
|
||||
and "not drawn" read as "not refused". Falling back to the by-record lookup for
|
||||
an undrawn key would close this, but the reviewer showed it wrongly refuses
|
||||
Feb × Revenue beside a collapsed `Money` group, so it is not the fix. The
|
||||
improvise-b114 rule is: no coordinate names a group, asked of the document.
|
||||
Rival: the group rule removed. Injected into a full workspace run, it fails this
|
||||
test and the b114 test, and nothing else.
|
||||
|
||||
### a_stored_record_the_hosts_view_aggregates_is_writable
|
||||
|
||||
improvise-sjdb, from the #217 review. South/Revenue is stored, then `Product` is
|
||||
|
||||
@@ -587,7 +587,20 @@ impl App {
|
||||
f.target
|
||||
)
|
||||
});
|
||||
derived.or_else(|| self.aggregate_refusal(cell.record, &key))
|
||||
let refusal = self.group_refusal(&key).or(derived);
|
||||
refusal.or_else(|| self.aggregate_refusal(cell.record, &key))
|
||||
}
|
||||
|
||||
/// A coordinate naming a group rather than an item is no cell in ANY view, so
|
||||
/// this asks the document, never the layout (improvise-b114).
|
||||
fn group_refusal(&self, key: &CellKey) -> Option<String> {
|
||||
let model = &self.model_state.workbook.model;
|
||||
key.0.iter().find_map(|(cat, name)| {
|
||||
let category = model.category(cat)?;
|
||||
let group = !category.items.contains_key(name.as_str())
|
||||
&& !category.items_in_group(name).is_empty();
|
||||
group.then(|| format!("{name} is a group of {cat}, so there is no one cell to write"))
|
||||
})
|
||||
}
|
||||
|
||||
/// A record the store does not hold is known only by its layout position, and
|
||||
|
||||
@@ -1606,6 +1606,58 @@ mod tests {
|
||||
assert_eq!(model.get_cell(&target), Some(&CellValue::Number(7.0)));
|
||||
}
|
||||
|
||||
/// improvise-b114: a coordinate naming a group is no item, in any view — see
|
||||
/// context/test-evidence/command.md.
|
||||
#[test]
|
||||
fn a_wire_write_naming_a_measure_group_is_rejected_in_any_view() {
|
||||
for collapsed in [true, false] {
|
||||
let mut wb = Workbook::new("T");
|
||||
wb.add_category("Month").unwrap();
|
||||
for month in ["Jan", "Feb"] {
|
||||
wb.add_item("Month", month).unwrap();
|
||||
for measure in ["Revenue", "Cost"] {
|
||||
wb.add_item_in_group("_Measure", measure, "Money").unwrap();
|
||||
let key = month_cell(month, measure);
|
||||
wb.model.set_cell(key, CellValue::Number(10.0));
|
||||
}
|
||||
}
|
||||
wb.add_item("_Measure", "Units").unwrap();
|
||||
wb.active_view_mut().set_axis("Month", Axis::Row);
|
||||
wb.active_view_mut().set_axis("_Measure", Axis::Column);
|
||||
if collapsed {
|
||||
wb.active_view_mut()
|
||||
.toggle_group_collapse("_Measure", "Money");
|
||||
}
|
||||
let mut app = App::new(wb, None);
|
||||
app.rebuild_layout();
|
||||
let mut host = Host::new(app);
|
||||
let before = document_text(&host);
|
||||
let answer = wire_write(&mut host, &month_cell("Feb", "Money"), false);
|
||||
assert!(refused(&answer), "collapsed={collapsed}: {answer:?}");
|
||||
let after = document_text(&host);
|
||||
assert_eq!(after, before, "collapsed={collapsed}: a group was stored");
|
||||
}
|
||||
}
|
||||
|
||||
/// improvise-xur5 (the reviewer's `probe_fold_hidden_measure`): a fold's record
|
||||
/// with a measure the host draws nowhere — see context/test-evidence/command.md.
|
||||
#[test]
|
||||
fn a_wire_write_to_a_fold_record_with_an_undrawn_measure_is_rejected() {
|
||||
let hidden = month_cell("Q2", "Cost");
|
||||
let bogus = month_cell("Q2", "Bogus");
|
||||
let bare = CellKey::new(vec![("Month".to_string(), "Q2".to_string())]);
|
||||
for key in [hidden, bogus, bare] {
|
||||
let mut host = host_on_a_fold();
|
||||
let view = host.app.model_state.workbook.active_view_mut();
|
||||
view.hide_item("_Measure", "Cost");
|
||||
host.app.rebuild_layout();
|
||||
let before = document_text(&host);
|
||||
let answer = wire_write(&mut host, &key, false);
|
||||
assert!(refused(&answer), "{key:?} accepted: {answer:?}");
|
||||
assert_eq!(document_text(&host), before, "{key:?} stored a group");
|
||||
}
|
||||
}
|
||||
|
||||
/// A STORED record is writable whatever the host's view does to it — see
|
||||
/// context/test-evidence/command.md (improvise-sjdb).
|
||||
#[test]
|
||||
|
||||
Reference in New Issue
Block a user