Small cleanup for user model/manager, add util for setting up new user account after its creation

misago/users/forms/admin.py

@@ -165,10 +165,10 @@ class EditUserForm(UserBaseForm):
     )
 
     subscribe_to_started_threads = forms.TypedChoiceField(
-        label=_("Started threads"), coerce=int, choices=UserModel.SUBSCRIBE_CHOICES
+        label=_("Started threads"), coerce=int, choices=UserModel.SUBSCRIPTION_CHOICES
     )
     subscribe_to_replied_threads = forms.TypedChoiceField(
-        label=_("Replid threads"), coerce=int, choices=UserModel.SUBSCRIBE_CHOICES
+        label=_("Replid threads"), coerce=int, choices=UserModel.SUBSCRIPTION_CHOICES
     )
 
     class Meta:

misago/users/models/__init__.py

@@ -1,5 +1,6 @@
 from .rank import Rank
-from .user import AnonymousUser, Online, User, UsernameChange
+from .online import Online
+from .user import AnonymousUser, User, UsernameChange
 from .activityranking import ActivityRanking
 from .avatar import Avatar
 from .audittrail import AuditTrail

misago/users/models/online.py

@@ -0,0 +1,19 @@
+from django.conf import settings
+from django.db import models
+from django.utils import timezone
+
+
+class Online(models.Model):
+    user = models.OneToOneField(
+        settings.AUTH_USER_MODEL,
+        primary_key=True,
+        related_name='online_tracker',
+        on_delete=models.CASCADE,
+    )
+    last_click = models.DateTimeField(default=timezone.now)
+
+    def save(self, *args, **kwargs):
+        try:
+            super().save(*args, **kwargs)
+        except IntegrityError:
+            pass  # first come is first serve in online tracker

misago/users/models/rank.py

@@ -40,6 +40,8 @@ class Rank(models.Model):
             self.set_order()
         else:
             clear_acl_cache()
+        if not self.slug:
+            self.slug = slugify(self.name)
         return super().save(*args, **kwargs)
 
     def delete(self, *args, **kwargs):

misago/users/models/user.py

@@ -20,90 +20,61 @@ from misago.users.audittrail import create_user_audit_trail
 from misago.users.signatures import is_user_signature_valid
 from misago.users.utils import hash_email
 
+from .online import Online
 from .rank import Rank
 
 
 class UserManager(BaseUserManager):
-    @transaction.atomic
-    def create_user(
-        self, username, email, password=None, create_audit_trail=False,
-        joined_from_ip=None, set_default_avatar=False, **extra_fields
-    ):
-        from misago.users.validators import validate_email, validate_username
-
-        email = self.normalize_email(email)
-        username = self.model.normalize_username(username)
-
+    def _create_user(self, username, email, password, **extra_fields):
+        """
+        Create and save a user with the given username, email, and password.
+        """
+        if not username:
+            raise ValueError("User must have an username.")
         if not email:
-            raise ValueError(_("User must have an email address."))
-
-        WATCH_DICT = {
-            'no': self.model.SUBSCRIBE_NONE,
-            'watch': self.model.SUBSCRIBE_NOTIFY,
-            'watch_email': self.model.SUBSCRIBE_ALL,
-        }
-
-        if not 'subscribe_to_started_threads' in extra_fields:
-            new_value = WATCH_DICT[settings.subscribe_start]
-            extra_fields['subscribe_to_started_threads'] = new_value
-
-        if not 'subscribe_to_replied_threads' in extra_fields:
-            new_value = WATCH_DICT[settings.subscribe_reply]
-            extra_fields['subscribe_to_replied_threads'] = new_value
-
-        extra_fields.update({'is_staff': False, 'is_superuser': False})
-
-        now = timezone.now()
-        user = self.model(
-            last_login=now, 
-            joined_on=now, 
-            joined_from_ip=joined_from_ip,
-            **extra_fields
-        )
+            raise ValueError("User must have an email address.")
 
+        user = self.model(**extra_fields)
         user.set_username(username)
         user.set_email(email)
         user.set_password(password)
 
-        validate_username(username)
-        validate_email(email)
-
         if not 'rank' in extra_fields:
             user.rank = Rank.objects.get_default()
 
+        now = timezone.now()
+        user.last_login=now
+        user.joined_on=now
+
         user.save(using=self._db)
+        self._assert_user_has_authenticated_role(user)
 
-        if set_default_avatar:
-            avatars.set_default_avatar(
-                user, settings.default_avatar, settings.default_gravatar_fallback
-            )
-        else:
-            # just for test purposes
-            user.avatars = [{'size': 400, 'url': 'http://placekitten.com/400/400'}]
+        Online.objects.create(user=user, last_click=now)
 
