Conversation
Reviewer's GuideThis hygiene-only change preserves permission behavior while making org-admin role definitions single-sourced from their templates, removing dead dual-state configuration, updating stale documentation, and adding a no-drift regression guard. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="apps/betterangels-backend/accounts/tests/test_template_permissions.py" line_range="167-170" />
<code_context>
+ drift from, or amplify beyond, the legacy bundle. This pins that property
+ so a future hand-edit of one side without the other fails here.
+ """
+ from accounts.groups import ORG_ADMIN, ORG_ADMIN_ROLE, ORG_SUPERUSER, ORG_SUPERUSER_ROLE
+
+ assert list(ORG_ADMIN_ROLE.permissions) == list(ORG_ADMIN.permissions)
+ assert list(ORG_SUPERUSER_ROLE.permissions) == list(ORG_SUPERUSER.permissions)
+ assert ORG_ADMIN_ROLE.is_invitable == ORG_ADMIN.is_invitable
+ assert ORG_SUPERUSER_ROLE.is_invitable == ORG_SUPERUSER.is_invitable
</code_context>
<issue_to_address>
**nitpick (testing):** The drift-guard test does not assert that `ORG_ADMIN_ROLE.name` and `ORG_SUPERUSER_ROLE.name` match their corresponding `TemplateConfig.name` values, so a future role-name divergence can pass this test while registering or synchronizing a grant role under a different name than its legacy template.
**Triggers:** When either role definition is later hand-edited independently of its template.
**Suggested fix:** Add assertions for both role names, and optionally `is_global`, so the guard covers the complete `RoleDef` contract claimed by the test.
```suggestion
assert list(ORG_ADMIN_ROLE.permissions) == list(ORG_ADMIN.permissions)
assert list(ORG_SUPERUSER_ROLE.permissions) == list(ORG_SUPERUSER.permissions)
assert ORG_ADMIN_ROLE.name == ORG_ADMIN.name
assert ORG_SUPERUSER_ROLE.name == ORG_SUPERUSER.name
assert ORG_ADMIN_ROLE.is_invitable == ORG_ADMIN.is_invitable
assert ORG_SUPERUSER_ROLE.is_invitable == ORG_SUPERUSER.is_invitable
```
</issue_to_address>| assert list(ORG_ADMIN_ROLE.permissions) == list(ORG_ADMIN.permissions) | ||
| assert list(ORG_SUPERUSER_ROLE.permissions) == list(ORG_SUPERUSER.permissions) | ||
| assert ORG_ADMIN_ROLE.is_invitable == ORG_ADMIN.is_invitable | ||
| assert ORG_SUPERUSER_ROLE.is_invitable == ORG_SUPERUSER.is_invitable |
There was a problem hiding this comment.
nitpick (testing): The drift-guard test does not assert that ORG_ADMIN_ROLE.name and ORG_SUPERUSER_ROLE.name match their corresponding TemplateConfig.name values, so a future role-name divergence can pass this test while registering or synchronizing a grant role under a different name than its legacy template.
Triggers: When either role definition is later hand-edited independently of its template.
Suggested fix: Add assertions for both role names, and optionally is_global, so the guard covers the complete RoleDef contract claimed by the test.
| assert list(ORG_ADMIN_ROLE.permissions) == list(ORG_ADMIN.permissions) | |
| assert list(ORG_SUPERUSER_ROLE.permissions) == list(ORG_SUPERUSER.permissions) | |
| assert ORG_ADMIN_ROLE.is_invitable == ORG_ADMIN.is_invitable | |
| assert ORG_SUPERUSER_ROLE.is_invitable == ORG_SUPERUSER.is_invitable | |
| assert list(ORG_ADMIN_ROLE.permissions) == list(ORG_ADMIN.permissions) | |
| assert list(ORG_SUPERUSER_ROLE.permissions) == list(ORG_SUPERUSER.permissions) | |
| assert ORG_ADMIN_ROLE.name == ORG_ADMIN.name | |
| assert ORG_SUPERUSER_ROLE.name == ORG_SUPERUSER.name | |
| assert ORG_ADMIN_ROLE.is_invitable == ORG_ADMIN.is_invitable | |
| assert ORG_SUPERUSER_ROLE.is_invitable == ORG_SUPERUSER.is_invitable |
afcb988 to
e2cc037
Compare
a041cb2 to
c3b46ec
Compare
e2cc037 to
7325c5c
Compare
c3b46ec to
7cd8202
Compare
7325c5c to
8ac9f0f
Compare
7cd8202 to
037aa50
Compare
…d dual state Architecture cleanup of the grant-based permission model (ADR 0001). Zero behavior change — the full role/domain contract is identical; only the way it is declared and documented is tightened. - accounts/groups.py: ORG_ADMIN_ROLE / ORG_SUPERUSER_ROLE are now built with RoleDef.from_template(ORG_ADMIN / ORG_SUPERUSER) — the grant-side bundle is the SAME source as the (inert) legacy template, so the two can never drift. Deletes the hand-duplicated ORG_ADMIN_ROLE_PERMISSIONS / ORG_SUPERUSER_ROLE_PERMISSIONS lists (the strongest latent drift risk: a mirrored Grant diverging from, or amplifying beyond, the legacy bundle). CASEWORKER_ROLE stays hand-defined (deliberate RFC 0003 subset) — noted. - common/permissions/domain.py: drop the DUAL_APPS transitional state (empty since the org-admin cutover completed); GLOBAL_TIER_ORG_APPS is now frozenset(LEGACY_INERT_APPS). Module docstring rewritten to the two real states (grant-only / legacy-only) with the stale HasOrgPerm/PR references removed. - accounts/selectors.py: fold docstring no longer references the retired "dual" transitional state. - accounts/tests/test_template_permissions.py: pins that the org-admin Role bundles equal their TemplateConfig bundles (no-drift guard).
8ac9f0f to
3af672f
Compare
037aa50 to
c31b215
Compare
permissions.md still described the deleted machinery — the HasOrgPerm extension, permissioned_queryset/_perm_q, get_user_permitted_org, and the header-first org context — as current architecture; graphql_errors.md listed HasOrgPerm in its extension and exception tables. Rewrite both to the grant model: - org context: payload first; the X-Organization-ID header survives only as the teams list-read fallback (strip = DEV-2566); - authority via Role/Grant + can()/scopes()/visible()/can_obj() and require_can() at the write boundary; the User.groups m2m edge mirrors dual-write memberships to Grants; legacy_inert roles are grant-only; - key files and testing sections point at the selectors and the per-domain grant-authorization suites.
Summary
Architecture/hygiene pass over the grant-based permission core (ADR 0001). Zero behavior change — the role/domain contract is byte-identical; only the declaration and documentation are tightened. Stacks on the org-admin stack (#2443 teams → #2444 reports → #2445 members → #2446 teardown → #2447 write tiers).
What changes
Single source of truth for the org-admin role bundles (
accounts/groups.py)ORG_ADMIN_ROLE/ORG_SUPERUSER_ROLEare now built viaRoleDef.from_template(ORG_ADMIN / ORG_SUPERUSER)— the same pattern shelters already use — instead of hand-duplicatedORG_ADMIN_ROLE_PERMISSIONS/ORG_SUPERUSER_ROLE_PERMISSIONSlists.from_templatemakes divergence impossible.CASEWORKER_ROLEstays hand-defined deliberately (a strict RFC 0003 subset) — the comment now says why.Dead transitional state removed (
common/permissions/domain.py)DUAL_APPS(empty since the org-admin cutover completed) is deleted;GLOBAL_TIER_ORG_APPSis nowfrozenset(LEGACY_INERT_APPS)(same value, kept as its own name for the fold's readability).LEGACY_INERT_APPS/ legacy-only notes-clients) — removing stale references to the deletedHasOrgPerm/HasOrgPermOrGrantand long-closed PR numbers.Doc drift — the
organization_effective_permissionsfold docstring no longer references the retired "dual" transitional state.Drift guard test — pins
ORG_ADMIN_ROLE.permissions == ORG_ADMIN.permissions(and superuser) so a future hand-edit of one side without the other fails CI.Review focus
RoleDef.from_templateconversion is byte-identical to the old manualRoleDefs (name, permission list order,is_invitable=False) — the targeted + full suites assert this.GLOBAL_TIER_ORG_APPSvalue is unchanged (it wasLEGACY_INERT_APPS | frozenset()); onlyDUAL_APPSand stale docs are gone.Test plan
manage.py checkclean;ruffclean.Deferred (noted, not done — too much import churn across five stacked PRs): splitting
common/permissions/utils.py(permission-catalog registry vs runtime predicates) into separate modules.Summary by Sourcery
Consolidate organization role definitions and permission-domain metadata around the current grant-based model without changing authorization behavior.
Enhancements:
Documentation:
Tests:
Chores: