feat(core): add inert organization structure (units + memberships) - #549
Open
abdulrafey1 wants to merge 8 commits into
Open
feat(core): add inert organization structure (units + memberships)#549abdulrafey1 wants to merge 8 commits into
abdulrafey1 wants to merge 8 commits into
Conversation
abdulrafey1
marked this pull request as draft
July 27, 2026 07:23
This was referenced Jul 27, 2026
abdulrafey1
marked this pull request as ready for review
July 27, 2026 08:27
abdulrafey1
marked this pull request as draft
July 27, 2026 10:02
abdulrafey1
changed the title
abdulrafey1
marked this pull request as ready for review
July 27, 2026 10:39
hamza-56
reviewed
Jul 28, 2026
| # expired_attributes so this never itself reads a possibly-expired column, and never | ||
| # disturbing unrelated objects (like the new parent) that the rewrite didn't touch. | ||
| # Session-wide by design: fetch-sync already limited expiry to UPDATE-touched rows, so filtering more is redundant. | ||
| for loaded in session.identity_map.values(): |
Contributor
There was a problem hiding this comment.
This loop is one SELECT per stale in-session unit, and it takes a 15-line comment to explain why it's needed. Running the UPDATE with execution_options(synchronize_session=False) and doing a single session.expire_all() before the commit gets the same safety in constant time, and the comment can go with it.
Contributor
Author
There was a problem hiding this comment.
Pushing back on this one: synchronize_session=False + expire_all() isn't equivalent under AsyncSession. With no fetch-sync the UPDATE expires nothing (in-session units keep silently stale paths), and expire_all() then expires every object in the session — so the next plain attribute access on any previously loaded unit raises MissingGreenlet in the caller instead of lazy-loading. That's exactly the failure this loop absorbs: it refreshes only the UPDATE-touched units (typically 0–2 objects — only ones already in the session), so callers never inherit expired state. We did try the expire-all variant first and it broke callers in exactly this way. What I did take from the comment: the 15-line essay is now 5 lines (44c6b6d), including a note on why expire_all isn't a substitute.
This comment was written with the assistance of an LLM (Claude).
An error occurred while trying to automatically change base from
rafey/docs/permission-groups
to
rafey/feat/permission-groups-api
July 31, 2026 10:32
OrgUnit encodes the organisation hierarchy as adjacency plus a materialized id path; OrgMembership seats users in units (soft-deleted history). Inert by design: no permission check reads the tree — its permission effect arrives later via rule-driven group membership (see #519 follow-up phases). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
create derives path from the parent; move re-parents with a cycle check (one path prefix comparison) and rewrites the subtree in a single portable UPDATE; delete refuses while children or active members exist (OrgUnitInUse). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
get_org_unit's populate_existing=True fixed a real bug (a partially-expired path after move_org_unit's bulk-UPDATE subtree rewrite) but in the wrong place: it forced a fresh SELECT on every call forever and diverged from the get_role/get_group bare-session.get convention. The bulk UPDATE's SET clause is SQL-side (func.substr), so SQLAlchemy's synchronize_session="fetch" fallback only partially expires the path attribute on matched in-session objects; session.get() only re-queries on a whole-instance expire, so a bare read after the move raised MissingGreenlet under AsyncSession. Scope the fix to move_org_unit instead: after the bulk UPDATE, refresh (awaited) exactly the in-session objects the rewrite touched, identified via InstanceState.expired_attributes so unrelated objects (like the new parent) are left untouched. get_org_unit is restored to a bare session.get, matching its siblings. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Seats users in org units: idempotent race-safe add, soft-deleted removal history. Membership is classification data only — no inheritance across the tree and no permission effect. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Add a test proving two roots with the same name collide via the coalesce(parent_id, 0) unique index, since SQL's NULL-is-distinct semantics would otherwise let duplicate roots slip through untested. Also clarify, with one comment line, that move_org_unit's identity-map refresh loop is intentionally session-wide: fetch-sync already scoped expiry to the rows the UPDATE touched, so filtering further is redundant. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Naming now follows the Organizational Structure design document: the package is sparkth/core/organization, OrgUnit is OrganizationalUnit (table organizational_unit), OrgMembership is OrganizationMembership (table organization_membership). The tables migration is regenerated with the new names — it existed only on this unmerged branch, so no applied environment is affected. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- give ix_organizational_unit_path varchar_pattern_ops so the Postgres btree can serve the LIKE-prefix descendant lookups; a plain btree under a non-C collation cannot, and every subtree query would seq-scan (the migration is edited in place: it is unreleased, this branch is its only consumer) - delete historical organization membership rows in one bulk statement instead of one DELETE per loaded row - compress the move_organizational_unit refresh-loop comment to the essentials Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
patch_organizational_unit applies rename/re-kind/re-parent in one transaction: validation runs against the final state (the name is checked against the destination's siblings) and nothing commits until every check passes, so a combined change can never half-apply. update_organizational_unit and move_organizational_unit now delegate to it, keeping their signatures and behaviour. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
abdulrafey1
force-pushed
the
rafey/feat/org-tree-core
branch
from
July 31, 2026 10:41
b92b0a9 to
7b72814
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes: Organizational structure has no representation
Part 1 of 3 stacked PRs (core → management surface → docs); stacked on the #519 groups stack (#546). Does not close the issue on its own.
What
Adds the organizational structure as inert people-classification data — an
OrganizationalUnithierarchy andOrganizationMembershipuser↔unit seats — so the org chart finally has a home; it grants nothing and no permission check reads it.Changes
sparkth/core/organization/domain —OrganizationalUnit(adjacency + materialized id path/1/7/42/, sibling-unique names; tableorganizational_unit) andOrganizationMembership(soft-deleted history, partial-unique active seat; tableorganization_membership) with migration94c26cf76059move_organizational_unitre-parents with a one-comparison cycle check and rewrites the whole subtree in a single portable UPDATE (SQLite + PostgreSQL); delete refuses withOrganizationalUnitInUsewhile children or active members existget_organizational_uniton the shared baresession.getconventionadd_organization_member, soft-deleteremove_organization_member,get_organization_members; membership deliberately does NOT inherit across the treeHow to Test
make test.backend.pytest— full suite green; coverage intests/organization/(models, path maintenance, moves, cycle prevention, delete guards, memberships).make mypy && make lint.backend— clean.94c26cf76059verified up and down against PostgreSQL.grep -rn "core.organization" sparkth/core/permissions/— no engine coupling in either direction.Notes
94c26cf76059(two new tables, no data changes).This PR description was written with the assistance of an LLM (Claude).