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

Shoppinglist fix #3048

Merged
merged 2 commits into from
Mar 21, 2024
Merged

Conversation

smilerz
Copy link
Collaborator

@smilerz smilerz commented Mar 19, 2024

fixes #3044 and #3036

bulk_entries.update(checked=(checked := serializer.validated_data['checked']), updated_at=timezone.now(), )

# update the onhand for food if shopping_add_onhand is True
if request.user.userpreference.shopping_add_onhand:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I really don't like this from a performance perspective, looping over an unknown amount of food entries and executing queries each time, but I will let it pass for now to restore the functionality.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I really don't like this from a performance perspective, looping over an unknown amount of food entries and executing queries each time, but I will let it pass for now to restore the functionality.

AFAIK there isn't any other way to update a ManyToMany relationship. I did spend a lot of time looking for a solution though and best I could find is to introduce queueing.

Copy link
Collaborator

Choose a reason for hiding this comment

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

you can actually bulk create relations using the trough model (I did that for open data) but I am generally not a friend of the whole system of user on hand, shared, stock, whatever, there are so many slow and inefficent queries in the shopping system, there needs to be a general overhaul at some point.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I looked into directly manipulating the through model - but couldn't reason through how to figure out which records to create/destroy reliably.

I agree on making improvements to the complex web of related models. I've been investigating options for denormalizing models for search/reporting purposes. Materialized views don't seem practical, maybe caching, there are some libraries that can do it for you (https://github.com/netzkolchose/django-computedfields) but don't seem widely used.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds interesting, I am not sure if we need to start on an even higher level. There comes the time were we might want to look into stock keeping (i consider "on hand" a subset of the complexity of that) which is always a big pain in database systems (transactions, multiple "warehouses", ...).
Keeping stock on a per user basis would not be what a users expects (at least thats what I think), similar to the current on hand system. Thats why I was thinking about adding a "Household" system were one space can have multiple househoulds and lots of the sharing related stuff will then be tied to households together with stock.

This might also make it easier to disable the definitely more complex queries to support stuff based on people have households or if households or spaces have stock keeping enabled.

These are just rough ideas, we probably also need to look into other established software systems (like grocy, open source ERP systems, ...) or maybe theoretical papers on how to efficiently handle the stock keeping/on hand system.

But all of this is a "after vue 3" topic, so we can think a bit more about it.

@vabene1111 vabene1111 merged commit d8c86a4 into TandoorRecipes:develop Mar 21, 2024
4 checks passed
@smilerz smilerz deleted the shoppinglist_fix branch March 22, 2024 19:58
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.

Shopping List Incomplete
2 participants