pinax / django-user-accounts

User accounts for Django
MIT License
1.22k stars 355 forks source link

Why does account.models.Account still exist? #220

Open irothschild opened 8 years ago

irothschild commented 8 years ago

It would make sense for Pinax to easily allow new fields to be added to the Account model (see https://github.com/pinax/django-user-accounts/issues/60 ) so that account settings can be stored together and edited and saved from a single form/view. Since the recommended way is to instead put fields in a custom User model, why have an Account model anymore? Why not just move the timezone and language into a Custom User model that we can inherit from?

Has the django community really not addressed the need to be able to subclass models, forms, and views and just have them work?

jtauber commented 8 years ago

This is worthy of further discussion. Some thoughts to include in such a discussion...

There is a school of thought that says that the user model should just be used for authentication and any other information should be in a different model (@brosner is that the heart of what you were saying the other day?)

I think it's useful to distinguish:

so the question is one model, two models or three?

irothschild commented 8 years ago

I tend to agree with keeping the auth User model just for auth so would favour a 2 or 3 model solution. However, it would be wonderful if there was an easy way for any app to be able to add new fields to the settings/profile model and form. Yes, there could be conflicts but I see that as an opportunity to share the same settings across apps. Pinax-compliant apps could have a registry of known settings fields with their defined valid contents.

jtauber commented 8 years ago

Yeah, I definitely want to keep this conversation going. Lots of good stuff could come of it.

the-chris-mitchell commented 7 years ago

Was any progress made on this? I realise we can subclass 'Account', but that would involve a bit of work (having to subclass all forms and views that refer to it). It would be really awesome if we could have something similar to the 'AUTH_USER_MODEL' option but for the 'Account' model.

jtauber commented 7 years ago

@eldonuts what is the nature of stuff you're wanting to add?

the-chris-mitchell commented 7 years ago

@jtauber specifically I was wanting to add additional region fields, such as Country and City.

PoDuck commented 7 years ago

I'm assuming that actual custom user models, as in using django's built in support for them, are not implemented. I see mention of "custom user model" in several places, but nothing seems to be actually done in regard to the django custom user model. Has this idea died?

It seems to me that it would be best to implement that, and allow people to do whichever they want, in terms of either storing information in another model or not. It would certainly make it easier to deal with the duplication of fields, as in storing a username and email address for those using email logins. As it is now, we don't have a choice, since attempting to use a custom user seems to break things.

jtauber commented 7 years ago

@PoDuck If there are bugs in custom user models (i.e. AUTH_USER_MODEL), that's a separate issue than what is being talked about here.

PoDuck commented 7 years ago

Is my assumption that AbstractBaseUser is neither implemented nor supported wrong, or is my implementation wrong? If it is the former, I gave my reason for not only supporting custom user models, but going completely to a custom user model. If it is the latter, I appologize.

In other words, I was discussing what the original post was talking about. Or at least I thought I was.

jtauber commented 7 years ago

The original post is asking why we need Account given the ability to have custom user models.

I had proposed that user models be restricted to auth purposes only (so custom user models are still relevant) and that Account be maintained for private account settings. @irothschild agreed with this division but asked for ways we would make it easier to extend Account.

If you're having problems using a custom user model, we should fix them, but it would be better to file issues for them separately and keep this issue for discussion about how to better extend Account for custom private account settings.

PoDuck commented 7 years ago

My question is, why should it be so limited? A custom user model as a base could be left as authentication only if desired, but could also be extended easily if desired. I'm not grasping why it should be so opinionated. Is there something that I'm missing that it would cause problems with?

jtauber commented 7 years ago

I am confused by what your actual issue is @PoDuck. At first you seemed to be saying you were unable to use a customer user model at all. Let's fix that (by filing a separate issue).

I'm not sure what that has to do with the justification I've given here for keeping auth, account settings and profiles separate (which is what we've done on Pinax sites for 9 years, btw) and the call for how to make things like account settings and profiles more extensible if separate models.

If you don't like that best practice, that's fine, you can just avoid this discussion; but please report actual bugs you're experiencing with custom user models separate from this issue.

PoDuck commented 7 years ago

It seems as though you are taking my comments as either an attack, or an off topic statement. I am certainly not attacking you, or the app. I also don't feel that my question is off topic.

Anyway, my opinion has been voiced. I'll leave it for others to decide.

jtauber commented 7 years ago

All I'm trying to do is separate bugs from design discussions.

You originally said "attempting to use a custom user seems to break things". I'm asking you to file an issue for that separate from discussing whether you think the model split is a good approach or not.

jtauber commented 7 years ago

What I'd like to see in this issue is proposals for how to make it easier to add new settings (and how that works, in particular, with the existing account settings UI).

The way we've typically done it on sites is to have multiple settings models, separate forms, and then separate pages off the account settings subnav.

This is more loosely coupled than what @irothschild was suggesting in his second comment and maybe it's still the right approach. In which case, is better documentation all that's needed or are there more things DUA could do to facilitate the approach?

jtauber commented 7 years ago

That does, however, make it more difficult to, say, combine multiple groups of settings into a single form (say you wanted to put timezone in same form as some site-specific setting like location).

PoDuck commented 7 years ago

I was only citing my problem because it seems it would make my life easier if it were already built in, so it was in explanation that I feel I have a use case for it. I don't think I am to the point that I need to open another issue, as I haven't reached the end of my ability to troubleshoot it yet.

I assumed that there was still some debate as to whether it is a good idea to go with an abstract user or not, not that the discussion was limited to how best to work with the more limiting accounts django has.

As far as the documentation is concerned, I think that one thing should be made clear that isn't. There is reference to "custom user model" being a feature, even in the markdown for this repo, as though there is some built in support for changing the actual user model, as would be done using AbstractBaseUser. It is by no means clear what the extent of that support is though, or if there is any built in ability to facilitate it. The only mention of it in the docs seems to be in the FAQ, where it explains that using a custom user model would remove the need for the duplication of storage.

As for how to make it easier for people to implement, I have just started trying to use this, so I'll see how it goes and get back to you on it.

jtauber commented 7 years ago

I still don't follow when you say "the discussion was limited to how best to work with the more limiting accounts django has". We're talking about this app's own Account model, not what's built in to Django. Did you mean the "more limiting accounts this app has" ?

We're open to changing how account settings works—that's the point of this issue—but the OP quickly agreed with me that distinguishing auth from account settings (and public profile information from both of them) in separate models was a good idea and I was encouraging further discussion of how best to achieve that, particularly with regard to extensibility in account settings.

When you said "Has this idea died?" it was confusing to me what idea you meant because even the OP agreed in multiple models and so no one was left advocating for using custom user models for account settings (until now, if I understand you correctly). And so when you said custom users seem to break things, I took that as an unrelated bug we needed to fix, not a return to the original question from the OP of why have a separate Account model at all.

Sorry for my confusion.

jtauber commented 7 years ago

I should add that I'm not even sure a single Account model is the way to go. Different apps might have their own settings and the current approach supports lots of loosely-coupled settings that can be plugged into the settings UI without the apps having to know about each other. As I mentioned earlier today, the main downside I see to this is if you wanted these different collections of settings to be on the same form rather than separate sub-pages of the settings UI.