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

Delete User Functionality Updates #1844

Merged
merged 1 commit into from
Jul 22, 2020
Merged

Conversation

WinnyTroy
Copy link
Contributor

Included deletion suffix to email associated with the user account
Included cron task to periodically perform task after 30 days

Closes #1843

Comment on lines 513 to 512
CELERY_BEAT_SCHEDULE = {
'Mark-Expired-Exports-Failed': {
'task':
'onadata.apps.viewer.tasks.mark_expired_pending_exports_as_failed',
'schedule': crontab(hour='*', minute='0'),
'options': {'ignore_result': True}
},
'Delete-Expired-Failed-Exports': {
'task': 'onadata.apps.viewer.tasks.delete_expired_failed_exports',
'schedule': crontab(hour='*', minute='10'),
'options': {'ignore_result': True}
},
'celery.backend_cleanup': {
'task': 'celery.backend_cleanup',
'schedule': crontab(hour='*', minute='30'),
'options': {'ignore_result': True}
},
'Delete-Inactive-Users': {
'task': 'onadata.apps.api.tasks.delete_user_async',
'schedule': crontab(
0, 0, day_of_month='30'), # Execute on the 30th of every month.
'options': {'ignore_result': True}
},
}

Copy link
Contributor

@DavisRayM DavisRayM Jul 6, 2020

Choose a reason for hiding this comment

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

I don't think this configuration should be placed within the common settings..... Celery beat scheduling seems like something the user should configure within their own local settings IMO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Places this here since the tasks are things that should run in all environments/deployments of onadata

Im unsure which settings file would be a better fit for this configurations.
Cc: @ivermac

Copy link
Contributor

@ivermac ivermac left a comment

Choose a reason for hiding this comment

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

It generally looks good. I've added a question and a couple of comments

'Delete-Inactive-Users': {
'task': 'onadata.apps.api.tasks.delete_user_async',
'schedule': crontab(
0, 0, day_of_month='30'), # Execute on the 30th of every month.
Copy link
Contributor

Choose a reason for hiding this comment

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

@WinnyTroy What about the month of February? Can we consider may be the 1st day of every month?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @DavisRayM on having the configuration on a user's local settings file

Copy link
Contributor

Choose a reason for hiding this comment

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

That said, I like the fact that functionality will be scheduled and executed once a month

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, sure, I'll remove this configurations from here.
And I see no problem with having this run on the 1st of every month. Let me change that

@WinnyTroy WinnyTroy force-pushed the Update_settings_crontab branch 2 times, most recently from 00aa23c to 5fd0378 Compare July 6, 2020 11:51
ivermac
ivermac previously approved these changes Jul 6, 2020
"User Ken deleted successfully.",
out.getvalue())

with self.assertRaises(ObjectDoesNotExist):
Copy link
Contributor

Choose a reason for hiding this comment

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

Tiny thing/change.... Please use User.DoesNotExist

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool cool. Changed that.

@WinnyTroy WinnyTroy force-pushed the Update_settings_crontab branch 2 times, most recently from 7e00fbf to d23c866 Compare July 6, 2020 12:31
DavisRayM
DavisRayM previously approved these changes Jul 6, 2020
DavisRayM
DavisRayM previously approved these changes Jul 6, 2020
@DavisRayM DavisRayM merged commit a52a793 into master Jul 22, 2020
@DavisRayM DavisRayM deleted the Update_settings_crontab branch July 22, 2020 11:38
@DavisRayM DavisRayM mentioned this pull request Jul 28, 2020
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Delete Users Functionality Updates
3 participants