5971: added organizations to invitation model#6008
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 |
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
Three blocking issues must be resolved before merge: broken method dispatch in accept(), a two-write invalid-state pattern in _accept_organization_invitation, and missing acceptance-criteria tests. One migration safety concern is flagged as a suggestion. CI was pending at review time.
- blocking
accept()calls non-existent method names — every invitation acceptance raisesAttributeError(see inline models.py:3754) - blocking
_accept_organization_invitationwrites an empty-stringroleon firstcreate(), then overwrites it; unknownshare_modesilently persists invalid data; re-acceptance raisesIntegrityError(see inline models.py:3767) - blocking Acceptance criteria require unit tests for the invitation–organization relationship, the mutual-exclusivity constraint violation case, and the
share_mode→ org-role mapping — none are present in this PR - suggestion
AddConstraintin the migration may fail at deployment if any existing rows havechannel=NULL(see inline migration:19) - praise Splitting
accept()into named per-type helpers is a clean refactor — the dispatch logic is now symmetrical and the original comment about the nullable channel field is correctly removed
@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
| ), | ||
| ) | ||
| ] | ||
|
|
There was a problem hiding this comment.
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().
| self.channel.editors.remove(user) | ||
| self.channel.viewers.add(user) | ||
| else: | ||
| self.channel.viewers.remove(user) |
There was a problem hiding this comment.
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},
)| name="organization", | ||
| field=models.ForeignKey( | ||
| blank=True, | ||
| null=True, |
There was a problem hiding this comment.
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).
| ), | ||
| migrations.AddConstraint( | ||
| model_name="invitation", | ||
| constraint=models.CheckConstraint( |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
3 participants
Summary
As detailed in Issue #5971, this PR adds organizations to the invitation model. The saving behavior for organizations is slightly different to channels, as there is a separate model for org roles.
I have also omitted tests in this PR, as this is just a model change and does not affect current code (organizations are fully optional in the invitation). I suggest moving testing to the next implementation step (Organization/Invitation viewsets).
References
Direct PR for Issue #5971.
Reviewer guidance
As part of these changes, migrations were run successfully. I tried running
make test, but it ran into 905 failures, mostly related to S3Storage errors. No linting was performed, since no frontend work was done.AI usage
Only basic VS code autocomplete was used.