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

Refs #31405 -- Moved redirect_to_login to utils.py. #18072

Closed
wants to merge 1 commit into from

Conversation

Hisham-Pak
Copy link
Contributor

@Hisham-Pak Hisham-Pak commented Apr 13, 2024

Trac ticket number

ticket-31405

Branch description

This pull request moves redirect_to_login to a new file utils.py from views.py in order to avoid circular import instead of importing locally in a function.
#17792 (comment)

Checklist

  • This PR targets the main branch.
  • The commit message is written in past tense, mentions the ticket number, and ends with a period.
  • I have checked the "Has patch" ticket flag in the Trac system.
  • I have added or updated relevant tests.
  • I have added or updated relevant docs, including release notes if applicable.
  • For UI changes, I have attached screenshots in both light and dark modes.

Copy link
Contributor

@sarahboyce sarahboyce left a comment

Choose a reason for hiding this comment

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

This also needs a release note in 5.1, see this PR as an example of a similar change 👍

@@ -231,7 +231,7 @@ def inner(request, *args, **kwargs):
return HttpResponseRedirect(index_path)
# Inner import to prevent django.contrib.admin (app) from
# importing django.contrib.auth.models.User (unrelated model).
from django.contrib.auth.views import redirect_to_login
from django.contrib.auth.utils import redirect_to_login
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you move this to be imported at the top?

Comment on lines +16 to +18

# For backwards compatibility with Django < 5.1
from django.contrib.auth.utils import redirect_to_login # NOQA: F401
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if we have to do this, if so we should have a deprecation message.
Looking at this PR which is a little similar (#17050) looks like we don't necessarily need to. 🤔

@sarahboyce
Copy link
Contributor

@Hisham-Pak I've had a discussion with some other contributors on Discord and, as this refactor requires a deprecation without a meaningful gain to a user, we've decided it doesn't make sense.

I appreciate you did this after my suggestion and thank you for doing so.
It takes time to build a picture of what makes the most sense, so please be patient with me 🌟

@sarahboyce sarahboyce closed this Apr 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants