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

Add per-medium breakouts to dashboard statistics inventories. (PP-728) #1549

Merged
merged 8 commits into from
Dec 8, 2023

Conversation

tdilauro
Copy link
Contributor

@tdilauro tdilauro commented Dec 4, 2023

Description

  • Add per-medium breakouts to dashboard statistics inventories.
  • Add supporting tests and clean up some tests.
  • Resolve some PEP-8 and mypy issues with SQLAlchemy filters.

This work requires changes to the circulation admin UI. The following work is required before this PR can be moved out of draft status and merged:

Note:

  • Based on testing, the changes in this PR are backward compatible with recent versions of the circulation admin UI.
  • The API has changed, so I am labeling as incompatible changes.

Motivation and Context

Need to support additional detail by type of medium (book, audiobook, etc.).

[Jira PP-728]

How Has This Been Tested?

Checklist

  • I have updated the documentation accordingly.
  • All new and existing tests passed.

@tdilauro tdilauro requested a review from a team December 4, 2023 02:56
Copy link
Member

@jonathangreen jonathangreen left a comment

Choose a reason for hiding this comment

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

Might be a couple lines of commented out testing code that should get cleaned up before merging, but otherwise looks good to me! 🎸

I also left a couple minor comments, but as always feel free to make those changes or just merge as is.

api/admin/dashboard_stats.py Outdated Show resolved Hide resolved
api/admin/dashboard_stats.py Show resolved Hide resolved
api/admin/dashboard_stats.py Outdated Show resolved Hide resolved
tests/api/admin/test_dashboard_stats.py Outdated Show resolved Hide resolved
Copy link

codecov bot commented Dec 5, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (9babdac) 90.17% compared to head (e6e92ea) 90.18%.
Report is 21 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1549      +/-   ##
==========================================
+ Coverage   90.17%   90.18%   +0.01%     
==========================================
  Files         234      234              
  Lines       28187    28215      +28     
  Branches     6455     6456       +1     
==========================================
+ Hits        25418    25447      +29     
  Misses       1831     1831              
+ Partials      938      937       -1     
Flag Coverage Δ
Api 75.79% <100.00%> (+0.02%) ⬆️
Core 59.56% <1.44%> (-0.06%) ⬇️
migration 29.84% <1.44%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tdilauro
Copy link
Contributor Author

tdilauro commented Dec 5, 2023

@jonathangreen I made some substantial changes while following your suggestion to use a dataclass, so you might want to take another look. No worried if you don't and no rush, as I won't be merging until the admin UI is updated.

Also, codecov seems to have lost its mind. I just manually verified that the tests were hitting lines that it claimed they were missing. 🤷🏽‍♂️

Copy link
Member

@jonathangreen jonathangreen left a comment

Choose a reason for hiding this comment

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

Changes look great!

@tdilauro tdilauro added UI Update This PR requires an admin UI update feature New feature incompatible changes Changes that require a new major version and removed incompatible changes Changes that require a new major version labels Dec 8, 2023
@tdilauro tdilauro marked this pull request as ready for review December 8, 2023 22:22
@tdilauro tdilauro added the incompatible changes Changes that require a new major version label Dec 8, 2023
@tdilauro tdilauro merged commit 5910e72 into main Dec 8, 2023
30 checks passed
@tdilauro tdilauro deleted the feature/stats-data-with-medium-breakout branch December 8, 2023 22:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature incompatible changes Changes that require a new major version UI Update This PR requires an admin UI update
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants