diff --git a/crates/engine/src/cmd/pivot.rs b/crates/engine/src/cmd/pivot.rs index 80f8d0205..3b6ed83a4 100644 --- a/crates/engine/src/cmd/pivot.rs +++ b/crates/engine/src/cmd/pivot.rs @@ -753,10 +753,10 @@ fn show_details(s: &mut Session, p: &Json) -> Result { .pivots .iter() .find(|pt| pt.last_range.is_some_and(|r| r.contains(at))) - .ok_or_else(|| EngineError::Disabled("pivot.showDetails".into(), "select a value cell in a PivotTable".into()))?; + .ok_or_else(|| EngineError::Disabled("pivot.showDetails".into(), pv::NOT_A_VALUE_CELL.into()))?; let origin = pt.last_range.map(|r| r.start).unwrap_or_default(); let found = pv::detail_records(&d.wb, pt, active, at.row.saturating_sub(origin.row), at.col.saturating_sub(origin.col)).map_err(other)?; - let (src, records) = found.ok_or_else(|| EngineError::Disabled("pivot.showDetails".into(), "select a value cell in a PivotTable".into()))?; + let (src, records) = found.ok_or_else(|| EngineError::Disabled("pivot.showDetails".into(), pv::NOT_A_VALUE_CELL.into()))?; if d.wb.protected_structure { return Err(other("The workbook is protected and cannot be changed.")); } @@ -2541,6 +2541,22 @@ mod tests { assert!(rows.iter().skip(1).all(|row| row[0] == "East" && row[1] == "Apples"), "{rows:?}"); } + #[test] + fn show_details_refuses_a_report_its_source_has_moved_on_from() { + let mut s = sales(); + s.execute("insert.pivotTable", json!({"rows": ["Region"], "columns": ["Product"], "values": ["Sales"]})).unwrap(); + // B5 still shows East × Apples (160), but the first East record is now Central's. + s.execute("range.setValues", json!({"range": "Sheet1!A2", "values": [["Central"]]})).unwrap(); + assert_eq!(val(&s, "B5"), Value::Number(160.0)); + let e = s.execute("pivot.showDetails", json!({"cell": "B5"})).unwrap_err().to_string(); + assert!(e.contains("out of date"), "{e}"); + assert_eq!(s.doc().unwrap().wb.sheets.len(), 2); + // After a refresh East × Apples (now B6) has the one record left. + s.execute("pivot.refresh", json!({})).unwrap(); + assert_eq!(text(&s, "A6"), "East"); + assert_eq!(s.execute("pivot.showDetails", json!({"cell": "B6"})).unwrap()["records"], 1); + } + #[test] fn show_details_needs_a_value_cell() { let mut s = sales(); diff --git a/crates/engine/src/pivot.rs b/crates/engine/src/pivot.rs index 43a77c11a..45c4eb0a5 100644 --- a/crates/engine/src/pivot.rs +++ b/crates/engine/src/pivot.rs @@ -806,14 +806,32 @@ pub fn compute(wb: &Workbook, pt: &PivotTable, pivot_sheet: usize) -> Result Result)>, String> { let src = read_source(wb, &pt.source, pivot_sheet)?; let mut probe = Probe { at: (row as usize, col as usize), hit: None, paths: Vec::new() }; - compute_from(wb, pt, &src, Some(&mut probe))?; + let out = compute_from(wb, pt, &src, Some(&mut probe))?; let Some((rp, cp)) = probe.hit else { return Ok(None) }; + // Excel reads its PivotTable cache; without one, the report written by the last refresh + // must still be what the source gives. + let sh = wb.sheet(pivot_sheet).ok_or("There's no such sheet.")?; + let current = pt.last_range.is_some_and(|r| { + r.height() == out.height().max(1) + && r.width() == out.width.max(1) + && out.grid.iter().enumerate().all(|(ri, line)| { + line.iter().enumerate().all(|(ci, oc)| sh.value(CellRef::new(r.start.row + ri as u32, r.start.col + ci as u32)) == oc.value) + }) + }); + if !current { + return Err("The PivotTable report is out of date. Refresh it first.".into()); + } let records = probe.paths.into_iter().filter(|(_, r, c)| r.starts_with(&rp) && c.starts_with(&cp)).map(|(i, _, _)| i).collect(); Ok(Some((src, records))) } diff --git a/crates/ui-egui/src/grid.rs b/crates/ui-egui/src/grid.rs index 1b72576e5..9fbdfc911 100644 --- a/crates/ui-egui/src/grid.rs +++ b/crates/ui-egui/src/grid.rs @@ -2388,10 +2388,17 @@ fn interact(app: &mut SheetApp, ui: &mut egui::Ui, resp: &egui::Response, geo: & app.grid.last_click = None; // A PivotTable's value cell shows its source records on a new sheet (Excel's Show // Details); its labels and headers still edit. - if sh.pivots.iter().any(|pt| pt.last_range.is_some_and(|r| r.contains(c))) - && app.run("pivot.showDetails", json!({"cell": c.a1()})).is_ok() - { - return; + if sh.pivots.iter().any(|pt| pt.last_range.is_some_and(|r| r.contains(c))) { + let not_a_value_cell = + gridcraft_engine::EngineError::Disabled("pivot.showDetails".into(), gridcraft_engine::pivot::NOT_A_VALUE_CELL.into()).to_string(); + match app.run("pivot.showDetails", json!({"cell": c.a1()})) { + Ok(_) => return, + Err(e) if e == not_a_value_cell => {} + Err(e) => { + app.message = Some(("GridCraft".into(), crate::clean_error(&e))); + return; + } + } } // Following a hyperlink requires a single click in Excel; double-click edits. app.begin_edit(None, false); diff --git a/crates/ui-egui/tests/it/pivot_show_details.rs b/crates/ui-egui/tests/it/pivot_show_details.rs index e78ca53ec..629774b04 100644 --- a/crates/ui-egui/tests/it/pivot_show_details.rs +++ b/crates/ui-egui/tests/it/pivot_show_details.rs @@ -74,3 +74,17 @@ fn double_click_on_a_row_label_still_edits() { assert!(app.editor.is_some()); assert_eq!(app.session.active().unwrap().wb.sheets.len(), 1); } + +#[test] +fn double_click_on_a_value_cell_of_a_protected_workbook_says_why_and_does_not_edit() { + let mut h = harness(); + h.state_mut().session.execute("review.protectWorkbook", json!({"on": true})).unwrap(); + h.run_steps(2); + let pos = center(&h, "E4"); + double_click(&mut h, pos); + + let app = h.state(); + assert!(app.editor.is_none(), "the value cell must not open for editing"); + assert!(app.message.as_ref().is_some_and(|(_, m)| m.contains("protected")), "{:?}", app.message); + assert_eq!(app.session.active().unwrap().wb.sheets.len(), 1); +}