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 2 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
9 changes: 8 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
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,12 @@ 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)
return reviews.aggregate(Avg("rating"))["rating__avg"]
Copy link
Author

Choose a reason for hiding this comment

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

@property functions cannot take arguments as mentioned here

However the original implementation is on run a reviews value that has the class method privacy_filter run on it before the rating calculation is done as shown here

Should we remove the privacy_filter call or switch to making this a function on the Edition model or a classmethod?

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@mouse-reeve mouse-reeve Aug 29, 2024

Choose a reason for hiding this comment

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

Ah I completely overlooked the fact that this function modifies a Review queryset for a reason! So you're right that it doesn't make sense for it to be a property of an Edition; 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 different ways this is queried really is a testament to how much this refactor is needed...

The method ought to filter out reviews where the status is deleted, or the rating is zero or None. It's probably fine to leave whether or not ratings from other Editions of the same Work (book__parent_work__editions=b for example) are included in the aggregate up to the lines of code that call it.

Copy link
Member

Choose a reason for hiding this comment

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

Using a classmethod seems like a solid choice to me


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(request.user),
"lists": lists,
"update_error": kwargs.get("update_error", False),
}
Expand Down