-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Refactor PublicTask into a decorator task #4656
Conversation
user = User.objects.get(pk=user_id) | ||
for service_cls in registry: | ||
for service in service_cls.for_user(user): | ||
service.sync() |
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.
The api for defining a public task is cleaner now :D
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.
Much nicer!
readthedocs/oauth/tasks.py
Outdated
@@ -22,20 +31,12 @@ | |||
|
|||
|
|||
@permission_check(user_id_matches) |
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.
In the celery docs, it says that we shouldn't do this (put the task decorator down), but the task decorator overrides all function's attributes and deletes the added by permission_check
, this way we can keep the added attribute.
I believe this is ready, I also tested it locally, it works as expected. |
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.
Looks great! We'll want to make sure this has some QA before release, but I don't think we use public task anywhere else at the moment.
user = User.objects.get(pk=user_id) | ||
for service_cls in registry: | ||
for service in service_cls.for_user(user): | ||
service.sync() |
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.
Much nicer!
Still wip, I need to update some docstrings, but I already tested it, and it works :)Fix #3974 and ref to #3973