fix(io): enforce category limit in import wizard
The import wizard now proactively checks the `MAX_CATEGORIES` limit during the proposal and configuration steps. Advancing is blocked with a descriptive message if the limit would be exceeded. Fixed UI rendering order in `ImportWizardWidget` so messages are correctly displayed. Add regression tests for category limit enforcement. Co-Authored-By: fiddlerwoaroof/git-smart-commit (unsloth/gemma-4-26B-A4B-it-GGUF:UD-Q5_K_XL)
This commit is contained in:
@@ -9,6 +9,11 @@ use crate::formula::parse_formula;
|
|||||||
use crate::model::cell::{CellKey, CellValue};
|
use crate::model::cell::{CellKey, CellValue};
|
||||||
use crate::workbook::Workbook;
|
use crate::workbook::Workbook;
|
||||||
|
|
||||||
|
/// Mirrors the model's private `MAX_CATEGORIES` limit (regular categories
|
||||||
|
/// only; virtual `_Index`/`_Dim`/`_Measure` don't count). Used to pre-check
|
||||||
|
/// proposals before the wizard's confirm step (improvise-mzv).
|
||||||
|
const MAX_CATEGORIES: usize = 12;
|
||||||
|
|
||||||
// ── Pipeline (no UI state) ────────────────────────────────────────────────────
|
// ── Pipeline (no UI state) ────────────────────────────────────────────────────
|
||||||
|
|
||||||
/// Pure data + logic for turning a JSON value into a Model.
|
/// Pure data + logic for turning a JSON value into a Model.
|
||||||
@@ -80,6 +85,22 @@ impl ImportPipeline {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Number of regular categories `build_model` would create: accepted
|
||||||
|
/// Category/TimeCategory fields plus derived date-component categories.
|
||||||
|
/// (Measures and labels map to non-regular categories and don't count.)
|
||||||
|
pub fn proposed_category_count(&self) -> usize {
|
||||||
|
self.proposals
|
||||||
|
.iter()
|
||||||
|
.filter(|p| p.accepted)
|
||||||
|
.map(|p| match p.kind {
|
||||||
|
FieldKind::Category => 1,
|
||||||
|
FieldKind::TimeCategory if p.date_format.is_some() => 1 + p.date_components.len(),
|
||||||
|
FieldKind::TimeCategory => 1,
|
||||||
|
FieldKind::Measure | FieldKind::Label => 0,
|
||||||
|
})
|
||||||
|
.sum()
|
||||||
|
}
|
||||||
|
|
||||||
/// Build a Workbook from the current proposals. Pure — no side effects.
|
/// Build a Workbook from the current proposals. Pure — no side effects.
|
||||||
pub fn build_model(&self) -> Result<Workbook> {
|
pub fn build_model(&self) -> Result<Workbook> {
|
||||||
let categories: Vec<&FieldProposal> = self
|
let categories: Vec<&FieldProposal> = self
|
||||||
@@ -297,6 +318,17 @@ impl ImportWizard {
|
|||||||
// ── Step transitions ──────────────────────────────────────────────────────
|
// ── Step transitions ──────────────────────────────────────────────────────
|
||||||
|
|
||||||
pub fn advance(&mut self) {
|
pub fn advance(&mut self) {
|
||||||
|
// Pre-check the category limit where the user can still fix it
|
||||||
|
// (improvise-mzv): block here instead of failing in build_model()
|
||||||
|
// after the final confirm.
|
||||||
|
if matches!(
|
||||||
|
self.step,
|
||||||
|
WizardStep::ReviewProposals | WizardStep::ConfigureDates
|
||||||
|
) && let Some(msg) = self.category_limit_message()
|
||||||
|
{
|
||||||
|
self.message = Some(msg);
|
||||||
|
return;
|
||||||
|
}
|
||||||
self.step = match self.step {
|
self.step = match self.step {
|
||||||
WizardStep::Preview => {
|
WizardStep::Preview => {
|
||||||
if self.pipeline.array_paths.len() > 1 && self.pipeline.needs_path_selection() {
|
if self.pipeline.array_paths.len() > 1 && self.pipeline.needs_path_selection() {
|
||||||
@@ -322,6 +354,18 @@ impl ImportWizard {
|
|||||||
self.message = None;
|
self.message = None;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// User-facing message when the accepted proposals would exceed the
|
||||||
|
/// model's regular-category limit; `None` when within the limit.
|
||||||
|
fn category_limit_message(&self) -> Option<String> {
|
||||||
|
let count = self.pipeline.proposed_category_count();
|
||||||
|
(count > MAX_CATEGORIES).then(|| {
|
||||||
|
format!(
|
||||||
|
"Too many categories: {count} proposed, max {MAX_CATEGORIES}. \
|
||||||
|
Mark fields as Measure (c) or toggle them off (Space)."
|
||||||
|
)
|
||||||
|
})
|
||||||
|
}
|
||||||
|
|
||||||
fn has_time_categories(&self) -> bool {
|
fn has_time_categories(&self) -> bool {
|
||||||
self.pipeline
|
self.pipeline
|
||||||
.proposals
|
.proposals
|
||||||
@@ -856,6 +900,73 @@ mod tests {
|
|||||||
assert!(!w.pipeline.records.is_empty());
|
assert!(!w.pipeline.records.is_empty());
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// ── Category limit pre-check ────────────────────────────────────────
|
||||||
|
|
||||||
|
/// Build records whose fields `c00..cNN` are all low-cardinality strings,
|
||||||
|
/// so the analyzer proposes every field as an accepted Category.
|
||||||
|
fn category_records(n_fields: usize) -> serde_json::Value {
|
||||||
|
let recs: Vec<serde_json::Value> = (0..3)
|
||||||
|
.map(|r| {
|
||||||
|
let mut m = serde_json::Map::new();
|
||||||
|
for i in 0..n_fields {
|
||||||
|
m.insert(format!("c{i:02}"), json!(format!("v{r}")));
|
||||||
|
}
|
||||||
|
serde_json::Value::Object(m)
|
||||||
|
})
|
||||||
|
.collect();
|
||||||
|
serde_json::Value::Array(recs)
|
||||||
|
}
|
||||||
|
|
||||||
|
/// BUG (improvise-mzv): with more than MAX_CATEGORIES (12) accepted
|
||||||
|
/// Category proposals, the wizard let the user advance through every
|
||||||
|
/// remaining step; build_model() only failed at the final confirm.
|
||||||
|
/// Advancing past ReviewProposals must be blocked with a message telling
|
||||||
|
/// the user to mark fields as Measure or toggle them off.
|
||||||
|
#[test]
|
||||||
|
fn wizard_blocks_advance_when_over_category_limit() {
|
||||||
|
let mut w = ImportWizard::new(category_records(13));
|
||||||
|
assert_eq!(w.step, WizardStep::ReviewProposals);
|
||||||
|
assert_eq!(
|
||||||
|
w.pipeline
|
||||||
|
.proposals
|
||||||
|
.iter()
|
||||||
|
.filter(|p| p.accepted && p.kind == FieldKind::Category)
|
||||||
|
.count(),
|
||||||
|
13,
|
||||||
|
"sanity: all 13 fields proposed as accepted categories"
|
||||||
|
);
|
||||||
|
|
||||||
|
w.advance();
|
||||||
|
assert_eq!(
|
||||||
|
w.step,
|
||||||
|
WizardStep::ReviewProposals,
|
||||||
|
"advance must be blocked while over the category limit"
|
||||||
|
);
|
||||||
|
let msg = w.message.as_deref().expect("a message explaining the block");
|
||||||
|
assert!(msg.contains("Measure"), "message should tell the user how to fix it: {msg}");
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn wizard_allows_advance_at_exactly_category_limit() {
|
||||||
|
let mut w = ImportWizard::new(category_records(12));
|
||||||
|
assert_eq!(w.step, WizardStep::ReviewProposals);
|
||||||
|
w.advance();
|
||||||
|
assert_ne!(w.step, WizardStep::ReviewProposals);
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn wizard_advance_unblocks_after_marking_field_as_measure() {
|
||||||
|
let mut w = ImportWizard::new(category_records(13));
|
||||||
|
w.advance();
|
||||||
|
assert_eq!(w.step, WizardStep::ReviewProposals);
|
||||||
|
|
||||||
|
// Toggle one field off; the wizard should now let the user through.
|
||||||
|
w.cursor = 0;
|
||||||
|
w.toggle_proposal();
|
||||||
|
w.advance();
|
||||||
|
assert_ne!(w.step, WizardStep::ReviewProposals);
|
||||||
|
}
|
||||||
|
|
||||||
// ── Formula editing in wizard ───────────────────────────────────────
|
// ── Formula editing in wizard ───────────────────────────────────────
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
|
|||||||
@@ -323,16 +323,18 @@ impl<'a> Widget for ImportWizardWidget<'a> {
|
|||||||
"Enter to import, Esc to cancel",
|
"Enter to import, Esc to cancel",
|
||||||
Style::default().fg(Color::DarkGray),
|
Style::default().fg(Color::DarkGray),
|
||||||
);
|
);
|
||||||
|
|
||||||
if let Some(msg) = &self.wizard.message {
|
|
||||||
let msg_y = inner.y + inner.height - 1;
|
|
||||||
buf.set_string(x, msg_y, truncate(msg, w), Style::default().fg(Color::Red));
|
|
||||||
}
|
|
||||||
}
|
}
|
||||||
WizardStep::Done => {
|
WizardStep::Done => {
|
||||||
buf.set_string(x, y, "Import complete!", Style::default().fg(Color::Green));
|
buf.set_string(x, y, "Import complete!", Style::default().fg(Color::Green));
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Wizard message (errors, blocked-step explanations) takes the bottom
|
||||||
|
// line on every step, overriding the key hint while present.
|
||||||
|
if let Some(msg) = &self.wizard.message {
|
||||||
|
let msg_y = inner.y + inner.height - 1;
|
||||||
|
buf.set_string(x, msg_y, truncate(msg, w), Style::default().fg(Color::Red));
|
||||||
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user