-
-
Notifications
You must be signed in to change notification settings - Fork 606
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
Shoppinglist fix #3048
Conversation
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: |
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
fixes #3044 and #3036