Add Organization and OrganizationRole models to studio#5953
Conversation
|
👋 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. |
yasinelmi
changed the title
bjester
left a comment
bjester
left a comment
There was a problem hiding this comment.
Overall, the foundational pieces are present, but I have some questions on the use cases of some fields. My requested changes are mostly around indices, but I think the area most in need of attention is the invitation architecture. The implementation would have benefited some planning on what that looked like, for example a Github issue/spec written up first. It feels like a really good idea to reuse the existing Invitation structure as much as possible, and without significant details in the PR description, I'm not able to gauge on what the decisions and motivations were for the current implementation.
| tagline = models.CharField( | ||
| max_length=150, blank=True, null=True, help_text="Short description" | ||
| ) |
There was a problem hiding this comment.
Does this have a use case? It seems influenced by Channel.tagline which does have a clear purpose.
| ) | ||
|
|
||
| # Metadata | ||
| preferences = models.TextField( |
There was a problem hiding this comment.
Having this for parity with User makes sense to me and will likely be useful at the organization level.
Although, this uses the same field type as User.preferences, which is a TextField, but that field should really have been a JSONField to start with. Please make this a JSONField.
| preferences = models.TextField( | ||
| default=DEFAULT_USER_PREFERENCES, help_text="Organization preferences as JSON" | ||
| ) | ||
| settings = JSONField(default=dict, help_text="Additional organization settings") |
There was a problem hiding this comment.
What's the use case for this field? We've been trying to be stricter with new JSON fields, e.g. validating against a JSON schema. Although, ideally we leverage relational models as much as possible. For existing patterns like the preferences field, it's totally acceptable to have it be JSON. For this field, it would nice if we either had well-defined models or a JSON schema for whatever capabilities this supports.
| verbose_name_plural = "Organizations" | ||
| ordering = ["name"] | ||
| indexes = [ | ||
| models.Index(fields=["name"], name="organization_name_idx"), |
There was a problem hiding this comment.
This is likely redundant. The field already has db_index=True. For a CharField, I believe with db_index=True, Django will create both a b-tree and text pattern ops index on it, which seems fine.
| models.Index( | ||
| fields=["deleted", "public"], name="organization_deleted_public_idx" | ||
| ), |
There was a problem hiding this comment.
I think we could make this a conditional index. Operationally, we're unlikely to query deleted=True often enough to warrant it. So this index could be conditional on deleted=False.
Although, I'd feel fine leaving out this index entirely until we see the need for it.
| models.Index( | ||
| fields=["user", "organization"], name="org_role_unique_idx" | ||
| ), |
There was a problem hiding this comment.
This is a redundant index with what's already defined by unique_together, but also, just FYI, it isn't actually a unique index either.
| models.Index( | ||
| fields=["user", "organization"], name="org_role_unique_idx" | ||
| ), | ||
| models.Index(fields=["status"], name="org_role_status_idx"), |
There was a problem hiding this comment.
Unless there's a planned use for this, my guess is we'd end up utilizing the compound index with organization, so this may not be needed. But depending on whether we consolidate with the existing Invitation model, we may not need status?
| permissions = JSONField( | ||
| default=dict, help_text="JSON object defining user's permissions" | ||
| ) |
| role = models.CharField( | ||
| max_length=100, | ||
| help_text="Role name (e.g., Admin, Editor, Viewer, Content Creator)", | ||
| ) |
There was a problem hiding this comment.
I think it would be good to have explicit choices for this field.
| organization_role = models.ForeignKey( | ||
| "OrganizationRole", | ||
| null=True, | ||
| blank=True, | ||
| related_name="organizations", | ||
| on_delete=models.SET_NULL, | ||
| help_text="Tracking the creator or primary role associated with this organization", | ||
| ) |
There was a problem hiding this comment.
This makes sense, but I do see some possibly problematic aspects to this in the data architecture. For example, presumably this FK role has something like "superuser" privileges for the org, but the model's role value could be something less privileged. There isn't a constraint to ensure logical integrity.
An alternative might be adding a boolean flag on the role model which signifies this, then a unique constraint can be made to ensure there's only one for an org, and another constraint to enforce the role has admin privileges. Enforcing this strictly means we can better rely on the data as a source of truth, making assumptions without as much defensive code.
3 participants
Summary
Added two new models for organizational structure and role-based access management.
Organization Model
Represents organizations that manage channels
Fields: name, description, tagline, website, email, thumbnail, public, deleted
Foreign keys to Channel and OrganizationRole
Includes preferences and settings (JSON)
OrganizationRole Model (Through Model)
Manages User-Organization relationship with role-based access
Fields: role, description, permissions (JSON), status, joined_at, invitation_accepted_at, invited_by
Status choices: active, inactive, pending, suspended, declined
Helper methods: accept_invitation(), decline_invitation()
Unique constraint: one role per user per organization
AI usage
Used AI to generate models but they were carefully reviewed