[change!] Use days instead of months in delete_old_radiusbatch_users #…

…525

Changed the delete_old_radiusbatch_users management command to use days instead of months also modified tasks and tests to use days.

Fixes #525

---------

Co-authored-by: Gagan Deep <pandafy.dev@gmail.com>

docs/user/management_commands.rst

@@ -81,13 +81,23 @@ date set.
 
 This command deletes users created using batch operation that have expired
 (and should have been deactivated by ``deactivate_expired_users``) for
-more than the specified ``<duration_in_months>``.
+more than the specified ``<duration_in_days>``.
+
+.. code-block:: shell
+
+    ./manage.py delete_old_radiusbatch_users --older-than-days <duration_in_days>
+
+Note that the default duration is set to **540 days** (18 months).
+
+For backward compatibility, the command also accepts the argument
+``--older-than-months``:
 
 .. code-block:: shell
 
     ./manage.py delete_old_radiusbatch_users --older-than-months <duration_in_months>
 
-Note that the default duration is set to **18 months**.
+If both ``--older-than-days`` and ``--older-than-months`` are provided,
+preference is given to ``--older-than-days``.
 
 ``delete_unverified_users``
 ---------------------------

docs/user/settings.rst

@@ -122,9 +122,9 @@ addition of users from the csv.
 ``OPENWISP_RADIUS_BATCH_DELETE_EXPIRED``
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
-**Default**: ``18``
+**Default**: ``540`` (18 months)
 
-It is the number of months after which the expired users are deleted.
+It is the number of days after which the expired users are deleted.
 
 ``OPENWISP_RADIUS_BATCH_PDF_TEMPLATE``
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

openwisp_radius/management/commands/base/delete_old_radiusbatch_users.py

@@ -13,18 +13,31 @@ class BaseDeleteOldRadiusBatchUsersCommand(BaseCommand):
     help = 'Deactivating users added in batches which have expired'
 
     def add_arguments(self, parser):
+        parser.add_argument(
+            '--older-than-days',
+            action='store',
+            type=int,
+            help='Delete RADIUS batch users that are older than days provided',
+        )
         parser.add_argument(
             '--older-than-months',
             action='store',
-            default=BATCH_DELETE_EXPIRED,
-            help='delete users which have expired before this time',
+            type=int,
+            help='Delete RADIUS batch users that are older than months provided',
         )
 
     def handle(self, *args, **options):
-        months = now() - timedelta(days=30 * int(options['older_than_months']))
-        batches = RadiusBatch.objects.filter(expiration_date__lt=months)
+        if options.get('older_than_days'):
+            days = options['older_than_days']
+        elif options.get('older_than_months'):
+            days = 30 * options['older_than_months']
+        else:
+            days = BATCH_DELETE_EXPIRED
+        threshold_date = now() - timedelta(days=days)
+
+        batches = RadiusBatch.objects.filter(expiration_date__lt=threshold_date)
+        time_period = threshold_date.strftime('%Y-%m-%d %H:%M:%S')
+
         for b in batches:
             b.delete()
-        self.stdout.write(
-            f'Deleted accounts older than {options["older_than_months"]} months'
-        )
+        self.stdout.write(f'Deleted accounts older than {time_period}')

openwisp_radius/settings.py

@@ -45,7 +45,7 @@ def get_default_password_reset_url(urls):
 DEFAULT_SECRET_FORMAT = get_settings_value('DEFAULT_SECRET_FORMAT', 'NT-Password')
 DISABLED_SECRET_FORMATS = get_settings_value('DISABLED_SECRET_FORMATS', [])
 BATCH_DEFAULT_PASSWORD_LENGTH = get_settings_value('BATCH_DEFAULT_PASSWORD_LENGTH', 8)
-BATCH_DELETE_EXPIRED = get_settings_value('BATCH_DELETE_EXPIRED', 18)
+BATCH_DELETE_EXPIRED = get_settings_value('BATCH_DELETE_EXPIRED', 540)  # 18 months
 BATCH_MAIL_SUBJECT = get_settings_value('BATCH_MAIL_SUBJECT', 'Credentials')
 BATCH_MAIL_SENDER = get_settings_value('BATCH_MAIL_SENDER', settings.DEFAULT_FROM_EMAIL)
 API_AUTHORIZE_REJECT = get_settings_value('API_AUTHORIZE_REJECT', False)

openwisp_radius/tasks.py

@@ -43,9 +43,14 @@ def deactivate_expired_users():
 
 
 @shared_task
-def delete_old_radiusbatch_users(older_than_months=12):
+def delete_old_radiusbatch_users(
+    older_than_months=None,
+    older_than_days=app_settings.BATCH_DELETE_EXPIRED,
+):
     management.call_command(
-        'delete_old_radiusbatch_users', older_than_months=older_than_months
+        'delete_old_radiusbatch_users',
+        older_than_months=older_than_months,
+        older_than_days=older_than_days,
     )
 
 

openwisp_radius/tests/static/test_batch_users.csv

@@ -0,0 +1,3 @@
+rahulyadav,cleartext$password,rahul.yadav@openwisp.com,Rahul,Yadav
+rahulyadav,pbkdf2_sha256$100000$x3DUBnOFwraV$PU2dZZq1FcuBjagxVLPhhFvpicLn18fFCN5xiLsxATc=,rahul@openwisp.com,,
+,,rahul.rajput@openwisp.com,,

openwisp_radius/tests/test_commands.py

@@ -154,6 +154,7 @@ def test_deactivate_expired_users_command(self):
 
     @capture_stdout()
     def test_delete_old_radiusbatch_users_command(self):
