-
Notifications
You must be signed in to change notification settings - Fork 644
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.
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
Switch Organizations to use TPT inheritance (was fix for Organization-User FK) #4926
Conversation
@@ -20,6 +20,11 @@ public class Organization | |||
public int Key { get; set; } | |||
|
|||
/// <summary> | |||
/// Organization account (User) foriegn key. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
foriegn
-> foreign
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But, what about 'i before e, except after c'? I blame my elementary teachers!
@joelverhagen @agr @cristinamanum @scottbommarito Pushed new commit per our discussion. Now Organizations are implemented using TPT inheritance. Note that the auto-generated migration tried to recreate the Users table as-is... I was able to remove this and not have it detect pending changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is much cleaner, looks great
/// 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. | ||
/// With the addition of organizations, the users table effectively becomes an account table. Organizations accounts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: This comment will be really awkward to read in the future when the organization feature has existed for a while. I would remove with the addition of organizations
and restructure the remainder of the paragraph accordingly.
/// Organizations should not support UserRoles or Credentials, but this is not constrained by the | ||
/// database. In the future, we could consider renaming the Users table to Accounts and adding a | ||
/// Users table to constrain UserRoles and Credentials to only User accounts. | ||
/// With the addition of organizations, the users table effectively becomes an account table. Organizations accounts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! We should probably run the migration against DEV, INT, and PROD using Update-Database -Script
first because of the weirdness we encountered.
@joelverhagen Sounds good... I'll update the deployment item to request this. |
During PR #4904 , FK property was removed based on feedback but I didn't check if this affected the migration.