Admin: Log in page #2371

machikoyasuda · 2024-09-16T23:29:23Z

closes #2265

What this PR does

Sets site_header, site_title for BenefitsAdminSite
CSS: Add default Benefit focus states to login inputs, buttons
CSS: Removes darkmode from Agency Base and Login
Templates: Login and Agency Base - Moves Django's Login.css code to Styles.css and changes some of the style declarations
Login template: Creates login.html to override parts of Django's login.html's branding section to add the Cal-ITP image.

How to test

Test with SSO_SHOW_FORM_ON_ADMIN_PAGE set to False
Test with SSO_SHOW_FORM_ON_ADMIN_PAGE set to True
Test with an error/info/warning message
Test on dark (browser set to dark mode) and light mode - Should only show light mode
Test focus states
Test with a wrong password, error message should show up

This PR also resolves all outstanding WCAG 2.0 issues:

github-actions · 2024-09-16T23:30:29Z

Coverage report

This PR does not seem to contain any modification to coverable code.

machikoyasuda · 2024-09-17T00:19:33Z

Starting to notice a slew of weirdness having Bootstrap loaded alongside Django Admin's default variables:

The ordering of CSS variable files matter. I had to move our own styles.css last here https://github.com/cal-itp/benefits/pull/2371/files#diff-cf7d89ad332a4e833961cb19b0e583671aa226405c701eea97bade4f51fc1790R14 for the variables to properly override Bootstrap's.
This is what happens in Dark mode: Bootstrap sets the base body font color to var(--bs-body-color), so that overrides the Django base.css's var(--body-fg).

Django's login template seems to be inserting its own jQuery into the HTML

machikoyasuda · 2024-09-17T23:53:05Z

benefits/templates/admin/login.html

+{% block extrastyle %}
+    <link href="{% static "img/favicon.ico" %}" rel="icon" type="image/x-icon" />
+    <script nonce="{{ request.csp_nonce }}"
+            src="https://cdn.jsdelivr.net/npm/[email protected]/dist/jquery.min.js"
+            integrity="sha256-o88AwQnZB+VDvE9tvIXrMQaPlFFSUTR+nldQm1LuPXQ="
+            crossorigin="anonymous"></script>
+    <link href="https://cdn.jsdelivr.net/npm/[email protected]/dist/css/bootstrap.min.css"
+          rel="stylesheet"
+          integrity="sha384-1BmE4kWBq78iYhFldvKuhfTAU6auU8tT94WrHftjDbrCEXSU1oBoqyl2QvZ6jIW3"
+          crossorigin="anonymous">
+    <link href="{% static "css/admin/styles.css" %}" rel="stylesheet">
+{% endblock extrastyle %}


Question! @lalver1 @angela-tran - I put this here b/c the login.html template is not already inheriting the extrastyle block from agency-base. Why is that? Is a way I can get login.html to inherit from agency-base?

Also note here: I had to move the call to styles.css to line 17 so that the variable overrides set in there actually do override the Bootstrap variables. That's why I also made this same change in agency-base.

@machikoyasuda I think the reason is that benefits/templates/admin/login.html extends contrib/admin/login.html and agency-base is not in that 'chain' of inheritance (agency-base extends contrib/admin/base.html) so login.html is not inheriting the extrastyle block from agency-base.

What you have works, but I guess you could also have benefits/templates/admin/login.html extend agency-base and duplicate some code from contrib/admin/login.html into benefits/templates/admin/login.html. Maybe that duplication would be ok since we are basically overriding the whole login template.

I tested this alternate approach about extending agency-base.html instead and login.html would look like this (we no longer need the extrastyle block):

