6009: add organizations to invitation querysets and sync - #6039
6009: add organizations to invitation querysets and sync#6039yasinelmi wants to merge 3 commits into
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. |
|
📢✨ Before we assign a reviewer, we'll turn on |
🔵 Review postedLast updated: 2026-08-04 21:57 UTC |
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #6039 — the permission querysets on Organization/Invitation are idiomatic (single filter() so the multi-valued join matches one role row, status checked as well as role) and well unit-tested. The problems are concentrated in the new organization branch of SyncView.handle_changes, which routes org changes onto a client-supplied, unvalidated user_id — that yields a silent-drop path, a change-injection path, and no error feedback to the actor. Test coverage also misses acceptance criteria from #6009.
CI passing. No UI files changed, so Phase 3 (UI verification / manual QA) did not apply.
- blocking — org change with no
user_idis accepted but never applied (endpoint.py:97); unvalidated clientuser_idallows task fan-out and cross-user change injection (endpoint.py:121); org-admin edit rights don't actually enable revoke (models.py:3841); missing delete-event / CRUD / co-owner test coverage (test_invitation.py:353, :512). - suggestion — no applied/errored feedback to the actor (endpoint.py:109), FK dereference cost in
validate()(invitation.py:53), unreachable accept event on the sync path (invitation.py:194),organization_idparam with no production caller (utils.py:37). - nitpick — soft-deleted organizations not excluded (models.py:1899).
@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
| if user.is_admin: | ||
| return queryset | ||
|
|
||
| return queryset.filter( |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
praise: Checking status=ORGANIZATION_ROLE_STATUS_ACTIVE alongside the role — and keeping all three conditions in one filter() so they match a single user_roles row rather than across rows — is the easy thing to get wrong here. test_filter_edit_queryset__pending_admin_cannot_edit locks it in.
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #6039 — 0 of 11 prior findings resolved: HEAD is still 22b1f51d, the diff is byte-identical to the last review, and none of the open threads have replies. All 11 remain open (breakdown collapsed below); the 5 blocking ones still gate merge. CI passing. No UI files changed, so Phase 3 did not run.
One new blocking finding this pass, inline on viewsets/invitation.py: org invitations created through the sync API never get invited populated, so the invitee can't accept or decline through sync.
Two further notes on already-open threads rather than new ones:
- models.py:3841 (existing thread) — beyond blocking revoke, the org-admin edit clause makes
InvitationSerializer.updatereachable for a non-invitee, and that method readsself.initial_data.get("accepted")rather thanvalidated_data. An org admin syncing{"accepted": true}on someone else's invitation callsinstance.accept()→OrganizationRole.update_or_create(status=ACTIVE), adding that user to the org without consent, whileaccepteditself is dropped as read-only so the invitation stays pending and the operation replays. Reading fromvalidated_datafixes both halves. - endpoint.py:97 (existing thread) — the escape hatch is table-agnostic: any change carrying
organization_idand nochannel_idbypasses theuser_id == request.user.idcheck, for every table inviewset_mappingpresent and future. Scoping it totable == INVITATION(mirroring how thecreated_channel_idsexception is scoped totable == CHANNEL) keeps it as narrow as the feature needs.
Prior-finding status
UNADDRESSED — contentcuration/contentcuration/viewsets/sync/endpoint.py:97 — org change with no user_id accepted but never applied (blocking)
UNADDRESSED — contentcuration/contentcuration/viewsets/sync/endpoint.py:121 — unvalidated client-supplied user_id allows task fan-out and change injection (blocking)
UNADDRESSED — contentcuration/contentcuration/models.py:3841 — org-admin edit rights don't enable revoke; get_fields still gates on sender == request.user (blocking)
UNADDRESSED — contentcuration/contentcuration/tests/viewsets/test_invitation.py:353 — missing org delete-event and org CRUDTestCase coverage required by #6009 (blocking)
UNADDRESSED — contentcuration/contentcuration/tests/viewsets/test_invitation.py:512 — no test for channel invitation with organization role (co-owner) (blocking)
UNADDRESSED — contentcuration/contentcuration/viewsets/sync/endpoint.py:109 — actor gets no applied/errored feedback for org changes (suggestion)
UNADDRESSED — contentcuration/contentcuration/viewsets/invitation.py:53 — validate() dereferences self.instance.channel/.organization, up to 2N queries per batch (suggestion)
UNADDRESSED — contentcuration/contentcuration/viewsets/invitation.py:194 — accept/decline event unroutable for org invitations (suggestion)
UNADDRESSED — contentcuration/contentcuration/viewsets/sync/utils.py:37 — organization_id parameter has no production caller after the broadcast revert (suggestion)
UNADDRESSED — contentcuration/contentcuration/models.py:1899 — soft-deleted organizations not excluded from the permission querysets (nitpick)
UNADDRESSED — contentcuration/contentcuration/models.py:1906 — single-filter() role+status check on Organization.filter_edit_queryset (praise)
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Compared the current PR state against findings from a prior review:
- 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
- Ran the same phased review passes as a first review (core, frontend/backend lenses, manual QA when required)
- 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 #6039 — 11 of 12 prior findings resolved; 1 still open (the initial_data read in InvitationSerializer.update, now reachable for organizations).
CI passing. No UI files changed, so Phase 3 (UI verification / manual QA) did not apply.
One important finding plus three minor notes, all inline.
contentcuration/contentcuration/viewsets/invitation.py:77 — this is the line to change for the org-admin-consent finding below; it isn't in a diff hunk, so the comment is anchored on the models.py clause that makes it reachable.
Prior-finding status
RESOLVED — contentcuration/contentcuration/viewsets/sync/endpoint.py:97 — org change with no user_id accepted but never applied
RESOLVED — contentcuration/contentcuration/viewsets/sync/endpoint.py:121 — unvalidated client-supplied user_id allows task fan-out and change injection
RESOLVED — contentcuration/contentcuration/viewsets/sync/endpoint.py:109 — actor gets no applied/errored feedback for org changes
RESOLVED — contentcuration/contentcuration/models.py:3841 — org-admin edit rights don't enable revoke
RESOLVED — contentcuration/contentcuration/models.py:1899 — soft-deleted organizations not excluded from the permission querysets
RESOLVED — contentcuration/contentcuration/viewsets/invitation.py:53 — validate() dereferences self.instance.channel/.organization
RESOLVED — contentcuration/contentcuration/viewsets/invitation.py:194 — accept/decline event unroutable for org invitations
RESOLVED — contentcuration/contentcuration/viewsets/invitation.py:45 — invited never populated, so the real invitee can't accept via sync
RESOLVED — contentcuration/contentcuration/viewsets/sync/utils.py:37 — organization_id parameter has no production caller
RESOLVED — contentcuration/contentcuration/tests/viewsets/test_invitation.py:353 — missing org delete-event and org CRUDTestCase coverage
RESOLVED — contentcuration/contentcuration/tests/viewsets/test_invitation.py:512 — no test for channel invitation with organization role (co-owner)
UNADDRESSED — contentcuration/contentcuration/models.py:3841 (round-2 review body) — InvitationSerializer.update reads accepted/revoked from initial_data, not validated_data
@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
| # existing behavior rather than introducing a new access tier: | ||
| # _accept_channel_invitation only special-cases VIEW_ACCESS, so | ||
| # anything else - including "admin" - grants the same editor access | ||
| # as "edit". This locks that in as intended, confirmed behavior. |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: The test is the right shape, but the comment asserts a confirmation that isn't on record — no maintainer has weighed in on the thread. Worth noting that ADMIN_ACCESS means materially different things on the two branches this issue series adds: _accept_organization_invitation maps it to ORGANIZATION_ADMIN (models.py:3820), while _accept_channel_invitation collapses it to plain editor. It also isn't reachable from the frontend today (SharingPermissions in frontend/shared/constants.js:73 exposes only edit/view), so this pins a state the UI can't produce.
Suggest rewording to describe the divergence and flag it as needing confirmation.
| | Q(sender=user) | ||
| | Q(channel__editors=user) | ||
| | Q(channel__viewers=user) | ||
| | Q( |
There was a problem hiding this comment.
nitpick: Organization.filter_edit_queryset/filter_view_queryset now filter deleted=False, but these org clauses on Invitation don't. Creating an invitation into a soft-deleted org is blocked (both the serializer field and SyncView's allowed_org_ids go through Organization.filter_edit_queryset), but updating or deleting an existing one isn't. Adding organization__deleted=False to both Q blocks would make them consistent. There's also no test covering the deleted=False exclusion in OrganizationTestCase.
| "accepted": True, | ||
| }, | ||
| channel_id=instance.channel_id, | ||
| user_id=self.context["request"].user.id, |
There was a problem hiding this comment.
nitpick: create guards with if "request" in self.context and get_fields uses self.context.get("request"); this indexes directly. Safe on both current call paths (sync_initial at base.py:524 always sets self.request, and DRF's get_serializer_context always includes it) — purely about matching the defensive style either side of it.
| "sender__first_name", | ||
| "sender__last_name", | ||
| "channel_id", | ||
| "organization_id", |
There was a problem hiding this comment.
nitpick: values carries channel__name so clients can label channel invitations, but the org side only gets the raw id. Frontend is out of scope for #6009, so this may be deliberate — worth confirming it's a conscious deferral, since it's the one remaining asymmetry with the channel path.
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #6039 — 1 of 5 prior findings resolved; 4 still open (all nitpick/suggestion, unchanged from last round). CI passing; no UI files changed, so no visual verification was needed.
The initial_data → validated_data fix is correct and the new test genuinely exercises it. Two follow-ups on that line inline.
Still-open prior findings (unchanged, restated in one line each):
- test_invitation.py:150-154 — suggestion: the comment records the channel
ADMIN_ACCESScollapse as "intended, confirmed behavior", but no maintainer has confirmed it on the thread. Either get confirmation or soften the wording to describe observed behavior. - models.py:3848 — nitpick:
organization__deleted=Falsemissing from theInvitationorg clauses, and no test covers the exclusion. - invitation.py:96 — nitpick:
self.context["request"]indexed directly wherecreate/get_fieldsguard within/.get. - invitation.py:175 — nitpick:
valuescarrieschannel__namefor labelling channel invitations but only the raworganization_idfor org ones.
Prior-finding status
RESOLVED — contentcuration/contentcuration/models.py:1906 — org role check includes status=ORGANIZATION_ROLE_STATUS_ACTIVE
RESOLVED — contentcuration/contentcuration/viewsets/invitation.py:83 — InvitationSerializer.update read accepted/revoked from initial_data, not validated_data
UNADDRESSED — contentcuration/contentcuration/tests/viewsets/test_invitation.py:154 — co-owner test comment asserts a maintainer confirmation that isn't on record
UNADDRESSED — contentcuration/contentcuration/models.py:3848 — organization__deleted=False missing from the Invitation org clauses; no test for the exclusion
UNADDRESSED — contentcuration/contentcuration/viewsets/invitation.py:96 — self.context["request"] indexed directly where surrounding code guards
UNADDRESSED — contentcuration/contentcuration/viewsets/invitation.py:175 — values carries channel__name but only the raw organization_id
@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
| # letting e.g. an org admin force instance.accept() (and thus an | ||
| # active OrganizationRole) onto an invitation that isn't theirs to | ||
| # accept, even though the field itself stays correctly unwritten. | ||
| accepted = validated_data.get("accepted") or instance.accepted |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: The or instance.accepted fallback makes this "was this invitation ever accepted", not "is it being accepted now" — super().update() on line 76 has already copied validated_data onto the instance, so for an already-accepted invitation the expression is just instance.accepted.
ValuesViewset uses the non-bulk UpdateModelMixin (base.py:745-777), whose perform_update calls serializer.save() unconditionally — test_update_invitation_empty (test_invitation.py:292) confirms {} mods still reach update(). So any later sync update on an accepted, non-revoked org invitation — including by the invitee, who has edit rights via Q(email__iexact=user.email) — re-enters instance.accept() → _accept_organization_invitation → OrganizationRole.objects.update_or_create(status=ORGANIZATION_ROLE_STATUS_ACTIVE) (models.py:3825-3829). Concretely: invitee accepts, admin later deactivates/deletes that role, invitee syncs {"declined": true} and the active role is recreated. Revoking blocks it, so there is a remediation, and the channel analogue predates this branch — but org membership is the new grant and this is the line being changed.
Reading only the incoming value scopes accept() to the transition:
accepted = validated_data.get("accepted")
revoked = validated_data.get("revoked") or instance.revokedThe legitimate path is unaffected — get_fields makes accepted writable for the email-matching invitee, so it lands in validated_data.
There was a problem hiding this comment.
Would be good to clean this up - we only need to run the accepted path when a toggle is happening.
| invitation.refresh_from_db() | ||
| self.assertTrue(invitation.revoked) | ||
|
|
||
| def test_admin_cannot_force_accept_on_behalf_of_invitee(self): |
There was a problem hiding this comment.
suggestion: This covers the org side of the initial_data fix; the channel side is still unasserted. As the commit message notes, test_update_invitation_sender_cannot_modify_invited_user_fields (line 245) only checks the accepted/declined booleans and never checked whether accept()'s side effect ran — which is why the bug survived there. One line in that test closes it, matching the assertion already used at line 285:
self.assertFalse(self.channel.editors.filter(pk=self.invited_user.id).exists())
rtibbles
left a comment
rtibbles
left a comment
There was a problem hiding this comment.
Please keep all changes to sync level machinery reverted, it should not appear in the code, it should not appear in the commit history. There is no need to add extra change level metadata just to transmit information about the organization, it can be encoded in the change kwargs.
|
|
||
|
|
||
| def generate_create_event(*args, **kwargs): | ||
| # organization_id isn't a parameter of the production event builders (no |
There was a problem hiding this comment.
This makes a claim about how this key is transmitted - but there is nothing that is using it like this right now.
We discussed previously not promoting organization_id to a top level change key and those changes were reverted in the previous PR, but this is not reintroducing this again.
There was a problem hiding this comment.
You're right — nothing transmits organization_id as a top-level change key today, so the comment asserts a protocol that doesn't exist.
This one descends from my utils.py:37 finding, where I noted that nothing passes organization_id to the event builders and that it would only ever land in Change.kwargs — I should have concluded the key shouldn't exist at the change level at all, rather than that the parameter should move into the test helper. Withdrawing that finding and deferring to your call that it belongs in the change kwargs.
| .values_list("id", flat=True) | ||
| .distinct() | ||
| ).union(created_channel_ids) | ||
| change_organization_ids = set( |
There was a problem hiding this comment.
This change was reverted in the last PR, but seems to have reappeared.
There was a problem hiding this comment.
Understood, deferring to this.
Flagging my own part in it: my earlier threads on this branch (endpoint.py :97, :115, :121) all treated the org branch in handle_changes as in scope and asked for fixes inside it, and the c["user_id"] = request.user.id routing line now in this hunk came directly from my endpoint.py:97 suggestion. Those findings are moot if the branch is removed — withdrawing them. Apologies for the noise.
|
Deferring to @rtibbles on the sync-level machinery. For the record on my own threads: the ones anchored on |
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #6039 — 3 of 9 prior findings resolved; 6 still open.
The code half of the sync-machinery revert is done: viewsets/sync/ and tests/viewsets/base.py are byte-identical to unstable, and the diff is down to five files. CI green on a366dd5f2. No UI files changed.
[important] Sync-machinery changes remain in the commit history. rtibbles asked for them gone from the code and the history. The branch still carries cf2a01edc (org branch in endpoint.py), 2045c3894 (organization_id in utils.py), adf18304c (Change.organization + migration 0170_change_organization.py), plus the three reverts. unstable merges with merge commits, so those land on the target branch as-is and an intermediate checkout picks up a stray Change migration. Rebasing the five-file end state into one or two logical commits would settle it.
Still open from earlier rounds (existing threads, not re-posted):
viewsets/invitation.py:83—or instance.acceptedre-firesaccept()on any later updateviewsets/invitation.py:96—self.context["request"]indexed where surrounding code guardsviewsets/invitation.py:175—valuescarrieschannel__namebut only the raworganization_idmodels.py:3849—organization__deleted=Falsemissing from theInvitationorg clausestests/viewsets/test_invitation.py:154— comment cites a maintainer confirmation not on recordtests/viewsets/test_invitation.py:245— channel-side assertion for theinitial_datafix
One new suggestion inline.
Prior-finding status
RESOLVED — contentcuration/contentcuration/models.py:1906 — praise: status=ORGANIZATION_ROLE_STATUS_ACTIVE checked alongside the role
RESOLVED — contentcuration/contentcuration/viewsets/sync/endpoint.py:70 — org branch in handle_changes reverted (dependent :97/:115/:121 findings withdrawn with it)
RESOLVED — contentcuration/contentcuration/tests/viewsets/base.py:43 — organization_id removed from the test event builders
UNADDRESSED — contentcuration/contentcuration/viewsets/invitation.py:83 — or instance.accepted recreates a deactivated OrganizationRole
UNADDRESSED — contentcuration/contentcuration/viewsets/invitation.py:96 — unguarded self.context["request"]
UNADDRESSED — contentcuration/contentcuration/viewsets/invitation.py:175 — no org name in values
UNADDRESSED — contentcuration/contentcuration/models.py:3849 — organization__deleted=False missing; no test for the exclusion
UNADDRESSED — contentcuration/contentcuration/tests/viewsets/test_invitation.py:154 — co-owner comment asserts an unrecorded confirmation
UNADDRESSED — contentcuration/contentcuration/tests/viewsets/test_invitation.py:245 — channel-side initial_data assertion not added
@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
| pass | ||
|
|
||
| def test_create_organization_invitation_without_user_id_is_rejected(self): | ||
| # Organization-scoped invitation changes get no special routing in |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: With the org branch gone, an org invitation change only syncs when the client tags user_id with its own id — which is what these tests do, and what this comment documents. No real client can do that today: Invitation in frontend/shared/data/resources.js:2078 defines getUserId(obj) { return obj.invited; }, and invited is never populated for sync-created invitations (not in Meta.fields; only send_invitation_email sets it, channel-only). So a client-emitted org invitation change carries channel_id: null, user_id: null and lands in disallowed.
Frontend is out of scope for #6009, so this isn't a defect here — but the server contract is now "org invitation changes must be self-tagged" and nothing outside these tests records it. Worth a line in the PR body or a follow-up issue, so it isn't rediscovered from a silent disallowed.
rtibbles
left a comment
rtibbles
left a comment
There was a problem hiding this comment.
A couple more things before merge.
Also, let's squash the commit history to just be the history of useful changes, not the entire history of this PR.
| # letting e.g. an org admin force instance.accept() (and thus an | ||
| # active OrganizationRole) onto an invitation that isn't theirs to | ||
| # accept, even though the field itself stays correctly unwritten. | ||
| accepted = validated_data.get("accepted") or instance.accepted |
There was a problem hiding this comment.
Would be good to clean this up - we only need to run the accepted path when a toggle is happening.
| instance = super(InvitationSerializer, self).update(instance, validated_data) | ||
| accepted = self.initial_data.get("accepted") or instance.accepted | ||
| revoked = self.initial_data.get("revoked") or instance.revoked | ||
| # Read from validated_data, not initial_data (the raw client mods |
There was a problem hiding this comment.
Please revisit all code comments in this PR and make them concise. The LLM writing here makes it harder to read the code comments, so they end up obscuring rather than clarifying. The focus should be on concise technical communication - one or two lines max is a good rule of thumb, especially for in-line code comments.
If more explanation is required, it is quite possible the code itself is too obfuscated.
Adds Organization.filter_edit_queryset/filter_view_queryset (active OrganizationRole membership: admin for edit, admin/editor/viewer for view - excluding soft-deleted organizations), mirroring the existing Channel pattern. Extends Invitation.filter_edit_queryset/ filter_view_queryset with the same organization-role checks so organization invitations are covered by the existing permission model. Adds org_admin edit rights to Invitation.filter_edit_queryset via a single filter() so the multi-valued OrganizationRole join matches one row rather than across rows. Includes testdata helpers (organization, organization_role) and direct queryset tests (OrganizationTestCase, InvitationOrganizationTestCase) covering admin vs editor vs viewer vs pending vs anonymous access.
- organization is now a serializer field alongside channel (via UserFilteredPrimaryKeyRelatedField, so it's scoped by Organization.filter_edit_queryset); channel becomes optional and validate() requires exactly one of channel/organization - not neither, not both. - get_fields(): match the invitee on email rather than the `invited` FK, since `invited` is only ever populated by the channel email-invite flow and is never set for invitations created through the sync API. Unlock `revoked` for any active org admin of the invitation's organization, not just the original sender, so multi-admin orgs can actually revoke each other's invitations. - update(): read accepted/revoked from validated_data, not initial_data, so get_fields' read-only flags can't be bypassed by the raw client payload; only trigger accept() on an actual incoming toggle rather than re-running it on every later update to an already-accepted invitation. - Tag user_id on the accept/decline echo events (sync-based update() and the REST accept/decline actions) so they're routable to the acting user. - filter_organization on InvitationFilter/InvitationViewSet, so the list endpoint and field_map expose organization_id like channel_id already does.
Adds OrganizationInvitationSyncTestCase covering create/accept/ revoke/delete of organization invitations via /sync: non-admin rejection, cross-org isolation (an admin of org A can't touch org B's invitations or create into an org they don't manage), a different admin of the same org revoking another admin's invitation, an org admin unable to force-accept on behalf of the real invitee, the channel/organization mutual-exclusivity rejection, the "requires at least one of channel/organization" validation, and that org-scoped changes route the same way as any other self-only change (client tags user_id as its own id; a missing or mismatched user_id is rejected, not silently dropped or re-routed). Also adds a CRUDTestCase case confirming the REST create endpoint stays 405 for organization invitations (mirroring the existing channel-invitation coverage - both go through /sync only), and a channel-invitation test locking in that the "admin" (co-owner) share_mode currently grants the same editor access as "edit" (only VIEW_ACCESS is special-cased in _accept_channel_invitation).
1cc63cf to
d4d88f2
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #6039 — 6 of 10 prior findings resolved; 4 still open, all on existing threads (nitpick/suggestion severity, none blocking merge). No new findings this pass. Python unit tests still running; everything else green.
Still open:
contentcuration/contentcuration/models.py:3847— nitpick:organization__deleted=Falsemissing from theInvitationorg clauses; no test for the exclusion.contentcuration/contentcuration/viewsets/invitation.py:92— nitpick:self.context["request"]indexed directly wherecreate/get_fieldsguard with.get.contentcuration/contentcuration/viewsets/invitation.py:169— nitpick:valuescarrieschannel__namebut only the raworganization_id.contentcuration/contentcuration/tests/viewsets/test_invitation.py:240— suggestion: channel-side assertion for theinitial_datafix not added.
Prior-finding status
RESOLVED — contentcuration/contentcuration/models.py:1906 — praise: active-status check alongside the role
RESOLVED — contentcuration/contentcuration/viewsets/invitation.py:79 — or instance.accepted re-fires accept() on any later update
RESOLVED — contentcuration/contentcuration/viewsets/invitation.py:77 — in-line comments too verbose
RESOLVED — contentcuration/contentcuration/tests/viewsets/test_invitation.py:149 — co-owner comment cited an unrecorded confirmation
RESOLVED — commit history — sync-machinery commits and 0170_change_organization.py dropped
RESOLVED — contentcuration/contentcuration/tests/viewsets/test_invitation.py:559 — org self-tag contract now documented in the PR body
UNADDRESSED — contentcuration/contentcuration/models.py:3847 — organization__deleted=False missing from the Invitation org clauses
UNADDRESSED — contentcuration/contentcuration/viewsets/invitation.py:92 — self.context["request"] indexed directly
UNADDRESSED — contentcuration/contentcuration/viewsets/invitation.py:169 — values lacks an organization name
UNADDRESSED — contentcuration/contentcuration/tests/viewsets/test_invitation.py:240 — channel-side assertion not added
@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
4 participants
Summary
Following #6008 (which added
Invitation.organization), this extends the invitation querysets and the/syncmechanism to actually support organization invitations end-to-end, per #6009.Organization.filter_edit_queryset/filter_view_queryset— new classmethods based on activeOrganizationRolemembership (admin = edit, admin/editor/viewer = view), mirroring the existingChannelpattern. Used both byUserFilteredPrimaryKeyRelatedFieldon the serializer (so a user can only submit an org they actually have edit rights to) and byInvitationSerializer.get_fields/Invitation.filter_edit_querysetfor org-admin revoke access.Invitation.filter_edit_queryset/filter_view_queryset— extended to grant the same access to organization invitations.InvitationSerializer— acceptsorganizationin place ofchannel;channelis now optional. Validation requires exactly one ofchannel/organization(not neither, not both) — this is the "Organizations XOR channels" requirement from Add Organizations to Invitation Model #5971 that was deliberately deferred to this API-layer work (per rtibbles' comment on Add Organizations to Invitation API Route (And Sync Events) #6009 and the review thread on 5971: added organizations to invitation model #6008)./sync'shandle_changes()is untouched — organization-scoped invitation changes route the same way any other self-only change does (client tagsuser_idas its own id). Permission is enforced entirely at the model layer (Invitation.filter_edit_queryset/Organization.filter_edit_queryset) when the change is actually applied, not by the sync-routing layer.Note on scope: two earlier versions of this PR added sync-level machinery for organizations that rtibbles asked to be fully reverted, both times: first, full
organization_idbroadcast parity withchannel_id(aChange.organizationfield/migration, a dedicated task,organization_revs), and second, a narrowerhandle_changes()permission gate/branch that special-casedorganization_idas a routing key and server-derived the targetuser_idfor org-scoped invitation changes. Neither is needed —channel_id/user_idonChangeexist specifically to decide broadcast/routing, there's no established need for organizations to have that same live cross-admin behavior, and the model-layer permission checks (filter_edit_queryset) already re-enforce access when a change is applied regardless of how it was routed.organization_idisn't a recognized top-level change key anywhere in this PR; if present in a change payload it just rides along inChange.kwargslike any other unrecognized key.Known gap (frontend, out of scope for #6009): the server contract for org-scoped invitation changes is now "the client must tag
user_idas its own id," matching every other self-only change. No real frontend client can do this today —Invitation'sgetUserIdinfrontend/shared/data/resources.jsreturnsobj.invited, which is never populated for invitations created through the sync API. A client-emitted org invitation change would currently carryuser_id: nulland land indisallowed. Flagging so it isn't rediscovered as a silent failure later; fixing it is frontend work outside this PR's scope.Testing
test_models.py(OrganizationTestCase,InvitationOrganizationTestCase) covering the queryset permission logic directly.test_invitation.py(OrganizationInvitationSyncTestCase) covering create/accept/revoke/delete via/sync, non-admin rejection, cross-org isolation, the "requires at least one of channel/organization" validation, and the mutual-exclusivity rejection.References
Closes #6009. Builds on #6008 (Invitation.organization field) and #5953 (Organization/OrganizationRole models).
AI usage
Implemented with Claude Code: I worked through the codebase with it to trace how the existing channel-invitation sync mechanism works end-to-end (
handle_changes,Changemodel,apply_channel_changes_task/apply_user_changes_task), identified that organization-scoped changes didn't fit either existing permission branch, and reviewed/directed the resulting implementation and test coverage rather than accepting it as-is. The channel/organization mutual-exclusivity requirement was added after re-reading the #6008 review thread and rtibbles' follow-up comment on #6009 flagging it as still outstanding. I initially built fullChange.organizationbroadcast parity withchannel_id, then reverted it after rtibbles' review correctly identified that permission-gating and broadcast-routing are separable concerns, and the latter wasn't needed here. When rtibbles flagged that sync-level machinery for organizations had reappeared in a narrower form (thehandle_changes()permission gate), I removed that branch entirely and reworked the affected tests to have org-scoped changes self-route like any other user-only change, rather than trying to preserve a server-derived routing mechanism rtibbles had already ruled out. I also used it to trim several of my own overly verbose in-line comments down to 1-2 lines per rtibbles' request, and to scopeInvitationSerializer.update()'saccept()call to actual incoming toggles instead of re-running on every update to an already-accepted invitation.