#779 Received state for orders

[lentschi]

No description provided.

[wvengen]

Oops, one thing I forgot, I realised just after merging: we probably need to migrate received orders to the state!
Would you mind adding that still, @lentschi?

[paroga]

can we PLEASE require tests for such fundamental changes. testing the core model should be no problem with the current testing infrastructure. this change nearly screams that i might cause a regression somewhere in the future

[wvengen]

Hmm I can see this causes tension if a fundamental change happens that not all developers are aware of. Perhaps such a change would better have been discussed on the developers mailing-list.

Regarding testing, do you mean the affected scopes and the Order#finished? and Order#received? methods? Controller or integration tests for methods that could be affected?

[paroga]

i don't see a problem with the change itself. it's fine and i support it. i just want to encourage us to write tests.
it's difficult to write test for ui/controller-stuff, but plain model changes should be easy to test. so i see no reason to merge model changes (app/models/*.rb) without (at least a few) tests. especially when it's such a fundamental entity like Order
i also think that the Order.transaction do part (of app/controllers/orders_controller.rb) should be moved into the model, to allow proper testing

[lentschi]

i just want to encourage us to write tests.

Sure, I can try and add some tests for the order model 😊 - I've started here: https://github.com/foodcoops/foodsoft/pull/809/files

i also think that the Order.transaction do part (of app/controllers/orders_controller.rb) should be moved into the model, to allow proper testing

That is a bit tricky though: This part calls OrdersController#update_order_amounts which contains a lot of controller logic that first would need to be extracted. (Which would in turn mean a log of refactoring, but guess I can give it a try! 😅 )

Before I take a go at this - some more questions:
I just stumbled across this source comment: "This was once wrapped in a transaction, but caused MySQL lock timeout exceeded errors. It's ok to do this article-by-article anway."
Is it really? For me as a user, it would be worse, if only part of my order gets updated and part of it wouldn't.
Also, the default MySQL/MariaDB value for innodb_lock_wait_timeout seems to be 50 seconds. If an update query really ever takes that long, I think we should refactor the whole save process instead of removing the transaction.

@wvengen Could you maybe describe how to reproduce the error fixed by your commit d906a73? (If it is still reproducible, I may have caused a regression with my new Order.transaction do 😨 I don't see anything that could use up that much time - even for huge orders - and I've never run into this with test data from my foodcoop. Could it be, that this just happened when some server was on overload?)

[paroga]

This part calls OrdersController#update_order_amounts

sorry, my bad. though that update_order_amounts would be already in the model. if you like to refactor the code it would be SUPER nice, but I don't see it as your responsibility. testing the scope as in #809 is all i can demand from you here.

If an update query really ever takes that long, I think we should refactor the whole save process instead of removing the transaction.

the main problem is, that the article_prices are calculated in ruby and not in the database and i expect some missing eager loading. changing that is a bigger refactoring, which should be planed wisely and target also some limitations also.

[lentschi]

Oops, one thing I forgot, I realised just after merging: we probably need to migrate received orders to the state!
Would you mind adding that still, @lentschi?

Yes, I added it (and @paroga has already merged it).
The reason I hadn't already added it before was that I thought the received state doesn't make any difference in the current master and for foodcoops switching to my potential new self service function (#766), manual db steps will be required anyway. (Switching to the new workflow with running orders would be difficult, so I think a clean cut when all orders are closed needs to be done there.)
But then I found that it is already useful in the master: https://github.com/foodcoops/foodsoft/pull/808/files
... so adding them was a good idea after all 🙂