tests user migrations

spirit/comment/flag/forms.py

@@ -4,7 +4,7 @@
 
 from django import forms
 from django.utils.translation import ugettext_lazy as _
-from django.db import IntegrityError
+from django.db import IntegrityError, transaction
 from django.utils import timezone
 
 from .models import Flag, CommentFlag
@@ -39,8 +39,10 @@ def save(self, commit=True):
             self.instance.comment = self.comment
 
             try:
-                CommentFlag.objects.update_or_create(comment=self.comment,
-                                                     defaults={'date': timezone.now(), })
+                with transaction.atomic():
+                    CommentFlag.objects.update_or_create(
+                        comment=self.comment,
+                        defaults={'date': timezone.now(), })
             except IntegrityError:
                 pass
 

spirit/user/migrations/0011_auto_20181124_2320.py

@@ -1,6 +1,6 @@
 # Generated by Django 2.1.3 on 2018-11-24 23:20
 
-from django.db import migrations
+from django.db import migrations, IntegrityError, transaction
 
 # This can be ran multiple times, I got your back ;)
 def populate_nickname(apps, schema_editor):
@@ -18,13 +18,38 @@ def populate_nickname(apps, schema_editor):
 
 def make_usernames_lower(apps, schema_editor):
     from django.db.models.functions import Lower
+    from django.db.models import Count
     from ...core.conf import settings
     User = apps.get_model(settings.AUTH_USER_MODEL)
 
     if settings.ST_CASE_INSENSITIVE_USERNAMES:
-        # XXX catch integrity error and show a descriptive message
-        User.objects.all().update(
-            username=Lower('username'))
+        try:
+            with transaction.atomic():
+                User.objects.all().update(
+                    username=Lower('username'))
+        except IntegrityError:
+            # Show all duplicated usernames
+            users_lower = (
+                User.objects
+                .annotate(username_lower=Lower('username'))
+                .values('username_lower')
+                .annotate(cnt=Count(Lower('username')))
+                .values('username_lower', 'cnt')
+                .filter(cnt__gt=1)
+                .order_by('username_lower', 'cnt'))
+            users = [
+                u.username
+                for u in (
+                    User.objects
+                    .annotate(username_lower=Lower('username'))
+                    .filter(username_lower__in=[
+                        u['username_lower'] for u in users_lower]))]
+            raise IntegrityError(
+                'There are two or more users with '
+                'similar name but different casing, for example: '
+                'someUser and SomeUser, either remove one of them '
+                'or switch the `ST_CASE_INSENSITIVE_USERNAMES` setting '
+                'to False. Duplicate users are {}'.format(users))
 
 
 class Migration(migrations.Migration):

spirit/user/tests.py

@@ -2,6 +2,7 @@
 
 from __future__ import unicode_literals
 import datetime
+import importlib
 
 from django.test import TestCase, RequestFactory
 from django.urls import reverse
@@ -11,6 +12,8 @@
 from django.utils import timezone
 from django.test.utils import override_settings
 from django.contrib.auth.models import AnonymousUser
+from django.apps import apps
+from django.db import IntegrityError
 
 from djconfig.utils import override_djconfig
 
@@ -27,6 +30,9 @@
 from . import middleware
 from .models import UserProfile
 
+data_migration_11 = importlib.import_module(
+    'spirit.user.migrations.0011_auto_20181124_2320')
+
 User = get_user_model()
 
 
@@ -1147,3 +1153,109 @@ def test_active_user_anonym_user(self):
         self.assertFalse(req.user.is_authenticated)
         self.assertIsNone(
             middleware.ActiveUserMiddleware().process_request(req))
