Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
# Generated by Django 3.2.24 on 2026-07-02 04:15

@rtibblesbot rtibblesbot Jul 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved — addressed in the current code.

blocking: This migration and unstable's new 0168_alter_assessmentitem_type.py both depend on 0167_add_organization and both claim number 0168, creating two leaf nodes. CI fails with:

CommandError: Conflicting migrations detected; multiple leaf nodes in the migration graph: (0168_alter_assessmentitem_type, 0168_invitation_organization in contentcuration).

Rebase onto current unstable, renumber this to 0169_invitation_organization.py, and update dependencies to point at 0168_alter_assessmentitem_type (or run manage.py makemigrations --merge).

import django.db.models.deletion
from django.db import migrations
from django.db import models
Comment thread
ArthurMousatov marked this conversation as resolved.


class Migration(migrations.Migration):
Comment thread
ArthurMousatov marked this conversation as resolved.

dependencies = [
("contentcuration", "0168_alter_assessmentitem_type"),
]

operations = [
migrations.AddField(
model_name="invitation",
name="organization",
field=models.ForeignKey(
blank=True,
null=True,
on_delete=django.db.models.deletion.CASCADE,
to="contentcuration.organization",
),
),
]
47 changes: 40 additions & 7 deletions contentcuration/contentcuration/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -79,13 +79,17 @@
from contentcuration.constants import feedback
from contentcuration.constants import user_history
from contentcuration.constants.contentnode import kind_activity_map
from contentcuration.constants.organization_roles import ORGANIZATION_ADMIN
from contentcuration.constants.organization_roles import ORGANIZATION_EDITOR
from contentcuration.constants.organization_roles import organization_role_choices
from contentcuration.constants.organization_roles import ORGANIZATION_ROLE_STATUS_ACTIVE
from contentcuration.constants.organization_roles import (
organization_role_status_choices,
)
from contentcuration.constants.organization_roles import (
ORGANIZATION_ROLE_STATUS_PENDING,
)
from contentcuration.constants.organization_roles import ORGANIZATION_VIEWER
from contentcuration.db.models.expressions import Array
from contentcuration.db.models.functions import ArrayRemove
from contentcuration.db.models.functions import Unnest
Expand All @@ -102,6 +106,7 @@

EDIT_ACCESS = "edit"
VIEW_ACCESS = "view"
ADMIN_ACCESS = "admin"

DEFAULT_CONTENT_DEFAULTS = {
"license": None,
Expand Down Expand Up @@ -3730,6 +3735,9 @@ class Invitation(models.Model):
)
first_name = models.CharField(max_length=100, blank=True)
last_name = models.CharField(max_length=100, blank=True, null=True)
organization = models.ForeignKey(
Comment thread
ArthurMousatov marked this conversation as resolved.
"Organization", null=True, blank=True, on_delete=models.CASCADE

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

blocking: channel (line 3734) and organization are both nullable FKs with no constraint preventing both from being set on one invitation — accept() would then run both _accept_channel_invitation and _accept_organization_invitation off a single share_mode. This is the "Organizations XOR channels" acceptance criterion from #5971. Options:

  • A DB CheckConstraint, added as NOT VALID and validated separately (per the earlier maintainer discussion on this PR, to avoid a migration-timeout risk on large tables).
  • A clean()/save() guard enforcing exactly one of the two.

Either way, add a test for the violation.

)

class Meta:
Comment thread
ArthurMousatov marked this conversation as resolved.
Comment thread
ArthurMousatov marked this conversation as resolved.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

blocking: Still no constraint enforcing channel/organization mutual exclusivity. Both FKs are nullable, and accept() (line 3748-3751) runs both _accept_channel_invitation and _accept_organization_invitation if both happen to be set on the same row — never intended per the "organizations XOR channels" requirement in #5971. Add a CheckConstraint here (consider NOT VALID + separate validation to avoid a migration-timeout on large tables), or at minimum a clean()/save() guard enforcing exactly one of the two, plus a test for the violation case.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be handled in follow up work at the API layer.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood, thanks for confirming — sounds good to track that in follow-up work.

verbose_name = "Invitation"
Expand All @@ -3738,13 +3746,38 @@ class Meta:
def accept(self):
user = User.objects.filter(email__iexact=self.email).first()
if self.channel:
# channel is a nullable field, so check that it exists.
if self.share_mode == VIEW_ACCESS:
self.channel.editors.remove(user)
self.channel.viewers.add(user)
else:
self.channel.viewers.remove(user)
self.channel.editors.add(user)
self._accept_channel_invitation(user)
if self.organization:
self._accept_organization_invitation(user)

def _accept_channel_invitation(self, user):
if self.share_mode == VIEW_ACCESS:
self.channel.editors.remove(user)
self.channel.viewers.add(user)
else:
self.channel.viewers.remove(user)
Comment thread
ArthurMousatov marked this conversation as resolved.
self.channel.editors.add(user)

def _accept_organization_invitation(self, user):
Comment thread
ArthurMousatov marked this conversation as resolved.
role = None
if self.share_mode == VIEW_ACCESS:
role = ORGANIZATION_VIEWER
elif self.share_mode == EDIT_ACCESS:
role = ORGANIZATION_EDITOR
elif self.share_mode == ADMIN_ACCESS:
role = ORGANIZATION_ADMIN
else:
raise ValidationError(f"Invalid share_mode: {self.share_mode}")
Comment thread
ArthurMousatov marked this conversation as resolved.
Comment thread
ArthurMousatov marked this conversation as resolved.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

blocking: ValidationError here resolves to django.core.exceptions.ValidationError (imported at models.py:21), not rest_framework.serializers.ValidationError. Invitation.accept() is called from viewsets/invitation.py:59 and :157, both DRF view/serializer code paths. DRF's exception_handler doesn't translate Django's ValidationError, so an invalid share_mode still surfaces as an unhandled 500 instead of a 400. Raise rest_framework.serializers.ValidationError instead.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

blocking: ValidationError here resolves to django.core.exceptions.ValidationError (imported at models.py:21). Invitation.accept() is called from DRF request paths (viewsets/invitation.py:59, :157), and DRF's default exception handler only translates Http404, Django's PermissionDenied, and rest_framework.exceptions.APIException subclasses — not Django's ValidationError. An invalid share_mode will surface as an unhandled 500 instead of a 400. Raise rest_framework.serializers.ValidationError instead.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be caught in the API layer and reraised as a DRF validation error - using one in the model code is also incorrect, as it's not DRF focused.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked the current code again: InvitationViewSet.accept (viewsets/invitation.py:157) calls invitation.accept() directly with no try/except, InvitationSerializer.update (line 59) doesn't wrap it either, and there's no custom EXCEPTION_HANDLER or middleware in this repo. DRF's default exception_handler (rest_framework/views.py) only translates Http404, Django's PermissionDenied, and APIException subclasses — django.core.exceptions.ValidationError isn't one of those, so nothing currently catches or reraises it. As it stands this would still surface as a 500, not a 400.

That said, you're right that having the model import rest_framework.serializers.ValidationError is the wrong layering — withdrawing that specific suggestion. The underlying gap (uncaught exception on invalid share_mode) still applies, just not the fix I proposed for it.


update_create_defaults = {
"role": role,
"status": ORGANIZATION_ROLE_STATUS_ACTIVE,
}
OrganizationRole.objects.update_or_create(
user=user,
organization=self.organization,
defaults=update_create_defaults,
)

@classmethod
def filter_edit_queryset(cls, queryset, user):
Expand Down
Loading