+        return user
+
+    def _assert_user_has_authenticated_role(self, user):
         authenticated_role = Role.objects.get(special_role='authenticated')
         if authenticated_role not in user.roles.all():
             user.roles.add(authenticated_role)
         user.update_acl_key()
+        user.save(update_fields=['acl_key'])
 
-        user.save(update_fields=['avatars', 'acl_key'])
+    def create_user(self, username, email=None, password=None, **extra_fields):
+        extra_fields.setdefault('is_staff', False)
+        extra_fields.setdefault('is_superuser', False)
+        return self._create_user(username, email, password, **extra_fields)
 
-        if create_audit_trail:
-            create_user_audit_trail(user, user.joined_from_ip, user)
+    def create_superuser(self, username, email, password=None, **extra_fields):
+        extra_fields.setdefault('is_staff', True)
+        extra_fields.setdefault('is_superuser', True)
 
-        # populate online tracker with default value
-        Online.objects.create(user=user, last_click=now)
+        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 user
-
-    @transaction.atomic
-    def create_superuser(self, username, email, password, set_default_avatar=False):
-        user = self.create_user(
-            username,
-            email,
-            password=password,
-            set_default_avatar=set_default_avatar,
-        )
+        user = self._create_user(username, email, password, **extra_fields)
 
         try:
             user.rank = Rank.objects.get(name=_("Forum team"))
@@ -111,11 +82,6 @@ class UserManager(BaseUserManager):
         except Rank.DoesNotExist:
             pass
 
-        user.is_staff = True
-        user.is_superuser = True
-
-        updated_fields = ('rank', 'acl_key', 'is_staff', 'is_superuser')
-        user.save(update_fields=updated_fields, using=self._db)
         return user
 
     def get_by_username(self, username):
@@ -135,14 +101,14 @@ class User(AbstractBaseUser, PermissionsMixin):
     ACTIVATION_USER = 1
     ACTIVATION_ADMIN = 2
 
-    SUBSCRIBE_NONE = 0
-    SUBSCRIBE_NOTIFY = 1
-    SUBSCRIBE_ALL = 2
+    SUBSCRIPTION_NONE = 0
+    SUBSCRIPTION_NOTIFY = 1
+    SUBSCRIPTION_ALL = 2
 
