Conversation
|
👋 Hi @yasinelmi, 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. |
93c0a0b to
67dc56c
Compare
|
📢✨ Before we assign a reviewer, we'll turn on |
🟡 Waiting for changesLast updated: 2026-09-21 20:39 UTC |
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #6146 makes every channel a user can edit also appear in the View-only list. The view annotation ORs in edit (channel.py:526). That is the failing ChannelListTestCase.test_no_viewable_channels.
- blocking: drop
| Q(edit=True)from theviewannotation. Org edit roles are a subset of org view roles, so the disjunct is redundant anyway. - suggestion: move the combined
editannotation into the model layer, so callers other than this viewset see it too. - suggestion: move the role tuples into
constants/organization_roles.py; they are spelled out in five places. - suggestion: extend the new tests — they cover
ContentNodeonly, with no negative API case, nothing pinningview, and no/api/sync/case.
CI failing. No UI files changed, so no visual verification.
@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
| edit=Exists(user_queryset.filter(editable_channels=OuterRef("id"))), | ||
| view=Exists(user_queryset.filter(view_only_channels=OuterRef("id"))), | ||
| view=ExpressionWrapper( | ||
| Q(view=True) | Q(edit=True) | Q(organization_view=True), |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
blocking: Every channel a user can edit is now also listed under View-only. | Q(edit=True) widens view, which previously meant strictly "is in the view_only_channels m2m". ChannelFilter.filter_view filters on this annotation. That is the CI failure: ChannelListTestCase.test_no_viewable_channels asserts 0 results and gets 1. serialize_object goes through the same get_queryset, so view: true is also written into the client's IndexedDB copy on every channel create or update.
The disjunct is redundant regardless. organization_edit roles (admin, editor) are a strict subset of organization_view roles (admin, editor, viewer), so org editors already get view=true from Q(organization_view=True).
Please drop | Q(edit=True). If view is instead meant to read "can view but cannot edit", make that explicit: (Q(view=True) | Q(organization_view=True)) & Q(edit=False).
| ) | ||
|
|
||
| # Fold organization grants into edit/view instead of overwriting them. | ||
| queryset = queryset.annotate( |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: The combined "can edit this channel" rule exists only on this code path. filter_view_queryset already computes edit and organization_edit separately. Every other consumer of it still sees the m2m-only edit.
Annotate the combined expression in the model layer instead. That keeps one definition and lets this override go away. filter_delete_queryset would keep its own annotation, since it deliberately leaves edit m2m-only so org editors don't gain delete.
| User.editable_channels.through, user_id | ||
| ).union( | ||
| cls._organization_channels( | ||
| user_id, (ORGANIZATION_ADMIN, ORGANIZATION_EDITOR) |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: The role tuples (ADMIN, EDITOR) and (ADMIN, EDITOR, VIEWER) are now duplicated across five sites (models.py:1056, 1068, 1282, 1352, 1364). Adding a role means finding all five, and a miss silently diverges edit from view.
Please move them to constants/organization_roles.py, which already owns the role names, as ORGANIZATION_CHANNEL_EDIT_ROLES and ORGANIZATION_CHANNEL_VIEW_ROLES. The four Exists(OrganizationRole...) blocks in Channel differ only by that tuple, so they could also collapse into one helper.
| "{}__tree_id".format(tree_name) for tree_name in CHANNEL_TREES | ||
| ] | ||
|
|
||
| def __init__(self, queryset, **kwargs): |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
nitpick: This __init__ override is identical to the inherited With.__init__. Delete it — PermissionCTE(queryset, name=...) keeps working unchanged.
| cls._organization_channels( | ||
| user_id, (ORGANIZATION_ADMIN, ORGANIZATION_EDITOR) | ||
| ), | ||
| all=True, |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
praise: all=True is the right call here. PermissionCTE.exists() is the only consumer and wraps the CTE in EXISTS, so the duplicate rows from a user holding both a personal and an org grant can't multiply results. Skipping the DISTINCT keeps the hot ContentNode permission query cheap.
| ) | ||
|
|
||
| self.assertEqual(response.status_code, status.HTTP_200_OK) | ||
| self.assertTrue(response.data["edit"]) |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: The new tests cover ContentNode only, and none of them assert on view. Gaps worth closing while this is open:
- Add a negative API case: an org viewer on
channel-detailshould getedit=False,view=True. - Pin
view: a personal editor with no org role must not be listed by?view=true. Nothing intests/viewsets/test_channel.pyasserts oneditorvieweither, which is why the blocking finding above passes this suite. - Cover
FileandAssessmentItem, which route through the same CTE.File.filter_edit_querysethas an extraQ(uploaded_by=user, ...)branch that interacts with the CTE result. - Cover
/api/sync/. The reported symptom was a sync rejection, so POST an org editor'sContentNodeupdate through that endpoint. - Cover
GET /api/channel/?edit=true. The bug named that filter path, andchannel-detailshares the annotation but not the filter.
67dc56c to
499cdb7
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #6146: 1 of 5 prior findings resolved. Four still stand, and the edit fix adds one new issue.
CI: all six workflows green on run 35632618377. No UI files changed, so visual verification and manual QA do not apply.
- suggestion — Organization editors now see a "Delete channel" menu item that
ChannelViewSet.destroyrefuses (channel.py:520, inline). Annotate a separatedeletevalue, or file a follow-up before merging. - Three prior suggestions and one nitpick stand on unchanged code; see the inline threads.
Prior-finding status
RESOLVED — contentcuration/contentcuration/viewsets/channel.py:526 — view annotation ORs in edit
UNADDRESSED — contentcuration/contentcuration/viewsets/channel.py:519 — combined edit rule belongs in the model layer
UNADDRESSED — contentcuration/contentcuration/models.py:1056 — role tuples duplicated across five sites
UNADDRESSED — contentcuration/contentcuration/models.py:1013 — redundant PermissionCTE.__init__ override
UNADDRESSED — contentcuration/contentcuration/tests/viewsets/test_organization.py:730 — no view, File, AssessmentItem, /api/sync/, or ?edit=true coverage
ACKNOWLEDGED — contentcuration/contentcuration/models.py:1058 — praise for all=True
@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
|
|
||
| # Fold organization grants into edit/view instead of overwriting them. | ||
| queryset = queryset.annotate( | ||
| edit=ExpressionWrapper( |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: Organization editors now see a "Delete channel" menu item that the API refuses.
edit: true makes ChannelItem.vue render the delete branch instead of "remove viewer". ChannelViewSet.destroy still requires a personal edit grant or ORGANIZATION_ADMIN. The click therefore fails with PermissionDenied. Before this PR these users had edit: false and never reached that branch.
Annotate a separate delete value from filter_delete_queryset's rule. Add it to ChannelViewSet.values. Gate ChannelItem's delete branch on it. If that is out of scope here, file a follow-up before merging — the affordance regression ships with the fix.
| ) | ||
|
|
||
| # Fold organization grants into edit/view instead of overwriting them. | ||
| queryset = queryset.annotate( |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: (unaddressed) This annotation keeps a second copy of the "can edit this channel" rule. filter_edit_queryset already owns that rule for every other consumer. The two will drift when organization roles change. Move the combined rule into the model layer and annotate from it.
| edit=Exists(user_queryset.filter(editable_channels=OuterRef("id"))), | ||
| view=Exists(user_queryset.filter(view_only_channels=OuterRef("id"))), | ||
| view=ExpressionWrapper( | ||
| Q(view=True) | Q(organization_view=True), |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
praise: Deriving view from org roles alone is the right resolution. organization_edit's roles are a subset of organization_view's, so org editors keep view=true without dragging every personally-editable channel into the View-only list.
| User.editable_channels.through, user_id | ||
| ).union( | ||
| cls._organization_channels( | ||
| user_id, (ORGANIZATION_ADMIN, ORGANIZATION_EDITOR) |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: (unaddressed) (ADMIN, EDITOR) and (ADMIN, EDITOR, VIEWER) are spelled out across five sites. Name them once as module constants, so a future role addition is a one-line change.
| "{}__tree_id".format(tree_name) for tree_name in CHANNEL_TREES | ||
| ] | ||
|
|
||
| def __init__(self, queryset, **kwargs): |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
nitpick: (unaddressed) This override is identical to the inherited With.__init__. PermissionCTE(queryset, name=...) already works without it. Drop it.
| ) | ||
|
|
||
| self.assertEqual(response.status_code, status.HTTP_200_OK) | ||
| self.assertTrue(response.data["edit"]) |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: (unaddressed) The new tests cover ContentNode only. None of them assert on view. Please pin the uncovered paths while the code is fresh:
- an org viewer getting
view=trueandedit=false FileandAssessmentItemaccess- a write through
/api/sync/ - the
?edit=truechannel-list filter the issue named
499cdb7 to
155ece7
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #6146: 9 of 12 prior findings settled, 3 still open — test coverage and the delete affordance. CI pending at review time.
The union CTE and the model-layer edit/view fold compile to correct SQL. Generated against Django 3.2 / django-cte 1.3.3, the UNION ALL parts come out parenthesised and column-aligned. ExpressionWrapper(Q(...) | Q(...)) renders as (EXISTS(...) OR EXISTS(...)).
Four inline suggestions:
- Org editors' channels land in both My Channels and View-only.
organization_edit/organization_vieware dead after the fold.- The view side of
PermissionCTEhas no test. - The delete-affordance regression is unaddressed.
Prior-finding status
RESOLVED — contentcuration/contentcuration/viewsets/channel.py — | Q(edit=True) widened view to every editable channel
ACKNOWLEDGED — contentcuration/contentcuration/models.py:1056 — praise: all=True on the CTE union
RESOLVED — contentcuration/contentcuration/viewsets/channel.py — second copy of the "can edit this channel" rule in get_queryset (id=4057836587)
RESOLVED — contentcuration/contentcuration/viewsets/channel.py — same, re-raised (id=4064908644)
RESOLVED — contentcuration/contentcuration/models.py — role tuples spelled out across five sites (id=4057836589)
RESOLVED — contentcuration/contentcuration/models.py — same, re-raised (id=4064908658)
RESOLVED — contentcuration/contentcuration/models.py — redundant PermissionCTE.__init__ override (id=4057836591)
RESOLVED — contentcuration/contentcuration/models.py — same, re-raised (id=4064908664)
ACKNOWLEDGED — contentcuration/contentcuration/viewsets/channel.py — praise: deriving view from org roles alone (id=4064908650)
UNADDRESSED — contentcuration/contentcuration/tests/viewsets/test_organization.py:733 — File/AssessmentItem and view-side coverage gaps (id=4057836598)
UNADDRESSED — contentcuration/contentcuration/tests/viewsets/test_organization.py:733 — same, re-raised (id=4064908670)
UNADDRESSED — contentcuration/contentcuration/viewsets/channel.py — org editors see a "Delete channel" item the API refuses (id=4064908638)
@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
| ), | ||
| ).annotate( | ||
| view=ExpressionWrapper( | ||
| Q(view=True) | Q(organization_view=True), |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: An org editor sees every org channel under both My Channels and View-only. ORGANIZATION_CHANNEL_VIEW_ROLES includes admin and editor, so this sets view=True on channels the same user can edit. Both the API filter (viewsets/channel.py:262) and the channel list key straight off that flag.
Last round you dropped | Q(edit=True) from view and pinned it with test_personal_editor_without_org_role_excluded_from_view_only_filter. That test states the invariant as "editable ⇒ not view-only". The org path breaks the same invariant.
Give the channel API's view a viewer-only role set (or view AND NOT edit). Leave PermissionCTE.view_only_channels on all three roles — content viewing genuinely needs editors and admins. If the overlap is intended instead, add a test asserting an org editor's channel does appear under ?view=true.
| queryset = queryset.annotate( | ||
| edit=edit, | ||
| view=view, | ||
| organization_edit=organization_edit, |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: The organization_edit and organization_view annotations are dead after the fold. The permission_filter below no longer references them, and ChannelViewSet.values doesn't list them. A repo-wide grep finds only these definitions. They still land in the SELECT list for every non-values() consumer of filter_view_queryset, duplicating both OrganizationRole EXISTS subqueries per row.
Build the wrappers straight from the expressions instead — same SQL, minus the intermediates:
edit = ExpressionWrapper(
Q(Exists(User.editable_channels.through.objects.filter(**filters)))
| Q(cls._organization_role_exists(user_id, ORGANIZATION_CHANNEL_EDIT_ROLES)),
output_field=models.BooleanField(),
)That also removes the need for the second chained .annotate().
| ContentNode.filter_edit_queryset( | ||
| ContentNode.objects.filter(pk=node.pk), self.viewer_user | ||
| ).exists() | ||
| ) |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: (partly unaddressed) No test exercises the view side of PermissionCTE. Channel.filter_view_queryset doesn't go through the CTE, and no test pairs an org viewer with a ContentNode queryset. view_only_channels could return nothing and the suite would still pass. File and AssessmentItem from last round are still uncovered too.
Add the mirror of the edit test to close the main gap:
def test_org_viewer_can_view_channel_content(self):
node = self.channel.main_tree
self.assertTrue(
ContentNode.filter_view_queryset(
ContentNode.objects.filter(pk=node.pk), self.viewer_user
).exists()
)| instance.save(update_fields=["deleted"], actor_id=self.request.user.id) | ||
|
|
||
| def get_queryset(self): | ||
| # edit/view are already annotated here with organization grants folded |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: (unaddressed) Org editors see a "Delete channel" item that the API refuses. edit: true here makes ChannelItem.vue render the delete branch instead of "remove viewer". ChannelViewSet.destroy still requires a personal edit grant or ORGANIZATION_ADMIN, so the click fails with PermissionDenied.
Annotate a separate delete value from filter_delete_queryset's rule, add it to ChannelViewSet.values, and gate the delete branch on it. If that's out of scope, file a follow-up before merge.
|
|
||
| self.assertEqual(response.status_code, 200, response.content) | ||
| node.refresh_from_db() | ||
| self.assertEqual(node.title, new_title) |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
praise: Asserting on the persisted node.title rather than the 200 alone is what makes this test bite. /api/sync/ returns 200 with per-change errors for rejected changes — exactly how #6123 failed.
155ece7 to
7fa48ee
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #6146 — three of the new tests error before asserting anything, so the Python suite fails. Inline at test_organization.py:761.
The permission logic itself is correct. I ran the model and affected-viewset permission tests against Postgres; 367 passed.
11 of 13 prior findings are resolved. One is acknowledged. One is still open — org editors see a delete affordance the API refuses. One new suggestion on ORGANIZATION_CHANNEL_VIEW_ROLES reverses a request I made last round.
Prior-finding status
RESOLVED — contentcuration/contentcuration/viewsets/channel.py — | Q(edit=True) widened view, listing every editable channel as view-only
RESOLVED — contentcuration/contentcuration/viewsets/channel.py — combined "can edit this channel" rule existed only on this code path
RESOLVED — contentcuration/contentcuration/viewsets/channel.py — annotation kept a second copy of the "can edit this channel" rule
RESOLVED — contentcuration/contentcuration/viewsets/channel.py — deriving view from org roles alone is the right resolution (praise)
RESOLVED — contentcuration/contentcuration/models.py — role tuples (ADMIN, EDITOR) / (ADMIN, EDITOR, VIEWER) duplicated across five sites
RESOLVED — contentcuration/contentcuration/models.py — PermissionCTE.__init__ override identical to inherited With.__init__
RESOLVED — contentcuration/contentcuration/models.py:1056 — all=True is the right call for the union (praise)
RESOLVED — contentcuration/contentcuration/models.py — org editor listed under both My Channels and View-only; channel API view is now viewer-only
RESOLVED — contentcuration/contentcuration/models.py — dead organization_edit / organization_view annotations in filter_view_queryset
RESOLVED — contentcuration/contentcuration/tests/viewsets/test_organization.py:723 — view side of PermissionCTE now covered
RESOLVED — contentcuration/contentcuration/tests/viewsets/test_organization.py:804 — sync test asserts the persisted value (praise)
ACKNOWLEDGED — contentcuration/contentcuration/tests/viewsets/test_organization.py:741 — test gaps; File and AssessmentItem remain uncovered
UNADDRESSED — contentcuration/contentcuration/viewsets/channel.py:505 — org editors see a delete affordance the API refuses
@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.assertEqual(response.status_code, status.HTTP_200_OK) | ||
| self.assertNotIn( | ||
| str(self.channel.id), [result["id"] for result in response.data["results"]] |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
blocking: This test errors before it asserts anything. ChannelListPagination.page_size is None, so /api/channel/ returns an unpaginated list unless page_size is passed. response.data is therefore a list. Indexing it as response.data["results"] raises TypeError: list indices must be integers or slices, not str.
Same fault at :771 and :783. These are the three tests covering the ?edit=true / ?view=true acceptance criteria, so nothing currently verifies them. The production behaviour they target is right — I ran all three with the indexing removed and all three pass.
Iterate response.data directly instead of indexing ["results"].
|
|
||
| # Organization roles that grant edit/view access to the organization's channels. | ||
| ORGANIZATION_CHANNEL_EDIT_ROLES = (ORGANIZATION_ADMIN, ORGANIZATION_EDITOR) | ||
| ORGANIZATION_CHANNEL_VIEW_ROLES = ( |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: Listing admin and editor here is redundant — this should be viewer-only. That reverses a request I made last round, where I said content viewing genuinely needs editors and admins. I was wrong.
All three consumers of view_only_channels filter Q(view=True) | Q(edit=True) | Q(public=True), and each builds its edit CTE from the org edit roles. An org editor or admin is admitted by the edit disjunct either way. view is in no viewset's values, so nothing observes the difference.
That makes the comment at models.py:1332-1336 false — content-level access does not need the full role set. Dropping admin and editor leaves "view" meaning viewer-only at both layers. It also removes an org-channel scan from the view CTE.
Drop the two roles, or keep them and reword that comment to say redundant-but-symmetric.
| instance.save(update_fields=["deleted"], actor_id=self.request.user.id) | ||
|
|
||
| def get_queryset(self): | ||
| # edit/view are already annotated here with organization grants folded |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: (unaddressed) Org editors see a delete affordance the API refuses. edit: true makes ChannelItem.vue render the delete branch rather than "remove viewer". ChannelViewSet.destroy still requires a personal edit grant or ORGANIZATION_ADMIN, so the click fails with PermissionDenied. models.py:1305 now documents the admin-only delete rule, but nothing gates the affordance.
Annotate a separate delete value from filter_delete_queryset's rule, add it to ChannelViewSet.values, and gate the delete branch on it. If that's out of scope here, file a follow-up before merge.
7fa48ee to
a03ae6a
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #6146 — all 17 prior findings are settled: 11 fixed, 6 acknowledged. Two new suggestions inline. Nothing blocking.
I am withdrawing the "Delete channel" affordance suggestion I raised across three rounds. edit: true for org editors is the intended outcome of this PR. The menu item lives in a frontend file outside this diff — please open a follow-up issue for it rather than changing anything here.
CI was pending when this was written. Locally test_organization.py is green (61 passed). tests/viewsets/ plus test_models.py are green against Postgres (608 passed, 5 skipped). The SQL generated for all three permission querysets compiles as intended. No UI files changed, so no manual QA.
Prior-finding status
RESOLVED — contentcuration/contentcuration/viewsets/channel.py — | Q(edit=True) widened view, listing every editable channel as View-only
RESOLVED — contentcuration/contentcuration/viewsets/channel.py — combined "can edit this channel" rule lived only on this code path
RESOLVED — contentcuration/contentcuration/viewsets/channel.py — annotation kept a second copy of the "can edit this channel" rule
RESOLVED — contentcuration/contentcuration/models.py — role tuples (ADMIN, EDITOR) / (ADMIN, EDITOR, VIEWER) duplicated across five sites
RESOLVED — contentcuration/contentcuration/models.py — PermissionCTE.__init__ override identical to inherited With.__init__
RESOLVED — contentcuration/contentcuration/models.py — org editor appeared under both My Channels and View-only
RESOLVED — contentcuration/contentcuration/models.py — organization_edit / organization_view annotations dead after the fold
RESOLVED — contentcuration/contentcuration/constants/organization_roles.py — view role set should be viewer-only, not admin/editor/viewer
RESOLVED — contentcuration/contentcuration/tests/viewsets/test_organization.py:741 — new tests covered ContentNode only and never asserted on view
RESOLVED — contentcuration/contentcuration/tests/viewsets/test_organization.py:723 — no test exercised the view side of PermissionCTE
RESOLVED — contentcuration/contentcuration/tests/viewsets/test_organization.py — channel-list test errored before asserting on ChannelListPagination.page_size
ACKNOWLEDGED — contentcuration/contentcuration/models.py:1055 — praise: all=True is right for an EXISTS-only consumer
ACKNOWLEDGED — contentcuration/contentcuration/viewsets/channel.py — praise: deriving view from org roles alone is the right resolution
ACKNOWLEDGED — contentcuration/contentcuration/tests/viewsets/test_organization.py:802 — praise: asserting on the persisted node.title is what makes the sync test bite
ACKNOWLEDGED — contentcuration/contentcuration/viewsets/channel.py — withdrawn: org editors see a "Delete channel" menu item the API refuses
ACKNOWLEDGED — contentcuration/contentcuration/viewsets/channel.py:505 — withdrawn: same delete affordance, re-raised
ACKNOWLEDGED — contentcuration/contentcuration/viewsets/channel.py:505 — withdrawn: same delete affordance, re-raised
@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
| # layer, so every consumer of this queryset sees combined | ||
| # permissions rather than just the personal editor/viewer | ||
| # grants. The view role set is deliberately viewer-only: an | ||
| # org editor/admin already gets view=True from edit=True |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: This comment states something untrue: an org editor/admin does not get view=True. view is annotated viewer-only, so the channel API returns view: false for them. What admits them to the result set is the edit=True check.
The behaviour is right; only the comment is wrong. Please reword it the way constants/organization_roles.py:12-14 already puts it — "admitted through the edit check" — so no later reader takes "view ⊇ edit" as an invariant and builds on it.
| self.assertFalse(self._viewable_channel(self.pending_user).exists()) | ||
| self.assertFalse(self._editable_channel(self.pending_user).exists()) | ||
|
|
||
| def test_org_editor_can_edit_channel_content_without_m2m_share(self): |
There was a problem hiding this comment.
suggestion: The CTE's own status=ORGANIZATION_ROLE_STATUS_ACTIVE filter (models.py:1036) is untested. Delete that filter and the whole suite still passes. The regression it would let through is pending invitees gaining edit access to org channel content. test_pending_role_grants_no_channel_access pins the status check at the Channel layer only.
Please add a content-layer case alongside the new ContentNode tests — self.pending_user is already in setUp:
def test_pending_role_grants_no_channel_content_access(self):
node = self.channel.main_tree
self.assertFalse(
ContentNode.filter_view_queryset(
ContentNode.objects.filter(pk=node.pk), self.pending_user
).exists()
)| queryset=queryset.values("user_id", "channel_id", "tree_id"), **kwargs | ||
|
|
||
| @classmethod | ||
| def _organization_channels(cls, user_id, roles): |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
praise: Extending the CTE with a second UNION ALL branch of the same shape carries the org grant to ContentNode, AssessmentItem and File for free — eight existing call sites, zero edits. The emitted SQL checks out. Column order and types match across both branches. The single OrganizationRole join means role, status and user all have to be satisfied by the same row.
…nd the channel API PermissionCTE only read the personal editor/viewer m2m tables, so organization editors/viewers could not edit or view content nodes, assessment items, or files inside org-owned channels. Channel.filter_view_queryset also only exposed the personal-only edit/view checks, so the channel API reported edit: false for organization editors even though the channel itself was already correctly org-aware. PermissionCTE.editable_channels/view_only_channels now union the personal m2m grants with channels reachable via an active OrganizationRole. Channel.filter_view_queryset now folds organization grants directly into edit/view, so every consumer of that queryset sees combined permissions. Both layers treat organization viewing as viewer-only: an org editor/admin is already admitted through the edit check wherever permissions are checked, so including them in the view role set too would be redundant. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
a03ae6a to
4c56f39
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #6146: 17 of 18 prior findings resolved or acknowledged. One is still open. CI passing.
One new suggestion inline: a channel can now come back with both edit=true and view=true. That double-lists it across My Channels and View-only.
Prior-finding status
RESOLVED — contentcuration/contentcuration/viewsets/channel.py — | Q(edit=True) widens view
RESOLVED — contentcuration/contentcuration/viewsets/channel.py — combined "can edit this channel" rule lives only on this code path
RESOLVED — contentcuration/contentcuration/viewsets/channel.py — second copy of the "can edit this channel" rule
RESOLVED — contentcuration/contentcuration/models.py — (ADMIN, EDITOR) / (ADMIN, EDITOR, VIEWER) duplicated across five sites
RESOLVED — contentcuration/contentcuration/models.py — PermissionCTE.__init__ override identical to inherited With.__init__
RESOLVED — contentcuration/contentcuration/models.py — org editor sees every org channel under both My Channels and View-only
RESOLVED — contentcuration/contentcuration/models.py — dead organization_edit / organization_view annotations after the fold
RESOLVED — contentcuration/contentcuration/models.py:1328 — comment claimed org editors/admins get view=True
RESOLVED — contentcuration/contentcuration/constants/organization_roles.py — view role set should be viewer-only
RESOLVED — contentcuration/contentcuration/tests/viewsets/test_organization.py — list test errors on response.data["results"]
RESOLVED — contentcuration/contentcuration/tests/viewsets/test_organization.py:733 — new tests cover ContentNode only, none assert on view
RESOLVED — contentcuration/contentcuration/tests/viewsets/test_organization.py:720 — no test exercises the view side of PermissionCTE
UNADDRESSED — contentcuration/contentcuration/tests/viewsets/test_organization.py:709 — CTE's own status=ACTIVE filter untested
ACKNOWLEDGED — contentcuration/contentcuration/viewsets/channel.py:505 — org editors see a "Delete channel" affordance the API refuses (withdrawn as out of scope)
ACKNOWLEDGED — contentcuration/contentcuration/models.py:1055 — praise: all=True on the CTE union
ACKNOWLEDGED — contentcuration/contentcuration/models.py:1029 — praise: second UNION ALL branch carries the org grant to ContentNode
ACKNOWLEDGED — contentcuration/contentcuration/viewsets/channel.py — praise: deriving view from org roles alone
ACKNOWLEDGED — contentcuration/contentcuration/tests/viewsets/test_organization.py:772 — praise: asserting on the persisted node.title
@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
| ORGANIZATION_VIEWER, | ||
| ), | ||
| ) | ||
| view = ExpressionWrapper( |
There was a problem hiding this comment.
suggestion: edit and view can now both be true for the same channel, listing it on My Channels and View-only at once.
A personal editor of an org-owned channel who is also an Org Viewer of that org gets edit=True from the m2m branch and view=True from the org branch. Those flags used to be mutually exclusive: accepting an invitation puts a user in Channel.editors or Channel.viewers, never both. The two channel pages filter on ?edit=true and ?view=true (viewsets/channel.py:259-263), so this user sees the channel twice.
#6123 spells that case out: "Org Viewer B at Org 1 … should be able to edit their own created channels within the org, if any."
Please AND this view expression with the negation of the edit expression above, so View-only stays disjoint from My Channels.
| self.assertFalse(self._viewable_channel(self.pending_user).exists()) | ||
| self.assertFalse(self._editable_channel(self.pending_user).exists()) | ||
|
|
||
| def test_org_role_channel_content_permissions(self): |
There was a problem hiding this comment.
suggestion: (unaddressed) The CTE's own status=ORGANIZATION_ROLE_STATUS_ACTIVE filter (models.py:1036) is still untested. test_pending_role_grants_no_channel_access covers the pending case through Channel.filter_*_queryset. That path checks status via _organization_role_exists, not via the CTE. Please add a ContentNode.filter_edit_queryset(..., self.pending_user) assertion here to pin the CTE filter.
| # Organizations grant channel permissions to their members, on top of | ||
| # whatever personal editor/viewer access a user already has. | ||
| return ( | ||
| Channel.objects.filter( |
There was a problem hiding this comment.
praise: Keeping user, role and status in one filter() call is what makes this correct. Chained .filter() calls would join user_roles three times. Three different rows could then satisfy the three conditions.
Superseded by my review of 4c56f39: no blocking findings remain.
3 participants
Summary
Fixes the two blocking follow-ups called out in #6123 (carried over from #6080):
PermissionCTE(used byContentNode/AssessmentItem/Fileedit & view querysets) only read the personal editor/viewer m2m tables, so organization editors/viewers had no edit or view access to content inside org-owned channels, even thoughChannel.filter_edit_queryset/filter_view_querysetalready granted them access to the channel itself.ChannelViewSet.get_querysetre-annotatededit/viewstraight from the m2m tables after the organization-aware queryset had already computed correct values, soGET /api/channel/reportededit: falsefor organization editors.Also addresses several rounds of review feedback from
@rtibblesbot— see "Review history" below.Changes
PermissionCTE.editable_channels/view_only_channelsnow union the personal m2m grants with channels reachable via an activeOrganizationRole(editor/admin for edit, viewer-only for view — org editors/admins are already admitted through the edit check, so including them in the view branch too would be redundant, at both the channel-API layer and the content-permission layer).Channel.filter_view_querysetnow folds organization grants directly into the exposededit/viewannotations, soChannelViewSetno longer needs its own override.ORGANIZATION_CHANNEL_EDIT_ROLES; there's no view equivalent since viewing is justORGANIZATION_VIEWERalone), and the repeatedOrganizationRoleexistence checks inChannelare collapsed into one_organization_role_existshelper.PermissionCTE.__init__override that was identical to the inherited one, and removed intermediateorganization_edit/organization_viewannotations that became dead weight once folded directly intoedit/view.Test plan
pre-commit run --fileson all changed files (flake8, black, import ordering) — passingstr(queryset.query)(no live Postgres available in this environment to runpytestmyself — a reviewer ran the suite against Postgres and reported 608 passed, 5 skipped)pytest contentcuration/contentcuration/tests/viewsets/test_organization.py— please re-run against a real DB before mergingOrganizationChannelPermissionTestCase:test_org_role_channel_content_permissions— an org editor can edit, and an org viewer can view but not edit, aContentNodeinside the org channel, without any personal m2m share (bothPermissionCTEbranches covered)test_channel_detail_reports_edit_view_by_org_role—GET /api/channel/{id}/reportsedit/viewcorrectly for both an org editor and an org viewertest_org_editor_channel_list_filters—?view=trueexcludes, and?edit=trueincludes, an org editor's channel (the two literal named symptoms from User - Organization - Channel Edit/View Permissions #6123)test_org_editor_can_edit_channel_content_via_sync— an org editor'sContentNodetitle update via/api/sync/actually persists (the original end-to-end failure mode), asserting on the persisted value rather than just the response codeKnown follow-up (raised by review, then withdrawn as out of scope)
An earlier review round flagged that org editors now see a "Delete channel" option in the channel list UI (gated on
channel.edit) that the backend still correctly refuses (deletion stays restricted to org Admins). The reviewer later withdrew this, agreeingedit: truefor org editors is the intended outcome of this PR and that the frontend affordance is a separate, out-of-scope follow-up.Also out of scope per the issue:
File/AssessmentItemdon't have their own dedicated org-permission tests beyond going through the samePermissionCTEasContentNode, and other direct usages ofUser.editable_channels/Channel.viewersoutside the sync/edit-permission path (e.g.viewsets/user.pyadmin search,views/settings.py, the channel digest email intasks.py) still bypass organization grants.Review history
viewannotation OR'd inedit, breakingtest_no_viewable_channels— fixed by dropping the disjunct.PermissionCTE.__init__; extend test coverage — all addressed.organization_edit/organization_viewannotations; untestedPermissionCTEview side — all addressed.response.data["results"]on an endpoint that returns an unpaginated plain list — fixed. Suggestion: narrowedPermissionCTE.view_only_channels's org role set to viewer-only, matching the channel-API layer, since admins/editors were already admitted via the edit check and nothing separately exposed that CTE'sviewboolean.Post-review cleanup: consolidated 9 tests down to 4 (merging related assertions in the file's existing style, removing one that duplicated a pre-existing
test_no_viewable_channelscase) and condensed a couple of multi-line comments, including fixing one that inaccurately described howview/editinteract.🤖 Generated with Claude Code