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

feat: User-specific Recipe Ratings #3345

Conversation

michael-genson
Copy link
Collaborator

@michael-genson michael-genson commented Mar 19, 2024

What type of PR is this?

(REQUIRED)

  • feature

What this PR does / why we need it:

(REQUIRED)

This PR migrates recipe ratings to a new user rating. Recipe ratings are now user-specific, and the recipe's "rating" property has been repurposed to display the average user rating.

More technically, a new secondary table UserToRecipe has been created, which stores the relationship between a user and a recipe. Existing favorites are also migrated to this secondary table, so we don't have two user/recipe tables. The UserToRecipe table has two data properties (aside from keys):

  • rating: a float representation of the user's rating
  • is_favorite: a boolean indicating if the recipe is favorited or not

This has a ton of consequences, which I'll go over:

Migration

The migration for this change converts favorites into UserToRecipe rows. This was pretty straightforward (before we delete the old table, migrate the favorites).

The ratings are a little more complicated; since we don't know who rated the recipe originally, ratings are created for all users. The migration also accounts for inserting new rows vs updating existing rows (from the favorites migration).

Previously, recipe ratings were ints, not floats. Since we're averaging user ratings, I thought it would give us an opportunity to display decimal ratings. I ended out not actually implementing this on the frontend now I did, see below.

Database Schema

I redid the recipe's favorited_by and the user's favorite_recipes to use the same data as the rating, with a filter on is_favorite == True. More specifically, it's defined like this:

favorited_by: Mapped[list["User"]] = orm.relationship(
    "User",
    secondary=UserToRecipe.__tablename__,
    primaryjoin="and_(RecipeModel.id==UserToRecipe.recipe_id, UserToRecipe.is_favorite==True)",
    back_populates="favorite_recipes",
    viewonly=True,
)

It's view-only, so it can't be set (you have to set it using the UserToRecipe table instead), but it can be filtered. I used this on the frontend to efficiently load the user's favorited recipes (more on why I had to do this below):

const { data } = await api.recipes.getAll(1, -1, { queryFilter: `favoritedBy.id = "${userId}"` });

Since the recipe rating is now the average user rating, it can't be set directly. This is done with an event trigger:

@event.listens_for(RecipeModel, "before_update")
def calculate_rating(mapper, connection, target: RecipeModel):
    session = object_session(target)
    if not session:
        return

    if session.is_modified(target, "rating"):
        history = get_history(target, "rating")
        old_value = history.deleted[0] if history.deleted else None
        new_value = history.added[0] if history.added else None
    if old_value == new_value:
        return

    target.rating = (
        session.query(sa.func.avg(UserToRecipe.rating))
        .filter(UserToRecipe.recipe_id == target.id, UserToRecipe.rating is not None, UserToRecipe.rating > 0)
        .scalar()
    )

Creating/modifying/deleting a UserToRecipe row triggers this by setting the recipe's rating to -1 (thus causing it to recalculate):

@event.listens_for(UserToRecipe, "after_insert")
@event.listens_for(UserToRecipe, "after_update")
@event.listens_for(UserToRecipe, "after_delete")
def update_recipe_rating_on_insert_or_delete(_, connection: Connection, target: UserToRecipe):
    session = Session(bind=connection)

    update_recipe_rating(session, target)  # the recipe's rating is set to -1 here
    session.commit()

API

I converted the existing UserFavorites controller and APIs into a new UserRatings controller. The old POST and DELETE methods for recipe favorites still exist, which are basically just adapters to the new ratings API.

I updated recipe rating sorting to sort by a combined value (as suggested by @Desecrate2337 below). That is the "sorting value" for rating is calculated such that:

  1. If a user has a rating for the recipe, that value is used
  2. Otherwise, the recipe's average rating is used

