From 5ea9ee88aeabaf4f67b1ca51a9fc37e9dcf03e2f Mon Sep 17 00:00:00 2001 From: kaushikaryan04 Date: Sun, 30 Jun 2024 16:59:26 +0530 Subject: [PATCH 1/4] [change] delete_old_radiusbatch_users management command is inconsistent #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 --- .../commands/base/delete_old_radiusbatch_users.py | 8 ++++---- openwisp_radius/settings.py | 2 +- openwisp_radius/tasks.py | 4 ++-- openwisp_radius/tests/test_commands.py | 2 +- openwisp_radius/tests/test_tasks.py | 2 +- 5 files changed, 9 insertions(+), 9 deletions(-) diff --git a/openwisp_radius/management/commands/base/delete_old_radiusbatch_users.py b/openwisp_radius/management/commands/base/delete_old_radiusbatch_users.py index b3504e7f..7a4985d9 100644 --- a/openwisp_radius/management/commands/base/delete_old_radiusbatch_users.py +++ b/openwisp_radius/management/commands/base/delete_old_radiusbatch_users.py @@ -14,17 +14,17 @@ class BaseDeleteOldRadiusBatchUsersCommand(BaseCommand): def add_arguments(self, parser): parser.add_argument( - '--older-than-months', + '--older-than-days', action='store', default=BATCH_DELETE_EXPIRED, help='delete users which have expired before this time', ) def handle(self, *args, **options): - months = now() - timedelta(days=30 * int(options['older_than_months'])) - batches = RadiusBatch.objects.filter(expiration_date__lt=months) + days = now() - timedelta(days=int(options['older_than_days'])) + batches = RadiusBatch.objects.filter(expiration_date__lt=days) for b in batches: b.delete() self.stdout.write( - f'Deleted accounts older than {options["older_than_months"]} months' + f'Deleted accounts older than {options["older_than_days"]} days' ) diff --git a/openwisp_radius/settings.py b/openwisp_radius/settings.py index 3d1db73d..b43e3306 100644 --- a/openwisp_radius/settings.py +++ b/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',30 * 18) 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) diff --git a/openwisp_radius/tasks.py b/openwisp_radius/tasks.py index 85e34384..0a570d03 100644 --- a/openwisp_radius/tasks.py +++ b/openwisp_radius/tasks.py @@ -43,9 +43,9 @@ def deactivate_expired_users(): @shared_task -def delete_old_radiusbatch_users(older_than_months=12): +def delete_old_radiusbatch_users(older_than_days=365): management.call_command( - 'delete_old_radiusbatch_users', older_than_months=older_than_months + 'delete_old_radiusbatch_users', older_than_days=older_than_days ) diff --git a/openwisp_radius/tests/test_commands.py b/openwisp_radius/tests/test_commands.py index 14de1e9a..70d5dd2b 100644 --- a/openwisp_radius/tests/test_commands.py +++ b/openwisp_radius/tests/test_commands.py @@ -174,7 +174,7 @@ def test_delete_old_radiusbatch_users_command(self): 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) + call_command('delete_old_radiusbatch_users', older_than_days=365) self.assertEqual(get_user_model().objects.all().count(), 0) @capture_stdout() diff --git a/openwisp_radius/tests/test_tasks.py b/openwisp_radius/tests/test_tasks.py index 97d9459e..b7915a10 100644 --- a/openwisp_radius/tests/test_tasks.py +++ b/openwisp_radius/tests/test_tasks.py @@ -70,7 +70,7 @@ 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) + result = tasks.delete_old_radiusbatch_users.delay(30) self.assertTrue(result.successful()) self.assertEqual(User.objects.all().count(), 0) From 979e88a68588a6f04f16cc639ec9f5ebe1d49ec6 Mon Sep 17 00:00:00 2001 From: kaushikaryan04 Date: Sun, 30 Jun 2024 17:04:32 +0530 Subject: [PATCH 2/4] [change] Changed the delete_old_radiusbatch_users command to use days instead of months #525 Changed the delete_old_radiusbatch_users command to take in days in argument instead of months and modified tests and tasks accordingly. Fixes #525 --- openwisp_radius/settings.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openwisp_radius/settings.py b/openwisp_radius/settings.py index b43e3306..83821c7f 100644 --- a/openwisp_radius/settings.py +++ b/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',30 * 18) +BATCH_DELETE_EXPIRED = get_settings_value('BATCH_DELETE_EXPIRED', 30 * 18) 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) From c7552525e6f193c0ee69c223056aece3d09cb3a2 Mon Sep 17 00:00:00 2001 From: kaushikaryan04 Date: Tue, 9 Jul 2024 19:56:36 +0530 Subject: [PATCH 3/4] [change] Changed the delete_old_radiusbatch_users command to use days as well #525 Added the option to delete users by specifying days or months.If nothing is provided 18 months will be taken as default. Fixes #525 --- docs/source/user/management_commands.rst | 5 ++-- .../base/delete_old_radiusbatch_users.py | 26 ++++++++++++++----- openwisp_radius/settings.py | 2 +- 3 files changed, 24 insertions(+), 9 deletions(-) diff --git a/docs/source/user/management_commands.rst b/docs/source/user/management_commands.rst index e3ac28a7..6426a16e 100644 --- a/docs/source/user/management_commands.rst +++ b/docs/source/user/management_commands.rst @@ -79,11 +79,12 @@ This command deactivates expired user accounts which were created temporarily -------------------------------- This command deletes users that have expired (and should have been deactivated by -``deactivate_expired_users``) for more than the specified ````. +``deactivate_expired_users``) for more than the duration specified by ``--older-than-days `` +or ``--older-than-months ```. .. code-block:: shell - ./manage.py delete_old_radiusbatch_users --older-than-months + ./manage.py delete_old_radiusbatch_users --older-than-days Note that the default duration is set to 18 months. diff --git a/openwisp_radius/management/commands/base/delete_old_radiusbatch_users.py b/openwisp_radius/management/commands/base/delete_old_radiusbatch_users.py index 7a4985d9..dcd7ac8e 100644 --- a/openwisp_radius/management/commands/base/delete_old_radiusbatch_users.py +++ b/openwisp_radius/management/commands/base/delete_old_radiusbatch_users.py @@ -16,15 +16,29 @@ def add_arguments(self, parser): parser.add_argument( '--older-than-days', action='store', + type=int, + help='Delete users that are older than days provided', + ) + parser.add_argument( + '--older-than-months', + action='store', + type=int, default=BATCH_DELETE_EXPIRED, - help='delete users which have expired before this time', + help='Delete users that are older than months provided', ) def handle(self, *args, **options): - days = now() - timedelta(days=int(options['older_than_days'])) - batches = RadiusBatch.objects.filter(expiration_date__lt=days) + days = options.get('older_than_days') + months = options.get('older_than_months') + + if days is not None: + threshold_date = now() - timedelta(days=days) + else: + threshold_date = now() - timedelta(days=30 * months) + + 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_days"]} days' - ) + self.stdout.write(f'Deleted accounts older than {time_period}') diff --git a/openwisp_radius/settings.py b/openwisp_radius/settings.py index 83821c7f..3d1db73d 100644 --- a/openwisp_radius/settings.py +++ b/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', 30 * 18) +BATCH_DELETE_EXPIRED = get_settings_value('BATCH_DELETE_EXPIRED', 18) 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) From a9af7a01d66bf8c2d108c58ea7ca8309e1b919b7 Mon Sep 17 00:00:00 2001 From: kaushikaryan04 Date: Sat, 20 Jul 2024 15:38:58 +0530 Subject: [PATCH 4/4] [change] Made the suggested changes and edited the testcase #525 Made the suggested changes and added the testcase so both old and new command could be tested. Fixes #525 --- .../commands/base/delete_old_radiusbatch_users.py | 8 ++++---- openwisp_radius/tasks.py | 4 +++- openwisp_radius/tests/static/test_batch_users.csv | 3 +++ openwisp_radius/tests/test_commands.py | 15 +++++++++++++-- 4 files changed, 23 insertions(+), 7 deletions(-) create mode 100644 openwisp_radius/tests/static/test_batch_users.csv diff --git a/openwisp_radius/management/commands/base/delete_old_radiusbatch_users.py b/openwisp_radius/management/commands/base/delete_old_radiusbatch_users.py index dcd7ac8e..c65817d1 100644 --- a/openwisp_radius/management/commands/base/delete_old_radiusbatch_users.py +++ b/openwisp_radius/management/commands/base/delete_old_radiusbatch_users.py @@ -17,13 +17,13 @@ def add_arguments(self, parser): '--older-than-days', action='store', type=int, + default=30 * BATCH_DELETE_EXPIRED, help='Delete users that are older than days provided', ) parser.add_argument( '--older-than-months', action='store', type=int, - default=BATCH_DELETE_EXPIRED, help='Delete users that are older than months provided', ) @@ -31,10 +31,10 @@ def handle(self, *args, **options): days = options.get('older_than_days') months = options.get('older_than_months') - if days is not None: - threshold_date = now() - timedelta(days=days) - else: + if months is not None: threshold_date = now() - timedelta(days=30 * months) + else: + 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') diff --git a/openwisp_radius/tasks.py b/openwisp_radius/tasks.py index 0a570d03..cef2c67a 100644 --- a/openwisp_radius/tasks.py +++ b/openwisp_radius/tasks.py @@ -43,7 +43,9 @@ def deactivate_expired_users(): @shared_task -def delete_old_radiusbatch_users(older_than_days=365): +def delete_old_radiusbatch_users( + older_than_days=30 * app_settings.BATCH_DELETE_EXPIRED, +): management.call_command( 'delete_old_radiusbatch_users', older_than_days=older_than_days ) diff --git a/openwisp_radius/tests/static/test_batch_users.csv b/openwisp_radius/tests/static/test_batch_users.csv new file mode 100644 index 00000000..5bb142c0 --- /dev/null +++ b/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,, diff --git a/openwisp_radius/tests/test_commands.py b/openwisp_radius/tests/test_commands.py index 70d5dd2b..31483c17 100644 --- a/openwisp_radius/tests/test_commands.py +++ b/openwisp_radius/tests/test_commands.py @@ -171,10 +171,21 @@ 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) + 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) call_command('delete_old_radiusbatch_users') + self.assertEqual(get_user_model().objects.all().count(), 6) + call_command('delete_old_radiusbatch_users', older_than_months=12) self.assertEqual(get_user_model().objects.all().count(), 3) - call_command('delete_old_radiusbatch_users', older_than_days=365) + call_command('delete_old_radiusbatch_users', older_than_days=9) self.assertEqual(get_user_model().objects.all().count(), 0) @capture_stdout()