search

foodcoops / foodsoft

Web-based software to manage a non-profit food coop (product catalog, ordering, accounting, job scheduling).
https://foodcoops.net/
Other
326 stars 146 forks source link

Critical bug: Article price changes when closing order #1056

Open twothreenine opened 7 months ago

twothreenine commented 7 months ago

There seems to be a bug regarding article_prices:

  1. Change an article price in the accounting menu without Globally update current price, e.g. 1.00 -> 2.00 €
  2. In the article menu and in current orders, the price remains unchanged, as well as in other closed orders for the same supplier (everything correct)
  3. But when an order is closed now, the price suddenly changes from 1.00 to 2.00 € (the last price this article was changed to in the accounting menu)

Changing a price in the receive menu, however, does not lead to this bug.

We've noticed this in our foodcoopsat hosting and I could reproduce it in demo.foodcoops.net.

lentschi commented 7 months ago

I think this has been the case for a long time. (I couldn't manage to run the old release version right now but I bet it was in there too; maybe I'll manage to check tomorrow).

As long as an order hasn't been closed, it's price is subject to changes in the article's master data form. When closing the order the article price becomes fixed to the article's latest article_price. I think that part has been intended.

However what this "latest article_price" is, can be quite confusing: As far as I can tell right now it's sorted by article_price.created_at in descending order. When editing the price in the accounting menu in turn, this date field is set to orders.end which is something the user can enter manually or even omit. If omitted it is just set to the current datetime.

twothreenine commented 7 months ago

I can confirm this was not the case two months ago.

We have a supplier which articles I update monthly (roughly), but we have an order every week. Dec 3rd price update Dec 16th - Dec 20th some prices changed via balancing menu Jan 6th: order PDF sent still shows the correct prices from the last update Jan 20th next price update

lentschi commented 7 months ago

I can confirm this was not the case two months ago.

I don't know exactly what your workflow was in your foodcoop prior to the upgrade to version 8, but I now could locally start up and reproduce the bug in v7:

https://github.com/foodcoops/foodsoft/assets/783944/945448df-0ae8-45c0-9af3-570cab06866d

Maybe in your workflow you've never updated the price through the accounting menu if another order of the same supplier was in the 'open' state...? (Because this particular bug only occurs in that case.)

lentschi commented 7 months ago

I've added a draft PR that would fix this specific error case. It's just an ugly workaround, but if nobody can think of where this might cause trouble, we could merge it. :thinking: (Would have to be tested thoroughly though.)

As already discussed elsewhere, the current article (price) versioning is fundamentally flawed IMO. That's part of why we aim to change the underlying db structure in our ongoing article unit project - so we might consider waiting for that instead of risking follow-up bugs here.

twothreenine commented 7 months ago

Could it be that #778 should have fixed that, but it was never merged into upstream?

c30542d had been committed to the foodcoopsat fork, I found it in the oldmain branch. @mortbauer seems to have reverted this commit in the recent rebase.

That would explain why it had been working for us in the last years and now it does not.

Maybe in your workflow you've never updated the price through the accounting menu if another order of the same supplier was in the 'open' state...?

Just a note, this doesn't only affect orders which are open, but orders which will be created after the price change, as well.

mortbauer commented 7 months ago

That is a very good find @twothreenine, you are completely right, I jumped over this one when I rebased since it was confusing and wasn't sure if this would still be needed.

So I cherry-picked c30542d it for current foodcoopsat/main, if that fixes these issue it would be good to have it finally upstream as well.

twothreenine commented 7 months ago

I assume you replaced updated_attributes with update as in https://github.com/lentschi/foodsoft/commit/3ffdb424d534b36d40c237e63cef8faaf67d6a81 ?

twothreenine commented 6 months ago

So I cherry-picked https://github.com/foodcoops/foodsoft/commit/c30542d82e07198332f5ea8db19e200d0355a965 it for current foodcoopsat/main, if that fixes these issue it would be good to have it finally upstream as well.

I've noticed that, in the foodcoopsat version, Globally update current price doesn't work anymore now. If you tick it and try to save, the modal won't close and the price won't be updated. Changing the price for one order works, though. Since you can also update the current price in the articles menu, I think this is not very important. It will be fixed with the new article versioning introduced in the extended units fork anyway, I guess.