{% extends "admin/agency-base.html" %} {% load i18n static %} {% block bodyclass %} {{ block.super }} login {% endblock bodyclass %} {% block usertools %} {% endblock usertools %} {% block nav-global %} {% endblock nav-global %} {% block content %} {% if form.errors and not form.non_field_errors %} <p class="errornote"> {% blocktranslate count counter=form.errors.items|length %}Please correct the error below.{% plural %}Please correct the errors below.{% endblocktranslate %} </p> {% endif %} {% if form.non_field_errors %} {% for error in form.non_field_errors %}<p class="errornote">{{ error }}</p>{% endfor %} {% endif %} <div id="content-main"> {% if user.is_authenticated %} <p class="errornote"> {% blocktranslate trimmed %} You are authenticated as {{ username }}, but are not authorized to access this page. Would you like to login to a different account? {% endblocktranslate %} </p> {% endif %} <form action="{{ app_path }}" method="post" id="login-form"> {% csrf_token %} <div class="form-row"> {{ form.username.errors }} {{ form.username.label_tag }} {{ form.username }} </div> <div class="form-row"> {{ form.password.errors }} {{ form.password.label_tag }} {{ form.password }} <input type="hidden" name="next" value="{{ next }}"> </div> {% url 'admin_password_reset' as password_reset_url %} {% if password_reset_url %} <div class="password-reset-link"> <a href="{{ password_reset_url }}">{% translate "Forgotten your password or username?" %}</a> </div> {% endif %} <div class="submit-row"> <input type="submit" value="{% translate "Log in" %}"> </div> </form> </div> {% endblock content %} {% block branding %} <div class="d-flex justify-content-center w-100 bg-primary"> <img class="my-3" src="{% static "img/logo-lg.svg" %}" width="220" height="50" alt="California Integrated Travel Project: Benefits logo (large)" /> </div> <div id="site-name"> <h1 class="text-center text-white fs-3 py-3 m-0">{{ site_header }}</h1> </div> {% endblock branding %}

Thank you for checking this! It makes sense to me now, how the inheriting happens. I think I want to err on the side of caution of not wanting to accidentally import jQuery twice, so I decided I actually don't want to inherit extrastyles from agency-base. So I am good with how the login.html template looks like now: minimal block overriding, just overriding branding basically, and an extrastyles that removes the jQuery script. Referenced here #2371 (comment)

machikoyasuda · 2024-09-18T00:00:10Z

benefits/templates/admin/login.html

+{% block dark-mode-vars %}
+{% endblock dark-mode-vars %}


This removes the dark mode CSS styles, effectively disabling dark mode on Admin. cc @thekaveman @angela-tran @lalver1

Right now, the Bootstrap body class declarations are overriding Django's, and we can't really change that. The extrastyle block comes after the style files from Django, and CSS reads the last file's overrides. So the dark mode looks messed up - the text is all dark (as set by Bootstrap). If we could easily set Django's CSS files to be declared last and Bootstrap's first, then we could implement dark mode. But for now it's kinda complex so I think we should just soft launch this site w/o dark mode.

machikoyasuda · 2024-09-18T00:06:12Z

benefits/static/css/admin/styles.css

+.login .password-reset-link {
+  text-align: center;
+}


This is directly from https://github.com/django/django/blob/9ca1f6eff6f19d1ae074d289c6c4209073351805/django/contrib/admin/static/admin/css/login.css#L59 but I don't know how to trigger it or if it shows up in our app

If your database has the user from the sample fixtures (CST User) and you log in as that user, you'll see a "Change password" link in the usertools section.

... speaking of which, that link takes us to a page that we have not overridden / that doesn't use our extrastyles, so it uses all the default Django styles.

I'm not sure if what we should have this link or if we should override it to use our extra styles. I'll create a separate ticket for this.

Looks like the link shows up based on if the user "has a usable password":

https://github.com/django/django/blob/1857b6663b664847a426e8c79b8aec42994660ba/django/contrib/admin/templates/admin/base.html#L56-L58

Created #2384

Oh wait, password-reset-link is actually a different thing from what I was talking about.

It's a link on the login page. I guess we wouldn't show it because we don't have a 'admin_password_reset' route defined.

https://github.com/search?q=repo%3Adjango%2Fdjango%20password-reset-link&type=code

https://github.com/django/django/blob/1857b6663b664847a426e8c79b8aec42994660ba/django/contrib/admin/templates/admin/login.html#L59-L61

