fix: Change User class in backend/models.py to use email as uniqu…

[James-Makela]

…e field.

Description

Here I have made changes to the User Model in backend/models.py. The changes make the email field unique, and required, rather than the username. At this stage, some other things will still need working out. All local tests have passed so far. I am open to creating some new tests to test the structure and behavior of the user model, but not sure how to go about that yet.

To Do

• Fix, Logged in as no longer displaying anything
• Fix, Unable to add first and last name after changes
• Identify any other bugs introduced

Checklist

• Ran the Black Formatter and
  djLint-er on any new code
  (checks
  will
  fail without)
• Made any changes or additions to the documentation where required
• Changes generate no new warnings/errors
• New and existing unit tests pass locally with my
  changes

What type of PR is this?

• 🐛 Bug Fix
• 🚨 Breaking Change

Helpful Info Found Along the Way

• Django ticket where another user had the same issue
• Relevant documentation

Added/updated tests?

• 🙋 no, because I need help

Related PRs, Issues etc

• Related Issue bug: Setup process allows you to create more than one superuser with the same email. #367

[TreyWW]

Hi @James-Makela, just wondering if this will be worked on further at any point?

[James-Makela]

Hi Trey, unfortunately I am flat out for the next couple of weeks. I could continue after that. However if you or anyone else wants to take over that is cool too.

[James-Makela]

Update:

• The issues with this change I have identified so far seem to go away after refreshing the page once or twice.

Next Steps:

• Manual testing of program behavior:
  • before the change - from scratch
  • After the change - from scratch
  • After the change - set up user before changing code

Questions:

• Is changing the user model in this way sufficient(so far seems to be), or do we want to create a custom user class from scratch?

[TreyWW]

Hey @James-Makela, what you said sounds good. As for the user class, we have a custom one already in models.py so you can override parts of that if that's what you mean?

Let me know if you have any more questions, great work though :)

[James-Makela]

Hey @James-Makela, what you said sounds good. As for the user class, we have a custom one already in models.py so you can override parts of that if that's what you mean?

Ah yes, that appears to be what I am doing.

I have gone through some testing today, Setting up from scratch after moving to test the branch for this fix.

Tests Involved:

• Setup according to installation instructions.
  • Identical behavior
• Create superuser
  • Prompts for email first after fix
• View profile
  • Some issues with profile display(only showing "logged in as" after refresh), however identical behavior.
• Change name
  • No issues - Identical behavior
• Attempt to create another superuser with the same email
  • Unable to do after fix is implemented - user is prompted to enter a unique email
• Attempt to create another superuser with the same username
  • Handled correctly before fix, however crashes after fix.
    • Doesn't appear to be checking if the username is unique before trying to insert superuser into the db.
    • I will need to figure out where to fix that behavior next.

[TreyWW]

Hi @James-Makela, sorry I missed the PR update. Anything you need me to do at this point?

I appreciate your patience

[James-Makela]

Hi Trey,

I made a little progress on this. I have extended the CustomUserManager class, and been able to get the superuser creation working with only email. However going into the admin panel, usernames are still there and all of the forms may need to be tweaked to go without usernames.

Alternatively I have tried to change the CustomUserManager class to also take in a unique username. But I am having trouble understanding this part in the original usermodel, to transfer over to the custom usermodel:

GlobalUserModel = apps.get_model(
            self.model._meta.app_label, self.model._meta.object_name
        )
        username = GlobalUserModel.normalize_username(username)

Here is my current state of the extended CustomUserManager class, mostly taken from the Django's implementation:

class CustomUserManager(BaseUserManager):
    def _create_user(self, email, password, **extra_fields):
        if not email:
            raise ValueError("The given email must be set")
        email = self.normalize_email(email)
        user = self.model(email=email, **extra_fields)
        user.set_password(password)
        user.save(using=self._db)
        return user

    def create_user(self, email, password=None, **extra_fields):
        extra_fields.setdefault("is_staff", False)
        extra_fields.setdefault('is_superuser', False)
        return self._create_user(email, password, **extra_fields)

    def create_superuser(self, email, password=None, **extra_fields):
        extra_fields.setdefault("is_staff", True)
        extra_fields.setdefault("is_superuser", True)

        if extra_fields.get("is_staff") is not True:
            raise ValueError("Superuser must have is_staff=True.")
        if extra_fields.get("is_superuser") is not True:
            raise ValueError("Superuser must have is_superuser=True.")

        return self._create_user(email, password, **extra_fields)

    def get_queryset(self):
        return (
            super()
            .get_queryset()
            .select_related("user_profile", "logged_in_as_team")
            .annotate(notification_count=(Count("user_notifications")))
        )

I feel like its almost there but I will need advice on whether you want to go in the direction of no usernames at all, or enforced unique usernames, as well as unique emails, and whether I am hopefully on the right track here.

[TreyWW]

I'm wondering if in models.py class User you could just add username and email fields manually and add unique=True and Django might do the rest

[James-Makela]

Trying that, with the whole user class as:

class User(AbstractUser):
    objects = CustomUserManager()  # type: ignore

    email = models.EmailField(max_length=150, unique=True)
    username = models.CharField(max_length=150, unique=True)

    USERNAME_FIELD = "email"
    REQUIRED_FIELDS = ["username"]

    logged_in_as_team = models.ForeignKey("Team", on_delete=models.SET_NULL, null=True, blank=True)
    awaiting_email_verification = models.BooleanField(default=True)

    class Role(models.TextChoices):
        #        NAME     DJANGO ADMIN NAME
        DEV = "DEV", "Developer"
        STAFF = "STAFF", "Staff"
        USER = "USER", "User"
        TESTER = "TESTER", "Tester"

    role = models.CharField(max_length=10, choices=Role.choices, default=Role.USER)

Strangely this results in allowing the user to enter a non-unique username without error, then crashes while trying to store the data in the database:

django.db.utils.IntegrityError: UNIQUE constraint failed: backend_user.username

[TreyWW]

Ah yeah, that's cause we don't do validation in the create account view. We'd have to just add User.objects.filter and check if one exists

[TreyWW]

Line 71 backend/views/core/auth/create_account.py should handle it... hmm

[James-Makela]

I notice backend/views/core/auth/create_account.py at line 81 sets the username to be the email.
Changing the overridden _create_user function to do the same seems to work perfectly so far.

_create_user function:

    def _create_user(self, email, password, **extra_fields):
        if not email:
            raise ValueError("The given email must be set")
        email = self.normalize_email(email)
        user = self.model(email=email, username=email, **extra_fields)
        user.set_password(password)
        user.save(using=self._db)
        return user