Fix cleanupregistration, skip if user.is_active (#344)

*  Fix cleanupregistration, skip if user.is_active

* requirements fix

* Drop python 3.3

Author: joshblum

.travis.yml

@@ -2,7 +2,6 @@ language: python
 sudo: false
 python:
   - 2.7
-  - 3.3
   - 3.4
   - 3.5
   - 3.6

CHANGELOG

@@ -4,6 +4,8 @@ django-registration-redux changelog
 
 Version 1.11, TBD
 ----------------
+* Bugfix: Don't delete if user.is_active=True in cleanupregistration. -
+`#342 <https://github.com/macropin/django-registration/pull/342>_`
 
 Version 1.10, 09 February, 2018
 ----------------

docs/upgrade.rst

@@ -14,7 +14,7 @@ Django version requirement
 
 As of |version|, |project| requires Django 1.8 or newer;
 older Django releases may work, but are officially unsupported. Additionally,
-|project| officially supports Python 2.7, 3.3, 3.4, 3.5, and 3.6.
+|project| officially supports Python 2.7, 3.4, 3.5, and 3.6.
 
 
 Backwards-incompatible changes

registration/models.py

@@ -57,11 +57,9 @@ def send_email(addresses_to, ctx_dict, subject_template, body_template,
     """
     Function that sends an email
     """
-    subject = (
-        getattr(settings, 'REGISTRATION_EMAIL_SUBJECT_PREFIX', '') +
-        render_to_string(
-            subject_template, ctx_dict)
-    )
+
+    prefix = getattr(settings, 'REGISTRATION_EMAIL_SUBJECT_PREFIX', '')
+    subject = prefix + render_to_string(subject_template, ctx_dict)
     # Email subject *must not* contain newlines
     subject = ''.join(subject.splitlines())
     from_email = get_from_email(ctx_dict.get('site'))
@@ -269,9 +267,13 @@ def delete_expired_users(self):
         be deleted.
 
         """
-        for profile in self.all():
+        profiles = self.filter(
+            models.Q(user__is_active=False) | models.Q(user=None),
+            activated=False,
+        )
+        for profile in profiles:
             try:
-                if profile.activation_key_expired() and not profile.activated:
+                if profile.activation_key_expired():
                     user = profile.user
                     logger.warning('Deleting expired Registration profile {} and user {}.'.format(profile, user))
                     profile.delete()
@@ -350,10 +352,10 @@ def activation_key_expired(self):
            method returns ``True``.
 
         """
-        expiration_date = datetime.timedelta(
+        max_expiry_days = datetime.timedelta(
             days=settings.ACCOUNT_ACTIVATION_DAYS)
-        return (self.activated or
-                (self.user.date_joined + expiration_date <= datetime_now()))
+        expiration_date = self.user.date_joined + max_expiry_days
+        return self.activated or expiration_date <= datetime_now()
 
     def send_activation_email(self, site, request=None):
         """
@@ -418,9 +420,11 @@ def send_activation_email(self, site, request=None):
             'expiration_days': settings.ACCOUNT_ACTIVATION_DAYS,
             'site': site,
         }
-        subject = (getattr(settings, 'REGISTRATION_EMAIL_SUBJECT_PREFIX', '') +
-                   render_to_string(
-                       activation_email_subject, ctx_dict, request=request))
+        prefix = getattr(settings, 'REGISTRATION_EMAIL_SUBJECT_PREFIX', '')
+        subject = prefix + render_to_string(
+            activation_email_subject, ctx_dict, request=request
+        )
+
         # Email subject *must not* contain newlines
         subject = ''.join(subject.splitlines())
         from_email = get_from_email(site)

registration/tests/forms_custom_user.py

@@ -8,11 +8,8 @@
 
 try:
     from importlib import reload  # Python 3.4+
-except ImportError:
-    try:
-        from imp import reload  # Python 3.3
-    except Exception:
-        pass  # Python 2 reload()
+except Exception:
+    pass  # Python 2 reload()
 
 
 @override_settings(AUTH_USER_MODEL='test_app.CustomUser')

registration/tests/models.py

@@ -169,10 +169,11 @@ def test_user_creation(self):
         self.assertEqual(new_user.email, '[email protected]')
         self.failUnless(new_user.check_password('swordfish'))
         self.failIf(new_user.is_active)
-        self.failIf(new_user.date_joined <=
-                    datetime_now() - timedelta(
-                        settings.ACCOUNT_ACTIVATION_DAYS)
-                    )
+
+        expiration_date = datetime_now() - timedelta(
+            settings.ACCOUNT_ACTIVATION_DAYS
+        )
+        self.failIf(new_user.date_joined <= expiration_date)
 
     def test_user_creation_email(self):
         """
@@ -204,10 +205,9 @@ def test_user_creation_old_date_joined(self):
         self.assertEqual(new_user.email, '[email protected]')
         self.failUnless(new_user.check_password('swordfish'))
         self.failIf(new_user.is_active)
-        self.failIf(new_user.date_joined <=
-                    datetime_now() - timedelta(
-                        settings.ACCOUNT_ACTIVATION_DAYS)
-                    )
+
+        expiry_date = datetime_now() - timedelta(settings.ACCOUNT_ACTIVATION_DAYS)
+        self.failIf(new_user.date_joined <= expiry_date)
 
     def test_unexpired_account_old_date_joined(self):
         """
@@ -481,6 +481,27 @@ def test_expired_user_deletion_missing_user(self):
         self.assertRaises(UserModel().DoesNotExist,
                           UserModel().objects.get, username='bob')
 
+    def test_manually_registered_account(self):
+        """
+        Test if a user failed to go through the registration flow but was
+        manually marked ``is_active`` in the DB.  Although the profile is
+        expired and not active, we should never delete active users.
+        """
+        active_user = (self.registration_profile.objects
+                       .create_inactive_user(
+                           site=Site.objects.get_current(),
+                           username='bob',
+                           password='secret',
+                           email='[email protected]'))
+        active_user.date_joined -= datetime.timedelta(
+            days=settings.ACCOUNT_ACTIVATION_DAYS + 1)
+        active_user.is_active = True
+        active_user.save()
+
+        self.registration_profile.objects.delete_expired_users()
+        self.assertEqual(self.registration_profile.objects.count(), 1)
+        self.assertEqual(UserModel().objects.get(username='bob'), active_user)
+
     def test_management_command(self):
         """
         The ``cleanupregistration`` management command properly

requirements.txt

@@ -3,4 +3,3 @@ Sphinx
 twine
 pytest>3.2.1
 Django>1.8
-docutils==0.12

setup.py

@@ -48,7 +48,6 @@ def run_tests(self):
         'Programming Language :: Python',
         'Programming Language :: Python :: 2.7',
         'Programming Language :: Python :: 3',
-        'Programming Language :: Python :: 3.3',
         'Programming Language :: Python :: 3.4',
         'Programming Language :: Python :: 3.5',
         'Programming Language :: Python :: 3.6',

test-requirements.txt

@@ -3,3 +3,4 @@ coveralls
 isort<4.3.0
 tox-travis
 invoke==0.12.2
+docutils

tox.ini

@@ -5,7 +5,7 @@
 
 [tox]
 envlist =
-    {py27,py33,py34,py35}-django18,
+    {py27,py34,py35}-django18,
     {py27,py34,py35}-{django19,django110},
     {py27,py34,py35,py36}-django111,
 
@@ -22,7 +22,6 @@ deps =
 [travis]
 python =
   2.7: py27
-  3.3: py33
   3.4: py34
   3.5: py35
   3.6: py36