[PP-3602] Add target_age range based on Work.target_age rather than b…#3041
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3041 +/- ##
==========================================
- Coverage 93.04% 93.04% -0.01%
==========================================
Files 480 480
Lines 43716 43720 +4
Branches 6027 6029 +2
==========================================
+ Hits 40677 40679 +2
- Misses 1968 1969 +1
- Partials 1071 1072 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
dbernstein
force-pushed
the
feature/PP-3602-add-age-range-information-to-inventory-report
branch
from
4e4fe67 to
91551ab
Compare
dbernstein
force-pushed
the
feature/PP-3602-add-age-range-information-to-inventory-report
branch
from
24473b8 to
002276f
Compare
tdilauro
left a comment
tdilauro
left a comment
There was a problem hiding this comment.
Looks good to me!
Approved now, but I've requested a comment about what is being formatted in the query.
| target_age_upper_raw = func.upper(Work.target_age) | ||
| target_age_lower_adj: ColumnElement[Any] = case( | ||
| (func.lower_inc(Work.target_age), target_age_lower_raw), | ||
| else_=target_age_lower_raw + 1, |
There was a problem hiding this comment.
I don't understand the intention of the case logic here. Please explain in a comment here or above l. 422.
There was a problem hiding this comment.
I understand better from the tests now, but I still think it would be good to add an overall comment in this section.
There was a problem hiding this comment.
I'm going to refactor this to use our existing numericrange_to_string function which implements the same logic with a slight modification (namely that ranges with open ended upper bounds are rended as {lower} rather than "{lower}-" to keep things consistent across the app.
| ) | ||
| target_age_upper_adj: ColumnElement[Any] = case( | ||
| (func.upper_inc(Work.target_age), target_age_upper_raw), | ||
| else_=target_age_upper_raw - 1, |
There was a problem hiding this comment.
I don't understand the intention of the case logic here. Please explain in a comment here or above l. 422.
dbernstein
force-pushed
the
feature/PP-3602-add-age-range-information-to-inventory-report
branch
from
002276f to
585701a
Compare
dbernstein
deleted the
feature/PP-3602-add-age-range-information-to-inventory-report
branch
2 participants
…isac subjects.
Description
Replace inventory report age range with Work.target_age
Added a target_age column in the inventory report using Work.target_age and the same formatting as numericrange_to_string (e.g. "12-14", "18-", "").
Motivation and Context
The current solution does not resolve target age values in the same way that they are resolved in the opds2 feed that drives the target age values displayed by Admin UI's classifications tab. As a result we are not seeing some age ranges that were imported under
schema:suggested_min_ageandschema:suggested_max_agetags.https://ebce-lyrasis.atlassian.net/browse/PP-3602
How Has This Been Tested?
Unit tests updated. Manually tested on minotaur.
Checklist