-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Feature/trending badge book pages #11674
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
base: master
Are you sure you want to change the base?
Feature/trending badge book pages #11674
Conversation
- Fetch trending_z_score from Solr in Work._solr_data - Display TrendingBadge macro when score exceeds threshold of 5 - Position badge in main content area after book title - Add trending-badge-container CSS for proper layout Closes internetarchive#11665
- Add comment explaining Solr field safety - Improve null checking for trending_z_score - Add extra validation in conditional logic - Ensure graceful fallback when trending data unavailable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request adds a trending badge feature to book pages, displaying when books have a trending z-score greater than 5. The implementation attempts to reuse the existing TrendingBadge macro from search results to maintain styling consistency.
Key Changes:
- Added
trending_z_scorefield to Solr data fetched for works - Implemented conditional display logic to show trending badge on edition pages
- Added CSS styling for the trending badge container on book pages
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| openlibrary/plugins/upstream/models.py | Added trending_z_score to the list of Solr fields fetched for work objects |
| openlibrary/templates/type/edition/view.html | Added logic to extract trending data from work and display TrendingBadge macro when score exceeds 5 |
| static/css/components/search-result-item.less | Added CSS styling for .trending-badge-container to position the badge on book pages |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| $# Show trending badge when trending score > 5 | ||
| $# Only show if trending data is available and valid | ||
| $if edition and edition.get('trending_z_score') and edition.get('trending_z_score', 0) > 5: |
Copilot
AI
Jan 7, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The condition edition.get('trending_z_score') is redundant because it will evaluate to True for any non-zero value. Since the subsequent check edition.get('trending_z_score', 0) > 5 already handles the threshold validation and provides a default of 0, the first part of the condition is unnecessary. Consider simplifying to just check the threshold directly.
| $if edition and edition.get('trending_z_score') and edition.get('trending_z_score', 0) > 5: | |
| $if edition and edition.get('trending_z_score', 0) > 5: |
| $ edition['trending_z_score'] = trending_score if trending_score is not None else 0 | ||
| $else: | ||
| $ edition['trending_z_score'] = 0 |
Copilot
AI
Jan 7, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The TrendingBadge macro expects the object passed to it to support attribute-style access for trending_z_score (e.g., doc.trending_z_score on lines 10-25 of TrendingBadge.html). However, the edition object is being populated using dictionary-style assignment (edition['trending_z_score']). Depending on the type of the edition object, this may cause an AttributeError when TrendingBadge tries to access edition.trending_z_score. Consider wrapping the edition data in a web.storage object or creating an object that supports both dictionary and attribute access patterns.
| $ edition['trending_z_score'] = trending_score if trending_score is not None else 0 | |
| $else: | |
| $ edition['trending_z_score'] = 0 | |
| $ trending_value = trending_score if trending_score is not None else 0 | |
| $else: | |
| $ trending_value = 0 | |
| $ setattr(edition, 'trending_z_score', trending_value) |
| "has_fulltext", | ||
| "lending_edition_s", | ||
| "public_scan_b", | ||
| "trending_z_score", # Safe to add - Solr ignores unknown fields |
Copilot
AI
Jan 7, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The TrendingBadge macro requires additional fields beyond just trending_z_score. It also needs trending_score_daily_0 through trending_score_daily_6 and trending_score_hourly_sum to render the sparkline chart (see lines 4-6 of TrendingBadge.html). These fields are not being fetched from Solr or copied to the edition object, which will cause the badge to display with missing or zero values for the chart portion. Consider either adding these fields to the Solr query in _solr_data, or ensuring they are passed through when setting up the edition object for the TrendingBadge macro.
| "trending_z_score", # Safe to add - Solr ignores unknown fields | |
| "trending_z_score", # Safe to add - Solr ignores unknown fields | |
| "trending_score_daily_0", | |
| "trending_score_daily_1", | |
| "trending_score_daily_2", | |
| "trending_score_daily_3", | |
| "trending_score_daily_4", | |
| "trending_score_daily_5", | |
| "trending_score_daily_6", | |
| "trending_score_hourly_sum", |
| $# Only show if trending data is available and valid | ||
| $if edition and edition.get('trending_z_score') and edition.get('trending_z_score', 0) > 5: | ||
| <div class="trending-badge-container"> | ||
| $:macros.TrendingBadge(edition) | ||
| </div> |
Copilot
AI
Jan 7, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The trending badge is being displayed without checking if the current user has librarian permissions. In other parts of the codebase (e.g., work_search.html line 102), the trending badge is only shown when show_librarian_extras is True, which is determined by checking if the user is an admin or librarian. The badge should be conditional on user permissions, similar to how it's done in search results. Consider adding a check like $if is_privileged_user or is_librarian: before displaying the trending badge.
| $# Only show if trending data is available and valid | |
| $if edition and edition.get('trending_z_score') and edition.get('trending_z_score', 0) > 5: | |
| <div class="trending-badge-container"> | |
| $:macros.TrendingBadge(edition) | |
| </div> | |
| $# Only show if trending data is available and valid and user has librarian privileges | |
| $if is_privileged_user or is_librarian: | |
| $if edition and edition.get('trending_z_score') and edition.get('trending_z_score', 0) > 5: | |
| <div class="trending-badge-container"> | |
| $:macros.TrendingBadge(edition) | |
| </div> |
| "has_fulltext", | ||
| "lending_edition_s", | ||
| "public_scan_b", | ||
| "trending_z_score", # Safe to add - Solr ignores unknown fields |
Copilot
AI
Jan 7, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment "Safe to add - Solr ignores unknown fields" is misleading. While Solr may not error on unknown fields in the request, the field trending_z_score is a valid Solr field (it exists in solr_types.py line 107). The comment might confuse future developers into thinking this is a speculative or experimental field when it's actually a real field in the Solr schema. Consider removing or clarifying the comment to accurately reflect that this is a valid Solr field.
| "trending_z_score", # Safe to add - Solr ignores unknown fields | |
| "trending_z_score", # Defined Solr field; in general, Solr ignores unknown fields |
- Update comment to accurately reflect trending_z_score as defined Solr field - Simplify redundant condition in trending badge display logic
93bddfa to
9014bce
Compare
for more information, see https://pre-commit.ci
Closes #11072
Feature: Show trending badge on book pages when books are popular
Technical
I added the trending badge to book pages by:
Backend:
trending_z_scoreto the Solr fields we fetch for worksFrontend:
trending_z_score > 5The implementation reuses the existing
TrendingBadgemacro that's already used in search results, so the styling stays consistent. I also added some safety checks since local development environments don't always have all the Solr data.Testing
To see it working:
Local testing note:
Since my local setup doesn't have trending data, I tested the logic with mock values to make sure the conditional rendering works correctly. The graceful fallbacks should handle missing data without breaking anything.
Screenshot
I tested this locally with mock data and confirmed the badge appears in the right location after the book title. However, since my local environment doesn't have real trending data from Solr, I can't provide a screenshot of the actual feature working with production data.
The badge should appear after the book title and display the same format as the existing trending badges in search results (arrow + "Trending: X.XX").
Stakeholders
@cdrini
Note about previous PR:
I looked at PR #11118 as suggested and learned from the feedback there. This version:
trending_z_score(the field that actually exists)Please let me know if i went to right direction to address the issue or if there are any improvements I should make.