[PP-3488] Add the published date, bisac tags, and age ranges to the inventory report.#2986
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2986 +/- ##
=======================================
Coverage 92.97% 92.98%
=======================================
Files 459 459
Lines 43276 43288 +12
Branches 6034 6034
=======================================
+ Hits 40238 40250 +12
Misses 1966 1966
Partials 1072 1072 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
dbernstein
force-pushed
the
feature/PP-3488-add-columns-to-inventory-report
branch
from
034eb17 to
6978b3c
Compare
jonathangreen
left a comment
jonathangreen
left a comment
There was a problem hiding this comment.
Looks good. I added a couple comments to consider before merging.
| classification_alias = aliased(Classification) | ||
| subject_alias = aliased(Subject) | ||
| bisac_value = func.coalesce(subject_alias.name, subject_alias.identifier) | ||
| age_range_value = cast(subject_alias.target_age, String) |
There was a problem hiding this comment.
The target_age column is an INT4RANGE type. What kind of output does casting it to a String produce? Is it the format that end users will expect? I think it'll end up using PG interval notation, which I'm not sure end users will understand.
| .outerjoin( | ||
| bisac_subquery, | ||
| (bisac_subquery.c.data_source_id == DataSource.id) | ||
| & (bisac_subquery.c.identifier_id == Edition.primary_identifier_id), | ||
| ) |
There was a problem hiding this comment.
Minor: If a book has BISAC classifications from multiple data sources, this would only show the ones from the same data source as the license pool. Not sure if we'll come across this case, but a book could have BISAC classifications from other sources, and those wouldn't be included. Might be worth validating this with production data.
| expected_age_range_one = db.session.scalar( | ||
| select(cast(Subject.target_age, String)).where( | ||
| Subject.id == bisac_subject_one.id | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Minor: The test queries the database to determine the expected age range format rather than asserting a specific format. While this ensures the test passes regardless of PostgreSQL's representation, it doesn't explicitly verify that the format is correct or user-friendly. Might be nice to have an explicit assertion.
dbernstein
force-pushed
the
feature/PP-3488-add-columns-to-inventory-report
branch
from
6978b3c to
5c84ed9
Compare
…an identifier, not just the ones associated with a particular data source. Also change the delimited from a comma to a pipe for bisac_subjects and age_ranges and ensure that the values are unique.
dbernstein
force-pushed
the
feature/PP-3488-add-columns-to-inventory-report
branch
from
5c84ed9 to
31f6b63
Compare
|
@jonathangreen : thanks for the review - I incorporated your feedback before merging. |
dbernstein
deleted the
feature/PP-3488-add-columns-to-inventory-report
branch
2 participants
Description
This PR adds the aforementioned fields to the inventory report. If there are multiple bisac tags and/or age ranges they are aggregated in the appropriate fields. If there are no associated bisac classifications, then the columns will appear empty.
The unit tests were updated .
Motivation and Context
https://ebce-lyrasis.atlassian.net/browse/PP-3488
How Has This Been Tested?
Unit tests updated.
Checklist