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

Feat: transit agency home dashboard #2313

Merged
merged 5 commits into from
Aug 26, 2024
Merged

Feat: transit agency home dashboard #2313

merged 5 commits into from
Aug 26, 2024

Conversation

lalver1
Copy link
Member

@lalver1 lalver1 commented Aug 20, 2024

Part of #2246

This PR implements overriding the default admin site so that transit agency staff see a home dashboard upon logging in:
image

A superuser or Cal-ITP user still sees the default Django admin site.

Another PR will implement the top right corner "user tools" panel for transit agency staff, and another PR will implement a base html template that will be extended by all templates in the in_person app.

Steps to test:

  • Reset database to use local_fixtures (bin/reset_db.sh)
  • Set GOOGLE_SSO_ALLOWABLE_DOMAINS, GOOGLE_SSO_STAFF_LIST, GOOGLE_SSO_SUPERUSER_LIST to appropriate values (your email and domain) in .env
  • Go to localhost/admin and sign in with your email address
  • Add a CST group
  • Add the CST group to CST transit agency’s “staff_group” (it will ask for audience and client_id, use dummy values)
  • Change your user to not superuser
  • Remove Cal-ITP from your user’s chosen groups
  • Add the CST group to your user’s chosen groups
  • Save (will log you out)

NOTE: If you logout and need to reach the agency home dashboard page again, you can skip the steps shown above by commenting out add_staff_user_to_group(user, request) in benefits/core/admin.py. Then, when you log in again, your user will not be automatically added to the Cal-ITP group in the Google SSO prelogin hook (pre_login_user). If you don’t do this, you’ll need to reset the database again since you’ll never see the agency home dashboard page. The reason is that you will be part of Cal-ITP, but you won't be able to edit the database since you are not a superuser.

  • Go to localhost/admin, verify that the agency home dashboard appears

@lalver1 lalver1 self-assigned this Aug 20, 2024
@github-actions github-actions bot added front-end HTML/CSS/JavaScript and Django templates tests Related to automated testing (unit, UI, integration, etc.) deployment-dev [auto] Changes that will trigger a deploy if merged to dev back-end Django views, sessions, middleware, models, migrations etc. and removed tests Related to automated testing (unit, UI, integration, etc.) labels Aug 20, 2024
Copy link

github-actions bot commented Aug 20, 2024

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  benefits
  admin.py
  apps.py
  settings.py
  benefits/core
  admin.py
Project Total  

This report was generated by python-coverage-comment-action

@thekaveman
Copy link
Member

This kind of makes me feel like we should have a real benefits/admin app? Aren't we sort of faking it here with everything at the top-level? Would that pose a problem? Is it better to do it this way for some reason?

@thekaveman
Copy link
Member

This kind of makes me feel like we should have a real benefits/admin app? Aren't we sort of faking it here with everything at the top-level? Would that pose a problem? Is it better to do it this way for some reason?

@angela-tran walked me through the thinking (and docs!) on this setup, thank you 👏 🙌 Makes total sense and please disregard my comment above.

benefits/admin.py Outdated Show resolved Hide resolved
@lalver1 lalver1 force-pushed the feat/agency-dashboard branch from 672fd69 to 7a20fca Compare August 21, 2024 16:41
@lalver1
Copy link
Member Author

lalver1 commented Aug 21, 2024

Thanks for the comments @thekaveman and @angela-tran! Quick update on this PR:

  • The BenefitsAdminSite's index view gets the user's transit agency (assuming a database that has been set up) and updates the session so that the template will display it
  • An index template that still needs work has been added
  • Update tests

@lalver1 lalver1 force-pushed the feat/agency-dashboard branch from 7a20fca to 77a684b Compare August 21, 2024 17:02
benefits/admin.py Outdated Show resolved Hide resolved
{% endblock branding %}

{% comment %} {% block coltype %}
colMS
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
colMS
w-100

<div class="col-lg-6">
<h1>{{ agency.long_name }}</h1>
<h1>In-person enrollment</h1>
<div>Verify transit benefit eligibility using agency criteria and complete a rider's open-loop card enrollment.</div>
Copy link
Member

Choose a reason for hiding this comment

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

This is extremely nit-picky of me:

rider’s

Copy link
Member Author

Choose a reason for hiding this comment

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

Not at all! Fixed it in 988b5b8

@lalver1 lalver1 force-pushed the feat/agency-dashboard branch 2 times, most recently from 62a2e8f to 83adb91 Compare August 22, 2024 16:11
@lalver1
Copy link
Member Author

lalver1 commented Aug 22, 2024

I think this one is ready for a review, but I still need to fix these items for sure:

  • Make the Administrator header white. I could've put an inline style to make it white, but it seems there should be a better solution.
    image
  • Not sure if the value for STAFF_GROUP_NAME should be set in settings (as of this PR) or read as an environment variable. Actually, I'm now leaning to an environment variable since this value will depend on where the app is deployed.

@lalver1 lalver1 marked this pull request as ready for review August 22, 2024 16:37
@lalver1 lalver1 requested a review from a team as a code owner August 22, 2024 16:37
@machikoyasuda
Copy link
Member

machikoyasuda commented Aug 22, 2024

I see Administrator as yellow:
image

My OS and browser are all set to Light mode.

The color is coming from here:

image

Where is your black color coming from? I don't see a variable set for --header-color in the dark_mode.css from Django.


I set my Firefox browser to Dark, and I still see a yellow header:

image

@angela-tran @thekaveman What color does the Administrator header show up for you two? And what are your browser / operating system dark/light settings?

<h1 class="pt-0 pb-3 text-left">{{ agency.long_name }}</h1>
<div class="border border-3 p-3">
<h2 class="pt-0 pb-2 text-left">In-person enrollment</h2>
<p>Verify transit benefit eligibility using agency criteria and complete a rider's open-loop card enrollment.</p>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<p>Verify transit benefit eligibility using agency criteria and complete a rider's open-loop card enrollment.</p>
<p>Verify transit benefit eligibility using agency criteria and complete a riders open-loop card enrollment.</p>

Copy link
Member

Choose a reason for hiding this comment

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

Just this small thing, otherwise this PR LGTM.

There are some questions about how we approach this Header section for example across all the new pages, but I think that kind of refactoring can happen later when we need it. And that for now, this is a good start.

Copy link
Member

Choose a reason for hiding this comment

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

There are some questions about how we approach this Header section for example across all the new pages, but I think that kind of refactoring can happen later when we need it.

Thanks for noting this; I think this is an important question to answer sooner than later.

@lalver1 and I talked more about his idea from #2313 (comment) when we paired on Friday, and the plan is for him to open a separate PR that extracts out a base template that all the new pages can use.

Copy link
Member

Choose a reason for hiding this comment

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

Here is the full plan: #2246 (comment)

It is the same as what @lalver1 wrote in this PR's description.

Another PR will implement the top right corner "user tools" panel for transit agency staff, and another PR will implement a base html template that will be extended by all templates in the in_person app.

@@ -395,6 +395,7 @@ footer .footer-links li a.footer-link:visited {
outline: 3px solid var(--focus-color) !important;
outline-offset: 2px !important;
box-shadow: none !important; /* override CA State Web Template */
text-decoration: none;
Copy link
Member

Choose a reason for hiding this comment

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

Just wanted to leave a comment here for everyone else: This is necessary to override a Django Admin CSS style for buttons. Without this, there would be an underline on the button.

@machikoyasuda
Copy link
Member

Just adding something I noticed here for people to see, but it doesn't need fixing or anything:

image

This PR made the log out area type small, and there's something funky going on with the underlines (the two links have different line heights, and there's a border-radius applied to one link). It's b/c of the way our current app CSS styles links. This is for documentation purposes only.

@lalver1 lalver1 force-pushed the feat/agency-dashboard branch 2 times, most recently from 7dd9c01 to f968a03 Compare August 23, 2024 14:02
@lalver1
Copy link
Member Author

lalver1 commented Aug 23, 2024

@angela-tran and @machikoyasuda, I was looking at the other in-person eligibility tickets and I'm wondering what you think if I add one minor thing to this PR. I would make a new template called something like base-in-person.html in the in_person/templates/in_person directory that will extend admin/base.html and will have all the common elements (like {% block extrastyle %} and {% block branding %}) from agency-dashboard-index that eligibility.html, enrollment.html and the current agency-dashboard-index.html need. Then, eligibility.html, enrollment.html and agency-dashboard-index.html will extend from base-in-person.html and that'll ensure that they have the same base styling.

@lalver1 lalver1 marked this pull request as draft August 23, 2024 21:16
@lalver1 lalver1 force-pushed the feat/agency-dashboard branch from f968a03 to 51ce1fd Compare August 26, 2024 15:03
@lalver1
Copy link
Member Author

lalver1 commented Aug 26, 2024

Thanks for the feedback last week @angela-tran. The tests were failing earlier but now they are all passing so I think this one is ready for a re-review. The key changes are:

  • The original description for this PR has been updated to reflect the change in scope
  • A styles.css was added for admin in 51ce1fd to avoid the situation mentioned in #2313 (comment) and to clean up the styling of the home dashboard