https://docs.djangoproject.com/en/5.1/ref/contrib/admin/#adding-a-password-reset-feature

I don't think we should worry about adding this now since we ideally want to use SSO long-term

On the other hand, any kind of self-service we can offer to users while we have to manage them locally would be handy.

Let's create another ticket to figure out what to do about the Reset link on the login page.

Created #2386 for the reset password link feature

machikoyasuda · 2024-09-18T00:06:44Z

benefits/static/css/admin/styles.css

 iframe.card-collection {
  border: 0px;
  width: 100%;
  height: 60vh;
 }
+
+/* Login Page */


This is copied + pasted from https://github.com/django/django/blob/9ca1f6eff6f19d1ae074d289c6c4209073351805/django/contrib/admin/static/admin/css/login.css#L1 except with a few modifications necessary for the design of our page.

machikoyasuda · 2024-09-18T00:41:56Z

benefits/admin.py

+
+
+BenefitsAdminSite.site_header = "Administrator"
+BenefitsAdminSite.site_title = "Cal-ITP Benefits - Administrator"


@indexing What should the default title of the Admin app be?

This will set it so that each page is something like, Log in | Cal-ITP Benefits - Administrator

We may want to set these 2 values in the templates themselves @machikoyasuda since site_title can be different across the pages of in-person enrollment and site_header is only used in the login page. So setting site_title using the title block and setting site_header directly in the branding block.

machikoyasuda · 2024-09-18T00:43:25Z

benefits/static/css/admin/styles.css

