Draft: Make average rating property of Edition model #3425
Conversation
Makes the query that returns the average rating of a book a @Property function of the Edition model and changes the view code to use this instead.
Added a return statement and doc string after running ./bw-dev formatters
bookwyrm/models/book.py
Outdated
| @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.
@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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Using a classmethod seems like a solid choice to me
Made average_rating a classmethod since it allows us to pass arguments to it as needed Added doc string explaining the different arguments that average_rating takes. For now the method covers the use case of the Book view. Will probably need more work to cover other use cases
| reviews = base_reviews.filter(Q(privacy=privacy)).exclude(rating=None) | ||
| else: | ||
| reviews = base_reviews.exclude(rating=None) | ||
|
|
||
| return reviews.aggregate(Avg("rating"))["rating__avg"] | ||
|
|
There was a problem hiding this comment.
I think using a method that takes a
Reviewqueryset (which can already be filtered with things likeprivacy_filterorprivacy="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
ratinguse theuserargument, 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
userargument.Another idea could be to have different methods that keep the behaviour of the methods mentioned but have them be on the
Editionmodel 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?
Description
Makes the query that returns the average rating of a book a
@propertyfunction of theEditionmodel and changes the view code to use this instead.What type of Pull Request is this?
Does this PR change settings or dependencies, or break something?
Details of breaking or configuration changes (if any of above checked)
Documentation
Tests