-
Notifications
You must be signed in to change notification settings - Fork 40
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
Used a plain HTML form for DjangoProject login #171
Conversation
@django/accessibility to review Apparently I'm not allowed to request review in this repo. |
76b8083
to
7987c02
Compare
@felixxm ready for review I think. I got feedback from @knyghty on discord that it would be better to use I started a small test suite for the new I've split the PR into 2 commits where the first one just move files around while the second one is the actual interesting bit. |
I'll rebase this once #172 is merged 👍🏻 |
Fixes django#95, django#61 and django#60 Also possibly django#100 and django#64
7987c02
to
dfbe6dd
Compare
Rebased and ready to review @felixxm 🕵🏻 |
@@ -23,6 +26,6 @@ | |||
] | |||
|
|||
|
|||
SECRET_KEY = str(SECRETS["secret_key"]) | |||
SECRET_KEY = str(SECRETS.get("secret_key", "")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd use a non-blank default, e.g.
SECRET_KEY = str(SECRETS.get("secret_key", "")) | |
SECRET_KEY = SECRETS.get("secret_key", "django-insecure-secret") |
Also, str()
seems unnecessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My idea was to prevent accidentally running a known secret key in production which is why I used an empty default.
Isn't it the case that Django will refuse to start if it detects an empty secret key?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, sorry I was thinking we have test settings here 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but str()
is still unnecessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but
str()
is still unnecessary
Most likely yes, unless you use an integer in your secrets.json but then you had it coming 😅
@bmispelon Looks great, thanks 👍 |
Fixes #95, #61 and #60
Also possibly #100 and #64
The new unified login page looks like this (not the prettiest I must admit):
There's 3 things I'd like to do before merging this:
TODO
about checking the redirection URL after login