search

foodcoopsat / foodsoft_hackathon

Other
1 stars 2 forks source link

units_history_line shows incorrect amount (only affects metadata) #68

Closed twothreenine closed 5 months ago

twothreenine commented 5 months ago

In the order/show view (order management), in the "full units" column on the right, the amount (to order) is shown correctly on the screen (in supplier_order_unit), but incorrectly in the metadata (in billing_unit -- should be 2 × 700 g Ordered for the bread example)

<td title="1.4 × 700 g Ordered">
2
<i class="package ">× 700 g</i>
</td>

The code at play here:

https://github.com/foodcoopsat/foodsoft_hackathon/blob/58ecae5646ac8d6a98e9e56705c8e59a0fa77c4b/app/views/orders/_articles.html.haml#L38

I tried units_history_line(order_article, plain: true, unit: order_article.article_version.supplier_order_unit) but it didn't change anything.

https://github.com/foodcoopsat/foodsoft_hackathon/blob/58ecae5646ac8d6a98e9e56705c8e59a0fa77c4b/app/helpers/orders_helper.rb#L26-L45

This method is obscure to me and I couldn't figure out what should be fixed here.

In the other cases where units_history_line is used, it seems correct as the billing unit is needed there.

btw, I suggest to rename order_article.price to version to avoid confusion in places like above. https://github.com/foodcoopsat/foodsoft_hackathon/blob/58ecae5646ac8d6a98e9e56705c8e59a0fa77c4b/app/models/order_article.rb#L36-L38

lentschi commented 5 months ago

This method is obscure to me [...]

Yeah... I just added the unit conversion part and left the rest untouched. But I can explain it to you in our next call, if you wish.

The error was in the unit conversion part however. (Units display was changed by fixes for issues like #44, but I had missed changing this history title helper method.)

btw, I suggest to rename order_article.price to version to avoid confusion in places like above.

Yes that would be nice - however it is quite some effort in an untyped languange such as ruby. We had discussed this when we started with our fork and decided to stick with the price method's original name until we have a higher test coverage, so we can be sure we didn't accidentally break stuff by renaming it.