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-29 14:52 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.
✅ Resolved — addressed in the current code.
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.
✅ Resolved — addressed in the current code.
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
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
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
|
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. 🙏 |
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #6080 — 26 of 34 prior findings resolved or acknowledged; 8 suggestions still open.
Conflict markers gone; both delete tests use reverse("channel-detail", ...). CI passing.
Still open, unchanged, on existing threads:
models.py:1235,models.py:1308— role grants ignoreorganization__deletedmodels.py:1245—channel.py:518annotatesedit/viewfrom m2m onlyorganization.py:215— duplicatesCreateModelMixin.perform_createorganization.py:295— duplicatesRESTUpdateModelMixin.updatebase.py:763—delete_from_changestreats a miss as successtest_organization.py:649—channel.save()is not permission-gated
Prior-finding status
RESOLVED — viewsets/organization.py:53 — no create route; Invitation.accept() channel-only
RESOLVED — viewsets/organization.py — Channel filter querysets 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 — tests/viewsets/test_organization.py:26 — move to tests/viewsets/ + StudioAPITestCase
RESOLVED — viewsets/organization.py:16 — unused role constants
RESOLVED — models.py — User.filter_edit_queryset had Channel-shaped logic
RESOLVED — models.py:1304 — org roles granted view but never edit
RESOLVED — viewsets/organization.py — commented-out get_queryset bodies
RESOLVED — models.py:1252 — delete grant one role too wide
RESOLVED — models.py:1234 — channel view grant uncovered
RESOLVED — models.py:1255 — filter_delete_queryset had no callers
RESOLVED — tests/viewsets/test_organization.py — 403-vs-400 expectations
RESOLVED — viewsets/organization.py — ?user=abc 500; NumberFilter/UUIDFilter
RESOLVED — viewsets/channel.py:487 — delete-denial and org-admin paths untested
RESOLVED — tests/viewsets/test_organization.py:605 — channel delete untested
RESOLVED — tests/viewsets/test_organization.py — /api/channel/<id>/ 404s
RESOLVED — tests/viewsets/test_organization.py — same URL after formatter collapsed the call
RESOLVED — tests/viewsets/test_organization.py:674 — raw merge conflict markers
ACKNOWLEDGED — viewsets/organization.py:217 — last-active-admin guard (praise)
ACKNOWLEDGED — tests/viewsets/test_organization.py:701 — pending-role coverage (praise)
ACKNOWLEDGED — viewsets/base.py:601 — filter_delete_queryset fallback (praise)
ACKNOWLEDGED — tests/viewsets/test_organization.py:374 — membership-create negative paths (praise)
ACKNOWLEDGED — tests/viewsets/test_organization.py:689 — endpoint status + refresh_from_db() (praise)
UNADDRESSED — models.py:1235 — organization__deleted unfiltered on edit grant
UNADDRESSED — models.py:1245 — ChannelViewSet.get_queryset re-annotates from m2m
UNADDRESSED — models.py:1308 — organization__deleted unfiltered on view grant
UNADDRESSED — viewsets/organization.py:215 — perform_create duplicates the mixin
UNADDRESSED — viewsets/organization.py:295 — update duplicates RESTUpdateModelMixin.update
UNADDRESSED — viewsets/organization.py:295 — same, second thread
UNADDRESSED — viewsets/base.py:763 — delete_from_changes swallows a queryset miss
UNADDRESSED — tests/viewsets/test_organization.py:649 — channel.save() 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
0ae228b to
9a847db
Compare
There was a problem hiding this comment.
thank you, @nairaj2! The cleanup here on the commit history is nice, and you've addressed all of the feedback. Can you address the failing checks? then this will be ready to merge 
Thanks!
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #6080 — 27 of 33 prior findings resolved or acknowledged, 6 open, 3 new blocking. All 3 are the red CI (5 failures + Linting).
Organization.filter_*_querysetduplicated byunstableget_delete_querysetbreaksClipboardViewSetorganization/uservalidated beforeupdate()pops them
@marcellamaki approved 9a847dbc; these are new in the merged tree.
Prior-finding status
RESOLVED — viewsets/organization.py:53 — no create route; Invitation.accept()
RESOLVED — viewsets/organization.py — Channel.filter_edit_queryset/filter_view_queryset vs OrganizationRole
RESOLVED — viewsets/organization.py:8 — pre-commit
RESOLVED — viewsets/organization.py — serialize_object() 404s
RESOLVED — viewsets/organization.py:142 — model-level filter_*_queryset
RESOLVED — viewsets/organization.py:264 — select_for_update(of=("self",))
RESOLVED — tests/viewsets/test_organization.py:26 — StudioAPITestCase
RESOLVED — viewsets/organization.py:16 — unused imports
RESOLVED — models.py — User.filter_edit_queryset
RESOLVED — models.py:1304 — org roles never granted edit
RESOLVED — viewsets/organization.py — commented-out get_queryset
RESOLVED — models.py:1252 — delete grant too wide
RESOLVED — models.py:1234 — org channel grant uncovered
RESOLVED — models.py:1255 — filter_delete_queryset unused
RESOLVED — tests/viewsets/test_organization.py — 403 vs 400
RESOLVED — tests/viewsets/test_organization.py — channel delete URL 404s
RESOLVED — tests/viewsets/test_organization.py — conflict markers 674-695
RESOLVED — viewsets/organization.py — ?user=abc → 500
RESOLVED — viewsets/channel.py:487 — org-admin delete paths uncovered
RESOLVED — tests/viewsets/test_organization.py:605 — channel delete uncovered
ACKNOWLEDGED — viewsets/organization.py:217 — praise
ACKNOWLEDGED — viewsets/base.py:601 — praise
ACKNOWLEDGED — tests/viewsets/test_organization.py:374 — praise
ACKNOWLEDGED — tests/viewsets/test_organization.py:689 — praise
ACKNOWLEDGED — tests/viewsets/test_organization.py:701 — praise
ACKNOWLEDGED — viewsets/organization.py:295 — update() duplicates RESTUpdateModelMixin.update — withdrawn, its queryset is the serialize_object() fix I asked for
ACKNOWLEDGED — viewsets/organization.py:295 — duplicate thread — withdrawn
UNADDRESSED — models.py:1235 — organization__deleted
UNADDRESSED — models.py:1308 — organization__deleted
UNADDRESSED — models.py:1245 — ChannelViewSet.get_queryset re-annotates
UNADDRESSED — viewsets/base.py:763 — delete_from_changes swallows a miss
UNADDRESSED — viewsets/organization.py:215 — perform_create
UNADDRESSED — tests/viewsets/test_organization.py:649 — .get() is the assertion
@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
| objects = CustomManager() | ||
|
|
||
| @classmethod | ||
| def filter_view_queryset(cls, queryset, user): |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
blocking: unstable added its own Organization.filter_edit_queryset/filter_view_queryset at models.py:1899/1914 (059ecefa1). This pair sits above class Meta, unstable's below __str__, so the merge is conflict-free and yields four defs in one class body — unstable's win.
Both CI failures: F811 at merged 2002/2017 (Linting), and test_authenticated_nonmember_can_list_public_organization / test_nonmember_can_retrieve_public_organization, since unstable's filter_view_queryset has no public=True branch.
Merge unstable and reconcile to one pair — unstable's needs an active role and returns none() for anonymous; this one serves public=True orgs to anonymous. Pick deliberately.
| Return a filtered copy of the queryset to only the objects | ||
| that a user is able to delete. | ||
| """ | ||
| queryset = super(BaseValuesViewset, self).get_queryset() |
There was a problem hiding this comment.
blocking: GenericAPIView.get_queryset asserts self.queryset is not None. ClipboardViewSet (clipboard.py:105) has no class queryset and overrides get_edit_queryset() (clipboard.py:131) for exactly that reason. Routing delete_from_changes (763, 944) through here raises before the fallback — test_delete_clipboard, test_delete_clipboards.
Take the model off the edit queryset:
def get_delete_queryset(self):
queryset = self.get_edit_queryset()
if hasattr(queryset.model, "filter_delete_queryset"):
return queryset.model.filter_delete_queryset(
super(BaseValuesViewset, self).get_queryset(), self.request.user
)
return queryset| or status. Read operations are handled by the viewset values map. | ||
| """ | ||
|
|
||
| organization = UserFilteredPrimaryKeyRelatedField( |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
blocking: validation runs before update() (79-84) pops these. UserFilteredPrimaryKeyRelatedField defaults to edit=True (common.py:258), so organization resolves against Organization.filter_edit_queryset — any other id gives Invalid pk, a 400. That is test_membership_user_and_organization_cannot_be_reassigned.
Set read_only on both fields in __init__ when self.instance is not None (or split an update serializer); the pops then go. Unrelated to the merge — unstable's filter_edit_queryset rejects it too.
| ) | ||
| queryset = queryset.annotate(edit=edit) | ||
| organization_edit = Exists( | ||
| OrganizationRole.objects.filter( |
There was a problem hiding this comment.
suggestion: (still open) filters status but not organization__deleted, unlike Organization.filter_view_queryset (1961). A soft-deleted org keeps granting edit. Same at 1267, 1305.
| ), | ||
| ) | ||
| ) | ||
| queryset = queryset.annotate( |
There was a problem hiding this comment.
suggestion: (still open) ChannelViewSet.get_queryset (channel.py:518-521) re-annotates edit/view from the m2m alone. An org admin passes the filter but serializes as edit: false.
| 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: (still open) a miss is swallowed as success (769-772), so edit-but-not-delete is a silent no-op here while RESTDestroyModelMixin (781) 404s.
| ).strip() | ||
| return items | ||
|
|
||
| def perform_create(self, serializer, change=None): |
There was a problem hiding this comment.
suggestion: (still open) identical to CreateModelMixin.perform_create (base.py:713) — drop it.
| 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: (still open) channel.save() is not permission-gated, so the .get() on 645 is the whole test. Same at 656-662.
| self.assertEqual(response.status_code, status.HTTP_200_OK) | ||
| self.assertEqual(response.data["results"], []) | ||
|
|
||
| def test_public_organization_does_not_expose_memberships_to_nonmember(self): |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
praise: pins public=True widening org visibility without widening the roster.
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #6080 — 29/35 prior findings resolved, 6 open; see inline.
Prior-finding status
RESOLVED — viewsets/organization.py:53 — no create route; Invitation.accept() channel-only
RESOLVED — viewsets/organization.py — Channel.filter_*_queryset ignored OrganizationRole
RESOLVED — viewsets/organization.py:8 — pre-commit failed on head
RESOLVED — viewsets/organization.py — serialize_object() 404 after self-deactivation
RESOLVED — viewsets/organization.py:154 — use model-level filter_view/edit_queryset
RESOLVED — viewsets/organization.py:276 — select_for_update(of=("self",)) lock order
RESOLVED — tests/viewsets/test_organization.py:26 — tests/viewsets/ + StudioAPITestCase
RESOLVED — viewsets/organization.py:16 — unused-import claim on org constants
RESOLVED — models.py — User.filter_edit_queryset had Channel-shaped logic
RESOLVED — models.py:1304 — org roles granted view but never edit
RESOLVED — viewsets/organization.py — commented-out get_queryset bodies
RESOLVED — viewsets/organization.py:307 — duplicated RESTUpdateModelMixin.update
RESOLVED — tests/viewsets/test_organization.py:374 — membership-create negative paths (praise)
RESOLVED — models.py:1252 — edit grant one role too wide on delete
RESOLVED — models.py:1234 — no coverage for the org edit grant
RESOLVED — models.py:1255 — filter_delete_queryset had no callers
RESOLVED — viewsets/base.py:600 — get_edit_queryset() fallback (praise)
RESOLVED — tests/viewsets/test_organization.py — 403 is now 400
RESOLVED — tests/viewsets/test_organization.py — channel delete URL 404d
RESOLVED — tests/viewsets/test_organization.py:689 — delete asserted via endpoint (praise)
RESOLVED — tests/viewsets/test_organization.py — merge-conflict markers from a3c0a10
RESOLVED — viewsets/organization.py — ?user=abc 500; now NumberFilter/UUIDFilter
RESOLVED — viewsets/organization.py:229 — last-active-admin guard (praise)
RESOLVED — viewsets/channel.py:487 — denial and allow paths now tested
RESOLVED — tests/viewsets/test_organization.py:605 — delete now tested
RESOLVED — tests/viewsets/test_organization.py:701 — PENDING coverage (praise)
RESOLVED — models.py:1960 — duplicate Organization.filter_edit_queryset gone
RESOLVED — viewsets/organization.py:59 — validation ran before update() popped; get_fields() now
RESOLVED — tests/viewsets/test_organization.py:299 — public=True widening (praise)
UNADDRESSED — viewsets/base.py:605 — GenericAPIView.get_queryset assert
UNADDRESSED — models.py:1235 — organization__deleted not filtered
UNADDRESSED — models.py:1245 — ChannelViewSet.get_queryset re-annotates edit/view
UNADDRESSED — viewsets/base.py:762 — delete-queryset miss swallowed as success
UNADDRESSED — viewsets/organization.py:227 — perform_create duplicates base
UNADDRESSED — tests/viewsets/test_organization.py:649 — channel.save() 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
| validated_data[ | ||
| relation_info.model_field.attname | ||
| ] = validated_data.pop(field_name) | ||
| validated_data[relation_info.model_field.attname] = ( |
There was a problem hiding this comment.
blocking: .pre-commit-config.yaml:95 pins black==20.8b1, which reverts this:
- validated_data[relation_info.model_field.attname] = (
- validated_data.pop(field_name)
- )
+ validated_data[
+ relation_info.model_field.attname
+ ] = validated_data.pop(field_name)Revert 187-192 and the blank at 40.
| Return a filtered copy of the queryset to only the objects | ||
| that a user is able to delete. | ||
| """ | ||
| queryset = super(BaseValuesViewset, self).get_queryset() |
There was a problem hiding this comment.
blocking: (still open) GenericAPIView.get_queryset asserts self.queryset is not None; ClipboardViewSet (clipboard.py:105) sets none — hence its get_edit_queryset override (clipboard.py:131). Clipboard deletes (sync/base.py:71,94) now hit this → AssertionError. Gate it:
def get_delete_queryset(self):
queryset = self.get_edit_queryset()
if hasattr(queryset.model, "filter_delete_queryset"):
return queryset.model.filter_delete_queryset(
super(BaseValuesViewset, self).get_queryset(), self.request.user
)
return queryset| ) | ||
| queryset = queryset.annotate(edit=edit) | ||
| organization_edit = Exists( | ||
| OrganizationRole.objects.filter( |
There was a problem hiding this comment.
suggestion: (still open) filters status, not organization__deleted (cf. Organization.filter_view_queryset); deleted orgs keep granting edit. Also 1266, 1305.
| ), | ||
| ) | ||
| ) | ||
| queryset = queryset.annotate( |
There was a problem hiding this comment.
suggestion: (still open) ChannelViewSet.get_queryset (channel.py:518-521) re-annotates edit/view from the m2m, so org-only members serialize edit: false.
| 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: (still open) a queryset miss is swallowed as success (769-772) — rejected deletes go silent.
| ).strip() | ||
| return items | ||
|
|
||
| def perform_create(self, serializer, change=None): |
There was a problem hiding this comment.
suggestion: (still open) duplicates CreateModelMixin.perform_create (base.py:712).
| 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: (still open) channel.save() is not permission-gated — the .get() on 645 is the whole test. Same below.
| list_serializer_class = BulkListSerializer | ||
|
|
||
| def get_fields(self): | ||
| fields = super(OrganizationMemberSerializer, self).get_fields() |
There was a problem hiding this comment.
praise: read_only in get_fields() — never reaches UserFilteredPrimaryKeyRelatedField, deepcopied per request.
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.