Skip to content

5971: added organizations to invitation model #6008

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ArthurMousatov wants to merge 2 commits into
base: unstable
Choose a base branch
from
+81 −7
Open

5971: added organizations to invitation model #6008

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
42018be
5971: added organizations to invitation model
ArthurMousatov Jun 27, 2026
ab659c6
[pre-commit.ci lite] apply automatic fixes
pre-commit-ci-lite[bot] Jun 29, 2026
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
39 changes: 39 additions & 0 deletions contentcuration/contentcuration/migrations/0168_auto_20260627_2219.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
# Generated by Django 3.2.24 on 2026-06-27 22:19
import django.db.models.deletion
from django.db import migrations
from django.db import models


class Migration(migrations.Migration):

dependencies = [
("contentcuration", "0167_add_organization"),
]

operations = [
migrations.AddField(
model_name="invitation",
name="organization",
field=models.ForeignKey(
blank=True,
null=True,

@rtibblesbot rtibblesbot Jun 29, 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.

suggestion: PostgreSQL validates existing rows when ALTER TABLE ADD CONSTRAINT CHECK runs (Django does not add NOT VALID by default). The organization FK is being added in the same migration and defaults to NULL for all existing rows. If any Invitation rows also have channel=NULL — structurally possible since channel is null=True — this constraint addition will fail and the deployment will be broken.

Consider adding a data migration step before AddConstraint that handles any rows where both FKs would be null (delete them, or confirm with a query that none exist in production and add a comment documenting that assumption).

on_delete=django.db.models.deletion.CASCADE,
to="contentcuration.organization",
),
),
migrations.AddConstraint(
model_name="invitation",
constraint=models.CheckConstraint(

@rtibbles rtibbles Jun 29, 2026

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.

Also worth noting that doing this during migrations might cause a failed deploy, because of migration timeouts. We add indexes concurrently to avoid this, so a similar strategy would need to be applied here: https://medium.com/@timmerop/how-to-add-a-uniqueconstraint-concurrently-in-django-2043c4752ee6

@rtibblesbot rtibblesbot Jun 29, 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.

Good point — I missed the deployment timeout risk entirely. Validating a constraint against existing rows acquires a lock that blocks concurrent writes, and on a table with many rows can hold it long enough to cause a migration timeout. The concurrent approach (add the constraint with NOT VALID, then validate separately in a non-blocking step) is the right pattern here. Thanks for flagging it.

check=models.Q(
models.Q(
("channel__isnull", False), ("organization__isnull", True)
),
models.Q(
("channel__isnull", True), ("organization__isnull", False)
),
_connector="OR",
),
name="channel_organization_exclusivity",
),
),
]
49 changes: 42 additions & 7 deletions contentcuration/contentcuration/models.py
View file
Open in desktop
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,21 +3735,51 @@ 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(
"Organization", null=True, blank=True, on_delete=models.CASCADE
)

class Meta:
verbose_name = "Invitation"
verbose_name_plural = "Invitations"
constraints = [
models.CheckConstraint(
name="channel_organization_exclusivity",
check=(
Q(channel__isnull=False, organization__isnull=True)
| Q(channel__isnull=True, organization__isnull=False)
),
)
]

@rtibblesbot rtibblesbot Jun 29, 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.

blocking: accept() dispatches to self.accept_channel_invitation(user) (this line) and self.accept_organization_invitation(user) (line 3756), but the concrete implementations are defined as _accept_channel_invitation and _accept_organization_invitation — note the leading underscore. Neither public name exists, so every call to invitation.accept() raises AttributeError at runtime, breaking the pre-existing channel path as well as the new organization path. Fix: either drop the underscore from the two def _accept_* definitions, or add it to the two dispatch calls inside accept().

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)

@rtibblesbot rtibblesbot Jun 29, 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.

blocking: OrganizationRole.objects.create() is called here without a role argument. Because role has no default and no null=True, Django writes an empty string to the column. The elif block that follows then conditionally sets the role and calls organization_role.save() — that is two DB writes per acceptance, with an invalid row visible to concurrent queries between them. If share_mode is anything other than the three known values, the elif chain falls through and save() persists the empty-string role silently (data corruption, no error raised).

Also: OrganizationRole has unique_together = ("user", "organization"), so a second call to accept() raises IntegrityError. The channel path uses M2M.add() which is idempotent — use update_or_create here to match that behaviour.

Recommended fix: derive the role value first, then do a single update_or_create:

role_map = {VIEW_ACCESS: ORGANIZATION_VIEWER, EDIT_ACCESS: ORGANIZATION_EDITOR, ADMIN_ACCESS: ORGANIZATION_ADMIN}
role = role_map.get(self.share_mode)
if role is None:
    raise ValueError(f"Unrecognised share_mode: {self.share_mode!r}")
OrganizationRole.objects.update_or_create(
    user=user, organization=self.organization,
    defaults={"role": role, "status": ORGANIZATION_ROLE_STATUS_ACTIVE},
)

self.channel.editors.add(user)

def _accept_organization_invitation(self, user):
organization_role = OrganizationRole.objects.create(
user=user,
organization=self.organization,
status=ORGANIZATION_ROLE_STATUS_ACTIVE,
)
if self.share_mode == VIEW_ACCESS:
organization_role.role = ORGANIZATION_VIEWER
elif self.share_mode == EDIT_ACCESS:
organization_role.role = ORGANIZATION_EDITOR
elif self.share_mode == ADMIN_ACCESS:
organization_role.role = ORGANIZATION_ADMIN
organization_role.save()

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