-
-
Notifications
You must be signed in to change notification settings - Fork 268
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
base: main
Are you sure you want to change the base?
Changes from 2 commits
0d5ba1d
bcd0ff2
4f31d6b
cfda8b7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
@@ -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, | ||
|
@@ -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"] | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The filter calls here have overlap with the initial filter in the At this stage of the refactor, I am okay with leaving the
I also noticed that there are some subtleties in how we use the @mouse-reeve what do you think? |
||
def get_rank(self): | ||
"""calculate how complete the data is on this edition""" | ||
rank = 0 | ||
|
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.
@property
functions cannot take arguments as mentioned hereHowever the original implementation is on run a
reviews
value that has the class methodprivacy_filter
run on it before the rating calculation is done as shown hereShould we remove the
privacy_filter
call or switch to making this a function on theEdition
model or aclassmethod
?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.
Also, other calculations of the
rating
use theuser
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.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.
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 anEdition
; I think using a method that takes a review queryset (which can already be filtered with things likeprivacy_filter
orprivacy="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.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.
Using a
classmethod
seems like a solid choice to me