mirror of
https://github.com/storytold/lightcraft.git
synced 2026-10-27 19:47:35 -06:00
LRC-LIB-SMARTCOLL: a rule that stopped checking doesn't block other changes
From final review: library.filter and album.setRules (without replace)
merged a change onto the current rules and re-checked all of them.
Once a rule stopped checking later (the album it tests was deleted, a
custom label renamed), every later change was refused: the search box,
keyword clicks and the filter bar silently stopped working, since the
UI doesn't show those errors, and partial edits of the album were
refused.
Only rules a change sets ("ruleSet" present) are checked now. Rules
that went stale still show in the dialog and the sidebar's ⚠ until
they're fixed. A regression test deletes the album a filter rule tests,
then changes the rating, edits the album, and still gets refused for a
bad new rule.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5.5
parent
a51224e6d4
commit
0a2ece4491
@@ -330,11 +330,14 @@ pub fn specs() -> Vec<CommandSpec> {
|
||||
if f.library_folder.is_some() && s.source == LibrarySource::LibraryFolder {
|
||||
return Err(bad("library.filter", "a folder is already shown (library.source): show another source first"));
|
||||
}
|
||||
// checked like a smart album's rules: an unknown field or a value that can't match is an error, not an empty grid
|
||||
// checked like a smart album's rules when this call sets them: an unknown field or a value
|
||||
// that can't match is an error, not an empty grid. Rules that stopped checking since
|
||||
// (their album was deleted) don't block other filter changes.
|
||||
if let Some(rules) = f.rule_set.as_mut() {
|
||||
rules.upgrade();
|
||||
}
|
||||
if let Some(problems) = f.rule_set.as_ref().map(|rs| rs.check(&s.catalog)).filter(|p| !p.is_empty()) {
|
||||
let sets_rules = p.get("ruleSet").is_some_and(|r| !r.is_null());
|
||||
if let Some(problems) = f.rule_set.as_ref().filter(|_| sets_rules).map(|rs| rs.check(&s.catalog)).filter(|p| !p.is_empty()) {
|
||||
return Err(bad("library.filter", problems.iter().map(ToString::to_string).collect::<Vec<_>>().join("; ")));
|
||||
}
|
||||
s.filter = f;
|
||||
@@ -1315,7 +1318,7 @@ fn merge_filter(base: &lightcraft_catalog::Filter, patch: &Value, c: &str) -> Re
|
||||
Ok(f)
|
||||
}
|
||||
|
||||
/// `patch` merged onto `base`, refused when its rule set has problems (`RuleSet::check_for`;
|
||||
/// `patch` merged onto `base`, refused when the rule set it sets has problems (`RuleSet::check_for`;
|
||||
/// `owner` is the smart album the rules are for, so testing an album that leads back to it is a
|
||||
/// problem too).
|
||||
fn merge_rules(
|
||||
@@ -1326,7 +1329,10 @@ fn merge_rules(
|
||||
owner: Option<AlbumId>,
|
||||
) -> Result<lightcraft_catalog::Filter> {
|
||||
let f = merge_filter(base, patch, c)?;
|
||||
let problems = f.rule_set.as_ref().map(|r| r.check_for(cat, owner)).unwrap_or_default();
|
||||
// only rules this change sets are checked: one that stopped checking since (its album was
|
||||
// deleted) doesn't block editing the album's other settings
|
||||
let sets_rules = patch.get("ruleSet").is_some_and(|r| !r.is_null());
|
||||
let problems = f.rule_set.as_ref().filter(|_| sets_rules).map(|r| r.check_for(cat, owner)).unwrap_or_default();
|
||||
if !problems.is_empty() {
|
||||
return Err(bad(c, problems.iter().map(ToString::to_string).collect::<Vec<_>>().join("; ")));
|
||||
}
|
||||
|
||||
@@ -637,3 +637,26 @@ fn rules_from_the_view_cant_loop() {
|
||||
assert_eq!(s.catalog.album(lightcraft_catalog::AlbumId(b)).unwrap().smart, before, "B unchanged");
|
||||
assert!(s.catalog.smart_album_problems(lightcraft_catalog::AlbumId(b)).is_empty());
|
||||
}
|
||||
|
||||
/// A rule that stops checking later (the album it tests is deleted) doesn't lock up what doesn't
|
||||
/// touch it: the filter bar keeps working, and an album's other settings can still be edited. A
|
||||
/// change to the rules themselves is still checked.
|
||||
#[test]
|
||||
fn a_stale_rule_doesnt_block_other_changes() {
|
||||
let mut s = crate::Session::with_demo();
|
||||
let trip = s.execute("album.create", &serde_json::json!({"name": "Trip", "addSelected": false})).unwrap()["id"].as_u64().unwrap();
|
||||
let rules = serde_json::json!({"rules": [{"field": "album", "op": "isNot", "value": trip}]});
|
||||
s.execute("library.filter", &serde_json::json!({"ruleSet": rules})).unwrap();
|
||||
let smart =
|
||||
s.execute("album.createSmart", &serde_json::json!({"name": "Not Trip", "rules": {"ruleSet": rules}})).unwrap()["id"].as_u64().unwrap();
|
||||
s.execute("album.delete", &serde_json::json!({"id": trip})).unwrap();
|
||||
// the filter bar: a rating still applies on top of the stale rule
|
||||
s.execute("library.filter", &serde_json::json!({"rating": 3})).unwrap();
|
||||
assert_eq!(s.filter.rating, 3);
|
||||
// the album: a partial edit that leaves its rules alone
|
||||
s.execute("album.setRules", &serde_json::json!({"id": smart, "rules": {"rating": 2}})).unwrap();
|
||||
// changing the rules is still checked
|
||||
let bad = serde_json::json!({"ruleSet": {"rules": [{"field": "rating", "op": "gte", "value": 9}]}});
|
||||
assert!(s.execute("library.filter", &bad).is_err());
|
||||
assert!(s.execute("album.setRules", &serde_json::json!({"id": smart, "rules": bad})).is_err());
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user