Issues/525 fix management command inconsistency #527
Conversation
…ent openwisp#525 changed the delete_old_radiusbatch_users management command to use days instead of months also modified tasks and tests to use days. Fixes openwisp#525
… instead of months openwisp#525 Changed the delete_old_radiusbatch_users command to take in days in argument instead of months and modified tests and tasks accordingly. Fixes openwisp#525
There was a problem hiding this comment.
Thank you for contributing @kaushikaryan04.
The current changes woulbe backward incompatible and can break existing instances.
We should implement this change in a backward compatible way:
- We should not remove the
--older-than-monthsoptions from the code - We should give precedence to
--older-than-daysand use the old option only if--older-than-daysis not present - We should update the documentation too (do a project wide search for
delete_old_radiusbatch_usersto find it, it's in docs/source/user/management_commands.rst`
So, both older-than-days and older-than-months should be optional but we must ensure there's at least one supplied or we should return an error.
… as well openwisp#525 Added the option to delete users by specifying days or months.If nothing is provided 18 months will be taken as default. Fixes openwisp#525
|
I think this should be good. All old features are working as they were before and --older-than-days is added as well. Documentation also updated. |
There was a problem hiding this comment.
@kaushikaryan04 looks good, I found something that doesn't convince me, please see my comment below.
| parser.add_argument( | ||
| '--older-than-months', | ||
| action='store', | ||
| type=int, | ||
| default=BATCH_DELETE_EXPIRED, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
openwisp_radius/tasks.py
Outdated
| @@ -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): | |||
There was a problem hiding this comment.
shouldn't the default here be the same as BATCH_DELETE_EXPIRED? This was probably a mistake.
| @@ -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) | |||
There was a problem hiding this comment.
We need a test that ensures the old behavior continues to work.
There was a problem hiding this comment.
Sure I can add some more users to test.
Made the suggested changes and added the testcase so both old and new command could be tested. Fixes openwisp#525
As mentioned in the issue I have changed the
delete_old_radiusbatch_usersargument to take in days instead of months and have changed the tasks and tests accordingly.