Channel Viewset Organization Updates - #6130
Conversation
|
👋 Hi @ArthurMousatov, 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 |
🟡 Waiting for changesLast updated: 2026-09-09 02:16 UTC |
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #6130 adds a channel↔organization change path; three of its branches misbehave in ways the new tests don't catch, and the contested-invitation flow is unreachable for the users it targets.
CI passing. Backend-only diff — no UI files, so no visual/QA pass applies. Findings verified locally against this branch (venv + Postgres).
- blocking — denial of an org removal is returned as
Internal server errorand reported to Sentry (channel.py:392) - blocking —
get_or_createreuses revoked/declined rows, permanently blocking re-requests (channel.py:375) - blocking — unfiltered
organizationrelation: any editor can target any org, including soft-deleted ones (channel.py:310) - blocking — contested invitations carry
email=None, so only Studio superadmins can accept them (invitation.py:212) - 4 suggestions, 1 nitpick inline.
invitation.py filter_channel — suggestion: loadChannelUsers (frontend/shared/vuex/channel/actions.js:275) renders every non-accepted invitation for a channel as a pending editor, so contested rows show up in the Share tab as invites with no email. Exclude organization__isnull=False from InvitationFilter.filter_channel, or make the two shapes distinguishable client-side.
@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 "organization" in validated_data: | ||
| organization = self._handle_organization_change( |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
blocking: ValidationError raised from inside update() never reaches serializer.errors. update_from_changes (viewsets/base.py:805) only converts errors when is_valid() fails; anything raised from serializer.save() hits the generic handler at base.py:817, which calls log_sync_exception (→ Sentry) and sets errors = ["Internal server error"]. Confirmed locally:
[{'mods': {'organization': None}, 'errors': ['Internal server error'], ...}]
So every non-admin removal attempt files a Sentry report and the message on line 365 is never seen.
Move the check into validate() / validate_organization() so is_valid() fails. self.instance is available there on both the single and bulk paths (BulkListSerializer.to_internal_value assigns self.child.instance per item, base.py:261) — InvitationSerializer.validate already relies on this.
| ).exists() | ||
|
|
||
| if instance.organization_id or not all_editors_have_access: | ||
| models.Invitation.objects.get_or_create( |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
blocking: The lookup matches on channel + organization only, ignoring revoked / declined / accepted. Once a request is revoked or declined, every later attempt returns the dead row, creates nothing, and reports success — the channel can never be proposed to that org again (and accept refuses revoked invitations, invitation.py:204). Same for re-joining an org it previously left.
Reproduced locally — request org B, admin revokes, re-request:
2nd request errors: []
invitations now: [{'revoked': True, 'accepted': False}] # no new invitation
Scope the lookup to live requests: filter(channel=…, organization=…, revoked=False, declined=False, accepted=False).first(), else create. Worth a "revoke, then re-request" test.
| "language", | ||
| "content_defaults", | ||
| "source_domain", | ||
| "organization", |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
blocking: Declared only in Meta.fields, so DRF generates PrimaryKeyRelatedField(queryset=Organization.objects.all()) — no deleted=False filter, no relationship to the requesting user. InvitationSerializer scopes the same FK with UserFilteredPrimaryKeyRelatedField (invitation.py:33-35).
Both consequences confirmed locally:
- A channel can be moved into a soft-deleted org. Since
Organization.filter_edit_querysetfiltersdeleted=False(models.py:1979), the removal branch on line 356 can then never succeed for any org admin — only a Studio admin can undo it. - Any channel editor can mint an invitation into any organization id, including deleted ones;
Invitation.filter_view_querysetsurfaces those rows to that org's admins.
Declare it explicitly: UserFilteredPrimaryKeyRelatedField(queryset=Organization.objects.all(), edit=False, required=False, allow_null=True) resolves through filter_view_queryset (common.py:290-292) — non-deleted orgs that are public or that the user belongs to.
| invitation.accept() | ||
| invitation.accepted = True | ||
| invitation.save() | ||
| if invitation.channel and invitation.organization: |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
blocking: This path is unreachable for the users it targets. _handle_organization_change creates contested invitations with only channel, organization and sender — email is None. _ensure_invitee (line 194) then compares request.user.email to "" and raises PermissionDenied for everyone except a Studio-wide superadmin (User.is_admin), which is exactly what _make_admin() sets in test_accept_contested_channel_organization_invitation_by_admin_migrates_channel — the test passes while an admin of the target organization is blocked.
The sync path is closed too: get_fields only un-read-onlys accepted on an email match. And InvitationFilter.filter_invited matches email__iexact=request.user.email, so these rows never surface in loadInvitationList either.
Add an org-admin branch to _ensure_invitee — Organization.filter_edit_queryset(...).exists() against invitation.organization_id, as get_fields already does for revoked — and test with an OrganizationRole(ORGANIZATION_ADMIN) user rather than is_admin=True.
There was a problem hiding this comment.
This is correct behavior - only website admins are supposed to handle channel-org contest invitations, not org admins.
| organization_roles__status=ORGANIZATION_ROLE_STATUS_ACTIVE, | ||
| ).exists() | ||
|
|
||
| if instance.organization_id or not all_editors_have_access: |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: Removal (organization is None) requires consent from the current org's admins; migration to another org does not. A plain editor of a channel owned by org X can request org Y, Y's admin accepts, and the channel leaves X with no X admin involved — the approval the None branch exists to enforce, routed around by naming a destination. test_update_channel_organization_migration_creates_contested_invitation encodes this: the user holds a role in the target org and none in the current one.
Is dual consent intended here? If so the migration branch needs the same current-org check; if not, worth a comment saying why the two differ.
| instance, validated_data["organization"], self.context["request"].user | ||
| ) | ||
| if organization is instance.organization: | ||
| validated_data.pop("organization") |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: A deferred or dropped organization change returns 200 with no error and no server-side change. serverSync.js doesn't roll back optimistic writes, so the client's IndexedDB keeps organization = <target> for a channel the server didn't move. create (line 318) has the same silent pop.
The accept path already does the right thing (invitation.py:213). Do the same with the value the server kept:
self.changes.append(generate_update_event(
instance.id, CHANNEL, {"organization": instance.organization_id}, channel_id=instance.id))Note the field is currently write-only — base_channel_values (line 430) and ChannelViewSet.values have no organization entry and channel_field_map has no mapping, so clients can never read it back and these CHANNEL mods set a field the read path doesn't populate. Adding "organization" to the values tuple is what makes the broadcast meaningful.
| "Invitation must specify either a channel or an organization." | ||
| ) | ||
| if channel and organization: | ||
| is_existing_contested_invitation = ( |
There was a problem hiding this comment.
suggestion: The exemption is keyed on the instance rather than on what's being changed, so once an invitation has both FKs set the both-set check is disabled for every subsequent update — including one that re-points channel or organization. A channel editor could retarget a pending contested invitation at a different org (both fields resolve through UserFilteredPrimaryKeyRelatedField).
The intent — allow updates that don't touch the pair — is expressible directly:
if channel and organization and ("channel" in data or "organization" in data):| ) | ||
|
|
||
| self.assertEqual(response.status_code, 200, response.content) | ||
| self.assertEqual(len(response.json()["errors"]), 1, response.content) |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: len(errors) == 1 passes regardless of what the error is, which is why the Internal server error behaviour above is invisible in CI — assert the payload.
The positive half is also missing: no test that an org admin (or a Studio admin) can remove the channel, so the filter_edit_queryset(...).exists() branch at channel.py:357 is unexercised and a regression denying everyone would pass.
| organization = self._handle_organization_change( | ||
| instance, validated_data["organization"], self.context["request"].user | ||
| ) | ||
| if organization is instance.organization: |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
nitpick: is works only because the contested branch returns the very object instance.organization caches in _state.fields_cache; it silently reverses meaning if the helper ever returns a re-fetched instance. It also fires an extra query on the early-return path, where the two are equal but not identical, so the code falls through to a no-op write. getattr(organization, "id", None) != instance.organization_id states the intent and avoids the fetch.
| "Only organization admins can remove a channel from an organization." | ||
| ) | ||
|
|
||
| all_editors_have_access = not instance.editors.exclude( |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
praise: Expressing "do all editors have access" as a single multi-condition exclude gets the multi-valued-relation semantics right — one subquery, not a query per editor.
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #6130 — 6 of 10 prior findings resolved; 4 suggestions open (channel.py:401, channel.py:426, invitation.py:62, test_channel.py:360); 2 new blockers below. CI red on a699f6e.
Prior-finding status
RESOLVED — channel.py:422 — error not surfaced
RESOLVED — channel.py:402 — dead invitations matched
RESOLVED — channel.py:317 — unscoped org field
RESOLVED — channel.py:395 — id comparison
ACKNOWLEDGED — invitation.py:212 — path unreachable
ACKNOWLEDGED — channel.py:395 — praise
UNADDRESSED — channel.py:401 — migration skips consent
UNADDRESSED — channel.py:426 — dropped change returns 200
UNADDRESSED — invitation.py:62 — exemption keyed on instance
UNADDRESSED — test_channel.py:360 — no org-admin removal test
@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
| operations, but read operations are handled by the Viewset. | ||
| """ | ||
|
|
||
| organization = UserFilteredPrimaryKeyRelatedField( |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
blocking: Breaks test_update_channel_organization_creates_contested_invitation and test_create_channel_ignores_organization. The requester has no role in the target org and testdata.organization() leaves public=False, so filter_view_queryset drops the pk before _handle_organization_change runs; create pops the field only after validation. Grant the role there and state the membership rule, or check the destination in validate_organization instead.
| user = testdata.user() | ||
| organization = testdata.organization() | ||
| deleted_organization = testdata.organization("Deleted Org") | ||
| deleted_organization.deleted = True |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
blocking: Green without the soft delete — no OrganizationRole on deleted_organization, so filter_view_queryset excludes it on membership alone; deleted = False passes locally with the same payload. Add testdata.organization_role(user, deleted_organization).
| ).exists() | ||
| ): | ||
| return organization | ||
| return instance.organization |
There was a problem hiding this comment.
suggestion: Unreachable: validate_organization raises on the same predicate, and update always dereferences self.context["request"]. Drop this branch.
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #6130 — 8 of 13 prior findings resolved; 5 still open.
CI pending at review time. No UI files in the delta.
Still open (existing threads):
channel.py:393— the removal branch in_handle_organization_changeis unreachable;validate_organizationraises on the same predicate.channel.py:401— migrating to another org needs no consent from the current org's admins; removal does.channel.py:426— a dropped organization change returns 200 with no error and no corrective broadcast.invitation.py:62— the both-FKs-set exemption is keyed on the instance, so it also skips updates that re-pointchannel/organization.test_channel.py:361— no positive test that an org admin can remove a channel.
Two new suggestions inline, both on the deleted-organization semantics this delta introduces.
Prior-finding status
RESOLVED — contentcuration/contentcuration/viewsets/channel.py:422 — ValidationError from update() never reaches serializer.errors
RESOLVED — contentcuration/contentcuration/viewsets/channel.py:402 — invitation lookup ignored revoked / declined / accepted
RESOLVED — contentcuration/contentcuration/viewsets/channel.py:318 — organization declared only in Meta.fields (user-filtering half withdrawn: AC 2 of #6124 requires naming an org the requester has no role in)
RESOLVED — contentcuration/contentcuration/viewsets/invitation.py:212 — accept path unreachable for org admins
RESOLVED — contentcuration/contentcuration/viewsets/channel.py — is comparison on instance.organization
RESOLVED — contentcuration/contentcuration/viewsets/channel.py — user-filtered organization field broke two tests
RESOLVED — contentcuration/contentcuration/tests/viewsets/test_channel.py:310 — deleted-org test was green without the soft delete
ACKNOWLEDGED — contentcuration/contentcuration/viewsets/channel.py:395 — praise, single multi-condition exclude
UNADDRESSED — contentcuration/contentcuration/viewsets/channel.py:393 — removal branch in _handle_organization_change unreachable
UNADDRESSED — contentcuration/contentcuration/viewsets/channel.py:401 — migration requires no current-org consent
UNADDRESSED — contentcuration/contentcuration/viewsets/channel.py:426 — dropped organization change returns 200 silently
UNADDRESSED — contentcuration/contentcuration/viewsets/invitation.py:62 — both-set exemption keyed on instance
UNADDRESSED — contentcuration/contentcuration/tests/viewsets/test_channel.py:361 — no positive admin-removal test
@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
| and "request" in self.context | ||
| ): | ||
| user = self.context["request"].user | ||
| has_org_admin_access = models.Organization.filter_edit_queryset( |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: A channel already inside an org that is later soft-deleted can no longer be detached. OrganizationViewSet.perform_destroy sets deleted=True without clearing Channel.organization_id (organization.py:174; the FK is SET_NULL on hard delete only), and filter_edit_queryset starts with .filter(deleted=False) (models.py:1979). So for every channel still pointing at it:
{"organization": null}→has_org_admin_accessisFalseeven for that org's admin → "Only organization admins can remove a channel from an organization."{"organization": <other org>}→instance.organization_idis truthy → contested invitation, channel unmoved.
Only a Studio admin can free the channel. Either clear organization_id in perform_destroy, or let the removal branch pass when the current org is deleted. (My earlier :318 request covered entry into a deleted org; this is the org-deleted-afterwards path.)
There was a problem hiding this comment.
Hmm, I think for now admin-only migrations in this edge case is okay. We can think of a better strategy for org-deletion cases in a separate issue.
| user = testdata.user() | ||
| organization = testdata.organization() | ||
| deleted_organization = testdata.organization("Deleted Org") | ||
| testdata.organization_role(user, deleted_organization, role=ORGANIZATION_EDITOR) |
There was a problem hiding this comment.
suggestion: With the role granted the test now reaches the guard, but line 332 still only asserts len(errors) == 1, which passes for any validation failure — a regression where the pk fails to resolve keeps it green. Assert the message, as test_non_admin_cannot_remove_channel_from_organization does: response.json()["errors"][0]["errors"]["organization"][0].
0545229 to
4157171
Compare
ArthurMousatov
changed the title
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #6130 — 10 of 15 prior findings resolved; 5 still open (see inline), plus one new test-coverage gap.
CI passing. No UI files changed, so Phase 3 / manual QA does not apply.
Prior-finding status
RESOLVED — contentcuration/contentcuration/viewsets/channel.py:422 — ValidationError from update() never reached serializer.errors
RESOLVED — contentcuration/contentcuration/viewsets/channel.py:402 — invitation lookup ignored revoked / declined / accepted
RESOLVED — contentcuration/contentcuration/viewsets/channel.py:318 — organization auto-generated by DRF from Meta.fields
RESOLVED — contentcuration/contentcuration/viewsets/invitation.py:212 — accept path unreachable for the users it targets
RESOLVED — contentcuration/contentcuration/viewsets/channel.py — is comparison depended on object identity of instance.organization
RESOLVED — contentcuration/contentcuration/viewsets/channel.py:395 — single multi-condition exclude for editor access (praise)
RESOLVED — contentcuration/contentcuration/viewsets/channel.py — broke test_update_channel_organization_creates_contested_invitation / test_create_channel_ignores_organization
RESOLVED — contentcuration/contentcuration/tests/viewsets/test_channel.py:310 — test was green without the soft delete
RESOLVED — contentcuration/contentcuration/viewsets/channel.py:401 — removal needs current-org admin consent; migration routes through a contested invitation
RESOLVED — contentcuration/contentcuration/tests/viewsets/test_channel.py:361 — now asserts the error message, not just the count
UNADDRESSED — contentcuration/contentcuration/viewsets/channel.py:426 — deferred org change returns 200 with no error and no change
UNADDRESSED — contentcuration/contentcuration/viewsets/channel.py:393 — unreachable branch
UNADDRESSED — contentcuration/contentcuration/viewsets/channel.py:371 — soft-deleted organization traps its channels
UNADDRESSED — contentcuration/contentcuration/tests/viewsets/test_channel.py:309 — asserts only the error count
UNADDRESSED — contentcuration/contentcuration/viewsets/invitation.py:62 — exemption keyed on the instance, not on what is changing
@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
| ).exists() | ||
|
|
||
| if instance.organization_id or not all_editors_have_access: | ||
| models.Invitation.objects.get_or_create( |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: get_or_create writes an Invitation — a synced model — directly, and update() then pops organization, so the sync response carries neither an error nor a change.
- No client learns the invitation exists until it refetches;
InvitationViewSet.acceptemitsChangerecords for exactly this reason. - The requesting client sees a plain 200, so its local row keeps
organization = <target>with nothing to revert it while the server kept the old value.
ChannelSerializer.create shows the in-serializer pattern (self.changes.append(generate_update_event(...))) for broadcasting the invitation and echoing organization back unchanged. Nothing is broken today — no organization anywhere in frontend/shared/data/ — but the admin UI that follows will land on it.
| ).exists() | ||
| if user.is_admin or has_org_admin_access: | ||
| return organization | ||
| return instance.organization |
There was a problem hiding this comment.
suggestion: Dead code — and it is dead because of my earlier request to move the removal error into validate_organization so it reaches serializer.errors. validate_organization now raises whenever value is None, instance.organization_id is set and the user lacks admin rights; the only other way in (no request in context) cannot happen because update() dereferences self.context["request"] unconditionally at line 423.
Deleting the whole if organization is None: block leaves _handle_organization_change with one concern (contested migrations); the early return at line 383 already covers the permitted-removal case.
| and "request" in self.context | ||
| ): | ||
| user = self.context["request"].user | ||
| has_org_admin_access = models.Organization.filter_edit_queryset( |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: A soft-deleted organization traps its channels. Organization.filter_edit_queryset starts with queryset.filter(deleted=False) (models.py:1979) and OrganizationViewSet.perform_destroy only flips deleted without detaching channels, so afterwards its admins can neither remove a channel from it nor move it elsewhere (the migration branch routes to a contested invitation only a Studio admin can accept).
Either check the ORGANIZATION_ADMIN role directly here instead of going through filter_edit_queryset, or treat removal from a deleted organization as always permitted.
| ) | ||
|
|
||
| self.assertEqual(response.status_code, 200, response.content) | ||
| self.assertEqual(len(response.json()["errors"]), 1, response.content) |
There was a problem hiding this comment.
suggestion: len(errors) == 1 passes whatever the error is — an unrelated 500 surfacing as a sync error would keep this green. test_non_admin_cannot_remove_channel_from_organization (line 362) now asserts the message; mirror that here with "Cannot assign a channel to a deleted organization."
| self.assertEqual( | ||
| models.Channel.objects.get(id=channel.id).organization_id, | ||
| organization.id, | ||
| ) |
There was a problem hiding this comment.
suggestion: The AC "only organization admins can remove" is tested only negatively — nothing covers an organization admin successfully setting organization to None, so a regression that blocks admins too would pass. Add the mirror test with testdata.organization_role(user, organization, role=ORGANIZATION_ADMIN) asserting organization_id is None and no errors.
| "Invitation must specify either a channel or an organization." | ||
| ) | ||
| if channel and organization: | ||
| is_existing_contested_invitation = ( |
There was a problem hiding this comment.
suggestion: Still open from the last round: the exemption is keyed on the instance rather than on what is being changed, so any update to an invitation that already has both fields skips the mutual-exclusion check — including one that swaps channel or organization. Key it on whether this update introduces the second field instead.
| revoked=False, | ||
| declined=False, | ||
| accepted=False, | ||
| ).count(), |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
praise: Driving revoke through the sync endpoint and asserting count() == 1 pins the revoked/declined/accepted scoping so a get_or_create regression fails loudly.
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #6130 — 11 of 17 prior findings resolved, 1 acknowledged; 5 still open.
The contested-invitation notification I asked for last round now fires on the right path, but it is modelled as three invented pseudo-fields on the channel record that nothing ever clears — see inline. CI: Python unit tests still pending, everything else green.
Still open from prior rounds:
- viewsets/invitation.py:62 — suggestion: mutual-exclusion exemption keyed on the instance, not on what the update changes.
- viewsets/channel.py:386 — suggestion:
organization is Nonebranch dead since the check moved tovalidate_organization. - viewsets/channel.py:446 — suggestion:
if "request" in self.contextunreachable;update()already dereferences it. - tests/viewsets/test_channel.py:342 — suggestion: deleted-org test still asserts only
len(errors) == 1. - tests/viewsets/test_channel.py:379 — suggestion: no positive test that an org admin can remove a channel.
Prior-finding status
RESOLVED — contentcuration/contentcuration/viewsets/channel.py — ValidationError from update() never reached serializer.errors
RESOLVED — contentcuration/contentcuration/viewsets/channel.py — invitation lookup ignored revoked/declined/accepted
RESOLVED — contentcuration/contentcuration/viewsets/channel.py:318 — organization declared only in Meta.fields
RESOLVED — contentcuration/contentcuration/viewsets/invitation.py:212 — contested-invitation path unreachable for its target users
RESOLVED — contentcuration/contentcuration/viewsets/channel.py:401 — removal vs. migration consent asymmetry
RESOLVED — contentcuration/contentcuration/tests/viewsets/test_channel.py:371 — len(errors) == 1 passes regardless of the error
RESOLVED — contentcuration/contentcuration/viewsets/channel.py — is identity comparison on instance.organization
RESOLVED — contentcuration/contentcuration/viewsets/channel.py:395 — single multi-condition exclude for editor access (praise)
RESOLVED — contentcuration/contentcuration/viewsets/channel.py — filter_view_queryset dropped the target org pk before the handler ran
RESOLVED — contentcuration/contentcuration/tests/viewsets/test_channel.py:320 — deleted-org test green without the soft delete
RESOLVED — contentcuration/contentcuration/viewsets/channel.py — get_or_create wrote an Invitation with no change emitted
ACKNOWLEDGED — contentcuration/contentcuration/viewsets/channel.py:371 — soft-deleted organization traps its channels (maintainer: separate issue)
UNADDRESSED — contentcuration/contentcuration/viewsets/invitation.py:62 — mutual-exclusion exemption keyed on the instance
UNADDRESSED — contentcuration/contentcuration/viewsets/channel.py:386 — organization is None branch dead
UNADDRESSED — contentcuration/contentcuration/viewsets/channel.py:446 — if "request" in self.context unreachable
UNADDRESSED — contentcuration/contentcuration/tests/viewsets/test_channel.py:342 — deleted-org test asserts only len(errors) == 1
UNADDRESSED — contentcuration/contentcuration/tests/viewsets/test_channel.py:379 — no positive test that an org admin can remove a channel
@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
| instance.id, | ||
| CHANNEL, | ||
| { | ||
| "organization_status": "pending_invitation", |
There was a problem hiding this comment.
blocking: I asked for this event last round and stand by that — this is about the shape it took.
organization_status, requested_organization_id and invitation_id are not Channel fields and appear nowhere else in the tree. applyRemoteChanges.js:124 applies UPDATED mods with applyMods, which Dexie.setByKeyPaths each key verbatim onto the stored object, so all three land on every editor's local channel row — and nothing ever removes them: accept emits only {"organization": ...} on CHANNEL (invitation.py:212-221), decline emits no CHANNEL change, revoke goes through the INVITATION viewset. organization_status: "pending_invitation" is a one-way latch: stale forever once set, which defeats the field's purpose.
The invitation is the record with the lifecycle, and the frontend already has the table for it — TABLE_NAMES.INVITATION (shared/data/constants.js:38) and the Invitation resource (resources.js:2055) — with invitation.py already broadcasting accepted/declined on it. Emit generate_create_event(invitation.id, INVITATION, <serialized invitation>, channel_id=instance.id) instead. That also drops the magic "pending_invitation" string (no constant, no constants.js counterpart) and keeps the record unpublishable — INVITATION is not in PUBLISHABLE_CHANGE_TABLES (sync/constants.py:80), whereas this CHANNEL event is stored unpublishable=False and is excluded from unpublished_changes_query only by the user__isnull=True filter that channel.py:534 documents as transitional.
| instance, validated_data["organization"], self.context["request"].user | ||
| ) | ||
| if getattr(organization, "id", None) == instance.organization_id: | ||
| validated_data.pop("organization") |
There was a problem hiding this comment.
suggestion: The pop discards the requested value server-side, but the client already wrote it optimistically before queueing the change. handleSuccesses (serverSync.js:151-159) only deletes the queued change row — it never rolls the table value back, and the new mods do not carry organization either, so the client's channel.organization keeps pointing at the refused org until a full refetch. Adding "organization": instance.organization_id to the mods corrects it in the same event, whichever table it ends up on.
| ) | ||
| self.assertTrue(invitation) | ||
|
|
||
| change = models.Change.objects.filter(channel=channel, table=CHANNEL).latest( |
There was a problem hiding this comment.
praise: Reading the persisted Change row through the real sync endpoint pins the notification end-to-end rather than asserting the serializer appended to self.changes.
3 participants
Summary
As per #6124, this PR introduces the ability for the API to handle channel updates to the organization property. With these changes, the API can now move individual channels to an organization and, if required, create a contested invitation that can be resolved manually by web admins.
References
#6124 Is the main issue resolved by the PR.
Reviewer guidance
Unit tests were added to cover the new scenarios introduced by code changes.
AI usage
Copilot was used to implement the majority of the issue. It was then refined, through manual changes and additional prompting.