Opened 22 months ago

Closed 17 months ago

Last modified 15 months ago

#21293 closed Bug (fixed)

Some authentication-related templates are broken

Reported by: EvilDMP Owned by: nobody
Component: contrib.auth Version: master
Severity: Release blocker Keywords:
Cc: bmispelon@…, timo Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: yes

Description

Commit a962286b74f1e8c8cb19fb45a057800da8c2fb56 https://github.com/django/django/commit/a962286b74f1e8c8cb19fb45a057800da8c2fb56 breaks the default template for /user/password/reset/, and possibly others.

See https://www.dropbox.com/s/zqng1mftd1rgcqb/Screenshot%202013-10-19%2009.19.51.png - the "Welcome, daniele. Change password / Log out" text in <div id="user-tools"> is not visible, because it has a white background.

Attachments (2)

ticket-21293.diff (750 bytes) - added by bmispelon 22 months ago.
21293-css.diff (645 bytes) - added by timo 17 months ago.

Download all attachments as: .zip

Change History (16)

Changed 22 months ago by bmispelon

comment:1 Changed 22 months ago by bmispelon

  • Cc bmispelon@… added
  • Triage Stage changed from Unreviewed to Accepted

The problem comes from the fact that password_reset does not pass a site_header variable to its context. Consequently, the header ends up being empty and shrinks.
Therefore, the text that should have been printed on the header's dark background gets printed onto the page's white background, which makes it invisible.

I attached a tentative solution but I'm not sure if it's ideal. Tweaking the CSS might be a better idea.

I couldn't find whether this had been backported to 1.6 (the comments on github by adrian indicate that was the original intent but I can't find a backport commit on stable/1.6.x).
If it has indeed been backported, I think it should be marked as a release blocker since it changes the old behavior.

comment:2 Changed 22 months ago by EvilDMP

The removal of {% trans 'Django administration' %} in admin/base_site.html is what breaks (for example) password_reset_form.html, since https://github.com/django/django/blob/master/django/contrib/auth/views.py#L133 doesn't pass in a site_header. I am not sure how to resolve this, since probably we should not be showing "Django administration" text on non-admin authentication pages.

Options:

  • change the views in auth.views
  • change the HTML/CSS in base_site.html so that the user_tools div is not at the mercy of the background on the h1.

comment:3 Changed 22 months ago by timo

I can confirm this wasn't backported to 1.6.

comment:4 Changed 21 months ago by Claude Paroz <claude@…>

In 9c6b57f709bb6123d8abae6ba05e92085ec2f426:

Merge pull request #1877 from Bouke/patches/4

Fixed missing admindocs' site_header, refs #21293

comment:5 Changed 19 months ago by claudep

  • Severity changed from Normal to Release blocker

Blocker for 1.7

Changed 17 months ago by timo

comment:6 Changed 17 months ago by timo

  • Cc timo added
  • Has patch set

Added a proposed fix for the CSS: removed the absolute positioning of the usertools div and used float positioning to allow the #header div to expand based on the height on its content.

comment:7 Changed 17 months ago by andrewgodwin

Timo: Is there anything that means we shouldn't commit this? The fix looks valid to me.

comment:8 follow-up: Changed 17 months ago by Tim Graham <timograham@…>

  • Resolution set to fixed
  • Status changed from new to closed

In 6f470650d097abbc06021efd50b2bdc3c9ce4d3b:

Fixed #21293 -- Adjusted admin header CSS to fix admin password reset template.

By removing the absolute positioning of the usertools div and using
float positioning, the #header div will expand based on the height of
its content.

Thanks EvilDMP for the report.

comment:9 in reply to: ↑ 8 Changed 17 months ago by ramiro

  • Resolution fixed deleted
  • Status changed from closed to new

Replying to EvilDMP:

Commit a962286b74f1e8c8cb19fb45a057800da8c2fb56 https://github.com/django/django/commit/a962286b74f1e8c8cb19fb45a057800da8c2fb56 breaks the default template for /user/password/reset/,

Are you sure this happened actually is for the password reset view (the one in which the user enters his email address so a token is sent there)?

AFAIK there is no access to that particular view from the admin app and the two password related pages there are

a) the one in which a staff user can set his new password by entering the old one (i.e. /admin/password_change/) and
b) the one in which a superuser can force a new password on any user (i.e. /admin/auth/user/<pk>/password/).

Yes, the naming is a bit confusing :)

I observed the problem reported by this ticket only in case b.

and possibly others.

Was this actually a problem for any other view?

Replying to EvilDMP:

I am not sure how to resolve this, since probably we should not be showing "Django administration" text on non-admin authentication pages.

In case b there is no danger of this because it's fully handled in the admin realm in django/contrib/auth/admin.py:userAdmin.user_change_password()

Replying to Tim Graham <timograham@…>:

Fixed #21293 -- Adjusted admin header CSS to fix admin password reset template.

By removing the absolute positioning of the usertools div and using
float positioning, the #header div will expand based on the height of
its content.

With this change the height of the header is fixed but, when compared to 1.6, the "Django administration" title text usually located on the left is still missing.

For that, I've opened https://github.com/django/django/pull/2418 that updates the context passed to the template in the same spirit of 9c6b57f709bb6123d8abae6ba05e92085ec2f426.

Version 0, edited 17 months ago by ramiro (next)

comment:10 Changed 17 months ago by Ramiro Morales <cramm0@…>

In 1d42a86ec7e16336955735474f88e7632a34f3cb:

Tweak password admin change form view context. Refs #21293.

comment:11 Changed 17 months ago by Ramiro Morales <cramm0@…>

In 6d3ebe10f4f8c4b91c9cbeccf9696ca79b6909f9:

Merge pull request #2418 from ramiro/21293-tweak

Tweak password admin change form view context. Refs #21293.

comment:12 Changed 17 months ago by ramiro

  • Resolution set to fixed
  • Status changed from new to closed

I've merged the fix for the missing "Django administration" title in the force-new-password-by-staff view. Will analyze the password reset view (https://docs.djangoproject.com/en/dev/ref/contrib/admin/#adding-a-password-reset-feature) issue after beta.

comment:13 Changed 15 months ago by Ramiro Morales <cramm0@…>

In 5ea34f3f86af307e1a1c02b49136f51a4f63da88:

Restored site header text in password reset view.

It's the one shown when the optional integration described in
https://docs.djangoproject.com/en/dev/ref/contrib/admin/#adding-a-password-reset-feature
is used.

Follow-up to commits 6f470650d0 and 1d42a86ec7, together they fix
different small UI regressions after a962286b74.

Refs #21293.

comment:14 Changed 15 months ago by Ramiro Morales <cramm0@…>

In bc82c0dbac33ced1a4d6953e8626236ae05af956:

[1.7.x] Restored site header text in password reset view.

It's the one shown when the optional integration described in
https://docs.djangoproject.com/en/dev/ref/contrib/admin/#adding-a-password-reset-feature
is used.

Follow-up to commits 6f470650d0 and 1d42a86ec7, together they fix
different small UI regressions after a962286b74.

Refs #21293.

5ea34f3f86 from master.

Note: See TracTickets for help on using tickets.
Back to Top