6009: add organizations to invitation querysets and sync - #6039
6009: add organizations to invitation querysets and sync#6039yasinelmi wants to merge 8 commits into
Conversation
Extends the channel-invitation flow to support organization invitations, per learningequality#6009 (following learningequality#6008's Invitation.organization field): - Add Organization.filter_edit_queryset/filter_view_queryset based on active OrganizationRole membership, and extend Invitation's equivalents to grant org admins edit access and org members view access. - Allow InvitationSerializer to accept an organization in place of (or alongside) a channel, requiring at least one of the two. - Teach the /sync endpoint's permission gate to recognize organization- scoped changes, since org admins acting on another user's invitation don't fit the existing channel-scoped or self-scoped checks.
|
👋 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. |
…tures - generate_create_event/generate_update_event/generate_delete_event (viewsets/sync/utils.py) never gained an organization_id parameter when handle_changes() was taught to read it, causing a TypeError in every organization invitation test that passed organization_id. - Two OrganizationInvitationSyncTestCase fixtures were missing invited=self.invited_user, which InvitationSerializer.get_fields() requires to unlock the accepted field for sync-based updates, matching the existing SyncTestCase fixture pattern.
Per the "Organizations XOR channels" requirement from learningequality#5971 and rtibbles' comment on learningequality#6009 confirming this was deliberately deferred to the API layer: InvitationSerializer.validate() now rejects an invitation that has both channel and organization set, not just one that has neither. This invalidates the "co-owner" scenario (one invitation with both fields set) the earlier test exercised, so that test is replaced with one asserting the combination is rejected.
Extends the Change/sync mechanism to give organization_id the same three roles channel_id already has, rather than reusing the per-user change queue as a stopgap: - Change.organization: new FK field + migration, so organization scope is a real, queryable column instead of being buried in the opaque kwargs JSON blob. - apply_organization_changes_task: dedicated Celery task mirroring apply_channel_changes_task, draining pending organization-scoped changes. apply_user_changes_task now excludes organization-tagged rows so a change is only ever processed by one task. - handle_changes(): organization-scoped changes route through their own permission-gated bucket via Organization.filter_edit_queryset, rather than being folded into user-only changes. The self-only check stays first in priority, since a user must be able to act on their own invitation even before holding any org role (e.g. accepting it). Also covers organization_ids that end up on self-only changes, so a self-accept carrying organization_id can't get stranded unapplied. - organization_revs: full broadcast support in return_changes(), mirroring channel_revs, so any org admin sees invitation changes live, not just the acting user. - generate_create_event/update_event/delete_event: gained an organization_id parameter so events can actually carry it. - accept/decline actions and the sync-based accept path now tag organization_id on their Change events. Also drops organization_name/organization__name from the invitation viewset's values/field_map - unused and not requested; organization_id is what's actually needed by the API consumer today.
…into invitation_api
0170_change_organization collided with unstable's own new 0170_merge_20260717_0136 (both depended on prior leaves). Renumbered to 0171 and repointed its dependency at the new merge migration.
rtibbles pointed out on PR review that channel_id/user_id exist on Change specifically to decide where a change propagates to, and there was no established need for organizations to have that same live cross-admin broadcast - permission checks belong at the viewset layer (which Organization.filter_edit_queryset via UserFilteredPrimaryKeyRelatedField already handles), not as a new routing/broadcast concept on the shared Change model. Reverts: - Change.organization field and its migration - apply_organization_changes_task, and the organization exclusion on apply_user_changes_task - The dedicated organization_changes bucket, organization_revs, and get_organization_revs() in handle_changes()/return_changes() - organization_id tagging on accept/decline Change events Keeps: - The handle_changes() permission gate for organization-scoped changes (still needed - an org admin acting on someone else's invitation doesn't fit the channel-scoped or self-scoped checks), now routing through the existing per-user change queue instead of a dedicated one - Organization.filter_edit_queryset/filter_view_queryset (used by both the permission gate and the serializer's UserFilteredPrimaryKeyRelatedField) - All queryset, validation (including XOR), and read-path behavior
|
📢✨ Before we assign a reviewer, we'll turn on |
🔵 Review postedLast updated: 2026-07-27 14:59 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
| user_only_changes.append(c) | ||
| elif c.get("channel_id") in allowed_ids: | ||
| channel_changes.append(c) | ||
| elif ( |
There was a problem hiding this comment.
blocking: An org-scoped change with no user_id is accepted here, then never applied — silent data loss with a 200.
Change.create_changes stores it as channel_id=NULL, user_id=NULL. apply_user_changes_task filters user_id=user_id, channel__isnull=True (tasks.py:36) — an exact match, so NULL matches no invocation; apply_channel_changes_task doesn't match either; and the reconciliation safety net explicitly excludes it (reconcile_change_tasks.py:44 filters channel_id__isnull=True, user_id__isnull=False). Meanwhile the change is returned in allowed, so the client marks it synced and drops its local copy.
This is the expected shape, not a hypothetical: the client's getUserId for invitations is obj.invited (frontend/shared/data/resources.js:2077), and invited is never populated for API-created invitations — it isn't in InvitationSerializer.Meta.fields and create() only sets sender. For invite-by-email the invitee may not have an account at all. The new tests pass only because they hand-supply user_id=self.invited_user.id.
Suggest deriving the routing target server-side instead of trusting/requiring the client — routing org changes onto the actor's queue (user_id=request.user.id) is the simplest option, since application already runs under the actor's permissions (apply_changes → viewset.sync_initial(change.created_by)), and it also restores error feedback to the initiating client. If the invitee needs the broadcast, resolve them server-side from the invitation email. A test creating an org invitation for an email with no matching User would catch this.
| apply_user_changes_task.fetch_or_enqueue( | ||
| request.user, user_id=request.user.id | ||
| ) | ||
| other_target_user_ids = set( |
There was a problem hiding this comment.
blocking: user_id reaching here is entirely unvalidated. The user branch above pins user_id == request.user.id and the channel branch gates on channel edit rights; the new org branch gates only on organization_id ∈ allowed_org_ids and is not restricted to the INVITATION table. Nothing checks that the target user is a member of the org, is the invitee, or is related to the change at all.
Two consequences:
- Client-controlled task fan-out — one POST with N changes each carrying a distinct
user_idand an org the caller admins enqueues Napply_user_changes_taskjobs for N unrelated users (fetch_or_enqueueonly dedupes per user). - Change injection into another user's feed — the
Changerow is persisted withuser_id=<arbitrary user>, andreturn_changesmatchesQ(user=request.user), so once applied it is delivered to that user's client and written to their IndexedDB. Write permissions are still checked againstchange.created_by, so this isn't DB-level escalation, but "create an invitation for my org" is legitimately allowed — so an org admin can make an arbitrary user's client display a pending invitation not addressed to their email, whichInvitation.filter_view_querysetwould never return over REST.
Both disappear if the target is derived server-side (invitee by email, or the actor). If client-supplied user_id must stay, restrict this branch to INVITATION changes and validate that user_id is the invitation target.
| # needs the change to be accepted and applied, so it's routed | ||
| # through the existing per-user queue, tagging the target user | ||
| # rather than the actor. | ||
| user_only_changes.append(c) |
There was a problem hiding this comment.
suggestion: Worth stating the trade-off this comment currently reads past: tagging the target user means the actor never gets the change back. return_changes builds Q(user=request.user) & relevant_to_session_filter | Q(channel_id=...) & ..., and an org change has neither the actor's user_id nor a channel_id — including the Q(errored=True, session_id=session_key) term, which is ANDed inside relevant_to_session_filter.
So when an org change fails validation during apply_changes (e.g. the new both-channel-and-organization rule), it is marked errored=True server-side but the initiating client is never told, having already discarded its local copy in allowed. The frontend rollback path can't fire. That's a departure from every other change type; if it's intentional, the comment should say so.
| @@ -3807,7 +3839,14 @@ def filter_edit_queryset(cls, queryset, user): | |||
| return queryset | |||
|
|
|||
| return queryset.filter( | |||
There was a problem hiding this comment.
blocking: This grants an active ORGANIZATION_ADMIN edit access to the org's invitations, but InvitationSerializer.get_fields is unchanged and still gates writability on channel-era relationships (if self.instance.sender == request.user: fields["revoked"].read_only = False). An org admin who isn't the sender therefore cannot revoke: revoked stays read-only, DRF drops it from validated_data, BulkListSerializer.update skips the object because obj.keys() is empty (viewsets/base.py:283), and the request returns 200 with nothing changed and no error.
Multi-admin orgs are the point of org-level roles, so "only the original inviter can revoke" is unlikely to be intended. test_revoke_organization_invitation_by_admin makes self.org_admin the sender, so it exercises the pre-existing sender rule; a test where admin A revokes an invitation sent by admin B would fail today. Extending get_fields to unlock revoked for org admins (mirroring the sender case) would close it.
Same shape for accept/decline over sync: gated on self.instance.invited == request.user, and invited is never set for API-created invitations. Latent, since the REST actions compare emails via _ensure_invitee, but worth noting.
| pass | ||
|
|
||
|
|
||
| class OrganizationInvitationSyncTestCase(SyncTestMixin, StudioAPITestCase): |
There was a problem hiding this comment.
blocking: #6009 asks for "create, update, delete" coverage of the invitation-organization relationship and for org invites to be "correctly synced with create/update/delete events", and frames the work as driven by CRUD/SYNC tests here. Two gaps:
- No org equivalent of
test_delete_invitation(line 297) /test_delete_invitations(line 318), even thoughgenerate_delete_eventis already imported anddelete_from_changesgoes through theInvitation.filter_edit_querysetthis PR changed. - No organization variant of
CRUDTestCase(line 541), so the/invitation/REST route itself is untested for orgs — POST withorganization, and the new?organization=filter at invitation.py:129.
Also missing, and reachable only because of this PR: an active admin of org A submitting a change for org B (allowed_org_ids is the only thing stopping them, with nothing asserting rejection), and an org change submitted without user_id — see the endpoint.py:97 finding; a test asserting the invitation is actually created would have caught that one.
| list_serializer_class = BulkListSerializer | ||
|
|
||
| def validate(self, data): | ||
| channel = data.get("channel", getattr(self.instance, "channel", None)) |
There was a problem hiding this comment.
suggestion: self.instance is a single Invitation set per item by BulkListSerializer.to_internal_value (base.py:262), loaded via _data_lookup_dict with no select_related — so .channel and .organization each trigger a SELECT, up to 2N extra queries for a sync batch of N updates. Only truthiness is used, so self.instance.channel_id / self.instance.organization_id gives the same answer with no query.
| @@ -163,6 +192,7 @@ def accept(self, request, pk=None): | |||
| INVITATION, | |||
| {"accepted": True}, | |||
| channel_id=invitation.channel_id, | |||
There was a problem hiding this comment.
suggestion: These two REST actions got user_id so their events are routable, but InvitationSerializer.update still emits generate_update_event(..., channel_id=instance.channel_id) with no user_id. For an org invitation that is channel_id=None, user_id=None, persisted by BulkListSerializer.save with applied=True — return_changes can never match it, so it is dead weight in the table.
The full org broadcast was deliberately reverted in 22b1f51, so that's out of scope here — but user_id=instance.invited_id (or the accepting user) would at least make the event reachable, matching what these actions now do. If the sync accept path is intentionally dead for organizations, a comment saying so would be worth more than the silent row.
| event["channel_id"] = channel_id | ||
| if user_id: | ||
| event["user_id"] = user_id | ||
| if organization_id: |
There was a problem hiding this comment.
suggestion: After the Change.organization revert, nothing in contentcuration/ outside the test helpers in tests/viewsets/base.py passes organization_id to these helpers, and a server-generated event carrying it would just deposit the key into Change.kwargs — return_changes routes on channel_id/user_id only. Consider dropping the parameter until something server-side actually emits an org-scoped event, so the signature doesn't imply routing support that doesn't exist.
| return self.name | ||
|
|
||
| @classmethod | ||
| def filter_edit_queryset(cls, queryset, user): |
There was a problem hiding this comment.
nitpick: Organization.deleted is a soft-delete flag (line 1880) and CustomManager doesn't filter it, so neither of these classmethods excludes deleted orgs — and both call sites pass an unfiltered base queryset (Organization.objects.all() in the serializer field, Organization.objects.filter(id__in=...) in SyncView). An admin of a soft-deleted org can still create invitations into it, and accepting one creates a live OrganizationRole. Channel defers deleted filtering to its viewset, so either layer is consistent with the repo — but with no Organization viewset yet, these methods are currently the only gate.
| if user.is_admin: | ||
| return queryset | ||
|
|
||
| return queryset.filter( |
There was a problem hiding this comment.
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
| "revoked", | ||
| "email", | ||
| "channel", | ||
| "organization", |
There was a problem hiding this comment.
blocking: invited isn't in Meta.fields and the only code that sets it is send_invitation_email (views/users.py:66-72), which is channel-only (request.data["channel_id"], request.user.can_edit(channel_id)) and isn't extended here. So an organization invitation created through the sync API has invited = NULL.
get_fields gates both accepted and declined on self.instance.invited == request.user (line 101), so the real invitee of a real org invitation can never flip either flag through sync. test_accept_organization_invitation_creates_role passes only because the fixture constructs the invitation with invited=self.invited_user — a state this branch's creation path cannot produce. (The accept/decline REST actions do work, since _ensure_invitee matches on email.)
Either resolve invited from email on create, or gate on email match the way _ensure_invitee does — and build the test fixture through the sync create path so the gap is caught.
- Route org-scoped changes through the actor's own queue instead of trusting a client-supplied user_id: fixes a silent-drop when user_id is omitted (endpoint.py:97) and a change-injection vector where an org admin could tag an arbitrary user as the target (endpoint.py:121). Scoped to the invitation table specifically, mirroring how the created_channel_ids exception is scoped to the channel table. - InvitationSerializer.get_fields(): unlock `revoked` for any active org admin of the invitation's organization, not just the original sender - org-admin edit rights on the queryset didn't actually enable revoke without this. - InvitationSerializer.get_fields(): gate accepted/declined on email match (mirroring _ensure_invitee) instead of the `invited` FK, which is never populated for invitations created through the sync API - the real invitee of a real org invitation couldn't accept/decline. - InvitationSerializer.validate(): use channel_id/organization_id instead of dereferencing the related objects, avoiding up to 2N extra queries per sync batch. - Organization.filter_edit_queryset/filter_view_queryset: exclude soft-deleted organizations. - New tests: org change with no user_id still applies; a client- supplied user_id is not trusted as the routing target; cross-org isolation; revoke by a non-sender org admin; org invitation delete via sync; accept of an invitation created through the real sync create path (not hand-constructed with `invited` set); organization filter on the list endpoint.
3 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 by the sync permission gate below and byUserFilteredPrimaryKeyRelatedFieldon the serializer, so a user can only submit an org they actually have edit rights to.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()— organization-scoped changes (e.g. an org admin creating/revoking an invitation for someone else) don't fit the existing channel-scoped or self-scoped permission checks, since the actor isn't the target and there's no channel. Adds a permission check viaOrganization.filter_edit_queryset, then routes accepted changes through the existing per-user change queue (tagging the target user, not the actor) rather than adding a dedicated organization concept to the sharedChangemodel.Note on scope: an earlier version of this PR also added full
organization_idbroadcast parity withchannel_id(aChange.organizationfield/migration, a dedicated task,organization_revs), so that other org admins would see invitation changes live rather than on next refresh. Per rtibbles' review, that's been reverted —channel_id/user_idexist onChangespecifically to decide broadcast routing, and there's no established need for that same live cross-admin behavior for organizations; permission checks belong at the viewset layer, which this PR already handles without touching the sharedChangeschema.Testing
test_models.py(OrganizationTestCase,InvitationOrganizationTestCase) covering the queryset permission logic directly.test_invitation.py(OrganizationInvitationSyncTestCase) covering create/accept/revoke via/sync, non-admin rejection, 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.