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 all commits
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
33 changes: 32 additions & 1 deletion 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 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 All @@ -19,6 +19,7 @@

from bookwyrm import activitypub
from bookwyrm.isbn.isbn import hyphenator_singleton as hyphenator
from bookwyrm.models.status import Review
from bookwyrm.preview_images import generate_edition_preview_image_task
from bookwyrm.settings import (
BASE_URL,
Expand Down Expand Up @@ -491,6 +492,36 @@ def hyphenated_isbn13(self):
"""generate the hyphenated version of the ISBN-13"""
return hyphenator.hyphenate(self.isbn_13)

@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):
"""calculate how complete the data is on this edition"""
rank = 0
Expand Down
4 changes: 2 additions & 2 deletions bookwyrm/views/books/books.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

from django.contrib.auth.decorators import login_required, permission_required
from django.core.paginator import Paginator
from django.db.models import Avg, Q
from django.db.models import Q
from django.http import Http404
from django.shortcuts import get_object_or_404, redirect
from django.template.response import TemplateResponse
Expand Down Expand Up @@ -98,7 +98,7 @@ def get(self, request, book_id, **kwargs):
if not user_statuses
else None
),
"rating": reviews.aggregate(Avg("rating"))["rating__avg"],
"rating": book.average_rating(include_parents=True, user=request.user),
"lists": lists,
"update_error": kwargs.get("update_error", False),
}
Expand Down
Loading