Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issues/525 fix management command inconsistency #527

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
5 changes: 3 additions & 2 deletions docs/source/user/management_commands.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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 ``<duration_in_months>``.
``deactivate_expired_users``) for more than the duration specified by ``--older-than-days <duration_in_days>``
or ``--older-than-months `<duration_in_months>``.

.. code-block:: shell

./manage.py delete_old_radiusbatch_users --older-than-months <duration_in_months>
./manage.py delete_old_radiusbatch_users --older-than-days <duration_in_days>

Note that the default duration is set to 18 months.

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,18 +13,32 @@ 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 users that are older than days provided',
)
parser.add_argument(
'--older-than-months',
action='store',
type=int,
default=BATCH_DELETE_EXPIRED,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we update the default so that it's used only on the new argument? The default for this old argument should be None, so that the new argument will be used by default.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes this could be done but the action would be same if no argument is provided that is why I did not changed it.I can do this in the next commit.

help='delete users which have expired before this time',
help='Delete 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)
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_months"]} months'
)
self.stdout.write(f'Deleted accounts older than {time_period}')
4 changes: 2 additions & 2 deletions openwisp_radius/tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't the default here be the same as BATCH_DELETE_EXPIRED? This was probably a mistake.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure we can change this

management.call_command(
'delete_old_radiusbatch_users', older_than_months=older_than_months
'delete_old_radiusbatch_users', older_than_days=older_than_days
)


Expand Down
2 changes: 1 addition & 1 deletion openwisp_radius/tests/test_commands.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need a test that ensures the old behavior continues to work.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure I can add some more users to test.

self.assertEqual(get_user_model().objects.all().count(), 0)

@capture_stdout()
Expand Down
2 changes: 1 addition & 1 deletion openwisp_radius/tests/test_tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
Loading