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. |
🟡 Waiting for changesLast updated: 2026-08-25 15:33 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
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
| 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.
| ) | ||
| 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.
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
| ) | ||
| 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.
| ) | ||
| 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
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 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.
rtibbles
left a comment
rtibbles
left a comment
There was a problem hiding this comment.
This is looking in decent shape - there has been some unnecessary flyby linting that would be nice to revert for the sake of a tidy commit history.
The instinct to follow the bot's suggestion for the delete queryset is a nice way of delineating the permissions for the Organization Admin neatly - I especially like that you used this to retain backwards compatibility with the existing "channel editor" permissions which are allowed to delete.
Actually blocking:
- Deleting the commented out code - no point committing unused code in comments or otherwise.
- Odd indirection of the export for the new viewset. We should choose a name and stick with it.
- Use of the UserPrimaryKeyRelatedField so that our permissions delegation consistently flows through the edit, view, and (now) delete querysets.
| model = OrganizationRole | ||
| fields = ( | ||
| "id", | ||
| "organization", |
There was a problem hiding this comment.
Our general pattern for permission gating is to use this serializer related field: https://github.com/learningequality/studio/blob/unstable/contentcuration/contentcuration/viewsets/common.py#L257
This then applies the same edit/view queryset pattern to the serializer (not affected by the new delete queryset pattern added here, because they don't touch the serializer) - and means that permissions can be managed consistently with a 404 rather than 403/401, because the Organization is only editable by someone with an admin role. Keeps the permissions delegated to the single path.
| ).strip() | ||
| return items | ||
|
|
||
| def _require_admin(self, organization): |
There was a problem hiding this comment.
See above how that might not be necessary.
| return Response(self.serialize(queryset)[0]) | ||
|
|
||
|
|
||
| # The model is named OrganizationRole, while existing work may already import |
There was a problem hiding this comment.
This seems like unnecessary indirection? What else would be importing this, if it is new in this PR?
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #6080 — 17 of 24 prior findings resolved; 7 open; 1 new blocking, inline.
CI has not run on 3771233.
Open blocking: models.py:1232, viewsets/channel.py:487 untested. Five suggestions unchanged.
Prior-finding status
RESOLVED — viewsets/organization.py:52 — create route
RESOLVED — viewsets/organization.py — Channel.filter_edit_queryset
RESOLVED — viewsets/organization.py:8 — pre-commit
RESOLVED — viewsets/organization.py — serialize_object()
RESOLVED — viewsets/organization.py:141 — filter_view_queryset
RESOLVED — viewsets/organization.py:263 — select_for_update(of=("self",))
RESOLVED — tests/viewsets/test_organization.py:23 — StudioAPITestCase
RESOLVED — viewsets/organization.py — stale constants
RESOLVED — models.py — User.filter_edit_queryset
RESOLVED — models.py:1302 — organization_view
RESOLVED — models.py:1250 — organization_edit
RESOLVED — models.py:1253 — filter_delete_queryset
RESOLVED — viewsets/organization.py:95 — NumberFilter/UUIDFilter
RESOLVED — viewsets/organization.py:263 — praise
RESOLVED — tests/viewsets/test_organization.py:371 — praise
RESOLVED — viewsets/organization.py:142 — commented-out bodies
RESOLVED — viewsets/base.py:601 — praise
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/channel.py:487 — untested
UNADDRESSED — viewsets/base.py:763 — delete_from_changes
UNADDRESSED — viewsets/organization.py:294 — duplicated 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
| ).strip() | ||
| return items | ||
|
|
||
| def perform_create(self, serializer, change=None): |
There was a problem hiding this comment.
suggestion: identical to CreateModelMixin.perform_create (base.py:712) — drop it.
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #6080 delta re-review: 19 of 27 prior findings resolved, 8 open.
Prior-finding status
- RESOLVED
organization.py:52Invitation.accept() - RESOLVED
organization.pyChannel.filter_edit_queryset/OrganizationRole - RESOLVED
organization.py:8pre-commit - RESOLVED
organization.pyserialize_object()404 - RESOLVED
organization.py:141filter_view_queryset - RESOLVED
organization.py:263select_for_update(of=("self",)) - RESOLVED
test_organization.py:27StudioAPITestCase - RESOLVED
organization.pyORGANIZATION_ADMIN - RESOLVED
models.pyUser.filter_edit_queryset - RESOLVED
models.py:1302organization_edit - RESOLVED
organization.pyget_queryset - RESOLVED
models.py:1250filter_edit_queryset/filter_delete_queryset - RESOLVED
models.py:1253filter_delete_queryset - RESOLVED
organization.py:95NumberFilter/UUIDFilter - RESOLVED
models.py:1232organization_edit - RESOLVED
test_organization.py:386403400 - RESOLVED
test_organization.py:375praise - RESOLVED
base.py:601praiseget_delete_queryset - RESOLVED
organization.pypraisetransaction - UNADDRESSED
channel.py:487get_delete_queryset - UNADDRESSED
models.py:1233organization__deleted - UNADDRESSED
models.py:1243ChannelViewSet.get_queryset - UNADDRESSED
models.py:1306organization__deleted - UNADDRESSED
organization.py:294update()/base.py:810 - UNADDRESSED
organization.py:214perform_create/base.py:712 - UNADDRESSED
base.py:763delete_from_changes
@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
| channel.name = "Admin Updated Channel" | ||
| channel.save(actor_id=self.organization_admin.id) | ||
|
|
||
| channel.refresh_from_db() |
There was a problem hiding this comment.
suggestion: .get() on 640 is the whole assertion; channel.save() is not permission-gated, so 644-645 and 655-656 pass for anyone reaching them. Drop them, or PATCH the channel route as editor_user.
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #6080 — 22 of 31 prior findings resolved; 9 still open (all in unchanged code; their existing threads stand). One new blocking finding on the delta.
The two new API-level delete tests fail: the router registers channel routes with trailing_slash=False (urls.py:79), so the hardcoded /api/channel/<id>/ 404s at URL resolution and neither request reaches ChannelViewSet.destroy. Inline at :676.
No workflow runs exist on 15792d7, so this was never surfaced. Head has since advanced to 145f9b6 (merge with unstable); those commits were not reviewed, but the two call sites are unchanged there.
Out of delta, not filed: test_membership_user_and_organization_cannot_be_reassigned (:508) also fails deterministically (400 != 200).
Prior-finding status
RESOLVED — viewsets/organization.py:52 — no create route; Invitation.accept() channel-only
RESOLVED — viewsets/organization.py — Channel.filter_edit_queryset/filter_view_queryset ignore OrganizationRole
RESOLVED — viewsets/organization.py:8 — pre-commit fails on head
RESOLVED — viewsets/organization.py — serialize_object() re-reads active-only get_queryset()
RESOLVED — viewsets/organization.py:141 — model-level filter_view_queryset/filter_edit_queryset
RESOLVED — viewsets/organization.py:263 — select_for_update(of=("self",)) lock order
RESOLVED — tests/viewsets/test_organization.py:27 — 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 view but never edit
RESOLVED — viewsets/organization.py — commented-out get_queryset/get_edit_queryset bodies
RESOLVED — models.py:1250 — delete-path grant one role too wide
RESOLVED — models.py:1232 — no coverage for the org edit grant
RESOLVED — models.py:1253 — filter_delete_queryset had no callers
RESOLVED — tests/viewsets/test_organization.py — 403 is now 400
RESOLVED — viewsets/base.py:601 — filter_delete_queryset falls back to get_edit_queryset() (praise)
RESOLVED — tests/viewsets/test_organization.py:375 — membership-create negative paths (praise)
RESOLVED — viewsets/organization.py — ?user=abc → 500; NumberFilter at :96, UUIDFilter at :95
RESOLVED — viewsets/organization.py:217 — last-active-admin guard (praise)
RESOLVED — tests/viewsets/test_organization.py:606 — filter_delete_queryset admin/editor split now covered by the queryset-level tests at :665-670
ACKNOWLEDGED — tests/viewsets/test_organization.py:702 — ORGANIZATION_ROLE_STATUS_PENDING coverage (praise)
UNADDRESSED — models.py:1306 — filter_view_queryset ignores organization__deleted
UNADDRESSED — models.py:1233 — filter_edit_queryset ignores organization__deleted
UNADDRESSED — models.py:1243 — ChannelViewSet.get_queryset (viewsets/channel.py:517-519) re-annotates edit/view from the m2m only
UNADDRESSED — viewsets/organization.py:294 — update duplicates RESTUpdateModelMixin.update (base.py:810)
UNADDRESSED — viewsets/organization.py:214 — perform_create identical to CreateModelMixin.perform_create (base.py:713)
UNADDRESSED — viewsets/channel.py:487 — destroy's denial branch and org-admin allow path still unexercised; the tests meant to cover them 404
UNADDRESSED — viewsets/base.py:763 — delete_from_changes swallows a queryset miss as success
UNADDRESSED — tests/viewsets/test_organization.py:650 — channel.save() is not permission-gated, so 644-645 and 655-656 assert nothing
UNADDRESSED — tests/viewsets/test_organization.py:676 — hardcoded trailing-slash URL (new this round)
@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
| self.authenticate_as(self.editor_user) | ||
|
|
||
| response = self.client.delete( | ||
| "/api/channel/{}/".format(self.channel.id) |
There was a problem hiding this comment.
blocking: this URL 404s, so neither this test nor test_org_admin_can_delete_channel_via_api (:687) reaches ChannelViewSet.destroy. The router is DefaultRouter(trailing_slash=False) (urls.py:79) — the detail route is /api/channel/<id>, no trailing slash.
Reproduced locally: 404 != 403 here and 404 != 204 at :687, with Not Found: /api/channel/<id>/ on stderr — URL resolution, not get_object_or_404.
Use reverse("channel-detail", kwargs={"pk": self.channel.id}) at both call sites, as test_channel.py:901 and membership_detail_url/organization_detail_url in this file do. With that swap the whole class passes, so the permission logic under test is correct.
| "/api/channel/{}/".format(self.channel.id) | ||
| ) | ||
|
|
||
| self.assertEqual(response.status_code, status.HTTP_204_NO_CONTENT) |
There was a problem hiding this comment.
praise: asserting the status code through the endpoint plus refresh_from_db() on deleted pins the get_edit_object() → get_delete_queryset() sequencing in destroy, which a queryset-only test cannot distinguish from a 404.
4 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.