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
Original file line number Diff line number Diff line change
Expand Up @@ -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,
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',
)

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