+.btn-ouline-dark:focus-visible,
+input[type="text"]:focus,
+input[type="password"]:focus,
+.google-login-btn a:focus,
+.login input[type="submit"]:focus {


Adds Benefits-style focus styles to all buttons and inputs

machikoyasuda · 2024-09-18T00:48:21Z

I'm marking this PR as complete.

Some deviations from the mocks:

I did not add the Choose an authentication method... b/c we only have one SSO method so far
I removed the dark/light mode toggle b/c it would require more work to get the styling right

Lastly, I removed the jQuery script from the extrastyle section on the login template b/c the Django template adds it in itself: 4e1e946

machikoyasuda · 2024-09-19T20:15:00Z

benefits/templates/admin/login.html

+{% block title %}
+    Log in | Cal-ITP Benefits Administrator
+{% endblock title %}


I hardcoded the title here, instead of setting variables for site_header and site_title. I discussed with Andy and he's going to be adding official titles into Miro so that we can update all of the titles throughout the Admin app in a clean way, and I made a ticket to document that these titles are still TBD, but need to fixed in the future #2381.

machikoyasuda · 2024-09-19T20:18:31Z

@thekaveman @angela-tran @lalver1 This PR is ready for review again.

I did a design review of this PR with @indexing earlier today, and we identified some design/copy requirements that were missing from the Miro/the ticket. I made tickets for them - #2380 and #2381 - and Andy is working on the design/copy requirements and updating Miro for it, so we can work on it in the future. This current PR will not address these and it can be addressed in the future. Both are small changes, but I didn't want these things to block this PR.

angela-tran

This looks good to me! I saw a place where it might be helpful to have a comment, but I won't block the PR for it. I can re-approve if you decide to add it.

angela-tran · 2024-09-19T21:34:58Z

benefits/templates/admin/login.html

+    Log in | Cal-ITP Benefits Administrator
+{% endblock title %}
+
+{% block extrastyle %}


Maybe add a comment here to say why we're not bringing in jQuery here / why we're using our own extrastyle block here instead of extending from agency-base.html?

Good idea! Added here: 27bbf27

machikoyasuda self-assigned this Sep 16, 2024

machikoyasuda force-pushed the feat/2265-admin-login branch from 22bb762 to 4729646 Compare September 17, 2024 00:50

machikoyasuda commented Sep 17, 2024

View reviewed changes

machikoyasuda commented Sep 18, 2024

View reviewed changes

machikoyasuda marked this pull request as ready for review September 18, 2024 00:45

machikoyasuda requested a review from a team as a code owner September 18, 2024 00:45

machikoyasuda force-pushed the feat/2265-admin-login branch 2 times, most recently from 3efa445 to 82e563c Compare September 18, 2024 00:54

thekaveman added this to the Admin tool: agency users milestone Sep 18, 2024

machikoyasuda added 5 commits September 19, 2024 13:12

feat(admin): first pass @ login page style, title override

ef113e7

feat(admin): style Google SSO button, branding, button, focus

a56e42d

fix(admin): remove dark mode, set styles.css to override Bootstrap

6de5abe

chore(admin): remove jQuery from extrastyle

a0bd133

fix(admin): hardcode title, header into template instead

f8b67fe

machikoyasuda force-pushed the feat/2265-admin-login branch from cf83904 to f8b67fe Compare September 19, 2024 20:12

machikoyasuda commented Sep 19, 2024

View reviewed changes

angela-tran mentioned this pull request Sep 19, 2024

Admin: Decide what to do with "Change password" page #2384

Open

angela-tran previously approved these changes Sep 19, 2024

View reviewed changes

chore: add comment to explain extrastyle

27bbf27

machikoyasuda dismissed angela-tran’s stale review via 27bbf27 September 19, 2024 21:53

machikoyasuda requested a review from angela-tran September 19, 2024 21:54

angela-tran mentioned this pull request Sep 19, 2024

Decide if we should add a "Reset password" link to the admin login page #2386

Open

3 tasks

angela-tran approved these changes Sep 19, 2024

View reviewed changes

machikoyasuda merged commit f4f38de into main Sep 19, 2024
10 checks passed

machikoyasuda deleted the feat/2265-admin-login branch September 19, 2024 22:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Admin: Log in page #2371

Admin: Log in page #2371

machikoyasuda commented Sep 16, 2024 •

edited

Loading

github-actions bot commented Sep 16, 2024 •

edited

Loading

machikoyasuda commented Sep 17, 2024 •

edited

Loading

machikoyasuda Sep 17, 2024

lalver1 Sep 18, 2024

lalver1 Sep 18, 2024

machikoyasuda Sep 18, 2024 •

edited

Loading

machikoyasuda Sep 18, 2024

machikoyasuda Sep 18, 2024

angela-tran Sep 19, 2024

angela-tran Sep 19, 2024 •

edited

Loading

angela-tran Sep 19, 2024

angela-tran Sep 19, 2024

angela-tran Sep 19, 2024

thekaveman Sep 19, 2024

angela-tran Sep 19, 2024

machikoyasuda Sep 18, 2024

machikoyasuda Sep 18, 2024

lalver1 Sep 18, 2024

machikoyasuda Sep 18, 2024

machikoyasuda commented Sep 18, 2024 •

edited

Loading

machikoyasuda Sep 19, 2024

machikoyasuda commented Sep 19, 2024

angela-tran left a comment

angela-tran Sep 19, 2024

machikoyasuda Sep 19, 2024



		BenefitsAdminSite.site_header = "Administrator"
		BenefitsAdminSite.site_title = "Cal-ITP Benefits - Administrator"

Admin: Log in page #2371

Admin: Log in page #2371

Conversation

machikoyasuda commented Sep 16, 2024 • edited Loading

What this PR does

How to test

github-actions bot commented Sep 16, 2024 • edited Loading

Coverage report

machikoyasuda commented Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

machikoyasuda Sep 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

angela-tran Sep 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

machikoyasuda commented Sep 18, 2024 • edited Loading

Choose a reason for hiding this comment

machikoyasuda commented Sep 19, 2024

angela-tran left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

machikoyasuda commented Sep 16, 2024 •

edited

Loading

github-actions bot commented Sep 16, 2024 •

edited

Loading

machikoyasuda commented Sep 17, 2024 •

edited

Loading

machikoyasuda Sep 18, 2024 •

edited

Loading

angela-tran Sep 19, 2024 •

edited

Loading

machikoyasuda commented Sep 18, 2024 •

edited

Loading