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: Add Households to Mealie #3970

Merged

Conversation

michael-genson
Copy link
Collaborator

@michael-genson michael-genson commented Jul 31, 2024

What type of PR is this?

(REQUIRED)

  • feature

What this PR does / why we need it:

(REQUIRED)

Base implementation for the households feature: #2134. This feature is massive and touches pretty much everything. I will explain the high-level changes, as well as break down into some more specifics.

What is a household?

A household is a subdivision of groups with a collection of users. While groups can be considered as completely separate instances, households mix their data with each other. Another way to think of a household is a family: your group may contain different families, each with their own shopping lists, meal plans, and recipes.

These previously group-level features now belong to a household:

  • Notifiers
  • Webhooks
  • Cookbooks
  • Recipe Actions
  • Users
  • Shopping Lists (tied to a user)
  • Meal Plans (tied to a user) and Meal Plan Rules (not tied to a user)
  • Recipes (tied to a user)

Of note, these features are not tied to a household, and are shared amongst households:

  • Organizers (Tags, Categories, Tools)
  • Ingredient Stores (Foods, Units, Labels)

What does it mean for a feature to belong to a household?

Features that belong to households (e.g. Notifiers) are only relevant in a household context. This means:

  • When querying for documents, only those that belong to your household are returned
  • When performing actions (e.g. editing a recipe) only documents in your household are affected (e.g. if a recipe is edited in someone else's household, your notifiers don't fire)
  • If the feature is tied to a user (e.g. Shopping Lists), if that user switches households, so does that feature. For instance, if I own a shopping list, only my household can see it. If I switch households, my old household can't see the shopping list anymore

What are some common use-cases this solves?

Generally speaking, families typically don't prepare meals for other families. This means they shop for their own family, plan meals for their own family, have their own rules on how meal plans work, etc. This allows each family to have their own shopping lists and meal plans, as well as their own automations (i.e. webhooks, notifiers, and actions).

However, in terms of data, there's no reason to have separate stores for foods, units, tags, etc. So those are all shared amongst families. Families also typically want to share recipes with each other, so everyone has their pool of recipes and cookbooks that they draw from to create their meal plans, shopping lists, etc.

What use-cases aren't solved by this PR? What are the limitations?

Despite the ridiculous diff, this PR attempts to limit its scope and leave some things for future PRs (see tasks in the special notes section below). There are known/intentional limitations to this implementation (some to be addressed in future PRs):

  • Households are currently mostly isolated. Specifically, recipes from one household cannot be viewed in another. There's more work to be done with household permissions, cross-household logic, etc. More on some of that below.
  • Cookbook and recipe names/slugs are unique across groups, not households. This is intentional, otherwise we would need to include the household id/slug in the URL. This means if household_1 has a recipe called "Brisket", and household_2 creates a recipe called "Brisket", the second recipe will be renamed to "Brisket (1)". Right now this is confusing to the end user, since you can't see other households' recipes, but this will be addressed later.
  • While households are isolated from each other, and some measures are put in place to not expose information between them, there are way too many edge-cases to assert privacy between households. There is a lot of potential to mix household users (e.g. commenting on another household's recipe) and due to this there's no feasible way to keep household data securely private from another household. At the group level the app does a good job of enforcing this already, and households can make things private to public users, but it's not realistic to enforce security between households.

Additionally, since data is filtered by household, admins can't see all data in some instances. For instance, there is no admin page for all recipes (the data management page is not an admin page; it only contains recipes for your household). If an admin wants to modify a recipe in another household, they have to move to the other household to do this. Same for webhooks, notifiers, and actions. To enable this, we need new admin pages for these items. In the meantime, non-admins that belong to other households can manage these things (with proper permissions).

In general, we probably want to have instance-admins (currently just admin) and group-admins (an admin for everything in the group, i.e. for all households). We probably don't need household admins (that's more-or-less what the existing user permissions do, e.g. "can organize").

What considerations are made for public instances?

Most group preferences have been migrated to households (e.g. recipe_public so households have the ability to hide recipes from public users). There's also a new private_household option (idential to private_group, just at the household level). If a household is private, but a group is public, no private data is exposed to public users (currently limited to cookbooks and recipes).

While above I stated that cross-household recipe sharing isn't implemented yet, for public users, all recipes from all households are shown (assuming the correct privacy settings as stated above). Currently this is the only way to see recipes from multiple households in one location. Similarly, all cookbooks from all households are shown to public users, but cookbooks are limited to recipes in their own households (or, put another way, if you open a cookbook from household_1, you will only see recipes from household_1, even if recipes in household_2 fit the cookbook criteria).


So how does this work internally?

