Skip to content
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

Make average rating property of Edition model #3425

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 29 additions & 5 deletions bookwyrm/models/book.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
from django.contrib.postgres.indexes import GinIndex
from django.core.cache import cache
from django.db import models, transaction
from django.db.models import Avg, Prefetch, ManyToManyField
from django.db.models import Avg, Prefetch, ManyToManyField, Q
from django.dispatch import receiver
from django.utils.translation import gettext_lazy as _
from model_utils import FieldTracker
Expand Down Expand Up @@ -492,10 +492,34 @@ def hyphenated_isbn13(self):
"""generate the hyphenated version of the ISBN-13"""
return hyphenator.hyphenate(self.isbn_13)

@property
def average_rating(self, user):
"""generate average rating of a book"""
reviews = Review.privacy_filter(user).filter(book__parent_work__editions=self)
@classmethod
def average_rating(cls, include_parents=False, user=None, privacy=None):
"""
Generate the average rating of a book

Args:
include_parents: allows the caller to determine if ratings
from other Editions of the Work should be included
user: if we're given a value for user, use privacy_filter
privacy: if we're given a value for privacy, use it in filtering the reviews
Returns:
an aggregation of the average rating of the book across its reviews.
"""
# get the base review queryset which we will refine
# based on the arguments provided
if include_parents:
base_reviews = Review.objects.filter(
Q(book__parent_work__editions=cls) & Q(deleted=False) & Q(rating_gt=0)
)
else:
base_reviews = Review.objects.filter(Q(deleted=False) & Q(rating_gt=0))
if user:
reviews = base_reviews.privacy_filter(user).exclude(rating=None)
elif privacy:
reviews = base_reviews.filter(Q(privacy=privacy)).exclude(rating=None)
else:
reviews = base_reviews.exclude(rating=None)

return reviews.aggregate(Avg("rating"))["rating__avg"]

Copy link
Author

@lenikadali lenikadali Sep 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think using a method that takes a Review queryset (which can already be filtered with things like privacy_filter or privacy="public") and returns an average rating would be a good solution.

The filter calls here have overlap with the initial filter in the Book view here

At this stage of the refactor, I am okay with leaving the Book view filter as is and then allowing a more elegant solution to come out of our iterations on this.

Also, other calculations of the rating use the user argument, strengthening the idea that this should be either a method on the model or a class method. Examples are here, and here. The latter also looks for ratings greater than 0.

The calculation here doesn't use the user argument.

Another idea could be to have different methods that keep the behaviour of the methods mentioned but have them be on the Edition model rather than in the different places they are in right now.

I also noticed that there are some subtleties in how we use the user arguments. In some queries, we want to apply privacy_filter while in others, we want to filter for the user's reviews of the Work. Not sure how to modify average_rating for the difference in queries.

@mouse-reeve what do you think?

def get_rank(self):
Expand Down
2 changes: 1 addition & 1 deletion bookwyrm/views/books/books.py
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ def get(self, request, book_id, **kwargs):
if not user_statuses
else None
),
"rating": book.average_rating(request.user),
"rating": book.average_rating(include_parents=True, user=request.user),
"lists": lists,
"update_error": kwargs.get("update_error", False),
}
Expand Down