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

units_history_line shows incorrect amount (only affects metadata) #68

Closed
twothreenine opened this issue May 12, 2024 · 1 comment
Closed
Labels
low-priority Minor issue with little consequences

Comments

@twothreenine
Copy link
Contributor

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:

%td{title: units_history_line(order_article, plain: true)}

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

# "1×2 ordered, 2×2 billed, 2×2 received"
def units_history_line(order_article, options = {})
if order_article.order.open?
nil
else
units_info = []
price = order_article.price
%i[units_to_order units_billed units_received].map do |unit|
next unless n = order_article.send(unit)
converted_quantity = price.convert_quantity(n, price.supplier_order_unit,
price.billing_unit.presence || price.supplier_order_unit)
line = converted_quantity.round(3).to_s + ' '
line += pkg_helper(price, options) + ' ' unless n == 0
line += OrderArticle.human_attribute_name("#{unit}_short", count: n)
units_info << line
end
units_info.join(', ').html_safe
end
end

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.

def price
article_version || article
end

@twothreenine twothreenine added the low-priority Minor issue with little consequences label May 12, 2024
@lentschi
Copy link
Contributor

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.

lentschi added a commit that referenced this issue May 24, 2024
lentschi added a commit that referenced this issue Jul 26, 2024
lentschi added a commit that referenced this issue Oct 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
low-priority Minor issue with little consequences
Projects
None yet
Development

No branches or pull requests

2 participants