feat(design-systems): resolve the chain, and keep the argument not the verdict
CI & Build / Python lint (push) Successful in 3s
CI & Build / Plugin hooks (push) Successful in 8s
CI & Build / integration (push) Successful in 15s
CI & Build / TypeScript typecheck (push) Successful in 33s
CI & Build / Python tests (push) Successful in 42s
CI & Build / Build & push image (push) Successful in 29s
CI & Build / Python lint (push) Successful in 3s
CI & Build / Plugin hooks (push) Successful in 8s
CI & Build / integration (push) Successful in 15s
CI & Build / TypeScript typecheck (push) Successful in 33s
CI & Build / Python tests (push) Successful in 42s
CI & Build / Build & push image (push) Successful in 29s
Milestone #254 step 2 (#2287). `resolve_tokens` flattens a system's inheritance chain into its effective token set — walk to the root, deepest wins by token name. Pure and duck-typed, so a test states a whole hierarchy in literals and the service hands the same function ORM rows. **Provenance is stored as the contest, not the winner.** A ResolvedToken carries every system that offered a value, per mode, deepest first — `[0]` won and `[1:]` are what it shadowed. "Which system supplied this?" and "what did it override?" are then two reads of one list and cannot disagree, where a winner plus a separate provenance field would be two things to keep in step. **Merging is per (name, MODE), and that is the storage decision paying off.** A system that deepens one accent for light backgrounds while leaving dark alone owns `base` and still inherits `dark`. A token-level "overridden here" flag would have to lie about one of them, and the two-column shape could not have represented it at all. Metadata cascades separately by the same deepest-wins rule, with one exception: `order_index` treats 0 as UNSTATED rather than "first", because 0 is the column default. Reading it as a real value would let a colour-only override drag its token to the top of its group — a visible reshuffle in return for a change that touched nothing structural. One fix to step 1 while wiring this up: `_parent_map` is now scoped to the SYSTEM'S OWNER rather than the caller. A caller reading through a shared project owns no link in the chain, so the caller-scoped version would have handed them an empty forest and truncated the cascade to a single system — a page rendering with plausible wrong values and no error anywhere. The ACL already grants read along the whole chain; this is the loading side keeping that promise, and it now has a test naming the shared-project case. `BASE_MODE` moves from the model to the cascade module, where it belongs: it is a resolution rule, not a storage fact, and design_cascade.py deliberately imports nothing so both access.py and the service can depend on it.
This commit is contained in:
@@ -4,7 +4,9 @@ Pure functions over a `{id: parent_id}` literal, so a whole hierarchy is one lin
|
||||
of setup and no database is involved. That is the reason the cascade lives in its
|
||||
own import-free module — see services/design_cascade.py.
|
||||
"""
|
||||
from scribe.services.design_cascade import ancestry, would_cycle
|
||||
from types import SimpleNamespace
|
||||
|
||||
from scribe.services.design_cascade import ancestry, resolve_tokens, would_cycle
|
||||
|
||||
|
||||
# --- ancestry ---------------------------------------------------------------
|
||||
@@ -92,3 +94,219 @@ def test_the_guard_survives_a_hierarchy_that_is_already_corrupt():
|
||||
parents = {1: 2, 2: 1, 3: None}
|
||||
assert would_cycle(3, 1, parents) is False
|
||||
assert would_cycle(1, 2, parents) is True
|
||||
|
||||
|
||||
# --- resolution -------------------------------------------------------------
|
||||
#
|
||||
# The cascade proper (milestone #254 step 2). Hierarchies are stated as literals
|
||||
# because resolve_tokens is pure and duck-typed — which is the whole reason it
|
||||
# lives here rather than inside the service.
|
||||
|
||||
def _token(name, value_by_mode, group_name=None, purpose=None, order_index=0):
|
||||
return SimpleNamespace(
|
||||
name=name, value_by_mode=value_by_mode, group_name=group_name,
|
||||
purpose=purpose, order_index=order_index,
|
||||
)
|
||||
|
||||
|
||||
# A family (1) and an app inheriting from it (2) — the shape the model exists for.
|
||||
FAMILY, APP = 1, 2
|
||||
PARENTS = {FAMILY: None, APP: FAMILY}
|
||||
|
||||
|
||||
def _by_name(tokens):
|
||||
return {t.name: t for t in tokens}
|
||||
|
||||
|
||||
def test_a_system_with_no_tokens_of_its_own_inherits_the_whole_family_set():
|
||||
"""The correct answer for an app that has not departed from the family yet —
|
||||
and the state every app system starts in."""
|
||||
resolved = resolve_tokens(
|
||||
APP, PARENTS,
|
||||
{FAMILY: [_token("--fs-obsidian", {"base": "#14171a"})], APP: []},
|
||||
)
|
||||
assert [t.name for t in resolved] == ["--fs-obsidian"]
|
||||
assert resolved[0].value_by_mode == {"base": "#14171a"}
|
||||
assert resolved[0].origin_by_mode == {"base": FAMILY}
|
||||
|
||||
|
||||
def test_the_deepest_system_wins_and_says_what_it_overrode():
|
||||
"""Provenance is the point of the whole model: the effective set is just a
|
||||
list without it, and "where does this app depart from the family?" becomes a
|
||||
diff someone has to compute."""
|
||||
resolved = _by_name(resolve_tokens(
|
||||
APP, PARENTS,
|
||||
{
|
||||
FAMILY: [_token("--fs-accent", {"base": "#6b2118"})],
|
||||
APP: [_token("--fs-accent", {"base": "#5b4a8a"})],
|
||||
},
|
||||
))
|
||||
accent = resolved["--fs-accent"]
|
||||
assert accent.value_by_mode == {"base": "#5b4a8a"}
|
||||
assert accent.origin_by_mode == {"base": APP}
|
||||
# The shadowed ancestor is still there, behind the winner.
|
||||
assert [c.system_id for c in accent.contributions["base"]] == [APP, FAMILY]
|
||||
assert accent.contributions["base"][1].value == "#6b2118"
|
||||
|
||||
|
||||
def test_overriding_one_mode_leaves_the_others_inherited():
|
||||
"""LOAD-BEARING, and the argument that decided the storage shape. An accent
|
||||
deepened for light backgrounds while dark is left alone must own `base` and
|
||||
still inherit `dark` — which is why merging is per (name, MODE) and why the
|
||||
value column is a map rather than a pair of columns."""
|
||||
resolved = _by_name(resolve_tokens(
|
||||
APP, PARENTS,
|
||||
{
|
||||
FAMILY: [_token("--fs-accent", {"base": "#34a877", "dark": "#34a877"})],
|
||||
APP: [_token("--fs-accent", {"base": "#15803d"})],
|
||||
},
|
||||
))
|
||||
accent = resolved["--fs-accent"]
|
||||
assert accent.value_by_mode == {"base": "#15803d", "dark": "#34a877"}
|
||||
assert accent.origin_by_mode == {"base": APP, "dark": FAMILY}
|
||||
|
||||
|
||||
def test_a_token_only_the_app_defines_is_not_an_override():
|
||||
"""Introducing a token and overriding one are different acts, and a UI that
|
||||
labelled both "overridden here" would misdescribe the first."""
|
||||
resolved = _by_name(resolve_tokens(
|
||||
APP, PARENTS,
|
||||
{FAMILY: [], APP: [_token("--fs-editor-caret", {"base": "#5b4a8a"})]},
|
||||
))
|
||||
caret = resolved["--fs-editor-caret"]
|
||||
assert caret.origin_by_mode == {"base": APP}
|
||||
assert caret.is_overridden_in(APP) is False
|
||||
|
||||
|
||||
def test_is_overridden_in_is_true_only_for_the_system_that_shadowed():
|
||||
resolved = _by_name(resolve_tokens(
|
||||
APP, PARENTS,
|
||||
{
|
||||
FAMILY: [_token("--fs-accent", {"base": "#6b2118"})],
|
||||
APP: [_token("--fs-accent", {"base": "#5b4a8a"})],
|
||||
},
|
||||
))
|
||||
accent = resolved["--fs-accent"]
|
||||
assert accent.is_overridden_in(APP) is True
|
||||
assert accent.is_overridden_in(FAMILY) is False
|
||||
|
||||
|
||||
def test_three_levels_stack_nearest_first():
|
||||
"""A chain deeper than family->app, to prove the walk isn't a single hop."""
|
||||
parents = {1: None, 2: 1, 3: 2}
|
||||
resolved = _by_name(resolve_tokens(
|
||||
3, parents,
|
||||
{
|
||||
1: [_token("--fs-bg", {"base": "a"})],
|
||||
2: [_token("--fs-bg", {"base": "b"})],
|
||||
3: [_token("--fs-bg", {"base": "c"})],
|
||||
},
|
||||
))
|
||||
bg = resolved["--fs-bg"]
|
||||
assert bg.value_by_mode == {"base": "c"}
|
||||
assert [c.value for c in bg.contributions["base"]] == ["c", "b", "a"]
|
||||
|
||||
|
||||
def test_resolving_the_family_itself_ignores_its_children():
|
||||
"""Inheritance runs one way. A family system resolved on its own must not
|
||||
pick up an app's overrides — otherwise every app would restyle the house."""
|
||||
resolved = _by_name(resolve_tokens(
|
||||
FAMILY, PARENTS,
|
||||
{
|
||||
FAMILY: [_token("--fs-accent", {"base": "#6b2118"})],
|
||||
APP: [_token("--fs-accent", {"base": "#5b4a8a"})],
|
||||
},
|
||||
))
|
||||
assert resolved["--fs-accent"].value_by_mode == {"base": "#6b2118"}
|
||||
|
||||
|
||||
def test_value_for_falls_back_to_the_base_mode():
|
||||
"""A token that isn't mode-dependent carries only `base`, and asking it for
|
||||
"dark" must yield that rather than nothing — the read rule the storage shape
|
||||
implies."""
|
||||
resolved = _by_name(resolve_tokens(
|
||||
FAMILY, PARENTS, {FAMILY: [_token("--fs-radius-md", {"base": "8px"})]},
|
||||
))
|
||||
radius = resolved["--fs-radius-md"]
|
||||
assert radius.value_for("dark") == "8px"
|
||||
assert radius.value_for("base") == "8px"
|
||||
|
||||
|
||||
def test_value_for_prefers_an_explicit_mode_over_the_fallback():
|
||||
resolved = _by_name(resolve_tokens(
|
||||
FAMILY, PARENTS,
|
||||
{FAMILY: [_token("--fs-bg", {"base": "#f7f5ef", "dark": "#14171a"})]},
|
||||
))
|
||||
assert resolved["--fs-bg"].value_for("dark") == "#14171a"
|
||||
|
||||
|
||||
def test_metadata_is_inherited_when_the_override_leaves_it_blank():
|
||||
"""A child overriding a colour routinely says nothing about what the token is
|
||||
FOR. Inheriting the family's description beats blanking it — the override was
|
||||
about the value, not the meaning."""
|
||||
resolved = _by_name(resolve_tokens(
|
||||
APP, PARENTS,
|
||||
{
|
||||
FAMILY: [_token(
|
||||
"--fs-obsidian", {"base": "#14171a"},
|
||||
group_name="surface", purpose="page bg, deepest surface",
|
||||
)],
|
||||
APP: [_token("--fs-obsidian", {"base": "#101317"})],
|
||||
},
|
||||
))
|
||||
obsidian = resolved["--fs-obsidian"]
|
||||
assert obsidian.group_name == "surface"
|
||||
assert obsidian.purpose == "page bg, deepest surface"
|
||||
assert obsidian.value_by_mode == {"base": "#101317"} # the value still won
|
||||
|
||||
|
||||
def test_an_override_that_states_metadata_wins_it_too():
|
||||
resolved = _by_name(resolve_tokens(
|
||||
APP, PARENTS,
|
||||
{
|
||||
FAMILY: [_token("--fs-x", {"base": "a"}, purpose="family says")],
|
||||
APP: [_token("--fs-x", {"base": "b"}, purpose="app says")],
|
||||
},
|
||||
))
|
||||
assert resolved["--fs-x"].purpose == "app says"
|
||||
|
||||
|
||||
def test_an_override_at_default_order_keeps_the_familys_position():
|
||||
"""order_index 0 is the column DEFAULT, so it reads as unstated. Treating it
|
||||
as "first" would let a colour-only override drag its token to the top of the
|
||||
group — a visible reshuffle nobody asked for."""
|
||||
resolved = _by_name(resolve_tokens(
|
||||
APP, PARENTS,
|
||||
{
|
||||
FAMILY: [_token("--fs-x", {"base": "a"}, order_index=7)],
|
||||
APP: [_token("--fs-x", {"base": "b"})],
|
||||
},
|
||||
))
|
||||
assert resolved["--fs-x"].order_index == 7
|
||||
|
||||
|
||||
def test_the_effective_set_is_ordered_by_group_then_position_with_ungrouped_last():
|
||||
resolved = resolve_tokens(
|
||||
FAMILY, PARENTS,
|
||||
{FAMILY: [
|
||||
_token("--fs-z", {"base": "1"}), # ungrouped
|
||||
_token("--fs-b", {"base": "2"}, group_name="text", order_index=1),
|
||||
_token("--fs-a", {"base": "3"}, group_name="surface", order_index=2),
|
||||
_token("--fs-c", {"base": "4"}, group_name="surface", order_index=1),
|
||||
]},
|
||||
)
|
||||
assert [t.name for t in resolved] == ["--fs-c", "--fs-a", "--fs-b", "--fs-z"]
|
||||
|
||||
|
||||
def test_resolution_terminates_on_a_corrupt_hierarchy():
|
||||
"""Resolution inherits ancestry's visited-set. A loop reaching this function
|
||||
must produce a truncated set, not a hung request — the defensive half of the
|
||||
cycle guard, exercised end to end."""
|
||||
parents = {1: 2, 2: 1}
|
||||
resolved = _by_name(resolve_tokens(
|
||||
1, parents,
|
||||
{1: [_token("--fs-a", {"base": "one"})], 2: [_token("--fs-b", {"base": "two"})]},
|
||||
))
|
||||
assert set(resolved) == {"--fs-a", "--fs-b"}
|
||||
# Each system contributes exactly once, not endlessly.
|
||||
assert len(resolved["--fs-a"].contributions["base"]) == 1
|
||||
|
||||
@@ -192,3 +192,47 @@ async def test_clearing_a_projects_design_system_skips_the_system_acl():
|
||||
|
||||
assert project.design_system_id is None
|
||||
acc.can_read_design_system.assert_not_awaited()
|
||||
|
||||
|
||||
# --- resolution (step 2) ----------------------------------------------------
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_resolve_denied_returns_none_not_an_empty_set():
|
||||
"""None and [] mean different things here: "you may not see this" versus
|
||||
"this chain genuinely holds no tokens". A caller that got [] for a denial
|
||||
would render an empty design system as though it were real."""
|
||||
with patch("scribe.services.design_systems.access") as acc:
|
||||
acc.can_read_design_system = AsyncMock(return_value=False)
|
||||
from scribe.services.design_systems import resolve_design_system
|
||||
assert await resolve_design_system(user_id=1, design_system_id=3) is None
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_resolve_scopes_the_hierarchy_to_the_systems_OWNER_not_the_caller():
|
||||
"""LOAD-BEARING for shared projects. A caller reading through a project
|
||||
shared with them owns no link in the chain, so a caller-scoped parent map
|
||||
returns an empty forest and the cascade truncates to one system — a page
|
||||
that renders with plausible wrong values and no error anywhere.
|
||||
|
||||
The ACL already grants read on the whole chain (reachability propagates
|
||||
upward); this is the loading side keeping that promise.
|
||||
"""
|
||||
owner, caller = 42, 7
|
||||
system = MagicMock(deleted_at=None, owner_user_id=owner)
|
||||
mock_session = _make_mock_session()
|
||||
mock_session.get = AsyncMock(return_value=system)
|
||||
mock_session.execute = AsyncMock(
|
||||
return_value=MagicMock(scalars=MagicMock(return_value=MagicMock(all=lambda: [])))
|
||||
)
|
||||
parent_map = AsyncMock(return_value={3: None})
|
||||
|
||||
with patch("scribe.services.design_systems.async_session") as mock_cls, \
|
||||
patch("scribe.services.design_systems._parent_map", parent_map), \
|
||||
patch("scribe.services.design_systems.access") as acc:
|
||||
acc.can_read_design_system = AsyncMock(return_value=True)
|
||||
mock_cls.return_value = mock_session
|
||||
from scribe.services.design_systems import resolve_design_system
|
||||
result = await resolve_design_system(user_id=caller, design_system_id=3)
|
||||
|
||||
assert result == [] # empty chain, not None
|
||||
assert parent_map.await_args.args[1] == owner # NOT `caller`
|
||||
|
||||
Reference in New Issue
Block a user