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 18:49 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.
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #6080 — 24 of 33 prior findings resolved; 9 still open (1 blocking, 8 suggestions).
The only new commit is [pre-commit.ci lite] formatter output — no behaviour change, and it removes drift rather than adding it. No checks reported on the branch. The nine open items are all pre-existing threads whose code is unchanged.
- blocking
tests/viewsets/test_organization.py:674,:683—/api/channel/<id>/404s underDefaultRouter(trailing_slash=False); see inline. - suggestion
models.py:1235,:1272,:1308— the org subqueries filterstatusbut notorganization__deleted, so roles keep conferring channel access after an org is soft-deleted. - suggestion
viewsets/channel.py:518—get_querysetannotatesedit/viewfrom the m2m tables alone, so an org admin seesedit: falseon channels the write endpoints accept from them. - suggestion
viewsets/organization.py:215—perform_createis identical toCreateModelMixin.perform_create(base.py:713). - suggestion
viewsets/organization.py:295—updatecopiesRESTUpdateModelMixin.update; nothing names or tests the reason it bypassesserialize_object(). - suggestion
viewsets/base.py:763—delete_from_changesreturns success for an org editor whose deletechannel.py:488refuses. - suggestion
tests/viewsets/test_organization.py:645-662—channel.save()is not permission-gated, so those update assertions pass for anyone.
Prior-finding status
RESOLVED — viewsets/organization.py:53 — 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 get_queryset(); self-deactivation 404s
RESOLVED — viewsets/organization.py:142 — use model-level filter_view_queryset/filter_edit_queryset
RESOLVED — viewsets/organization.py:264 — select_for_update(of=("self",)) lock order
RESOLVED — tests/viewsets/test_organization.py:26 — 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:1304 — org roles grant channel view but never channel edit
RESOLVED — viewsets/organization.py — commented-out old get_queryset/get_edit_queryset bodies
RESOLVED — models.py:1252 — delete grant one role too wide
RESOLVED — models.py:1234 — no coverage for the org channel grant
RESOLVED — models.py:1255 — filter_delete_queryset had no callers
RESOLVED — tests/viewsets/test_organization.py — expected 403 is now 400
RESOLVED — viewsets/organization.py:96 — ?user=abc → 500; use NumberFilter/UUIDFilter
RESOLVED — viewsets/channel.py:487 — delete-denial and org-admin allow paths untested
RESOLVED — tests/viewsets/test_organization.py:605 — channel delete untested
RESOLVED — tests/viewsets/test_organization.py:374 — praise: membership-create negative paths
RESOLVED — viewsets/organization.py:217 — praise: last-active-admin guard
RESOLVED — viewsets/base.py:601 — praise: filter_delete_queryset fallback
RESOLVED — tests/viewsets/test_organization.py:697 — praise: pending-status coverage
ACKNOWLEDGED — tests/viewsets/test_organization.py:685 — praise: delete asserted through the endpoint
UNADDRESSED — tests/viewsets/test_organization.py:674 — delete URL has a trailing slash and 404s
UNADDRESSED — models.py:1235 — organization_edit ignores organization__deleted
UNADDRESSED — models.py:1308 — organization_view ignores organization__deleted
UNADDRESSED — models.py:1245 — ChannelViewSet.get_queryset re-annotates edit/view from the m2m only
UNADDRESSED — viewsets/organization.py:215 — perform_create duplicates base.py:713
UNADDRESSED — viewsets/organization.py:295 — update copies RESTUpdateModelMixin.update (raised twice)
UNADDRESSED — viewsets/base.py:763 — delete_from_changes silently succeeds on a delete-permission miss
UNADDRESSED — tests/viewsets/test_organization.py:649 — channel.save() assertions are not permission-gated
@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 test_org_editor_delete_channel_returns_forbidden(self): | ||
| self.authenticate_as(self.editor_user) | ||
|
|
||
| response = self.client.delete("/api/channel/{}/".format(self.channel.id)) |
There was a problem hiding this comment.
blocking: (still open) the formatter collapsed this call onto one line but the URL is unchanged — /api/channel/<id>/ still 404s, here and at :683, so neither test reaches ChannelViewSet.destroy. urls.py:78 is DefaultRouter(trailing_slash=False), so the detail route is /api/channel/<id> with no trailing slash. Use reverse("channel-detail", kwargs={"pk": self.channel.id}) at both call sites, as test_channel.py:901 does; the permission logic under test passes once the URL resolves.
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #6080 — 16 prior findings resolved (5 praise acknowledged); 11 still open, plus 1 new blocking finding.
a3c0a10 committed the raw merge conflict in test_organization.py, so the module fails to import and none of the organization coverage runs — inline. That also leaves the earlier channel.py:487 and channel-delete findings unverified: the tests exist but never execute.
The prior suggestion threads are unchanged and stay open in place (organization__deleted not filtered in the Channel grants, the update/perform_create overrides, delete_from_changes, the _editable_channel assertions).
The rest of the delta is pre-commit.ci reformatting, but it reverted three files to an older black style (base.py:188, channel.py:1178, models.py:2050) — the bot appears to run a different black version than the repo pins. Worth reconciling before the next push. gh pr checks also reports no checks on a3c0a10, so there is no CI signal for this head.
Prior-finding status
RESOLVED — viewsets/organization.py:53 — 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:142 — use model-level filter_view_queryset/filter_edit_queryset
RESOLVED — viewsets/organization.py:264 — select_for_update(of=("self",)) and lock ordering
RESOLVED — viewsets/organization.py:16 — unused ORGANIZATION_ADMIN/ORGANIZATION_ROLE_STATUS_ACTIVE imports
RESOLVED — viewsets/organization.py — commented-out get_queryset/get_edit_queryset bodies
RESOLVED — viewsets/organization.py — ?user=abc → 500; now NumberFilter/UUIDFilter (:96-97)
RESOLVED — models.py — User.filter_edit_queryset replaced with Channel-shaped logic
RESOLVED — models.py:1304 — org roles grant channel view but not edit
RESOLVED — models.py:1252 — delete grant one role too wide
RESOLVED — models.py:1234 — no coverage for the org edit grant
RESOLVED — models.py:1255 — filter_delete_queryset has no callers
RESOLVED — tests/viewsets/test_organization.py:26 — tests/viewsets/ + StudioAPITestCase convention
RESOLVED — tests/viewsets/test_organization.py — 403 is now 400
ACKNOWLEDGED — viewsets/organization.py:217 — praise, last-active-admin guard
ACKNOWLEDGED — viewsets/base.py:601 — praise, filter_delete_queryset fallback
ACKNOWLEDGED — tests/viewsets/test_organization.py:374 — praise, membership-create negative paths
ACKNOWLEDGED — tests/viewsets/test_organization.py:697 — praise, status code plus refresh_from_db()
ACKNOWLEDGED — tests/viewsets/test_organization.py:709 — praise, ORGANIZATION_ROLE_STATUS_PENDING coverage
UNADDRESSED — viewsets/channel.py:487 — denial branch and org-admin allow path untested; tests written but the module does not parse
UNADDRESSED — tests/viewsets/test_organization.py:605 — channel delete untested; same cause
UNADDRESSED — tests/viewsets/test_organization.py:679 — delete URL fix sits inside the conflict markers, so it is not in effect
UNADDRESSED — tests/viewsets/test_organization.py — /api/channel/<id>/ 404s in both delete tests
UNADDRESSED — tests/viewsets/test_organization.py:649 — .get() on 640 is the whole assertion; 644-645/655-656 add nothing
UNADDRESSED — models.py:1235 — filters status but not organization__deleted
UNADDRESSED — models.py:1245 — ChannelViewSet.get_queryset re-annotates edit/view from the m2m
UNADDRESSED — models.py:1308 — filters OrganizationRole.status but not organization__deleted
UNADDRESSED — viewsets/organization.py:215 — perform_create identical to CreateModelMixin.perform_create
UNADDRESSED — viewsets/organization.py:295 — update is RESTUpdateModelMixin.update verbatim bar the requery
UNADDRESSED — viewsets/base.py:763 — delete_from_changes swallows a queryset miss as success
@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 test_org_editor_delete_channel_returns_forbidden(self): | ||
| self.authenticate_as(self.editor_user) | ||
|
|
||
| <<<<<<< HEAD |
There was a problem hiding this comment.
blocking: a3c0a10 landed the raw merge conflict — markers at 674-680 and again at 689-695. ast.parse on the file fails at 680 (SyntaxError: invalid decimal literal), so collection aborts and none of the ~750 lines of organization coverage runs, including the two channel-delete permission tests meant to close the earlier channel.py:487 and delete-untested findings.
Keep the reverse("channel-detail", ...) side; it matches tests/viewsets/test_channel.py:901 and the router registration at urls.py:80. Then run pytest contentcuration/contentcuration/tests/viewsets/test_organization.py and confirm the two API-level delete tests pass, not just collect.
|
Hi @nairaj2 -- still one blocking rtibblesbot comment, but hopefully an easy fix. Also, from my side, before I do a final review here, I just want to raise this comment on Slack from Richard (which was a general comment to the TCM team, not to you specifically)
Since there has been some iterative review here, we've drifted a bit in the commit history. As a general rule, we are more than happy for you to "rewrite" history (and indeed prefer it) with an interactive rebase, If this is confusing, happy to discuss more either in the comments here or in slack! Thank you -- I know it takes a bit of extra effort but will go a long way with maintainability when this gets handed back to the core LE team. 🙏 |
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.