This also allows public users to sort by average recipe rating (since user ratings don't exist).

There are two breaking changes to the API:

  1. The regular User API no longer returns favorites. Previously you could fetch a user annotated with its favorite recipes (slugs for most endpoints, but actual recipe objects for the favorites endpoint). For various reasons this doesn't work anymore (mostly because of performance), but thankfully our fancy query API makes this easy: favoritedBy.id = "${userId}"
  2. If you attempt to update the recipe's rating directly, the API will return 200 OK, but it ignores your rating value and recalculates the average rating (as detailed above). This one is pretty minor, IMO, since the rating functionality is fundamentally changed, so API changes are expected.

Frontend

Several changes are made to the frontend to accommodate this. I won't go into details on how it works since it's reasonably straightforward (you can check the diff), but I will go into what changes the user sees. For favorites, the experience is the same, the only difference is how the data is populated/updated/refreshed. See below for the rating changes:

If a user has not rated a recipe, but another user has, the recipe's rating (i.e. the average user rating) is shown in grey:
image

If a user has rated a recipe, it's red (i.e. the secondary color), same as before:
image

If the user is logged-out, or visiting from another group (i.e. they're an anonymous user) the grey color logic is ignored and the rating is always the secondary color (i.e. how it used to be).

Since averages are floats, when viewing an average rating, half-stars are enabled (note the color is red since I'm viewing as a non-logged-in user):
image

However you can still only select whole stars as a user (because selecting a half-star is kinda weird and adds unnecessary complexity, IMO, though we can change this if users would rather have that granularity).

If a user unsets a recipe, it shows as empty until the next page refresh. This is for two reasons:

  1. It's awkward to fade from the user rating to the average rating (but I just cleared my rating!!!)
  2. The rating is wrong (a new average needs to be calculated since you just removed your rating)

When hovering over a grey rating, the color changes from grey to secondary (since you're setting the user rating, not the average rating). Here's a video demonstrating both this and the unset logic:
2024-03-19_15h55_33

Obviously we can change this behavior, but I thought this made the most sense for now. Happy to gather feedback!

Since sorting uses a combined value (preferring user ratings over global, see API changes above), this is communicated to the user through the different star colors:
image

Which issue(s) this PR fixes:

(REQUIRED)

Closes #889

Special notes for your reviewer:

(fill-in or delete this section)

While this PR obviously does a ton, it also opens the door for some interesting average recipe vs user recipe components. I implemented the simple one with grey vs red stars, but there's definitely a lot more room there.

One small issue is that sorting by rating only sorts by average rating fixed, see above.

Testing

(fill-in or delete this section)

Lots of frontend clicking and pytest

@Desecrate2337
Copy link

Wow, what seemed like a small suggestion looks like it was quite a bit of work. Thanks!

For the sorting by the rating I have a few ideas you could implement (if it's not too difficult)

  1. Toggle between average rating and user rating
  2. Have the ratings mixed together. I'll try to explain as best as I can. The user's rating should outrank the weighted average rating on a specific recipe. ie) If a user rates it a 5 and the average is a 3.5, when sorting it should be at the top of the list as a 5. Then say there's a recipe that a user has not yet rated - but the average rating is a 4.9. It should show that one next. If the next highest recipe is rated a 4/5 from the user then that would be 3rd in the list.

@michael-genson
Copy link
Collaborator Author

Thanks for the feedback!

Toggle between average rating and user rating

At least for now I want to avoid this, I think it will add clutter to the sort menu (and I don't think it would be super easy to implement). Maybe something down the line.

Have the ratings mixed together

Yeah I think this is the best solution. I'll see if it's feasible, right now I'm not sure if it's possible to sort by a value on another table, but there might be some workaround we can do.

@michael-genson michael-genson marked this pull request as ready for review March 20, 2024 20:32
Copy link
Collaborator

@hay-kot hay-kot left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@michael-genson michael-genson merged commit 2a541f0 into mealie-recipes:mealie-next Apr 12, 2024
10 checks passed
@michael-genson michael-genson deleted the feat/user-specific-ratings branch April 12, 2024 02:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants