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

feat: Support Django 5.1's LoginRequiredMiddleware #4049

Closed
wants to merge 1 commit into from

Conversation

adamchainz
Copy link
Contributor

Fixes #4001.

This is just a first pass to add the decorator to the views that I think should be public. I am not sure if a test is required but I also haven't loaded this into a project to check.

Submitting Pull Requests

General

  • Make sure you use semantic commit messages.
    Examples: "fix(google): Fixed foobar bug", "feat(accounts): Added foobar feature".
  • All Python code must formatted using Black, and clean from pep8 and isort issues.
  • JavaScript code should adhere to StandardJS.
  • If your changes are significant, please update ChangeLog.rst.
  • If your change is substantial, feel free to add yourself to AUTHORS.

@coveralls
Copy link

coveralls commented Aug 20, 2024

Coverage Status

coverage: 95.799% (-0.005%) from 95.804%
when pulling 8be3e7d on adamchainz:login_not_required
into 03debcf on pennersr:main.

@pennersr
Copy link
Owner

I am not sure if a test is required but I also haven't loaded this into a project to check.

Without having tests in place it would be too easy to introduce regression once this feature is merged. But also now, there is no assurance that this is complete. For example, I would have expected allauth.mfa and allauth.headless to be impacted as well.

This cannot really be covered by a few tests. You actually need to run the whole test suite with a project that uses the LoginRequiredMiddleware. Therefore, we need to introduce a tests.login_required_middleware project and run all tests for that project as part of the pipelines.

@pennersr
Copy link
Owner

pennersr commented Sep 9, 2024

There is not much point on keeping this PR open as it is far from complete. Feel free to file a PR once the work is sufficiently complete.

@pennersr pennersr closed this Sep 9, 2024
@pennersr
Copy link
Owner

pennersr commented Sep 13, 2024

I've taken a first stab at what I mentioned above here:

https://codeberg.org/allauth/django-allauth/pulls/4104

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Django-allauth views not compatible with Django 5.1 LoginRequiredMiddleware
3 participants