All database objects that depend on a household have a new household association. However, since we always want to keep household objects that are tied to users with the user, those household relationships are proxied. For instance: a recipe belongs to a user. The recipe's household_id is effectively recipe.user.household_id. This is accomplished with sqlachemy's association proxies. It basically creates a "fake" attribute on the model that depends on the associated table. Honestly we should probably do this for groups too, to solve issues like #3839, but for now we're only doing this with households.

Note that while this could potentially lead to a performance hit (you need to join on the related table to get the id to filter on) I've added several joined loads to prefetch the related tables, meaning we're not increasing the number of queries by doing this (in most instances we were joining on the proxied table anyway, so pre-joining actually results in a performance boost).


While the database changes are the lowest-level changes, the biggest app-wide changes come from an overhaul on how our repositories work. There were already issues with the current repo implementation, resulting in data leakage between groups, so the new implementation makes it a lot harder to leak data.

Repos can only be filtered by group (and by household) at instantiation. If you want to change a repo's filters, you have to make a new instance of the repo (or repo factory). This ensures that repos are always filtered the way you expect them to be. Furthermore, repos are subclassed into RepositoryGeneric, GroupRepositoryGeneric, and HouseholdRepositoryGeneric. Each repo type requires a particular id (or ids) to instantiate. So, for instance, if you instantiate a HouseholdRepositoryGeneric repo with no household_id, an exception is thrown. You must either:

  1. Pass a household_id, thus filtering by the household
  2. Explicitly pass None, indicating that you do not want to filter by household.

This ensures that you're not accidentally retrieving records without the intended filters. There are times you want this (e.g. if an admin wants to manage all households in the instance, you don't want to filter by group), but now you have to do it explicitly.

To make repo management not a huge pain, the AllRepositories factory injects its group_id and household_id into the repo when it creates it. When a controller instantiates the factory, it passes the authenticated user's group_id and household_id to the factory. Effectively, nothing changes downstream when using the repository factory under most scenarios, it just magically works with dependency injection. It's only when you want to do cross-group or cross-household funky operations where you have to explicitly break this pattern (which is good, since it makes those operations explicit).

A typical usage (from scratch) would be:

unfiltered_repos = get_repositories(session)  # for anything that doesn't need a group or household, e.g. fetching all groups
group_repos = get_repositories(session, group_id=group_id)  # for anything filtered by group, but not by household, e.g. foods
household_repos = get_repositories(session, group_id=group_id, household_id=household_id)  # for everything else, filtered by household_id, e.g. recipes

In the above example, this would throw an error:

recipes = group_repos.recipes  # ValueError: household_id must be set

If you wanted recipes not filtered by household, this is how you would do it:

unfiltered_household_repos = get_repositories(session, group_id=group_id, household_id=None)
recipes = unfiltered_household_repos.recipes  # no ValueError is raised

