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

Moved utilization reports #966

Merged
merged 9 commits into from
Feb 26, 2020
Merged

Moved utilization reports #966

merged 9 commits into from
Feb 26, 2020

Conversation

tbaxter-18f
Copy link
Contributor

@tbaxter-18f tbaxter-18f commented Feb 7, 2020

Moves utilization reports from behind a menu only superusers can see and into a location where they can be viewed by anyone. Transparency!

@codecov-io
Copy link

codecov-io commented Feb 7, 2020

Codecov Report

Merging #966 into master will increase coverage by 0.07%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #966      +/-   ##
==========================================
+ Coverage   91.45%   91.52%   +0.07%     
==========================================
  Files          39       39              
  Lines        1685     1699      +14     
==========================================
+ Hits         1541     1555      +14     
  Misses        144      144
Impacted Files Coverage Δ
employees/models.py 100% <0%> (ø) ⬆️
projects/admin.py 91.22% <0%> (ø) ⬆️
hours/views.py 88.23% <0%> (+0.03%) ⬆️
projects/models.py 97.61% <0%> (+0.09%) ⬆️
hours/models.py 96.33% <0%> (+0.11%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7e8d490...c8dfce7. Read the comment docs.

@amymok
Copy link
Member

amymok commented Feb 20, 2020

@tbaxter-18f Can you update the PR description above: #966 (comment)

Jkrzy
Jkrzy previously requested changes Feb 20, 2020
Copy link
Contributor

@Jkrzy Jkrzy left a comment

Choose a reason for hiding this comment

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

Needs description of PR

<span>Reports</span>
</a>
</li>
{% if user.is_staff %}
Copy link
Member

Choose a reason for hiding this comment

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

This should be request.user.is_staff

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Huh. I thought we passed user into context, but it appears you're right!

</a>
</li>
{% if user.is_staff %}
<li>
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't render for me. No menu actually shows up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We may need to be sure you're set up as superuser and staff in the admin.

{% if request.user.is_staff %}
<li>
<button class="
usa-accordion-button usa-nav-link" aria-expanded="false" aria-controls="side-nav-2">
Copy link
Member

Choose a reason for hiding this comment

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

The side-nav-2 is conflicting with the one under Administrative, this is causing the submenu not rendering. So choose something else here.

usa-accordion-button usa-nav-link" aria-expanded="false" aria-controls="side-nav-2">
<span>Reports</span>
</button>
<ul id="side-nav-2" class="usa-nav-submenu" aria-hidden="true">
Copy link
Member

Choose a reason for hiding this comment

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

Same comment as above for the side-nav-2. Choose another id.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, Amy. Those are also bad IDs, so I've made them a bit more descriptive.

Copy link
Member

Choose a reason for hiding this comment

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

@tbaxter-18f Did you push the commit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did now :-)

Copy link
Member

@amymok amymok left a comment

Choose a reason for hiding this comment

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

Looks great to me!

@amymok amymok dismissed Jkrzy’s stale review February 26, 2020 21:16

The request has been completed, description has been added

@amymok
Copy link
Member

amymok commented Feb 26, 2020

There's no changes in dependency, Snyk is being funky here for pending. Will bypass and merge.

@amymok amymok merged commit 1f7e2ee into master Feb 26, 2020
@Jkrzy Jkrzy deleted the utilization-report branch May 29, 2020 15:52
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.

4 participants