Schema changes for Organization membership #4904
Conversation
chenriksson
requested review from
joelverhagen,
agr,
loic-sharma,
xavierdecoster,
shishirx34,
ryuyu,
cristinamanum,
skofman1 and
scottbommarito
| /// - Pushing new package versions | ||
| /// - Unlisting packages | ||
| /// </summary> | ||
| public bool IsCollaborator |
There was a problem hiding this comment.
We should be careful with this, because I don't think it is usable in LINQ to SQL.
There was a problem hiding this comment.
In general I prefer methods here since it's a stronger signal to the caller that this is not a real SQL property and should only be used after a query is completed (i.e. in memory).
There was a problem hiding this comment.
Removed Membership.IsCollaborator and User.IsOrganization properties. If callers need something like that, we can add back helpers in later PRs.
| .WillCascadeOnDelete(false); // Disabled to prevent multiple cascade paths. | ||
|
|
||
| modelBuilder.Entity<Membership>() | ||
| .HasKey(m => new { m.OrganizationKey, m.MemberKey }) |
There was a problem hiding this comment.
Why the composite key? EF will already be creating an index for OrganizationKey.
There was a problem hiding this comment.
EF requires a PK on the Membership table, and throws an exception if I don't have it.
This is essentially a many-to-many table where the 2 foreign keys become the composite PK, same as with the PackageRegistrationOwners table. The only difference is that this includes additional state (IsAdmin), so I need to treat it as two one-to-many relations.
There was a problem hiding this comment.
Oh, I see MemberKey is a Key on the Users table. I misunderstood. Perfect 👌
| /// <summary> | ||
| /// Whether this is an <see cref="Organization"/> account. | ||
| /// </summary> | ||
| public bool IsOrganization |
There was a problem hiding this comment.
AFAIK this is unusable in LINQ to SQL.
| /// <summary> | ||
| /// Whether the <see cref="Member"/> is an administrator for the <see cref="Organization"/>. | ||
| /// | ||
| /// Administrators have the following capabilities: |
There was a problem hiding this comment.
Should say explicitly that members that are not admins are collaborators.
It might not be best to list out all of the capabilities here. This comment is unlikely to change if we change the actual capabilities, so I'm concerned this list will become outdated and misleading.
If you do leave the list, you should specify what collaborators can do.
| /// | ||
| /// The Users table contains both User and Organization accounts. The Organizations table exists both | ||
| /// as a constraint for Membership as well as a place for possible Organization-only settings. If User | ||
| /// and Organization settings diverge, we can consider creating a separate UserSettings table as well. |
There was a problem hiding this comment.
nit: there are already fields in the Users table that don't apply to Organizations.
Ideally we should already create UserSettings but the value of doing so is just too low to prioritize it.
| } | ||
| set | ||
| { | ||
| throw new NotSupportedException(); |
There was a problem hiding this comment.
Why do these all have set? Is set ever used? Can we remove set and get rid of these NotSupportedExceptions?
There was a problem hiding this comment.
The setters exist on the IEntitiesContext properties. The test context doesn't support it, so throw ensures it isn't used.
There was a problem hiding this comment.
Didn't realize these were tests, woops
| /// <summary> | ||
| /// Account (User) name. | ||
| /// </summary> | ||
| public string Name |
There was a problem hiding this comment.
Don't think this works in LINQ to SQL. Not sure tho.
|
In general I think we should avoid convenience properties on EF entities since it is unclear from the interface if they are usable in LINQ to SQL. Maybe we can add analogous methods as they are needed by our business logic. Also, what SQL does this migration generate? |
| /// <summary> | ||
| /// Organization account (User). | ||
| /// </summary> | ||
| public User Account { get; set; } |
There was a problem hiding this comment.
AccountKey = Account.Key , right? If so, why do we need AccountKey as a separate property?
|
@scottbommarito I don't think we need a feature branch for this feature. Most of these changes should be transparent to the user until we enable onboarding. Let me know if there are concerns with this. |
Core schema changes required for membership in organizations. Tracked by #4868
The following were excluded, but can be added in a future migration once the design is finalized:
/cc @anangaur