Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the book tracking feature by allowing users to choose how they record their reading progress, either by page count or by percentage completion. This change addresses the diverse ways users consume books, offering a personalized and intuitive experience. The update includes both backend model and form modifications, new database fields, and dynamic frontend elements to facilitate seamless unit switching and accurate progress representation. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new feature allowing users to track book progress using either pages or percentages, with a configurable default unit in user preferences. The changes include adding progress_unit fields to Book and User models, updating MediaForm and BookForm to manage this new unit, and modifying Media model methods to correctly calculate and display progress. Frontend updates provide a dynamic 'Toggle % / Pages' button in the tracking form and adjust progress bar rendering. Additionally, performance was improved by using prefetch methods for media retrieval. The review suggests refactoring duplicated completion logic in the process_progress method for better maintainability and extracting the hardcoded 'Pages' unit name in the JavaScript to allow for future reusability with other media types.
src/app/models.py
Outdated
| if self.get_progress_unit() == users.models.ProgressUnit.PERCENTAGE: | ||
| self.progress = min(self.progress, 100) | ||
| if self.progress == 100: | ||
| self.status = Status.COMPLETED.value | ||
| now = timezone.now().replace(second=0, microsecond=0) | ||
| self.end_date = now | ||
| return | ||
|
|
||
| max_progress = providers.services.get_media_metadata( | ||
| self.item.media_type, | ||
| self.item.media_id, | ||
| self.item.source, | ||
| )["max_progress"] | ||
|
|
||
| if self.progress == max_progress: | ||
| self.status = Status.COMPLETED.value | ||
| if max_progress: | ||
| self.progress = min(self.progress, max_progress) | ||
|
|
||
| now = timezone.now().replace(second=0, microsecond=0) | ||
| self.end_date = now | ||
| if self.progress == max_progress: | ||
| self.status = Status.COMPLETED.value | ||
|
|
||
| now = timezone.now().replace(second=0, microsecond=0) | ||
| self.end_date = now | ||
|
|
There was a problem hiding this comment.
This method contains duplicated logic for marking an item as completed. To improve maintainability and adhere to the DRY principle, you could extract the completion logic into a local function.
def mark_completed():
self.status = Status.COMPLETED.value
now = timezone.now().replace(second=0, microsecond=0)
self.end_date = now
if self.get_progress_unit() == users.models.ProgressUnit.PERCENTAGE:
self.progress = min(self.progress, 100)
if self.progress == 100:
mark_completed()
return
max_progress = providers.services.get_media_metadata(
self.item.media_type,
self.item.media_id,
self.item.source,
)["max_progress"]
if max_progress:
self.progress = min(self.progress, max_progress)
if self.progress == max_progress:
mark_completed()| // Update label suffix via custom event or direct DOM manipulation | ||
| const label = this.$el.querySelector(`label[for="${progressField.id}"]`); | ||
| if (label) { | ||
| label.textContent = newUnit === 'percentage' ? 'Progress (%)' : `Progress (Pages)`; |
There was a problem hiding this comment.
The unit name 'Pages' is hardcoded here. While this works for books, it would be more maintainable and reusable for other media types in the future if this was not hardcoded. You could pass the unit name from the template to the javascript via a data- attribute on the form element.
For example, in fill_track.html:
<form ... data-unit-name-plural="{{ media_type|long_unit }}s">Then in the Javascript:
const unitNamePlural = this.$el.dataset.unitNamePlural || 'Pages';
label.textContent = newUnit === 'percentage' ? 'Progress (%)' : `Progress (${unitNamePlural})`;
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #1245 +/- ##
==========================================
+ Coverage 81.30% 81.39% +0.09%
==========================================
Files 70 70
Lines 7327 7386 +59
==========================================
+ Hits 5957 6012 +55
- Misses 1370 1374 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1 participant
Full disclosure, Gemini wrote all the code. I carefully planned and reviewed the work, making many changes to make it more idiomatic, fix bugs and keep it in-line with the existing code. I am not an expert in Python and I know there are some areas that I am not 100% happy with that I will highlight with inline comments.
This feature solves #1110 and #768
It adds two primary features
The reason I chose to do it this way is that if a user reads a mix of physical and digital book their preference will change. We shouldn't expect users to have to convert their page number to % every time they want to update their progress, so it's important to give them that flexibility.
By default the preference is pages, so no change to behaviour. Two migrations are added, one to add the two units, defaulting to pages, and the other two set all existing books to NULL as their progress unit (as the user has not specified what unit they want).
I chose to have an overloaded method on the book model to return the progress unit, and a media model method that is pretty much a no-op.
For the UI, I chose to put the toggle in the label line, but I can change this if you prefer something else, I looked over the app and could not find an existing pattern for this. Another nice feature here is that it will auto-convert the %/pages to the other unit when toggling, so that it's easy to swap between them for existing books. Progress bar and other places where progress is used like the details box all handle the % correctly.
See below screenshots/videos.
Screencast_20260310_012729.webm