Book on flex #1625
Book on flex #1625
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new BookListBlock to support the display of book lists on pages and refactors how book data is retrieved in the Book model. Key changes include:
- Importing and adding BookListBlock in pages/models.py for new block functionality.
- Defining the BookListBlock in pages/custom_blocks.py that leverages a new get_book_data() helper.
- Refactoring the book data aggregation in books/models.py to use get_book_data(), along with minor updates to the codecov ignore rules.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pages/models.py | Added BookListBlock to the existing block imports and stream block mappings. |
| pages/custom_blocks.py | Defined the new BookListBlock with a get_api_representation method using get_book_data. |
| codecov.yml | Updated ignore rules to include additional files and paths. |
| books/models.py | Added get_book_data helper and refactored the book aggregation loop to use it. |
| @@ -0,0 +1,412 @@ | |||
| # Generated by Django 5.0.14 on 2025-05-08 05:14 | |||
There was a problem hiding this comment.
this is generated, focus on the models.py file for actual structure changes
| has_faculty_resources = BookFacultyResources.objects.filter(book_faculty_resource=book).exists() | ||
| has_student_resources = BookStudentResources.objects.filter(book_student_resource=book).exists() | ||
| try: | ||
| return { |
There was a problem hiding this comment.
this is what's returned on the current subjects page - let me know if you need other fields (or don't need them anymore 🔥)
There was a problem hiding this comment.
For the book tiles, the FE only needs id, cover_url, title, and slug. It pulls the rest of the details from the main book query (which is used for the subjects page).
Book tiles are currently generated from the book lists included in the specific-subjects page, so that's the model of list to use. The query for that is
`pages/${slug}-books?type=pages.Subject`
There was a problem hiding this comment.
@RoyEJohnson it doesn't make a separate query for every book tile does it?
There was a problem hiding this comment.
pretty sure it does, the console log was spitting out a constant stream of
Can't load Microbiology
Can't load A&P
...
or it might have been subjects... but it was a lot of stuff
i bet it is subjects, considering the endpoint being called
There was a problem hiding this comment.
Ok well id rather include extra data and have the option of changing it to like... Not do a separate query per tile if that's what it does
There was a problem hiding this comment.
It's not a separate query to the CMS for each book. The main subjects query is pulled once, and is searched for the id of whatever book we're looking for.
There was a problem hiding this comment.
But will that work if we're searching for arbitrary books and we're not in the context of a particular subject? The API endpoint that you quoted before looked like it needed to be about a particular subject?
There was a problem hiding this comment.
here's a test page with the book block, it has nothing to do with subject (beyond it returning in the response), it's just returning book metadata
https://dev.openstax.org/apps/cms/api/v2/pages/562/
https://dev.openstax.org/admin/pages/562/edit/
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
|
good catch, i didn't realize that new page was in the pages model |
|
well, it sounded like a good idea to abstract out, but completely broke the subjects pages |
This reverts commit 1a17671.
|
I am going to merge this without consolidating the book lists. I will add a card for me to look at this in the future, but something unusual is happening with how books are fetched on subjects - and I've spent too much time on this already. |
https://openstax.atlassian.net/browse/CORE-900