feat: user profile model (for subscriptions) #526

2025-04-30T15:54:38Z

yannham commented

2025-04-30 15:54:38 +00:00

(Migrated from github.com)

This is preliminary work for #173. The former mandates adding a subscriptions many-to-many relation between users and nixpkgs issues, but since User is a built-in model in Django, we have to do something special. Common approaches are:

Use a custom MyUser model that extends the standard one, and configure Django to use that by default
Create a separate Profile model in one to one relationship with users

I went for 2., which seems to be popular in Django when one just wants to add data and relationships to User that don't really relate to the core functions of authentication, passwords, access rights, etc. This PR thus adds a new Profile model in one-to-one relationship with User, which stores the subscriptions, and the corresponding migration.

This is preliminary work for #173. The former mandates adding a `subscriptions` many-to-many relation between users and nixpkgs issues, but since `User` is a built-in model in Django, we have to do something special. Common approaches are: 1. Use a custom `MyUser` model that extends the standard one, and configure Django to use that by default 2. Create a separate `Profile` model in one to one relationship with users I went for 2., which seems to be popular in Django when one just wants to add data and relationships to `User` that don't really relate to the core functions of authentication, passwords, access rights, etc. This PR thus adds a new `Profile` model in one-to-one relationship with `User`, which stores the subscriptions, and the corresponding migration.

fricklerhandwerk commented

2025-04-30 18:24:08 +00:00

(Migrated from github.com)

Why not add a ManyToManyField to User on NixpkgsIssue with an appropriate related_name?

Why not add a ManyToManyField to User on NixpkgsIssue with an appropriate `related_name`?

yannham commented

2025-05-02 08:43:19 +00:00

(Migrated from github.com)

Why not add a ManyToManyField to User on NixpkgsIssue with an appropriate related_name?

This is a possibility I thought about, but while many-to-many relations can indeed be defined on both end of the relation, I still feel like there's a privileged model where it makes more sense to define it, because you'll want to get the subscriptions from a user, much less all user subscribed from an issue (I know it doesn't make a difference in practice at runtime, but if only for code discoverability and readability). Defining it on NixpkgsIssue instead felt like leaking a technical Django detail (User is predefined so it's more annoying to extend).

Another consideration is if we'll add more things to a user profile, defining the relationship on the other end is going to be even worse. If you're discovering the codebase and try to understand what is associated to a user, you have to either know it in advance or search all the other models. That being said, I don't know if we plan to add more relations to User.

All of this made me lean toward the Profile approach (which is also a common one mentioned in Django docs), but it wasn't a clear-cut choice. Happy to reconsider if you think it's worth moving the relation to NixpkgsIssue to get rid of the profile handling.

> Why not add a ManyToManyField to User on NixpkgsIssue with an appropriate related_name? This is a possibility I thought about, but while many-to-many relations can indeed be defined on both end of the relation, I still feel like there's a privileged model where it makes more sense to define it, because you'll want to get the subscriptions from a user, much less all user subscribed from an issue (I know it doesn't make a difference in practice at runtime, but if only for code discoverability and readability). Defining it on `NixpkgsIssue` instead felt like leaking a technical Django detail (`User` is predefined so it's more annoying to extend). Another consideration is if we'll add more things to a user profile, defining the relationship on the other end is going to be even worse. If you're discovering the codebase and try to understand what is associated to a user, you have to either know it in advance or search all the other models. That being said, I don't know if we plan to add more relations to `User`. All of this made me lean toward the `Profile` approach (which is also a common one mentioned in Django docs), but it wasn't a clear-cut choice. Happy to reconsider if you think it's worth moving the relation to `NixpkgsIssue` to get rid of the profile handling.

fricklerhandwerk commented

2025-05-02 08:57:33 +00:00

(Migrated from github.com)

We'll need the m2m on Users anyway if we want to notify all subscribers

yannham commented

2025-05-02 09:13:11 +00:00

(Migrated from github.com)

We'll need the m2m on Users anyway if we want to notify all subscribers

Since Profile is in one-to-one relationship with User, it's entirely isomorphic to have a separate profile or to put the relationship directly on User (in that you can always achieve the same in both situations). I think the question is rather: do we optimize for code readability, where IMHO, I would expect the relationship to be defined on User or Profile, but at the cost of a separate model, an additional DB indirection, and the boilerplate (albeit very small) of handling the case of a User that doesn't have yet a profile, etc. I think both approaches are reasonable; I slightly prefer the current one obviously, but once again, happy to go the other way if you think this is better.

> We'll need the m2m on Users anyway if we want to notify all subscribers Since `Profile` is in one-to-one relationship with `User`, it's entirely isomorphic to have a separate profile or to put the relationship directly on `User` (in that you can always achieve the same in both situations). I think the question is rather: do we optimize for code readability, where IMHO, I would expect the relationship to be defined on `User` or `Profile`, but at the cost of a separate model, an additional DB indirection, and the boilerplate (albeit very small) of handling the case of a `User` that doesn't have yet a profile, etc. I think both approaches are reasonable; I slightly prefer the current one obviously, but once again, happy to go the other way if you think this is better.

fricklerhandwerk (Migrated from github.com) reviewed

2025-05-02 13:15:01 +00:00

src/website/webview/models.py

					
				@ -2,0 +11,4 @@

				class Profile(models.Model):

				    """

				    Profile associated to a user, storing extra non-auth-related data such as

				    active issue subscriptions.

fricklerhandwerk (Migrated from github.com) commented

2025-05-02 13:15:01 +00:00

    Profile associated to a user, storing data for our business logic,
    as opposed to Django's native `User` which deals with authentication and authorisation.

```suggestion Profile associated to a user, storing data for our business logic, as opposed to Django's native `User` which deals with authentication and authorisation. ```

fricklerhandwerk commented

2025-05-02 13:15:37 +00:00

(Migrated from github.com)

Alright, then please add the signal to auto-create a profile with each user here as well, so we don't leave opportunities for breakage.

yannham commented

2025-05-05 14:19:48 +00:00

(Migrated from github.com)

I added a signal for newly created users, but that won't apply to existing users. Is that an issue at this point? Though we can just use get_or_create in place of get also and that shouldn't a problem.

I added a signal for newly created users, but that won't apply to existing users. Is that an issue at this point? Though we can just use `get_or_create` in place of `get` also and that shouldn't a problem.

👍 1

fricklerhandwerk (Migrated from github.com) approved these changes

2025-05-05 14:43:45 +00:00

Sign in to join this conversation.

No reviewers

fricklerhandwerk