+        # Create RadiusBatch users that expired more than 18 months ago
         path = self._get_path('static/test_batch.csv')
         options = dict(
             organization=self.default_org.slug,
@@ -162,6 +163,7 @@ def test_delete_old_radiusbatch_users_command(self):
             name='test',
         )
         self._call_command('batch_add_users', **options)
+        # Create RadiusBatch users that expired 15 months ago
         expiration_date = (now() - timedelta(days=30 * 15)).strftime('%d-%m-%Y')
         path = self._get_path('static/test_batch_new.csv')
         options = dict(
@@ -171,11 +173,41 @@ def test_delete_old_radiusbatch_users_command(self):
             name='test1',
         )
         self._call_command('batch_add_users', **options)
-        self.assertEqual(get_user_model().objects.all().count(), 6)
-        call_command('delete_old_radiusbatch_users')
-        self.assertEqual(get_user_model().objects.all().count(), 3)
-        call_command('delete_old_radiusbatch_users', older_than_months=12)
-        self.assertEqual(get_user_model().objects.all().count(), 0)
+        # Create RadiusBatch users that expired 10 days ago
+        path = self._get_path('static/test_batch_users.csv')
+        expiration_date = (now() - timedelta(days=10)).strftime('%d-%m-%Y')
+        options = dict(
+            organization=self.default_org.slug,
+            file=path,
+            expiration=expiration_date,
+            name='test2',
+        )
+        self._call_command('batch_add_users', **options)
+        self.assertEqual(get_user_model().objects.all().count(), 9)
+
+        with self.subTest('Test executing command without arguments'):
+            call_command('delete_old_radiusbatch_users')
+            # Only users that expired more than 18 months ago should be deleted
+            self.assertEqual(get_user_model().objects.all().count(), 6)
+
+        with self.subTest('Test executing command with older_than_months argument'):
+            call_command('delete_old_radiusbatch_users', older_than_months=12)
+            # Users that expired more than 12 months ago should be deleted
+            self.assertEqual(get_user_model().objects.all().count(), 3)
+
+        with self.subTest('Test executing command with older_than_days argument'):
+            call_command('delete_old_radiusbatch_users', older_than_days=9)
+            # Users that expired more than 9 days ago should be deleted
+            self.assertEqual(get_user_model().objects.all().count(), 0)
+
+        with self.subTest('Test executing command with both arguments'):
+            options['name'] = 'test3'
+            call_command('batch_add_users', **options)
+            call_command(
+                'delete_old_radiusbatch_users', older_than_days=9, older_than_months=12
+            )
+            # Users that expired more than 9 days ago should be deleted
+            self.assertEqual(get_user_model().objects.all().count(), 0)
 
     @capture_stdout()
     def test_prefix_add_users_command(self):

openwisp_radius/tests/test_tasks.py

@@ -34,14 +34,16 @@ def setUpClass(cls):
         app.autodiscover_tasks()
         cls.celery_worker = start_worker(app)
 
-    def _get_expired_user_from_radius_batch(self):
+    def _get_expired_user_from_radius_batch(self, **kwargs):
         reader = [['sample', 'admin', 'user@openwisp.com', 'SampleName', 'Lastname']]
-        batch = self._create_radius_batch(
+        options = dict(
             name='test',
             strategy='csv',
             csvfile=self._get_csvfile(reader),
             expiration_date='1998-01-28',
         )
+        options.update(kwargs)
+        batch = self._create_radius_batch(**options)
         batch.add(reader)
         return batch.users.first()
 
@@ -70,9 +72,41 @@ def test_deactivate_expired_users(self):
     def test_delete_old_radiusbatch_users(self):
         self._get_expired_user_from_radius_batch()
         self.assertEqual(User.objects.all().count(), 1)
-        result = tasks.delete_old_radiusbatch_users.delay(1)
-        self.assertTrue(result.successful())
-        self.assertEqual(User.objects.all().count(), 0)
+
+        with self.subTest('Ensure first argument is "older_than_month"'):
+            # The first argument should be processed as "older_than_month"
+            # to maintain backward compatibility.
+            result = tasks.delete_old_radiusbatch_users.delay(30)
+            self.assertTrue(result.successful())
+            self.assertEqual(User.objects.all().count(), 0)
+
+        with self.subTest('Test "older_than_days" keyword argument'):
+            self._get_expired_user_from_radius_batch(
+                name='test-older-than-days',
+                expiration_date=(now() - timedelta(days=31)),
+            )
+            self.assertEqual(User.objects.all().count(), 1)
+            result = tasks.delete_old_radiusbatch_users.delay(older_than_days=30)
+            self.assertTrue(result.successful())
+            self.assertEqual(User.objects.all().count(), 0)
+
+        with self.subTest('Test with both positional arguments'):
+            self._get_expired_user_from_radius_batch(
+                name='test-argument-precedence',
+                expiration_date=(now() - timedelta(days=31)),
+            )
+            self.assertEqual(User.objects.all().count(), 1)
+            # Precedence should be given to "older_than_days" (second argument).
+            # Hence, the user should NOT be deleted.
+            result = tasks.delete_old_radiusbatch_users.delay(1, 60)
+            self.assertTrue(result.successful())
+            self.assertEqual(User.objects.all().count(), 1)
+
+            # Precedence should be given to "older_than_days" (second argument).
+            # Hence, the user should be deleted.
+            result = tasks.delete_old_radiusbatch_users.delay(2, 30)
+            self.assertTrue(result.successful())
+            self.assertEqual(User.objects.all().count(), 0)
 
     @capture_any_output()
     def test_delete_old_postauth(self):