Skip to content

Fix transport cost related issues #1082

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fix transport cost related issues #1082

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0c37785
Fix transport cost related issues
twothreenine Jan 27, 2025
1c219c7
First try for better calculation of groups_without_markup
twothreenine Apr 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 18 additions & 12 deletions app/models/order.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,13 +224,16 @@ def sum(type = :gross)
end
elsif %i[groups groups_without_markup].include?(type)
for go in group_orders.includes(group_order_articles: { order_article: :article_version })
for goa in go.group_order_articles
case type
when :groups
total += goa.result * goa.order_article.article_version.fc_group_order_price
when :groups_without_markup
total += goa.result * goa.order_article.article_version.gross_group_order_price
case type
when :groups
total += go.total
when :groups_without_markup
go_price_without_markup = 0
for goa in go.group_order_articles
go_price_without_markup += goa.result * goa.order_article.article_version.gross_group_order_price
end
total += go_price_without_markup.round(2) || 0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why's the || 0 needed here?

total += go.transport if go.transport
end
end
end
Expand Down Expand Up @@ -273,7 +276,7 @@ def close!(user, transaction_type = nil, financial_link = nil, create_foodcoop_t
update_price_of_group_orders!

transaction do # Start updating account balances
charge_group_orders!(user, transaction_type, financial_link)
total_group_charges = charge_group_orders!(user, transaction_type, financial_link)

if stockit? # Decreases the quantity of stock_articles
for oa in order_articles.includes(article_version: :article)
Expand All @@ -285,7 +288,7 @@ def close!(user, transaction_type = nil, financial_link = nil, create_foodcoop_t
if create_foodcoop_transaction
ft = FinancialTransaction.new({ financial_transaction_type: transaction_type,
user: user,
amount: sum(:groups),
amount: total_group_charges * -1,
note: transaction_note,
financial_link: financial_link })
ft.save!
Expand Down Expand Up @@ -405,12 +408,15 @@ def update_price_of_group_orders!

def charge_group_orders!(user, transaction_type = nil, financial_link = nil)
note = transaction_note
total = 0
group_orders.includes(:ordergroup).each do |group_order|
if group_order.ordergroup
price = group_order.total * -1 # decrease! account balance
group_order.ordergroup.add_financial_transaction!(price, note, user, transaction_type, financial_link, group_order)
end
next unless group_order.ordergroup

price = group_order.total * -1 # decrease! account balance
transaction = group_order.ordergroup.add_financial_transaction!(price, note, user, transaction_type, financial_link, group_order)
total += transaction.amount
end
total
end
Comment on lines 409 to 420
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm personally not a big fan of having a method alter stuff and returning other stuff (except for maybe the object itself).

Disregarding that, if you'd like to keep it in there, I'd prefer this from a coding style perspective (untested!):

Suggested change
def charge_group_orders!(user, transaction_type = nil, financial_link = nil)
note = transaction_note
total = 0
group_orders.includes(:ordergroup).each do |group_order|
if group_order.ordergroup
price = group_order.total * -1 # decrease! account balance
group_order.ordergroup.add_financial_transaction!(price, note, user, transaction_type, financial_link, group_order)
end
next unless group_order.ordergroup
price = group_order.total * -1 # decrease! account balance
transaction = group_order.ordergroup.add_financial_transaction!(price, note, user, transaction_type, financial_link, group_order)
total += transaction.amount
end
total
end
def charge_group_orders!(user, transaction_type = nil, financial_link = nil)
note = transaction_note
amounts = group_orders.includes(:ordergroup).map do |group_order|
price = group_order.total * -1 # decrease! account balance
group_order.ordergroup.add_financial_transaction!(price, note, user, transaction_type, financial_link, group_order)
group_order.total
end
amounts.sum
end

The if group_order.ordergroup seems to be superfluous: group_orders.ordergroup_id is not nullable, but maybe I'm missing something here. If so, it could be rewritten to use a select instead.

I'm summing the positive total instead of price, which would remove the necessity to call * -1 again in close!

However actually I'd even prefer not returning the sum here, but using this in close! instead (untested too!):

total_group_charges = group_orders.map(&:total).sum

You'd have to test it for performance of course, but I think it shouldn't do much harm there, as caching should alleviate things and it would be much more readable IMO.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When creating a foodcoop transaction when settling an order, use the exact inverted sum of the group transactions to prevent rounding issues.

Ah, I had missed that - by that you mean that the sum should actually be less precise...?
(When assigning a number with a higher precision than FinancialTransaction.amount allows, it is automatically rounded and you're using that effect, right?)
But can you again describe what your intention about that was in the PR description? (I have an assumption, but it would be good for other reviewers to know about that too and it's kind of hard to read the whole discussion about it in #474 to find out which change is for what.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for reviewing :) I don't remember whether I had requested it ...

The if group_order.ordergroup seems to be superfluous: group_orders.ordergroup_id is not nullable

It is nullable, there's no null: false. This is for the case of stock orders as far as I know. See

foodsoft/db/schema.rb

Lines 219 to 221 in 02853a5

create_table "group_orders", id: :integer, charset: "utf8mb4", collation: "utf8mb4_general_ci", force: :cascade do |t|
t.integer "ordergroup_id"
t.integer "order_id", default: 0, null: false

Ah, I had missed that - by that you mean that the sum should actually be less precise...?
(When assigning a number with a higher precision than FinancialTransaction.amount allows, it is automatically rounded and you're using that effect, right?)

Exactly.
The group sum should show how much the groups are (resp. will be) actually charged.
Also, the financial link should always be balanced, so the food coop transaction should be the exact sum of the group charges.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is nullable, there's no null: false. This is for the case of stock orders as far as I know.

Ah, you're right, sorry - I forgot about that logic (kind of quirky how stock orders currently work 🙄 ). And I wonder if the if stockit? the shouldn't then be extended to include the charge_group_orders! as well. But nvm, that's not part of this PR.

Exactly.
The group sum should show how much the groups are (resp. will be) actually charged.
Also, the financial link should always be balanced, so the food coop transaction should be the exact sum of the group charges.

Okay, then how about this?:

# Query shorthand method to make things more readable:
def non_stock_group_orders
  group_orders.where.not(ordergroup_id: nil)
end

# Let the `charge_group_orders!` method return the `FinancialTransaction` instances themselves 
# as IMO that's the least unexpected return value:
def charge_group_orders!(user, transaction_type = nil, financial_link = nil)
  note = transaction_note
  non_stock_group_orders.includes(:ordergroup).map do |group_order|
    price = group_order.total * -1 # decrease! account balance
    group_order.ordergroup.add_financial_transaction!(price, note, user, transaction_type, financial_link, group_order)
  end
end

# ... then in `close!` use this to calculate the sum:
transactions = charge_group_orders!(user, transaction_type, financial_link)
# ...
amount: transactions.sum(&:amount) * -1

Copy link
Contributor

@lentschi lentschi Apr 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again - I think you should edit your PR description to something like to make this clearer for other people reviewing:

When creating a foodcoop transaction when settling an order, use the exact inverted sum of the group
transactions to prevent rounding issues. This is actually _less_ precise than before, but has the advantage of
mirroring the loss of precision happening elsewhere and to thus avoid confusion.

... Maybe you could elaborate on the "elsewhere" though.


def transaction_note
Expand Down
12 changes: 6 additions & 6 deletions app/views/finance/balancing/_edit_results_by_articles.html.haml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,15 +49,15 @@
- for order_article in @articles.select { |oa| oa.units > 0 }
= render :partial => "order_article_result", :locals => {:order_article => order_article}

%tr
%td{ colspan: 10 } The following were not ordered

- for order_article in @articles.select { |oa| oa.units == 0 }
= render :partial => "order_article_result", :locals => {:order_article => order_article}

- if @order.transport
%tr
%td{ colspan: 5 }= heading_helper Order, :transport
%td{ colspan: 3, data: {value: @order.transport} }= number_to_currency(@order.transport)
%td= link_to t('ui.edit'), edit_transport_finance_order_path(@order), remote: true,
class: 'btn btn-mini' unless order_article.order.closed?

%tr
%td{ colspan: 10 } The following were not ordered

- for order_article in @articles.select { |oa| oa.units == 0 }
= render :partial => "order_article_result", :locals => {:order_article => order_article}
Loading