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

Used a plain HTML form for DjangoProject login #171

Merged
merged 4 commits into from
Feb 2, 2024

Conversation

bmispelon
Copy link
Member

@bmispelon bmispelon commented Jan 28, 2024

Fixes #95, #61 and #60
Also possibly #100 and #64

The new unified login page looks like this (not the prettiest I must admit):
Screenshot 2024-01-28 at 14-29-35 Login – Django

There's 3 things I'd like to do before merging this:

  • Validate the html/css with the accessibility team
  • Address the TODO about checking the redirection URL after login
  • Add some tests

@knyghty
Copy link
Member

knyghty commented Jan 28, 2024

@django/accessibility to review

Apparently I'm not allowed to request review in this repo.

@bmispelon bmispelon force-pushed the simplify-djangoproject-auth branch 3 times, most recently from 76b8083 to 7987c02 Compare January 29, 2024 13:35
@bmispelon bmispelon changed the title [WIP] Used a plain HTML form for DjangoProject login Used a plain HTML form for DjangoProject login Jan 29, 2024
@bmispelon
Copy link
Member Author

@felixxm ready for review I think.

I got feedback from @knyghty on discord that it would be better to use form.as_div() but that will only become available in Django 4.1 so I'll have to come back to it then.

I started a small test suite for the new PlainLoginComponent and plugged it in to CI but my knowledge of github actions is pretty limited so if you see anything weird there it's probably ignorance on my part and you should definitely flag it.

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.

.github/workflows/tests.yml Outdated Show resolved Hide resolved
@felixxm
Copy link
Member

felixxm commented Feb 1, 2024

I merged the first commit in 84a01a0 and pushed noshadows tests to the #172.

@bmispelon
Copy link
Member Author

I'll rebase this once #172 is merged 👍🏻

@bmispelon
Copy link
Member Author

Rebased and ready to review @felixxm 🕵🏻

@@ -23,6 +26,6 @@
]


SECRET_KEY = str(SECRETS["secret_key"])
SECRET_KEY = str(SECRETS.get("secret_key", ""))
Copy link
Member

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.

Suggested change
SECRET_KEY = str(SECRETS.get("secret_key", ""))
SECRET_KEY = SECRETS.get("secret_key", "django-insecure-secret")

Also, str() seems unnecessary.

Copy link
Member Author

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?

Copy link
Member

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 😅

Copy link
Member

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

Copy link
Member Author

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 😅

.github/workflows/tests.yml Outdated Show resolved Hide resolved
@felixxm
Copy link
Member

felixxm commented Feb 2, 2024

@bmispelon Looks great, thanks 👍

@felixxm felixxm merged commit eb16ed0 into django:main Feb 2, 2024
3 checks passed
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.

Add a link to the password reset URL.
3 participants