-    SUBSCRIBE_CHOICES = [
-        (SUBSCRIBE_NONE, _("No")),
-        (SUBSCRIBE_NOTIFY, _("Notify")),
-        (SUBSCRIBE_ALL, _("Notify with e-mail")),
+    SUBSCRIPTION_CHOICES = [
+        (SUBSCRIPTION_NONE, _("No")),
+        (SUBSCRIPTION_NOTIFY, _("Notify")),
+        (SUBSCRIPTION_ALL, _("Notify with e-mail")),
     ]
 
     LIMIT_INVITES_TO_NONE = 0
@@ -252,12 +218,12 @@ class User(AbstractBaseUser, PermissionsMixin):
     sync_unread_private_threads = models.BooleanField(default=False)
 
     subscribe_to_started_threads = models.PositiveIntegerField(
-        default=SUBSCRIBE_NONE,
-        choices=SUBSCRIBE_CHOICES,
+        default=SUBSCRIPTION_NONE,
+        choices=SUBSCRIPTION_CHOICES,
     )
     subscribe_to_replied_threads = models.PositiveIntegerField(
-        default=SUBSCRIBE_NONE,
-        choices=SUBSCRIBE_CHOICES,
+        default=SUBSCRIPTION_NONE,
+        choices=SUBSCRIPTION_CHOICES,
     )
 
     threads = models.PositiveIntegerField(default=0)
@@ -466,22 +432,6 @@ class User(AbstractBaseUser, PermissionsMixin):
             return False
 
 
-class Online(models.Model):
-    user = models.OneToOneField(
-        settings.AUTH_USER_MODEL,
-        primary_key=True,
-        related_name='online_tracker',
-        on_delete=models.CASCADE,
-    )
-    last_click = models.DateTimeField(default=timezone.now)
-
-    def save(self, *args, **kwargs):
-        try:
-            super().save(*args, **kwargs)
-        except IntegrityError:
-            pass  # first come is first serve in online tracker
-
-
 class UsernameChange(models.Model):
     user = models.ForeignKey(
         settings.AUTH_USER_MODEL,

misago/users/setupnewuser.py

@@ -0,0 +1,29 @@
+from .avatars import set_default_avatar
+from .audittrail import create_user_audit_trail
+from .models import User
+
+
+def setup_new_user(user, settings):
+    set_default_subscription_options(user, settings)
+    
+    set_default_avatar(
+        user, settings.default_avatar, settings.default_gravatar_fallback
+    )
+
+    if user.joined_from_ip:
+        create_user_audit_trail(user, user.joined_from_ip, user)
+
+
+SUBSCRIPTION_CHOICES = {
+    'no': User.SUBSCRIPTION_NONE,
+    'watch': User.SUBSCRIPTION_NOTIFY,
+    'watch_email': User.SUBSCRIPTION_ALL,
+}
+
+
+def set_default_subscription_options(user, settings):
+    started_threads = SUBSCRIPTION_CHOICES[settings.subscribe_start]
+    user.subscribe_to_started_threads = started_threads
+    
+    replied_threads = SUBSCRIPTION_CHOICES[settings.subscribe_reply]
+    user.subscribe_to_replied_threads = replied_threads

misago/users/tests/test_new_user_setup.py

@@ -0,0 +1,70 @@
+from django.contrib.auth import get_user_model
+from django.test import TestCase
+
+from misago.cache.versions import get_cache_versions
+from misago.conf.dynamicsettings import DynamicSettings
+from misago.conf.tests import override_dynamic_settings
+
+from misago.users.setupnewuser import (
+    set_default_subscription_options, setup_new_user
+)
+
+User = get_user_model()
+
+
+class NewUserSetupTests(TestCase):
+    def test_default_avatar_is_set_for_user(self):
+        user = User.objects.create_user("User", "test@example.com")
+        cache_versions = get_cache_versions()
+        settings = DynamicSettings(cache_versions)
+        setup_new_user(user, settings)
+        assert user.avatars
+        assert user.avatar_set.exists()
+
+    def test_default_started_threads_subscription_option_is_set_for_user(self):
+        user = User.objects.create_user("User", "test@example.com")
+        cache_versions = get_cache_versions()
+        settings = DynamicSettings(cache_versions)
+
+        with override_dynamic_settings(subscribe_start="no"):
+            set_default_subscription_options(user, settings)
+            assert user.subscribe_to_started_threads == User.SUBSCRIPTION_NONE
+
+        with override_dynamic_settings(subscribe_start="watch"):
+            set_default_subscription_options(user, settings)
+            assert user.subscribe_to_started_threads == User.SUBSCRIPTION_NOTIFY
+
+        with override_dynamic_settings(subscribe_start="watch_email"):
+            set_default_subscription_options(user, settings)
+            assert user.subscribe_to_started_threads == User.SUBSCRIPTION_ALL
+
+    def test_default_replied_threads_subscription_option_is_set_for_user(self):
+        user = User.objects.create_user("User", "test@example.com")
+        cache_versions = get_cache_versions()
+        settings = DynamicSettings(cache_versions)
+
+        with override_dynamic_settings(subscribe_reply="no"):
+            set_default_subscription_options(user, settings)
+            assert user.subscribe_to_replied_threads == User.SUBSCRIPTION_NONE
+
+        with override_dynamic_settings(subscribe_reply="watch"):
+            set_default_subscription_options(user, settings)
+            assert user.subscribe_to_replied_threads == User.SUBSCRIPTION_NOTIFY
+
+        with override_dynamic_settings(subscribe_reply="watch_email"):
+            set_default_subscription_options(user, settings)
+            assert user.subscribe_to_replied_threads == User.SUBSCRIPTION_ALL
+
+    def test_if_user_ip_is_available_audit_trail_is_created_for_user(self):
+        user = User.objects.create_user("User", "test@example.com", joined_from_ip="0.0.0.0")
+        cache_versions = get_cache_versions()
+        settings = DynamicSettings(cache_versions)
+        setup_new_user(user, settings)
+        assert user.audittrail_set.count() == 1
+
+    def test_if_user_ip_is_not_available_audit_trail_is_not_created(self):
+        user = User.objects.create_user("User", "test@example.com")
+        cache_versions = get_cache_versions()
+        settings = DynamicSettings(cache_versions)
+        setup_new_user(user, settings)
+        assert user.audittrail_set.exists() is False

misago/users/tests/test_user_creation.py

@@ -0,0 +1,80 @@
+from django.contrib.auth import get_user_model
+from django.test import TestCase
+
+from misago.users.models import Rank
+from misago.users.utils import hash_email
+
+User = get_user_model()
+
+
+class UserCreationTests(TestCase):
+    def test_user_is_created(self):
+        user = User.objects.create_user("User", "test@example.com")
+        assert user.pk
+        assert user.joined_on
+
+    def test_user_is_created_with_username_and_slug(self):
+        user = User.objects.create_user("UserName", "test@example.com")
+        assert user.slug == "username"
+
+    def test_user_is_created_with_normalized_email_and_email_hash(self):
+        user = User.objects.create_user("User", "test@eXamPLe.com")
+        assert user.email == "test@example.com"
+        assert user.email_hash == hash_email(user.email)
+
+    def test_user_is_created_with_online_tracker(self):
+        user = User.objects.create_user("User", "test@example.com")
+        assert user.online_tracker
+        assert user.online_tracker.last_click == user.last_login
+
+    def test_user_is_created_with_useable_password(self):
+        password = "password"
+        user = User.objects.create_user("UserUserame", "test@example.com", password)
+        assert user.check_password(password)
+
+    def test_user_is_created_with_default_rank(self):
+        user = User.objects.create_user("User", "test@example.com")
+        assert user.rank == Rank.objects.get_default()
+    
+    def test_user_is_created_with_custom_rank(self):
+        rank = Rank.objects.create(name="Test rank")
+        user = User.objects.create_user("User", "test@example.com", rank=rank)
+        assert user.rank == rank
+    
+    def test_newly_created_user_last_login_is_same_as_join_date(self):
+        user = User.objects.create_user("User", "test@example.com")
+        assert user.last_login == user.joined_on
+    
+    def test_user_is_created_with_authenticated_role(self):
+        user = User.objects.create_user("User", "test@example.com")
+        assert user.roles.get(special_role="authenticated")
+
+    def test_user_is_created_with_diacritics_in_email(self):
+        email = "łóć@łexąmple.com"
+        user = User.objects.create_user("UserName", email)
+        assert user.email == email
+
+    def test_creating_user_without_username_raises_value_error(self):
+        with self.assertRaises(ValueError):
+            User.objects.create_user("", "test@example.com")
+
+    def test_creating_user_without_email_raises_value_error(self):
+        with self.assertRaises(ValueError):
+            User.objects.create_user("User", "")
+
+    def test_create_superuser(self):
+        user = User.objects.create_superuser("User", "test@example.com")
+        assert user.is_staff
+        assert user.is_superuser
+
+    def test_superuser_is_created_with_team_rank(self):
+        user = User.objects.create_superuser("User", "test@example.com")
+        assert "team" in str(user.rank)
+
+    def test_creating_superuser_without_staff_status_raises_value_error(self):
+        with self.assertRaises(ValueError):
+            user = User.objects.create_superuser("User", "test@example.com", is_staff=False)
+
+    def test_creating_superuser_without_superuser_status_raises_value_error(self):
+        with self.assertRaises(ValueError):
+            user = User.objects.create_superuser("User", "test@example.com", is_superuser=False)

misago/users/tests/test_user_getters.py

@@ -0,0 +1,62 @@
+from django.contrib.auth import get_user_model
+from django.test import TestCase
+
+User = get_user_model()
+
+
+class UserGettersTests(TestCase):
+    def test_get_user_by_username(self):
+        user = User.objects.create_user("User", "test@example.com")
+        assert User.objects.get_by_username("User") == user
+
+    def test_getting_user_by_username_is_case_insensitive(self):
+        user = User.objects.create_user("User", "test@example.com")
+        assert User.objects.get_by_username("uSeR") == user
+
+    def test_getting_user_by_username_raises_does_not_exist_for_no_result(self):
+        with self.assertRaises(User.DoesNotExist):
+            User.objects.get_by_username("user")
+
+    def test_getting_user_by_username_supports_diacritics(self):
+        with self.assertRaises(User.DoesNotExist):
+            User.objects.get_by_username("łóć")
+
+    def test_getting_user_by_username_is_not_doing_fuzzy_matching(self):
+        user = User.objects.create_user("User", "test@example.com")
+        with self.assertRaises(User.DoesNotExist):
+            User.objects.get_by_username("usere")
+
+    def test_get_user_by_email(self):
+        user = User.objects.create_user("User", "test@example.com")
+        assert User.objects.get_by_email("test@example.com") == user
+
+    def test_getting_user_by_email_is_case_insensitive(self):
+        user = User.objects.create_user("User", "test@example.com")
+        assert User.objects.get_by_email("tEsT@eXaMplE.com") == user
+
+    def test_getting_user_by_email_supports_diacritics(self):
+        user = User.objects.create_user("User", "łóć@łexĄmple.com")
+        assert User.objects.get_by_email("łÓć@ŁexĄMple.com") == user
+
+    def test_getting_user_by_email_raises_does_not_exist_for_no_result(self):
+        with self.assertRaises(User.DoesNotExist):
+            User.objects.get_by_email("test@example.com")
+
+    def test_get_user_by_username_using_combined_getter(self):
+        user = User.objects.create_user("User", "test@example.com")
+        assert User.objects.get_by_username_or_email("user") == user
+
+    def test_get_user_by_email_using_combined_getter(self):
+        user = User.objects.create_user("User", "test@example.com")
+        assert User.objects.get_by_username_or_email("test@example.com") == user
+
+    def test_combined_getter_handles_username_slug_and_email_collision(self):
+        email_match = User.objects.create_user("Bob", "test@test.test")
+        slug_match = User.objects.create_user("TestTestTest", "bob@test.com")
+
+        assert User.objects.get_by_username_or_email("test@test.test") == email_match
+        assert User.objects.get_by_username_or_email("TestTestTest") == slug_match
+
+    def test_combined_getter_raises_does_not_exist_for_no_result(self):
+        with self.assertRaises(User.DoesNotExist):
+            User.objects.get_by_username_or_email("User")

misago/users/tests/test_user_model.py

@@ -1,6 +1,5 @@
 from pathlib import Path
 
-from django.core.exceptions import ValidationError
 from django.test import TestCase
 
 from misago.conf import settings
@@ -10,79 +9,6 @@ from misago.users.avatars import dynamic
 from misago.users.models import Avatar, User
 
 
-class UserManagerTests(TestCase):
-    def test_create_user(self):
-        """create_user created new user account successfully"""
-        user = User.objects.create_user(
-            'Bob',
-            'bob@test.com',
-            'Pass.123',
-            set_default_avatar=True,
-        )
-
-        db_user = User.objects.get(id=user.pk)
-
-        self.assertEqual(user.username, db_user.username)
-        self.assertEqual(user.slug, db_user.slug)
-        self.assertEqual(user.email, db_user.email)
-        self.assertEqual(user.email_hash, db_user.email_hash)
-
-    def test_create_user_twice(self):
-        """create_user is raising validation error for duplicate users"""
-        User.objects.create_user('Bob', 'bob@test.com', 'Pass.123')
-        with self.assertRaises(ValidationError):
-            User.objects.create_user('Bob', 'bob@test.com', 'Pass.123')
-
-    def test_create_superuser(self):
-        """create_superuser created new user account successfully"""
-        user = User.objects.create_superuser('Bob', 'bob@test.com', 'Pass.123')
-
-        db_user = User.objects.get(id=user.pk)
-
-        self.assertTrue(user.is_staff)
-        self.assertTrue(db_user.is_staff)
-        self.assertTrue(user.is_superuser)
-        self.assertTrue(db_user.is_superuser)
-
-    def test_get_user(self):
-        """get_by_ methods return user correctly"""
-        user = User.objects.create_user('Bob', 'bob@test.com', 'Pass.123')
-
-        db_user = User.objects.get_by_username(user.username)
-        self.assertEqual(user, db_user)
-
-        db_user = User.objects.get_by_email(user.email)
-        self.assertEqual(user, db_user)
-
-        db_user = User.objects.get_by_username_or_email(user.username)
-        self.assertEqual(user, db_user)
-
-        db_user = User.objects.get_by_username_or_email(user.email)
-        self.assertEqual(user, db_user)
-
-    def test_get_by_username_or_email_multiple_results(self):
-        """get_by_username_or_email method handles multiple results"""
-        email_match = User.objects.create_user('Bob', 'test@test.test', 'Pass.123')
-        slug_match = User.objects.create_user('TestTestTest', 'bob@test.com', 'Pass.123')
-
-        db_user = User.objects.get_by_username_or_email('test@test.test')
-        self.assertEqual(email_match, db_user)
-
-        db_user = User.objects.get_by_username_or_email('TestTestTest')
-        self.assertEqual(slug_match, db_user)
-
-    def test_getters_unicode_handling(self):
-        """get_by_ methods handle unicode"""
-        with self.assertRaises(User.DoesNotExist):
-            User.objects.get_by_username('łóć')
-
-        with self.assertRaises(User.DoesNotExist):
-            User.objects.get_by_email('łóć@polskimail.pl')
-
-        with self.assertRaises(User.DoesNotExist):
-            User.objects.get_by_username_or_email('łóć@polskimail.pl')
-
-
 class UserModelTests(TestCase):
     def test_anonymize_data(self):
         """anonymize_data sets username and slug to one defined in settings"""