Feature organization model - #6080
Conversation
|
👋 Hi @nairaj2, thanks for contributing! For the review process to begin, please verify that the following is satisfied:
Also check that issue requirements are satisfied & you ran Pull requests that don't follow the guidelines will be closed. Reviewer assignment can take up to 2 weeks. |
🔵 Review postedLast updated: 2026-08-13 14:31 UTC |
|
📢✨ Before we assign a reviewer, we'll turn on |
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #6080 — blockers inline; newest commits unreviewed.
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran a phased review pipeline over the pull request diff:
- Classified the diff to select review passes (core, frontend, backend) and whether manual QA was required
- Core review pass checked correctness, design, architecture, testing, completeness, and DRY/SRP/Rule-of-Three principles
- Specialized frontend/backend review passes applied framework-specific lenses where those files changed
- For UI changes: manual QA and an accessibility audit against a live dev server, when available
- Checked CI status and linked issue acceptance criteria
- Synthesized one review from those passes and chose the verdict from the findings, CI status, and QA evidence
| def _is_site_admin(user): | ||
| return bool(getattr(user, "is_admin", False)) | ||
|
|
||
|
|
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
blocking: Channel.filter_edit_queryset/filter_view_queryset (models.py:1216, 1235) ignore OrganizationRole — no Channel.organization access (#5967). Add Exists(OrganizationRole).
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #6080 — moving permission filtering down to model-level filter_view_queryset/filter_edit_queryset is the right Studio pattern, but the import-block rewrite dropped two constants that are still used, so every organization and membership write raises NameError. 11 of the PR's own 43 tests fail locally.
8 of 10 prior findings resolved, 1 withdrawn; 1 still open, plus 3 new blocking findings inline.
Withdrawing my earlier request for UUIDFilter on the organization filter: Studio's UUIDField (models.py:804) subclasses CharField, so CharFilter is correct and ?organization=abc returns 200. No change needed there.
flake8 catches all ten F821s in a single run — worth checking why they reached review.
Prior-finding status
RESOLVED — contentcuration/contentcuration/viewsets/organization.py:91 — user filter is now NumberFilter; ?user=abc returns 400
RESOLVED — contentcuration/contentcuration/viewsets/organization.py:91 — withdrawn: UUIDFilter on organization was my error
RESOLVED — contentcuration/contentcuration/viewsets/organization.py:273 — last-active-admin guard, now with deterministic lock order
RESOLVED — contentcuration/contentcuration/viewsets/organization.py:51 — membership create route added
UNADDRESSED — contentcuration/contentcuration/viewsets/organization.py:108 — Channel.filter_edit_queryset still ignores OrganizationRole
RESOLVED — contentcuration/contentcuration/viewsets/organization.py:9 — import ordering / black
RESOLVED — contentcuration/contentcuration/viewsets/organization.py — self-deactivation 404 from serialize_object()
RESOLVED — contentcuration/contentcuration/viewsets/organization.py:141 — moved to model-level filter querysets
RESOLVED — contentcuration/contentcuration/viewsets/organization.py:325 — select_for_update(of=("self",)) with deterministic lock order
RESOLVED — contentcuration/contentcuration/tests/viewsets/test_organization.py:23 — moved to tests/viewsets/, uses StudioAPITestCase
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran an automatic code-only delta review triggered by new commits on a previously reviewed PR:
- Retrieved prior bot reviews via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Core review pass only — specialized frontend/backend lenses and manual QA run when a review is explicitly requested
- Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence
| from contentcuration.constants.organization_roles import ( | ||
| organization_role_status_choices, | ||
| ) | ||
| from contentcuration.constants.organization_roles import ORGANIZATION_VIEWER |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
blocking: ORGANIZATION_ADMIN and ORGANIZATION_ROLE_STATUS_ACTIVE are still referenced at lines 191, 192, 265, 266, 288, 289, 297, 298, 313, 314 but no longer imported — 10 F821s.
Runtime effect: POST /api/organization and create/update/delete on the membership endpoint all 500 with NameError: name 'ORGANIZATION_ADMIN' is not defined. pytest contentcuration/tests/viewsets/test_organization.py → 11 failed, 32 passed; restoring the two imports takes it to 43 passed.
Also drop the now-unused Q (line 2) and ReadOnlyValuesViewset (line 20) imports.
| organization_edit = Exists( | ||
| OrganizationRole.objects.filter( | ||
| user_id=user_id, | ||
| organization_id=OuterRef("organization_id"), |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
blocking: User.filter_edit_queryset was replaced with Channel-shaped logic. User has no organization field (the FK is on Channel/OrganizationRole), so OuterRef("organization_id") raises FieldError: Cannot resolve keyword 'organization_id' into field. The edit annotation above is equally wrong — on a User queryset OuterRef("id") is a user id, not a channel id. Both annotations are applied before the is_admin short-circuit, so every caller fails.
AdminUserViewSet (viewsets/user.py:572) routes through get_edit_object() → get_edit_queryset() → this classmethod, so admin user management is dead: test_admin_delete_user and test_admin_update_user both fail.
The prior semantics (return queryset.filter(pk=user.pk) — a non-admin may edit only their own record) are also gone with no replacement, and nothing in #5967 asks for User edit permissions to change. This looks like a copy-paste of Channel.filter_edit_queryset (models.py:1246) landing in the wrong class; suggest reverting the method.
| ) | ||
| ) | ||
|
|
||
| organization_view = Exists( |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
blocking: org roles now grant channel view but never channel edit. Channel.filter_edit_queryset (models.py:1246) is unchanged and still consults only editable_channels, so an organization admin or editor cannot create, update, or delete a channel their organization owns. #5967 lists both as role permissions ("Create, update, and delete organization-owned channels" / "Create and update organization-owned channels and content").
Either mirror this annotation into filter_edit_queryset restricted to ORGANIZATION_ADMIN/ORGANIZATION_EDITOR, or say in the PR body that channel-level enforcement lands in a follow-up so the criterion is not silently dropped.
The new view grant is also untested — test_channel.py never mentions organizations. A channel visible to an org viewer and invisible to a non-member is the assertion to add.
| OrganizationRole.objects.filter( | ||
| user_id=user_id, | ||
| organization_id=OuterRef("organization_id"), | ||
| status=ORGANIZATION_ROLE_STATUS_ACTIVE, |
There was a problem hiding this comment.
suggestion: this filters OrganizationRole.status but not organization__deleted. Organization.filter_view_queryset (1941) and OrganizationRole.filter_view_queryset (2031) both exclude soft-deleted orgs, and perform_destroy only sets deleted=True — so after deleting an organization its channels stay visible to former members while the org itself disappears. Adding organization__deleted=False here keeps the three filters consistent.
| "updated_at", | ||
| ) | ||
|
|
||
| # def get_queryset(self): |
There was a problem hiding this comment.
suggestion: lines 142-182 are the old get_queryset/get_edit_queryset bodies left commented out after the logic moved to the model. They will drift from Organization.filter_view_queryset/filter_edit_queryset, and they are the only remaining "live-looking" reference to ORGANIZATION_ROLE_STATUS_ACTIVE in this file. Git history keeps them — delete the blocks.
| ) | ||
| membership.delete() | ||
|
|
||
| def update(self, request, *args, **kwargs): |
There was a problem hiding this comment.
suggestion: this is RESTUpdateModelMixin.update (viewsets/base.py:810) verbatim except that it builds its own response queryset instead of calling self.serialize_object(). I asked for the self-deactivation 404 fix in an earlier round and this looks like the answer — the object is permission-checked via get_edit_object(), so there is no leak, but the reason isn't visible here and the copy has to be kept in sync with base.py.
A one-line comment saying it bypasses get_queryset() deliberately (deactivating your own membership removes it from filter_view_queryset) plus a test covering that path would lock the behaviour in — deleting the method leaves the suite green, so nothing currently guards it.
| self.assertEqual(membership.role, ORGANIZATION_VIEWER) | ||
| self.assertEqual(membership.status, ORGANIZATION_ROLE_STATUS_ACTIVE) | ||
|
|
||
| def test_editor_cannot_create_membership(self): |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
praise: the membership-create negative paths are well covered — editor 403, non-member 403, invalid role 400, duplicate 400 with a count assertion that the existing row survived. Each asserts database state, not just status codes.
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #6080 — 13 of 16 prior findings resolved or acknowledged; 3 still open.
CI passing. Delta touches Channel.filter_edit_queryset and the organization filterset only; no UI files, so visual verification did not apply.
The org→channel edit grant I asked for last round landed on Channel, where the FK lives — right place. It also gates deletion, and it has no tests. Details inline.
Prior-finding status
RESOLVED — viewsets/organization.py:52 — no create route; Invitation.accept() channel-only
RESOLVED — viewsets/organization.py:9 — pre-commit fails on head
RESOLVED — viewsets/organization.py — serialize_object() re-reads active-only get_queryset()
RESOLVED — viewsets/organization.py:142 — use model-level filter_view_queryset/filter_edit_queryset
RESOLVED — viewsets/organization.py:326 — select_for_update(of=("self",)) and lock ordering
RESOLVED — tests/viewsets/test_organization.py:23 — move to tests/viewsets/ + StudioAPITestCase
RESOLVED — viewsets/organization.py:89 — ?user=abc → 500; NumberFilter/UUIDFilter now in place
RESOLVED — viewsets/organization.py:109 — Channel filter querysets ignore OrganizationRole
RESOLVED — viewsets/organization.py — ORGANIZATION_ADMIN/ORGANIZATION_ROLE_STATUS_ACTIVE imports missing
RESOLVED — models.py:691 — User.filter_edit_queryset given Channel-shaped logic
RESOLVED — models.py:1272 — org roles grant channel view but never edit
ACKNOWLEDGED — viewsets/organization.py:274 — last-active-admin guard (praise)
ACKNOWLEDGED — tests/viewsets/test_organization.py:371 — membership-create negative paths (praise)
UNADDRESSED — models.py:1272 — organization_view ignores organization__deleted
UNADDRESSED — viewsets/organization.py:143 — commented-out get_queryset/get_edit_queryset bodies
UNADDRESSED — viewsets/organization.py:357 — update() duplicates RESTUpdateModelMixin.update
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran an automatic code-only delta review triggered by new commits on a previously reviewed PR:
- Retrieved prior bot reviews via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Core review pass only — specialized frontend/backend lenses and manual QA run when a review is explicitly requested
- Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence
| return queryset | ||
|
|
||
| return queryset.filter(edit=True) | ||
| return queryset.filter(Q(edit=True) | Q(organization_edit=True)) |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
blocking: this grant is the one I asked for last round, but it is one role too wide on the delete path. get_edit_queryset (viewsets/base.py:597) dispatches here, and ChannelViewSet.destroy (viewsets/channel.py:484) resolves its target through get_edit_object() — so an active ORGANIZATION_EDITOR can soft-delete any channel their organization owns. #5967 gives delete to Admin only; Editor is scoped to "Create and update organization-owned channels and content".
Split edit from delete: annotate the role alongside the boolean and check it in ChannelViewSet.destroy, or add a filter_delete_queryset restricted to ORGANIZATION_ADMIN and call it from destroy.
| ) | ||
| ) | ||
| queryset = queryset.annotate(edit=edit) | ||
| organization_edit = Exists( |
There was a problem hiding this comment.
blocking: no coverage for this grant — tests/viewsets/test_organization.py never references Channel, and tests/viewsets/test_channel.py never references organizations. #5967 requires automated tests verifying permission enforcement, and lists channel access for all three roles.
Minimum cases: admin and editor can update an org-owned channel they have no m2m share on; viewer cannot; a non-member cannot; an ORGANIZATION_ROLE_STATUS_PENDING role grants nothing.
| ) | ||
| queryset = queryset.annotate(edit=edit) | ||
| organization_edit = Exists( | ||
| OrganizationRole.objects.filter( |
There was a problem hiding this comment.
suggestion: filters status but not organization__deleted, so roles keep conferring channel edit after OrganizationViewSet.perform_destroy soft-deletes the org. Every other org queryset in this PR excludes deleted orgs (1929, 1947, 2018, 2038). Same gap in the organization_view subquery at 1272. Intended?
| ), | ||
| ) | ||
| ) | ||
| queryset = queryset.annotate( |
There was a problem hiding this comment.
suggestion: ChannelViewSet.get_queryset (viewsets/channel.py:499-517) re-annotates edit/view from the m2m tables alone, and organization_edit/organization_view are surfaced nowhere. An org admin listing channels therefore sees edit: false on channels the write endpoints will accept from them. Consider folding the org Exists into the edit annotation there so read and write agree.
| "updated_at", | ||
| ) | ||
|
|
||
| # def get_queryset(self): |
There was a problem hiding this comment.
suggestion: (still open) the old get_queryset/get_edit_queryset bodies are still here commented out after the move to model-level filters — delete them.
| ) | ||
| membership.delete() | ||
|
|
||
| def update(self, request, *args, **kwargs): |
There was a problem hiding this comment.
suggestion: (still open) this is RESTUpdateModelMixin.update (viewsets/base.py:810) verbatim except that it builds its own queryset instead of calling serialize_object(). A one-line comment naming the reason (self-deactivation 404s through the filtered queryset) would stop it reading as an accidental copy.
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #6080 — delta of 82f0990. 13 of 20 prior findings resolved, 7 open. CI passing; no UI files.
Delta adds Channel.filter_delete_queryset, which nothing calls — inline.
Still open threads: models.py:1233 untested (blocking); 1233/1306 organization__deleted; 1243 ChannelViewSet.get_queryset; organization.py:143 commented-out; :357 RESTUpdateModelMixin.update.
Prior-finding status
RESOLVED — viewsets/organization.py:52 — Invitation.accept()
RESOLVED — viewsets/organization.py:109 — OrganizationRole
RESOLVED — viewsets/organization.py:9 — pre-commit
RESOLVED — viewsets/organization.py — serialize_object()
RESOLVED — viewsets/organization.py:142 — filter_view_queryset
RESOLVED — viewsets/organization.py:326 — select_for_update(of=("self",))
RESOLVED — tests/viewsets/test_organization.py:23 — StudioAPITestCase
RESOLVED — viewsets/organization.py — ORGANIZATION_ADMIN
RESOLVED — models.py — User.filter_edit_queryset
RESOLVED — models.py:1302 — filter_edit_queryset
RESOLVED — viewsets/organization.py:92 — NumberFilter/UUIDFilter
RESOLVED — viewsets/organization.py:280 — _ensure_not_last_active_admin
RESOLVED — tests/viewsets/test_organization.py:371 — praise
UNADDRESSED — models.py:1250 — ORGANIZATION_EDITOR, destroy
UNADDRESSED — models.py:1232 — untested
UNADDRESSED — models.py:1233 — organization__deleted
UNADDRESSED — models.py:1306 — organization__deleted
UNADDRESSED — models.py:1243 — ChannelViewSet.get_queryset
UNADDRESSED — viewsets/organization.py:143 — commented-out
UNADDRESSED — viewsets/organization.py:357 — RESTUpdateModelMixin.update
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran an automatic code-only delta review triggered by new commits on a previously reviewed PR:
- Retrieved prior bot reviews via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Core review pass only — specialized frontend/backend lenses and manual QA run when a review is explicitly requested
- Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence
| return queryset.filter(Q(edit=True) | Q(organization_edit=True)) | ||
|
|
||
| @classmethod | ||
| def filter_delete_queryset(cls, queryset, user): |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
blocking: no callers; grep finds only this definition. Delete goes through get_edit_queryset() (base.py:591) from ChannelViewSet.destroy (channel.py:484) and delete_from_changes (base.py:750); filter_edit_queryset admits ORGANIZATION_EDITOR, so org editors still delete org channels — #5967 says admin only.
My last-round ask, unwired. Add get_delete_queryset() beside get_edit_queryset() in BaseValuesViewset, wire both delete paths, test editor 403 / admin 204 — or drop until wired.
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #6080 — 15 of 20 prior findings resolved; 5 still open. CI passing.
filter_delete_queryset is now wired through all three delete paths, so org editors can edit but not delete. The new denial branch has no test coverage (inline, blocking).
Still open from earlier rounds:
- models.py:1233, 1306 — suggestion: org-role grants still ignore
organization__deleted, so a soft-deleted org keeps conferring channel edit and view. - channel.py:518-521 — suggestion:
ChannelViewSet.get_querysetre-annotatesedit/viewfrom the m2m alone, so org-role holders serialize asedit: false. - organization.py:143-183 — suggestion: commented-out
get_queryset/get_edit_querysetbodies. - organization.py:357 — suggestion:
updateduplicatesRESTUpdateModelMixin.updateapart from itsselect_relatedqueryset.
Prior-finding status
RESOLVED — viewsets/organization.py:52 — no create route; Invitation.accept() channel-only
RESOLVED — viewsets/organization.py:109 — Channel.filter_edit_queryset/filter_view_queryset ignore OrganizationRole
RESOLVED — viewsets/organization.py:9 — pre-commit fails on head
RESOLVED — viewsets/organization.py — serialize_object() re-reads active-only queryset; self-deactivation 404s
RESOLVED — viewsets/organization.py:142 — use model-level filter_view_queryset/filter_edit_queryset
RESOLVED — viewsets/organization.py:326 — select_for_update(of=("self",)) lock order
RESOLVED — tests/viewsets/test_organization.py:23 — tests/viewsets/ + StudioAPITestCase convention
RESOLVED — viewsets/organization.py — stale ORGANIZATION_ADMIN/ORGANIZATION_ROLE_STATUS_ACTIVE references
RESOLVED — models.py — User.filter_edit_queryset replaced with Channel-shaped logic
RESOLVED — models.py:1302 — org roles grant channel view but never channel edit
RESOLVED — viewsets/organization.py — ?user=abc → 500; verified NumberFilter/UUIDFilter at organization.py:92-93
RESOLVED — tests/viewsets/test_organization.py:371 — praise, membership-create negative paths
RESOLVED — viewsets/organization.py:274 — praise, demotion/deactivation/deletion locked in-transaction
RESOLVED — models.py:1253 — filter_delete_queryset had no callers; now wired via get_delete_queryset
RESOLVED — models.py:1250 — edit grant was one role too wide on the delete path; delete path is now admin-only
UNADDRESSED — models.py:1232 — no test coverage for the org-role channel-edit grant
UNADDRESSED — models.py:1233, 1306 — org-role grants ignore organization__deleted
UNADDRESSED — models.py:1243 — ChannelViewSet.get_queryset re-annotates edit/view from the m2m
UNADDRESSED — viewsets/organization.py:143 — commented-out get_queryset/get_edit_queryset bodies
UNADDRESSED — viewsets/organization.py:357 — update duplicates RESTUpdateModelMixin.update
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran an automatic code-only delta review triggered by new commits on a previously reviewed PR:
- Retrieved prior bot reviews via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Core review pass only — specialized frontend/backend lenses and manual QA run when a review is explicitly requested
- Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence
|
|
||
| def destroy(self, request, *args, **kwargs): | ||
| instance = self.get_edit_object() | ||
| if not self.get_delete_queryset().filter(pk=instance.pk).exists(): |
There was a problem hiding this comment.
blocking: neither this denial branch nor the org-admin allow path has a test. No test in the repo builds an OrganizationRole and touches a channel — tests/viewsets/test_organization.py has zero channel references, tests/viewsets/test_channel.py zero organization references. test_channel.py:892 and :317/:351 cover direct editors only, who pass the exists() check either way. Issue #5967 makes deleting organization-owned channels admin-only and requires tests verifying enforcement.
Three cases:
- org admin with no
editorsrow →DELETE /api/channel/<id>returns 204,channel.deletedis True - org editor → 403
- org editor via
sync_changesgenerate_delete_event→channel.deletedstays False
The first matters most: it is the only one that would catch filter_delete_queryset (models.py:1253) matching nothing for a legitimate org admin, which would silently make org-owned channels undeletable.
| def delete_from_changes(self, changes): | ||
| errors = [] | ||
| queryset = self.get_edit_queryset().order_by() | ||
| queryset = self.get_delete_queryset().order_by() |
There was a problem hiding this comment.
suggestion: delete_from_changes swallows a queryset miss as success (768-771), a convention documented at test_channel.py:343. That was safe while a miss meant the user could not see the object at all. An org editor is in filter_edit_queryset but not filter_delete_queryset: they see the channel, delete it, get 200, and it reappears on the next sync. channel.py:488 raises PermissionDenied for that same user, so the two paths disagree on whether the denial is visible. Worth deciding deliberately — return an error here, or confirm the silent 200 is what the client should see.
| return queryset.model.filter_edit_queryset(queryset, self.request.user) | ||
| return self.get_queryset() | ||
|
|
||
| def get_delete_queryset(self): |
There was a problem hiding this comment.
praise: falling back to get_edit_queryset() keeps every viewset without a filter_delete_queryset unchanged, and all three delete paths were switched together, so none still routes deletes through edit permissions.
2 participants
Summary
Adds backend API and controller support for the Organization and OrganizationRole models.
This change provides:
Organization administrators can manage organization settings, memberships, and roles. Editors can view memberships but cannot manage organization settings, memberships, or roles. Viewers have read-only access to organization resources.
Frontend changes and data model changes are outside the scope of this PR.
…
References
References
Closes #5967
Builds on #5962
…
Reviewer guidance
Run the organization API tests with:
pytest -q contentcuration/contentcuration/tests/test_organization.py
Reviewers can verify that:
This PR does not include frontend changes.
…
AI usage
Used AI to help review the existing Studio API patterns and draft portions of the organization viewsets and tests.
I reviewed and edited the generated code to align it with Studio's ValuesViewset, serializer, routing, pagination, and permission conventions. I also ran the organization tests locally and used the failures to correct routing, response formatting, authentication expectations, and role-permission behavior.