@lalver1 lalver1 marked this pull request as ready for review August 26, 2024 16:24
benefits/settings.py Outdated Show resolved Hide resolved
benefits/static/css/admin/styles.css Show resolved Hide resolved
benefits/settings.py Outdated Show resolved Hide resolved
benefits/static/css/admin/styles.css Outdated Show resolved Hide resolved
benefits/templates/admin/agency-dashboard-index.html Outdated Show resolved Hide resolved
benefits/admin.py Outdated Show resolved Hide resolved
<img class="lg d-none d-lg-block" src="{% static "img/logo-lg.svg" %}" width="220" height="50" alt="{% translate "California Integrated Travel Project: Benefits logo (large)" context "image alt text" %}" />
</span>
</div>
<h1 class="p-0 branding-title">Administrator</h1>
Copy link
Member

@machikoyasuda machikoyasuda Aug 26, 2024

Choose a reason for hiding this comment

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

Suggested change
<h1 class="p-0 branding-title">Administrator</h1>
<h1 class="p-0 m-0 text-white">Administrator</h1>

Adding m-0 here will vertically center the Administrator text and the logo.

Comment on lines 51 to 58
@font-face {
font-family: "Public Sans";
font-weight: var(--bold-font-weight);
font-style: normal;
/* prettier-ignore */
src: local("PublicSans"), url("../../fonts/PublicSans-Bold.woff") format("woff");
/* prettier-ignore */
}
Copy link
Member

Choose a reason for hiding this comment

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

I think this can be deleted.

Copy link
Member Author

@lalver1 lalver1 Aug 26, 2024

Choose a reason for hiding this comment

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

👍 removed it in 57b8522

Comment on lines 111 to 117
:root {
--branding-title-color: #ffffff;
}
.branding-title {
color: var(--branding-title-color);
}

Copy link
Member

Choose a reason for hiding this comment

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

You can also just use text-white utility class from Bootstrap so we don't have to have this one-off style. Documented here: https://getbootstrap.com/docs/5.3/utilities/colors/#colors

@lalver1 lalver1 force-pushed the feat/agency-dashboard branch 2 times, most recently from a88cc40 to f759564 Compare August 26, 2024 20:43
machikoyasuda
machikoyasuda previously approved these changes Aug 26, 2024
Copy link
Member

@machikoyasuda machikoyasuda left a comment

Choose a reason for hiding this comment

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

Front-end changes LGTM 👍 👍 This is a great foundation for the rest of the In Person work.

angela-tran
angela-tran previously approved these changes Aug 26, 2024
Copy link
Member

@angela-tran angela-tran left a comment

Choose a reason for hiding this comment

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

I left some comments about naming in the tests that you can commit if you agree with them.

And there is one comment that no action is needed on -- I'll create a separate ticket for it.

Nice work 👍 👍



@pytest.fixture
def model_SuperAgencyUser(model_User):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def model_SuperAgencyUser(model_User):
def model_StaffGroupUser(model_User):


@pytest.fixture
def model_User():
return User.objects.create(is_active="1", is_staff="1")
Copy link
Member

Choose a reason for hiding this comment

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

This is nitpicky, but I think we should just use True here instead of this roundabout way of setting the boolean.

There's not a specific reason to use "1" here, right? It's a little distracting because it's not how we normally set these as far as I know.

Suggested change
return User.objects.create(is_active="1", is_staff="1")
return User.objects.create(is_active=True, is_staff=True)

Copy link
Member Author

Choose a reason for hiding this comment

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

Not at all, this makes sense to use True 👍 I was looking at the database and got confused because I saw True gets stored as 1, but we are working with the Django model, not the database value and we should use True for setting the boolean.


@pytest.fixture
def model_SuperUser(model_User):
model_User.is_superuser = "1"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
model_User.is_superuser = "1"
model_User.is_superuser = True

Comment on lines 44 to 45
def test_admin_override_super_agency_user(app_request, model_SuperAgencyUser):
app_request.user = model_SuperAgencyUser
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def test_admin_override_super_agency_user(app_request, model_SuperAgencyUser):
app_request.user = model_SuperAgencyUser
def test_admin_override_staff_group_user(app_request, model_StaffGroupUser):
app_request.user = model_StaffGroupUser

tests/pytest/test_admin.py Show resolved Hide resolved
@lalver1 lalver1 force-pushed the feat/agency-dashboard branch from 9dffa90 to b5d1114 Compare August 26, 2024 21:48
Copy link
Member

@angela-tran angela-tran left a comment

Choose a reason for hiding this comment

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

🚀

@lalver1 lalver1 merged commit cd782b7 into main Aug 26, 2024
15 checks passed
@lalver1 lalver1 deleted the feat/agency-dashboard branch August 26, 2024 21:54
@machikoyasuda
Copy link
Member

Woohoo! 🔥 🔥 🔥
Great job @lalver1 on a big PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
back-end Django views, sessions, middleware, models, migrations etc. deployment-dev [auto] Changes that will trigger a deploy if merged to dev front-end HTML/CSS/JavaScript and Django templates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants