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

Add official support for Django 5.1 #9514

Merged
merged 11 commits into from
Sep 7, 2024

Conversation

browniebroke
Copy link
Contributor

@browniebroke browniebroke commented Aug 29, 2024

Description

Add Django 5.1 to the CI matrix following the supported Python versions and declare official support.

Comment on lines -45 to -49
[testenv:py38-djangomain]
ignore_outcome = true

[testenv:py39-djangomain]
ignore_outcome = true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't test Django main against 3.8 or 3.9 so this is unused

Copy link
Member

Choose a reason for hiding this comment

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

right

@browniebroke
Copy link
Contributor Author

Should we try to fix the issue with the new LoginRequiredMiddleware as part of this PR? #9503

@auvipy
Copy link
Member

auvipy commented Aug 30, 2024

Should we try to fix the issue with the new LoginRequiredMiddleware as part of this PR? #9503

I think we should include that in this PR

@browniebroke browniebroke marked this pull request as draft August 30, 2024 09:08
@browniebroke browniebroke marked this pull request as ready for review August 30, 2024 17:57
@browniebroke
Copy link
Contributor Author

browniebroke commented Aug 30, 2024

I initially went with a custom LoginRequiredMiddleware but then realised that authentication status of a request in DRF isn't easy to figure out at the middleware level. This is because it's a combination of authentication_classes + permission_classes settings, which may come from the view or the global settings.

Additionally, I struggled a bit to test that solution as I couldn't find out a way to configure test views with a non-default DEFAULT_PERMISSION_CLASSES, and I always ended up with AllowAny in my views. I was aiming for integration style of tests: configuring a view, with URLs and setting up the whole middleware chain. I left the commit if someone is interested...

I took a step back and realised that DRF already offered a way to achieve what LoginRequiredMiddleware does, via its settings. So I ended marking DRF views as opted-out from Django's middleware, and documenting how to achieve the same thing with API views.

What do people think of the approach?

@RobKuipers
Copy link

Very nice solution. I feel LoginRequiredMiddleware is intended for 'normal' Django views, not for API calls.
Thank you very much for making this change.

@browniebroke
Copy link
Contributor Author

Added some more integration-like type of tests to make sure every type of views are covered: class based views, function based views, viewsets and viewsets actions.

@auvipy auvipy merged commit 2ede857 into encode:master Sep 7, 2024
7 checks passed
@browniebroke browniebroke deleted the update/django-5.1 branch September 7, 2024 16:02
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.

4 participants