Skip to content
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

12795 custom user model #15005

Merged
merged 11 commits into from
Feb 5, 2024
Merged

12795 custom user model #15005

merged 11 commits into from
Feb 5, 2024

Conversation

arthanson
Copy link
Collaborator

@arthanson arthanson commented Feb 1, 2024

Fixes: #12795

Did custom user model - dumped and compared SQL schemas and they look correct. Any extra testing suggestions welcome.

Did not do groups for several reasons:

  1. All suggestions (SO mostly) suggest just sub-classing Group: https://stackoverflow.com/questions/2181039/how-do-i-extend-the-django-group-model, https://www.reddit.com/r/django/comments/lvakzm/how_should_i_customize_the_group_model/ - Not sure if there is anything in Django that would be an issue with a custom group model (I don't think so off hand) but Django doesn't have any get_group_model like it does for User.
  2. There is no BaseGroup or AbstractBaseGroup - although Group model is very simple.
  3. There is an old Django ticket (https://code.djangoproject.com/ticket/29748) to allow swapping group model, although nothings been done in 2 years. Bit worried if we do a custom group and Django does theirs it will cause issues (though more minor concern).
  4. There is no pressing current need to have a custom Group, then only potential suggestion is for adding a description.
  5. Adding a description would not require anything besides subclassing it as it wouldn't be a perf hit, description would'nt be needed in perf critical areas.

Wouldn't be a big thing to convert Groups bug just some potential issues and not a big reason to do it right now IMHO. Also, an option is to potentially do the work for Django and submit a PR - though more work to handle the relation between customizable User and Group models.

@jeremystretch jeremystretch added this to the v4.0 milestone Feb 1, 2024
@arthanson arthanson changed the title DRAFT: 12795 custom user model 12795 custom user model Feb 2, 2024
@arthanson arthanson marked this pull request as ready for review February 2, 2024 17:34
Copy link
Member

@jeremystretch jeremystretch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! On first pass this seems to work just fine, however I'm extremely cautious of running across an unforeseen landmine. Will continue to do some further experimentation. We also need to figure out what to do with the NetBoxUser content type.

netbox/users/models.py Outdated Show resolved Hide resolved
netbox/users/migrations/0002_squashed_0004.py Show resolved Hide resolved
netbox/users/models.py Outdated Show resolved Hide resolved
netbox/users/models.py Outdated Show resolved Hide resolved
netbox/netbox/settings.py Outdated Show resolved Hide resolved
@jeremystretch
Copy link
Member

One other nit: We'll need to ensure that the user and group content types are available in ContentType.objects.public().

@arthanson
Copy link
Collaborator Author

One other nit: We'll need to ensure that the user and group content types are available in ContentType.objects.public().

@jeremystretch updated the registry code and updated the migration to migrate the CustomField from any existing ct pointers to the new one.

netbox/users/apps.py Show resolved Hide resolved
@jeremystretch jeremystretch merged commit 317bef6 into feature Feb 5, 2024
6 checks passed
@jeremystretch jeremystretch deleted the 12795-custom-user branch February 5, 2024 18:24
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants