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

* Fix cleanupregistration, skip if user.is_active

* Fix W504

* changelog

* fix

Author: joshblum

CHANGELOG

@@ -6,6 +6,8 @@ Version 2.5, TBD
 ----------------
 * Feature: Add support for Django 2.1. -
 `#337 <https://github.com/macropin/django-registration/pull/337>_`
+* Bugfix: Don't delete if user.is_active=True in cleanupregistration. -
+`#342 <https://github.com/macropin/django-registration/pull/342>_`
 
 Version 2.4, 11 April, 2018
 ----------------

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/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