+
+
+class UserMigrationsTest(TestCase):
+
+    def setUp(self):
+        utils.cache_clear()
+
+    def test_migration_11(self):
+        # create users with the CI feature off
+        # to replicate pre feature database state
+        with override_settings(ST_CASE_INSENSITIVE_USERNAMES=False):
+            utils.create_user(username='FOO')
+            utils.create_user(username='BaR')
+            utils.create_user(username='baz')
+        # default all nicknames to empty
+        self.assertEqual(
+            UserProfile.objects.all().update(nickname=''), 3)
+        data_migration_11.populate_nickname(apps, None)
+        self.assertEqual(
+            [u.nickname for u in UserProfile.objects.all()],
+            ['FOO', 'BaR', 'baz'])
+
+        self.assertEqual(
+            [u.username for u in User.objects.all()],
+            ['FOO', 'BaR', 'baz'])
+        data_migration_11.make_usernames_lower(apps, None)
+        self.assertEqual(
+            [u.username for u in User.objects.all()],
+            ['foo', 'bar', 'baz'])
+        self.assertEqual(
+            [u.nickname for u in UserProfile.objects.all()],
+            ['FOO', 'BaR', 'baz'])
+
+    @override_settings(ST_CASE_INSENSITIVE_USERNAMES=False)
+    def test_migration_11_no_ci_usernames(self):
+        utils.create_user(username='FOO')
+        utils.create_user(username='foo')
+        utils.create_user(username='BaR')
+        utils.create_user(username='bar')
+        utils.create_user(username='baz')
+
+        self.assertEqual(
+            UserProfile.objects.all().update(nickname=''), 5)
+        data_migration_11.populate_nickname(apps, None)
+        self.assertEqual(
+            [u.nickname for u in UserProfile.objects.all()],
+            ['FOO', 'foo', 'BaR', 'bar', 'baz'])
+
+        self.assertEqual(
+            [u.username for u in User.objects.all()],
+            ['FOO', 'foo', 'BaR', 'bar', 'baz'])
+        data_migration_11.make_usernames_lower(apps, None)
+        self.assertEqual(
+            [u.username for u in User.objects.all()],
+            ['FOO', 'foo', 'BaR', 'bar', 'baz'])
+        self.assertEqual(
+            [u.nickname for u in UserProfile.objects.all()],
+            ['FOO', 'foo', 'BaR', 'bar', 'baz'])
+
+    def test_migration_11_make_usernames_lower_integrity_err(self):
+        with override_settings(ST_CASE_INSENSITIVE_USERNAMES=False):
+            utils.create_user(username='FOO')
+            utils.create_user(username='fOo')
+            utils.create_user(username='Foo')
+            utils.create_user(username='bar')
+            utils.create_user(username='bAr')
+            utils.create_user(username='baz')
+
+        self.assertEqual(
+            [u.username for u in User.objects.all()],
+            ['FOO', 'fOo', 'Foo', 'bar', 'bAr', 'baz'])
+
+        # transaction is already handled
+        with self.assertRaises(IntegrityError) as cm:
+            data_migration_11.make_usernames_lower(apps, None)
+            self.maxDiff = None
+            self.assertEqual(
+                str(cm.exception),
+                "There are two or more users with similar name but "
+                "different casing, for example: someUser and SomeUser, "
+                "either remove one of them or switch the "
+                "`ST_CASE_INSENSITIVE_USERNAMES` setting to False. "
+                "Duplicate users are ['FOO', 'fOo', 'Foo', 'bar', 'bAr']")
+
+    def test_migration_11_idempotency(self):
+        """Should be idempotent"""
+        with override_settings(ST_CASE_INSENSITIVE_USERNAMES=False):
+            utils.create_user(username='FOO')
+        self.assertEqual(
+            UserProfile.objects.all().update(nickname=''), 1)
+        data_migration_11.populate_nickname(apps, None)
+        data_migration_11.make_usernames_lower(apps, None)
+        self.assertEqual(
+            [u.username for u in User.objects.all()],
+            ['foo'])
+        self.assertEqual(
+            [u.nickname for u in UserProfile.objects.all()],
+            ['FOO'])
+        data_migration_11.populate_nickname(apps, None)
+        data_migration_11.make_usernames_lower(apps, None)
+        self.assertEqual(
+            [u.username for u in User.objects.all()],
+            ['foo'])
+        self.assertEqual(
+            [u.nickname for u in UserProfile.objects.all()],
+            ['FOO'])