-
Notifications
You must be signed in to change notification settings - Fork 415
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
feat(task-processor): Add recurring task to clean password reset #3153
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@@ -21,7 +22,7 @@ | |||
|
|||
logger = logging.getLogger(__name__) | |||
|
|||
TASKS_MODULES_TO_RELOAD = [processor_tasks, sse_tasks] | |||
TASKS_MODULES_TO_RELOAD = [processor_tasks, sse_tasks, auth_tasks] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to add auth_tasks here because that module is not imported by anything, i.e: python will not load it otherwise... It's not perfect, please feel free to suggest a better idea
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's considered good practice to use AppConfig.ready()
for such cases, e.g.
# custom_auth/apps.py
class CustomAuthAppConfig(AppConfig):
name = "custom_auth"
def ready(self) -> None:
from custom_auth import tasks
# settings/common.py
...
INSTALLED_APPS = [
...
"custom_auth",
...
]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the phrased the comment wrong, we need a way to load it but not before we set RUN_BY_PROCESSOR
. Importing tasks in ready will mean that we need to reload tasks here anyway
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd set RUN_BY_PROCESSOR
outside the command then, e.g. in the run_task_processor
function in the entrypoint script. Seems less hacky this way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's a great idea! I wonder why it was not implemented like this in the first place
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Uffizzi Preview |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3153 +/- ##
==========================================
- Coverage 96.08% 96.02% -0.07%
==========================================
Files 1054 1062 +8
Lines 31702 32220 +518
==========================================
+ Hits 30462 30938 +476
- Misses 1240 1282 +42 ☔ View full report in Codecov by Sentry. |
Thanks for submitting a PR! Please check the boxes below:
pre-commit
to check lintingdocs/
if required so people know about the feature!Changes
Create a recurring task to clean-up
UserPasswordResetRequest
tablecloses #2649
How did you test this code?
Adds unit test cases