Alternatively you can construct it directly (though you probably shouldn't):

recipes = RepositoryRecipes(session, "slug", RecipeModel, Recipe, group_id=group_id, household_id=None)

Breaking Changes

This PR introduces a ton of breaking changes. Obviously there are new concepts/workflow changes, but strictly from a developer/integrator standpoint:

Many API routes have moved:

  • /groups/webhooks -> /households/webhooks
  • /groups/shopping/items -> /households/shopping/items
  • /groups/shopping/lists -> /households/shopping/lists
  • /groups/mealplans -> /households/mealplans
  • /groups/mealplans/rules -> /households/mealplans/rules
  • /groups/invitations -> /households/invitations
  • /groups/recipe-actions -> /households/recipe-actions
  • /groups/events/notifications -> /households/events/notifications
  • /groups/cookbooks -> /households/cookbooks
  • /explore/foods/{group_slug} -> /explore/groups/{group_slug}/foods
  • /explore/organizers/{group_slug}/categories -> /explore/groups/{group_slug}/categories
  • /explore/organizers/{group_slug}/tags -> /explore/groups/{group_slug}/tags
  • /explore/organizers/{group_slug}/tools -> /explore/groups/{group_slug}/tools
  • /explore/cookbooks/{group_slug} -> /explore/groups/{group_slug}/cookbooks
  • /explore/recipes/{group_slug} -> /explore/groups/{group_slug}/recipes

One API route changed behavior:

  • /groups/members outputs a UserSummary instead of UserOut, since households can call this API. It can also be filtered by household with an optional householdId param.

Some API routes have been removed:

  • /admin/analytics (it's deprecated anyway)
  • /groups/permissions (see household permissions)
  • /groups/statistics (see household statistics)
  • /groups/categories (see organizers, not sure what the purpose of this was originally)
  • /recipes/summary/untagged (no longer used)
  • /recipes/summary/uncategorized (no longer used)
  • /users/group-users (see group/members and households/members)

Additionally, the following frontend pages have moved:

  • /group/mealplan/... -> /household/mealplan/...
  • /group/members -> /household/members
  • /group/notifiers -> /household/notifiers
  • /group/webhooks -> /household/webhooks

Lastly, updateAt/update_at has been renamed to updatedAt/updated_at. The API still accepts updateAt (I didn't change the database column), but it will spit out updatedAt now. This isn't directly relevant to households, but we're introducing a ton of breaking changes anyway.

I also fixed some codegen issues, including migrating us to a fork of pydantic2ts which actually works (the old one is dead, never worked with Pydantic V2).

Which issue(s) this PR fixes:

(REQUIRED)

Fixes #3727
Fixes #3839

Special notes for your reviewer:

(fill-in or delete this section)

Naturally, with all of these breaking changes. this warrants a V2 release. There's plenty of additional work to get done before then, but this PR takes care of the majority of the difficult bits.

After this is merged, I will create some tasks that need to be completed before fully releasing this (along with a new tag for Mealie V2). Some include:

  • missing features (e.g. logged-in users viewing other households' recipes, household permissions, etc.)
  • a developer migration guide (i.e. breaking changes)
  • updating the docs (references to group that should be household, adding new household docs, updating the how-do-private-groups-and-recipes-work page, etc.)

Testing

(fill-in or delete this section)

Backend

Backend is almost entirely covered by automated tests. Exceptions are related to webhooks and notifiers; I've done manual tests on all of these, but it's worth other people testing these workflows as well.

Frontend

While extensive manual testing was done, we definitely should have more testing here. Here's how I recommend setting up your data for testing:

  1. Import your existing data from your real instance
  2. Create one (or more) households in your group(s)
  3. Assign some of your users to other households

You should observe all of the new "household" rules (e.g. only recipes that belong to the household are seen when logged-in, or all non-private households recipes when logged-out). You should also test that webhooks and notifiers fire for the household data (rather than the entire group's data).

Also, keep a close eye on the language: household pages should say "household" rather than "group" (e.g. manage users in your household).

@Kuchenpirat
Copy link
Collaborator

Kuchenpirat commented Aug 22, 2024

Think i found one bug to squash.
When adding whole day to a shoping list via the meal planner i get a "Failed to add to list" error.
Its not 100% of the time but always when i reload the page and adding to the shopping list is the first thing i do on it. Its not a timing issue or something, have waited for 15 seconds with the same result.

It gets the recipes of the day (in my case just the cookies one) and opens the screen correctly. There is nothing getting posted when adding to the list.

Also the snackbar is green when it fails which got me for longer than i like to admit.

Logs:

test-mealie  | INFO     2024-08-22T10:37:44 - [192.168.188.132:61995] 200 OK "GET /api/households/shopping/lists?page=1&perPage=-1&orderBy=name&orderDirection=asc HTTP/1.1"
test-mealie  | INFO     2024-08-22T10:37:44 - [192.168.188.132:61995] 200 OK "GET /api/recipes/salomes-cookies HTTP/1.1"

@michael-genson
Copy link
Collaborator Author

michael-genson commented Aug 22, 2024

Is it just on the dev container? I vaguely remember you had a similar issue when I implemented that feature: #2653

Edit: just kidding that was boc, not you

@Kuchenpirat
Copy link
Collaborator

Nope not in dev container.

@michael-genson
Copy link
Collaborator Author

Fixed. Not a new bug with this PR, apparently we showed the wrong text any time we added more than one recipe to the shopping list at once.

@michael-genson
Copy link
Collaborator Author

I feel like that commit shouldn't do anything, but it does, and it's consistent, so whatever

@Kuchenpirat
Copy link
Collaborator

I feel like that commit shouldn't do anything, but it does, and it's consistent, so whatever

i'll build a new image and try in a prod system to see if i can reproduce but as it seems that the issue was present before this PR i have no problem with pushing this over the finish line.

Copy link
Collaborator

@Kuchenpirat Kuchenpirat left a comment

Choose a reason for hiding this comment

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

Seems to all be working 🚀

@michael-genson michael-genson merged commit eb170cc into mealie-recipes:mealie-next Aug 22, 2024
11 checks passed
@michael-genson michael-genson deleted the feat/households branch August 22, 2024 15:14
boc-the-git pushed a commit to boc-the-git/mealie that referenced this pull request Sep 28, 2024
boc-the-git pushed a commit to boc-the-git/mealie that referenced this pull request Sep 28, 2024
@Kuchenpirat Kuchenpirat added the v2 Version 2 Issue/PR label Oct 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants