feat(design-systems): the model, the parent chain, and the guard on it
CI & Build / Python lint (push) Successful in 3s
CI & Build / Plugin hooks (push) Successful in 7s
CI & Build / integration (push) Successful in 18s
CI & Build / TypeScript typecheck (push) Successful in 20s
CI & Build / Python tests (push) Successful in 42s
CI & Build / Build & push image (push) Successful in 27s
CI & Build / Python lint (push) Successful in 3s
CI & Build / Plugin hooks (push) Successful in 7s
CI & Build / integration (push) Successful in 18s
CI & Build / TypeScript typecheck (push) Successful in 20s
CI & Build / Python tests (push) Successful in 42s
CI & Build / Build & push image (push) Successful in 27s
Milestone #254 step 1 (#2286). A design system becomes a record Scribe holds rather than prose in a rulebook: a named set of tokens with an OPTIONAL parent, so a family system carries the house style and an app system carries only what it changes. Answering "what does this app alter?" is then `list its tokens` — nothing to compute. `parent_id` is the whole model. It replaces both an `always_on` flag (a family system is one with no parent) and a subscription join table (a project points at ONE system; the chain supplies the rest) — less schema than the rulebook shape it mirrors. Two decisions the task left open, settled here: - **Token values are JSONB keyed by mode**, not `value_light`/`value_dark` columns. The deciding argument was not flexibility, it was ambiguity: in a child system an unset mode means "inherit", in a root it means "not mode-dependent", and as columns both are NULL and the resolver cannot tell them apart. As a map, resolution is `{**parent, **child}` at every level with no special case for roots. Against it: queryability — but nothing filters tokens by value in SQL, so that buys a query no caller makes. - **`group_name` is free text, no CHECK enum.** Groupings are each design system's own vocabulary; a whitelist would bake one install's kit into the schema. No CHECK is introduced anywhere, so rule #36 does not fire. The cascade lives in `services/design_cascade.py` as pure functions over a `{id: parent_id}` map, importing nothing — which is what lets both the service and `access.py` use it without a cycle, and lets a test state a whole hierarchy in one literal. Cycles are refused on WRITE by walking up from the proposed parent (the cheap direction), and survived on READ by a visited-set, because a loop from a direct DB edit must truncate rather than hang. ACL (rule #78) is deliberately asymmetric: owning a system grants write, reaching one through a project you can see grants READ ONLY. An editor on a shared project must not be able to rewrite the family system every other project in that family resolves through. Also renames `services/design_system.py` -> `design_rulebook_import.py`. It is the #251 prose extractor, whose role is already scheduled to become a one-shot importer (#2288), and leaving it one character away from the new `design_systems.py` was a trap for every later session. Rule #115 throughout: nothing seeds a system or implies a default. An install with zero design systems is ordinary, not degraded.
This commit is contained in:
@@ -0,0 +1,94 @@
|
||||
"""The parent chain: walking it, and refusing to close it (milestone #254 step 1).
|
||||
|
||||
Pure functions over a `{id: parent_id}` literal, so a whole hierarchy is one line
|
||||
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
|
||||
|
||||
|
||||
# --- ancestry ---------------------------------------------------------------
|
||||
|
||||
def test_ancestry_returns_the_chain_deepest_first():
|
||||
"""Deepest first because that is the order resolution consumes it in: the
|
||||
system being resolved wins over its parent, which wins over the root."""
|
||||
parents = {1: None, 2: 1, 3: 2}
|
||||
assert ancestry(3, parents) == [3, 2, 1]
|
||||
|
||||
|
||||
def test_a_root_is_its_own_whole_chain():
|
||||
"""A family system has no parent, and that is an ordinary state — not an
|
||||
incomplete one. It must resolve to exactly itself."""
|
||||
assert ancestry(1, {1: None}) == [1]
|
||||
|
||||
|
||||
def test_a_missing_parent_truncates_rather_than_raising():
|
||||
"""A parent that was soft-deleted (or filtered out of the caller's scope) is
|
||||
a shorter chain, not a failed request. The alternative — raising — would make
|
||||
one deleted system break every descendant's rendering."""
|
||||
assert ancestry(3, {3: 2}) == [3, 2]
|
||||
|
||||
|
||||
def test_a_system_absent_from_the_map_still_yields_itself():
|
||||
assert ancestry(9, {}) == [9]
|
||||
|
||||
|
||||
def test_ancestry_terminates_on_a_cycle_instead_of_hanging():
|
||||
"""LOAD-BEARING, and the reason a visited-set exists even though writes are
|
||||
guarded. A loop introduced by a direct DB edit or a future bug must degrade
|
||||
to a truncated chain: truncation shows up in the result, a hang shows up as
|
||||
an outage. Every id appears exactly once."""
|
||||
parents = {1: 3, 2: 1, 3: 2}
|
||||
chain = ancestry(1, parents)
|
||||
assert chain == [1, 3, 2]
|
||||
assert len(chain) == len(set(chain))
|
||||
|
||||
|
||||
def test_ancestry_terminates_on_a_self_parent():
|
||||
assert ancestry(1, {1: 1}) == [1]
|
||||
|
||||
|
||||
# --- would_cycle ------------------------------------------------------------
|
||||
|
||||
def test_clearing_the_parent_never_cycles():
|
||||
"""None means "make this a root", which is always safe."""
|
||||
assert would_cycle(2, None, {1: None, 2: 1}) is False
|
||||
|
||||
|
||||
def test_a_system_cannot_be_its_own_parent():
|
||||
assert would_cycle(1, 1, {1: None}) is True
|
||||
|
||||
|
||||
def test_an_ordinary_reparent_is_allowed():
|
||||
"""family <- app is the shape the whole model exists for; it must not trip
|
||||
the guard."""
|
||||
assert would_cycle(2, 1, {1: None, 2: None}) is False
|
||||
|
||||
|
||||
def test_a_direct_swap_is_refused():
|
||||
"""A -> B, then B -> A. The two-system case, and the one a UI produces first
|
||||
because both systems are on screen together."""
|
||||
parents = {1: None, 2: 1}
|
||||
assert would_cycle(1, 2, parents) is True
|
||||
|
||||
|
||||
def test_an_indirect_loop_is_refused():
|
||||
"""Three deep: root <- mid <- leaf, then root's parent set to leaf. Catching
|
||||
this is what makes the check a chain walk rather than a parent comparison."""
|
||||
parents = {1: None, 2: 1, 3: 2}
|
||||
assert would_cycle(1, 3, parents) is True
|
||||
|
||||
|
||||
def test_reparenting_onto_a_sibling_subtree_is_allowed():
|
||||
"""Two branches off one root. Moving one under the other is legitimate — the
|
||||
guard must refuse loops, not reorganisation."""
|
||||
parents = {1: None, 2: 1, 3: 1}
|
||||
assert would_cycle(3, 2, parents) is False
|
||||
|
||||
|
||||
def test_the_guard_survives_a_hierarchy_that_is_already_corrupt():
|
||||
"""If a cycle somehow already exists, the guard still has to answer rather
|
||||
than spin — the write path is exactly where such a hierarchy gets repaired."""
|
||||
parents = {1: 2, 2: 1, 3: None}
|
||||
assert would_cycle(3, 1, parents) is False
|
||||
assert would_cycle(1, 2, parents) is True
|
||||
@@ -11,7 +11,7 @@ be testing the instance, not the parser.
|
||||
"""
|
||||
from types import SimpleNamespace
|
||||
|
||||
from scribe.services.design_system import (
|
||||
from scribe.services.design_rulebook_import import (
|
||||
expand_token_shorthand,
|
||||
extract_expectations,
|
||||
normalize_hex,
|
||||
@@ -132,3 +132,40 @@ def test_clauses_are_pure_builders(clause_fn):
|
||||
import inspect
|
||||
assert not inspect.iscoroutinefunction(clause_fn)
|
||||
assert _sql(clause_fn(7)) # builds without touching a database
|
||||
|
||||
|
||||
# --- design systems ----------------------------------------------------------
|
||||
#
|
||||
# A design system is reachable two ways: you own it, or you can see a project
|
||||
# that inherits from it. The gap between what those two grant is the invariant
|
||||
# worth guarding — see get_design_system_permission.
|
||||
|
||||
@pytest.mark.asyncio
|
||||
@pytest.mark.parametrize(
|
||||
"permission, readable, writable",
|
||||
[
|
||||
("owner", True, True),
|
||||
# Project-derived. Being an EDITOR on a shared project must not confer
|
||||
# the right to rewrite the family system that project inherits from —
|
||||
# that would let one project's collaborator restyle every other project
|
||||
# in the family.
|
||||
("viewer", True, False),
|
||||
(None, False, False),
|
||||
],
|
||||
)
|
||||
async def test_reaching_a_design_system_via_a_project_reads_but_never_writes(
|
||||
permission, readable, writable
|
||||
):
|
||||
from unittest.mock import AsyncMock, patch
|
||||
|
||||
from scribe.services.access import (
|
||||
can_read_design_system,
|
||||
can_write_design_system,
|
||||
)
|
||||
|
||||
with patch(
|
||||
"scribe.services.access.get_design_system_permission",
|
||||
AsyncMock(return_value=permission),
|
||||
):
|
||||
assert await can_read_design_system(1, 3) is readable
|
||||
assert await can_write_design_system(1, 3) is writable
|
||||
|
||||
@@ -0,0 +1,194 @@
|
||||
"""ACL gating and field handling for services/design_systems.py (unit, mocked).
|
||||
|
||||
Mirrors tests/test_services_systems.py — same mocked-session shape, because the
|
||||
question is the same one: does the service refuse before it touches a row?
|
||||
"""
|
||||
from unittest.mock import AsyncMock, MagicMock, patch
|
||||
|
||||
import pytest
|
||||
|
||||
from scribe.services.design_systems import DesignSystemCycle
|
||||
|
||||
|
||||
def _make_mock_session():
|
||||
s = AsyncMock()
|
||||
s.__aenter__ = AsyncMock(return_value=s)
|
||||
s.__aexit__ = AsyncMock(return_value=False)
|
||||
s.add = MagicMock()
|
||||
s.commit = AsyncMock()
|
||||
s.refresh = AsyncMock()
|
||||
return s
|
||||
|
||||
|
||||
# --- creating ---------------------------------------------------------------
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_create_with_a_parent_is_denied_without_write_on_that_parent():
|
||||
"""Parenting to someone else's system would let their delete or re-parent
|
||||
silently restyle your app, so the chain may only be built from systems you
|
||||
can write — which, per the ACL, means ones you own."""
|
||||
with patch("scribe.services.design_systems.access") as acc:
|
||||
acc.can_write_design_system = AsyncMock(return_value=False)
|
||||
from scribe.services.design_systems import create_design_system
|
||||
result = await create_design_system(user_id=1, title="Scribe", parent_id=7)
|
||||
assert result is None
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_create_without_a_parent_never_consults_the_parent_acl():
|
||||
"""A family system has no parent, and creating one must not be gated on a
|
||||
permission check for a system that does not exist."""
|
||||
mock_session = _make_mock_session()
|
||||
captured = {}
|
||||
mock_session.add = MagicMock(
|
||||
side_effect=lambda obj: captured.update(
|
||||
title=obj.title, parent_id=obj.parent_id
|
||||
)
|
||||
)
|
||||
with patch("scribe.services.design_systems.async_session") as mock_cls, \
|
||||
patch("scribe.services.design_systems.access") as acc:
|
||||
acc.can_write_design_system = AsyncMock(return_value=False)
|
||||
mock_cls.return_value = mock_session
|
||||
from scribe.services.design_systems import create_design_system
|
||||
await create_design_system(user_id=1, title=" FabledSword ")
|
||||
assert captured["title"] == "FabledSword" # stripped
|
||||
assert captured["parent_id"] is None
|
||||
acc.can_write_design_system.assert_not_awaited()
|
||||
|
||||
|
||||
# --- the cycle guard, through the service -----------------------------------
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_reparenting_into_a_loop_raises_rather_than_returning_none():
|
||||
"""None already means "not found, or not yours". A caller that conflated the
|
||||
two would report "no such design system" for what is really "that parent is
|
||||
one of its own descendants", so the cycle gets its own exception type."""
|
||||
mock_session = _make_mock_session()
|
||||
mock_session.get = AsyncMock(return_value=MagicMock(deleted_at=None, parent_id=None))
|
||||
|
||||
with patch("scribe.services.design_systems.async_session") as mock_cls, \
|
||||
patch("scribe.services.design_systems.access") as acc, \
|
||||
patch("scribe.services.design_systems._parent_map",
|
||||
AsyncMock(return_value={1: None, 2: 1})):
|
||||
acc.can_write_design_system = AsyncMock(return_value=True)
|
||||
mock_cls.return_value = mock_session
|
||||
from scribe.services.design_systems import update_design_system
|
||||
with pytest.raises(DesignSystemCycle):
|
||||
await update_design_system(user_id=1, design_system_id=1, parent_id=2)
|
||||
|
||||
mock_session.commit.assert_not_awaited() # refused BEFORE the write
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_clearing_the_parent_is_allowed_and_is_not_read_as_no_change():
|
||||
"""`parent_id=None` means "make this a root" — the one field where None is a
|
||||
value rather than "leave alone", which is why it is handled apart from the
|
||||
others."""
|
||||
system = MagicMock(deleted_at=None, parent_id=5)
|
||||
mock_session = _make_mock_session()
|
||||
mock_session.get = AsyncMock(return_value=system)
|
||||
|
||||
with patch("scribe.services.design_systems.async_session") as mock_cls, \
|
||||
patch("scribe.services.design_systems.access") as acc, \
|
||||
patch("scribe.services.design_systems._parent_map",
|
||||
AsyncMock(return_value={1: 5, 5: None})):
|
||||
acc.can_write_design_system = AsyncMock(return_value=True)
|
||||
mock_cls.return_value = mock_session
|
||||
from scribe.services.design_systems import update_design_system
|
||||
await update_design_system(user_id=1, design_system_id=1, parent_id=None)
|
||||
|
||||
assert system.parent_id is None
|
||||
|
||||
|
||||
# --- tokens -----------------------------------------------------------------
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_create_token_denied_without_write_on_the_system():
|
||||
with patch("scribe.services.design_systems.access") as acc:
|
||||
acc.can_write_design_system = AsyncMock(return_value=False)
|
||||
from scribe.services.design_systems import create_token
|
||||
result = await create_token(user_id=1, design_system_id=3, name="--fs-obsidian")
|
||||
assert result is None
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_token_values_default_to_an_empty_map_not_json_null():
|
||||
"""The column is NOT NULL so that absence has exactly ONE spelling. Passing
|
||||
None straight through would store JSON null and hand every reader back the
|
||||
second empty state the schema was shaped to remove."""
|
||||
mock_session = _make_mock_session()
|
||||
captured = {}
|
||||
mock_session.add = MagicMock(
|
||||
side_effect=lambda obj: captured.update(value_by_mode=obj.value_by_mode)
|
||||
)
|
||||
|
||||
with patch("scribe.services.design_systems.async_session") as mock_cls, \
|
||||
patch("scribe.services.design_systems.access") as acc:
|
||||
acc.can_write_design_system = AsyncMock(return_value=True)
|
||||
mock_cls.return_value = mock_session
|
||||
from scribe.services.design_systems import create_token
|
||||
await create_token(
|
||||
user_id=1, design_system_id=3, name="--fs-radius-md", value_by_mode=None
|
||||
)
|
||||
|
||||
assert captured["value_by_mode"] == {}
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_list_tokens_denied_returns_empty():
|
||||
with patch("scribe.services.design_systems.access") as acc:
|
||||
acc.can_read_design_system = AsyncMock(return_value=False)
|
||||
from scribe.services.design_systems import list_tokens
|
||||
assert await list_tokens(user_id=1, design_system_id=3) == []
|
||||
|
||||
|
||||
# --- the project pointer ----------------------------------------------------
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_pointing_a_project_at_a_system_needs_write_on_the_project():
|
||||
with patch("scribe.services.design_systems.access") as acc:
|
||||
acc.can_write_project = AsyncMock(return_value=False)
|
||||
acc.can_read_design_system = AsyncMock(return_value=True)
|
||||
from scribe.services.design_systems import set_project_design_system
|
||||
assert await set_project_design_system(1, project_id=5, design_system_id=3) is False
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_pointing_a_project_needs_only_READ_on_the_system():
|
||||
"""Consuming a design system is not changing it, so a system reachable
|
||||
through another project is a legitimate choice here. Requiring write would
|
||||
make a shared family style unusable by the people it was shared with."""
|
||||
project = MagicMock(deleted_at=None, design_system_id=None)
|
||||
mock_session = _make_mock_session()
|
||||
mock_session.get = AsyncMock(return_value=project)
|
||||
|
||||
with patch("scribe.services.design_systems.async_session") as mock_cls, \
|
||||
patch("scribe.services.design_systems.access") as acc:
|
||||
acc.can_write_project = AsyncMock(return_value=True)
|
||||
acc.can_read_design_system = AsyncMock(return_value=True)
|
||||
acc.can_write_design_system = AsyncMock(return_value=False)
|
||||
mock_cls.return_value = mock_session
|
||||
from scribe.services.design_systems import set_project_design_system
|
||||
assert await set_project_design_system(1, project_id=5, design_system_id=3) is True
|
||||
|
||||
assert project.design_system_id == 3
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_clearing_a_projects_design_system_skips_the_system_acl():
|
||||
"""Un-styling a project must not require permission on the system it is
|
||||
letting go of — including one that has since been deleted."""
|
||||
project = MagicMock(deleted_at=None, design_system_id=3)
|
||||
mock_session = _make_mock_session()
|
||||
mock_session.get = AsyncMock(return_value=project)
|
||||
|
||||
with patch("scribe.services.design_systems.async_session") as mock_cls, \
|
||||
patch("scribe.services.design_systems.access") as acc:
|
||||
acc.can_write_project = AsyncMock(return_value=True)
|
||||
acc.can_read_design_system = AsyncMock(return_value=False)
|
||||
mock_cls.return_value = mock_session
|
||||
from scribe.services.design_systems import set_project_design_system
|
||||
assert await set_project_design_system(1, project_id=5, design_system_id=None) is True
|
||||
|
||||
assert project.design_system_id is None
|
||||
acc.can_read_design_system.assert_not_awaited()
|
||||
Reference in New Issue
Block a user