Skip to content

Conversation

@Natureshadow
Copy link
Contributor

Description of the Change

This adds a Celery task to clean expired tokens. It can be used to schedule token cleanup in the background, for applications using Celery and Celery beat.

The tasks module is autodiscovered in projects that use Celery, and should not be loaded in other projects, so this does not introduce any new dependency,

Checklist

  • PR only contains one change (considered splitting up PR)
  • unit-test added
  • documentation updated
  • CHANGELOG.md updated (only for user relevant changes)
  • author name in AUTHORS

@codecov
Copy link

codecovbot commented Jan 5, 2022

Codecov Report

Merging #1070 (2d96223) into master (ac20152) will decrease coverage by 0.27%.
The diff coverage is 0.00%.

❗ Current head 2d96223 differs from pull request most recent head af39ed1. Consider uploading reports for the commit af39ed1 to get more accurate results
Impacted file tree graph

@@ Coverage Diff @@## master #1070 +/- ## ========================================== - Coverage 96.87% 96.59% -0.28%  ========================================== Files 31 32 +1 Lines 1759 1764 +5 ========================================== Hits 1704 1704 - Misses 55 60 +5 
Impacted FilesCoverage Δ
oauth2_provider/tasks.py0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ac20152...af39ed1. Read the comment docs.

@n2ygkn2ygk added this to the 1.7.0 milestone Jan 5, 2022
Copy link
Contributor

@n2ygkn2ygk left a comment

Choose a reason for hiding this comment

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

Hey, this is cool. Is there a way to provide some test coverage of this?

@Natureshadow
Copy link
ContributorAuthor

Hey, this is cool. Is there a way to provide some test coverage of this?

Sure there is. But it requires to run Celery alongside the test suite. I don't think it is worth adding that complexity to the test suite, given that the test would ensure nothing more that a simple function call calls a function, and Celery can run Python code (which is hopefully tested in Celery itself).

@n2ygk
Copy link
Contributor

n2ygk commented Jan 6, 2022

Can this functionality be accommodated external to DOT such as in your Django app? I'm not sure we should be adding code that's not core DOT functionality here.
@MattBlack85 what do you think?

@Natureshadow
Copy link
ContributorAuthor

Natureshadow commented Jan 6, 2022 via email

@n2ygk
Copy link
Contributor

n2ygk commented Jan 9, 2022

@Natureshadow I'm trying to understand what to expect with Celery. Is the defined task supposed to show up in Django Admin? What additional configuration is needed to make this happen in, for example, https://github.com/n2ygk/dot-tutorial?

I made a few changes but am missing something fundamental I think. Do some templates need to be added oauth2_provider?

@Natureshadow
Copy link
ContributorAuthor

Natureshadow commented Jan 9, 2022 via email

Copy link
Contributor

@auvipyauvipy left a comment

Choose a reason for hiding this comment

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

if the defined scheduled task's to be shown in admin might need to use django-celery-beat & django-celery-results to be used/configured to specific project. [I actively maintain both but more work need to be done before deciding to add celery as dependency]

@n2ygk
Copy link
Contributor

if the defined scheduled task's to be shown in admin might need to use django-celery-beat & django-celery-results to be used/configured to specific project. [I actively maintain both but more work need to be done before deciding to add celery as dependency]

I'm looking for specific configuration changes to my test DOT app so that I can see the celery tasks in the admin console.

@n2ygk
Copy link
Contributor

I will look into that tomorrow.

Hi @Natureshadow, just checking in to see if you've had time to look at this.

@Natureshadow
Copy link
ContributorAuthor

I checked what we did in AlekSIS for this, and it seems the steps laid out in the Celery documentation are sufficient.

@n2ygk
Copy link
Contributor

@Natureshadow I finally had time to try this out. I basically followed the instructions at:

And see the admin interface and was able to autodiscover the task. I didn't get it completely working (econnrefused) but close enough to understand what I needed to.

Please rebase this PR and I'll merge it.

Thanks.

@NatureshadowNatureshadowforce-pushed the feature/celery-task-clear-expired branch from a689741 to 2d96223CompareJanuary 18, 2022 20:44
@NatureshadowNatureshadowforce-pushed the feature/celery-task-clear-expired branch from 2d96223 to af39ed1CompareJanuary 18, 2022 20:45
@Natureshadow
Copy link
ContributorAuthor

Please rebase this PR and I'll merge it.

Thanks.

Done!

@n2ygkn2ygk merged commit a6a21d3 into django-oauth:masterJan 18, 2022
n2ygk added a commit to n2ygk/django-oauth-toolkit that referenced this pull request Mar 19, 2022
n2ygk added a commit to n2ygk/django-oauth-toolkit that referenced this pull request Mar 19, 2022
@n2ygkn2ygk mentioned this pull request Mar 19, 2022
5 tasks
n2ygk added a commit to n2ygk/django-oauth-toolkit that referenced this pull request Mar 19, 2022
n2ygk added a commit to n2ygk/django-oauth-toolkit that referenced this pull request Mar 19, 2022
n2ygk added a commit that referenced this pull request Mar 19, 2022
* Revert #1070: tasks.py raises an import exception with Celery and conflicts with Huey.
n2ygk added a commit that referenced this pull request Mar 19, 2022
* Revert #1070: tasks.py raises an import exception with Celery and conflicts with Huey. * bump version to 1.7.1
Sign up for freeto 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.

3 participants

@Natureshadow@n2ygk@auvipy