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.

Sign up for GitHub

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

Jump to bottom

Fixed #35030 -- Made django.contrib.auth decorators to work with async functions. #17621

Merged
merged 1 commit into from Mar 7, 2024
Merged

Fixed #35030 -- Made django.contrib.auth decorators to work with async functions. #17621

merged 1 commit into from Mar 7, 2024
+288 −19

Conversation

HappyDingning
Copy link
Contributor

Hi, dear reviewers,

Follow up to ticket-35030, I have achieved the alogin_required , the apermission_required and the corresponding test cases. I haven't write the documentation yet. Could you please review the code first? I will write the documentation soon.

Thank you.

@HappyDingning HappyDingning force-pushed the ticket-35030-alogin_required branch 4 times, most recently from 9e069c4 to ae336d9 Compare January 24, 2024 16:51
@HappyDingning HappyDingning changed the title Refs #35030: Addded alogin_required and apermission_required Refs #35030: Make django.contrib.auth decorators work with async functions. Jan 25, 2024
felixxm
felixxm reviewed Feb 2, 2024
View reviewed changes
Copy link
Member

@felixxm felixxm left a comment

Choose a reason for hiding this comment

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

@HappyDingning Thanks 👍 I left more comments.

@bigfootjon Do you want to take a look?

django/contrib/auth/decorators.py Outdated Show resolved Hide resolved
docs/topics/auth/default.txt Outdated Show resolved Hide resolved
tests/async/test_async_auth_decorators.py Outdated Show resolved Hide resolved
tests/async/test_async_auth_decorators.py Outdated Show resolved Hide resolved
tests/async/test_async_auth_decorators.py Outdated Show resolved Hide resolved
docs/topics/auth/default.txt Outdated Show resolved Hide resolved
tests/async/test_async_auth_decorators.py Outdated Show resolved Hide resolved
django/contrib/auth/decorators.py Outdated Show resolved Hide resolved
@felixxm
Copy link
Member

felixxm commented Mar 1, 2024

@bigfootjon Do you wan to take a look?

bigfootjon
bigfootjon reviewed Mar 2, 2024
View reviewed changes
Copy link
Contributor

@bigfootjon bigfootjon left a comment

Choose a reason for hiding this comment

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

Looks on the right track! I think we should cover all sync/async combinations, but otherwise this looks good to me!

django/contrib/auth/decorators.py Outdated Show resolved Hide resolved
django/contrib/auth/decorators.py Outdated Show resolved Hide resolved
@bigfootjon
Copy link
Contributor

bigfootjon commented Mar 5, 2024 •
edited

(@HappyDingning you might want to refer to this section of the contributing guide to make sure you automatically comply with Django's code formatting rules: https://docs.djangoproject.com/en/dev/internals/contributing/writing-code/coding-style/#pre-commit-checks. Or otherwise read that document page so you know what the CI jobs expect)

@HappyDingning
Copy link
Contributor Author

(@HappyDingning you might want to refer to this section of the contributing guide to make sure you automatically comply with Django's code formatting rules: https://docs.djangoproject.com/en/dev/internals/contributing/writing-code/coding-style/#pre-commit-checks. Or otherwise read that document page so you know what the CI jobs expect)

I'm so sorry, I'm using vscode in chrome, because I'm outside and inconvenient to use my own computer. I will rebase my commit later.

@bigfootjon
Copy link
Contributor

bigfootjon commented Mar 5, 2024 •
edited

I'm so sorry, I'm using vscode in chrome, because I'm outside and inconvenient to use my own computer. I will rebase my commit later.

No problem on my end! Just looked like you were having a bad time fighting the code-style rules and thought that could help.

(I'm heading to bed, good luck with getting CI to pass, I can take another pass at reviewing tomorrow [America timezone])

@HappyDingning HappyDingning force-pushed the ticket-35030-alogin_required branch 3 times, most recently from dedb192 to 3b1a4d3 Compare March 5, 2024 08:02
bigfootjon
bigfootjon approved these changes Mar 6, 2024
View reviewed changes
Copy link
Contributor

@bigfootjon bigfootjon left a comment

Choose a reason for hiding this comment

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

LGTM

@felixxm
Copy link
Member

felixxm commented Mar 6, 2024

@HappyDingning Thanks 👍 I'll start working on this patch after merging some extra tests, #17943.

@felixxm felixxm mentioned this pull request Mar 6, 2024
Merged
@felixxm felixxm self-assigned this Mar 6, 2024
@felixxm
Copy link
Member

felixxm commented Mar 6, 2024

Please don't push changes to this branch. I'll work on it tomorrow.

@felixxm felixxm changed the title Refs #35030: Make django.contrib.auth decorators work with async functions. Fixed #35030 -- Made django.contrib.auth decorators to work with async functions. Mar 7, 2024
@felixxm felixxm force-pushed the ticket-35030-alogin_required branch from 3b1a4d3 to 4b1bd61 Compare March 7, 2024 08:57
@felixxm
Copy link
Member

felixxm commented Mar 7, 2024

@HappyDingning Thanks 👍

I pushed final edits.

@felixxm felixxm force-pushed the ticket-35030-alogin_required branch from 4b1bd61 to 5493209 Compare March 7, 2024 08:59
@felixxm
Copy link
Member

felixxm commented Mar 7, 2024

Test failure is not related with this PR.

@felixxm felixxm merged commit 5493209 into django:main Mar 7, 2024
34 of 35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@felixxm felixxm felixxm approved these changes

@bigfootjon bigfootjon bigfootjon approved these changes

Assignees

@felixxm felixxm

Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@HappyDingning @felixxm @bigfootjon