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

correct invoice balance with stock orders and deliveries #1075

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

Conversation

mjavurek
Copy link
Contributor

@mjavurek mjavurek commented Nov 10, 2024

Correct invoice balance with stock orders and deliveries

According to issue #1074, stock orders and deliveries were not correctly considered in invoice balances (invoice view and unpaid invoices), leading to unbalanced invoices in case of deliveries and/or stock orders.

This patch includes:

  • generally ignore stock orders when calculating the groups-sum for an order, since no order-group is charged for it
  • invoice view: for the total amount, use only the order sums without stock orders, but display both sums for orders if the sums differ (without/with stock order)
  • invoice class: exclude stock orders from orders_sum, new deliveries_sum, consider both orders_sum and deliveries_sum in expected_amount

Example: the order of this invoice contains a stock-order with 36,16 €. so only 59,89 € of the order are considered in the total amount, together with the stock-delivery this sums up to a total amount of 96,05 €.

grafik

grafik

exclude stock_orders in orders_sum, consider deliveries_sum
exclude stock-orders from sum(:groups)
show order sums without  and with stock order, consider only sums without stock order for total
replaced for loop for summation by .sum()
@mjavurek
Copy link
Contributor Author

mjavurek commented Nov 11, 2024

Show markup for invoice balances

I noticed that a markup is not considered in balancing invoices, showing only the total order and delivery amount without markup. In my opinion, both total amounts without and with markup should be displayed in case a markup is set. The total without markup should match the net invoice amount, and the total with markup should be displayed to see the total amount that is received from the ordergroups.

So my proposal is, in case of a markup > 0:

invoice view

  • orders and deliveries: show only sums without markup to keep it clear (as currently implemented)
  • total without markup: ...
  • total with markup: ...
  • or: total: (without markup) / (with markup)

I decided to implement the first for more clarity:
grafik

I changed "Gewinn" (German for "profit") to "Überschuss" (German for "surplus", as it is also used in the balance order summary).

unpaid invoices

show two balance amounts: one without and one with markup, e.g.: 0 € / +9,50 € which means that without markup, the grouporders sum up exactly to the invoice amount, and with markup, the foodcoop has 9,50 € "profit". Currently, only the second value is displayed, which makes it difficult to see if the profit is e.g. reduced by an imbalance between invoice net amount and ordergroup sum without markup.

grafik

order sum without markup like in the current implementation
sum_og without markup, like order.sum
added type argument for order and delivery sum and for expected_amount for calculation without and with markup
added additional total with markup
added foodcoop profit without/with markup
show invoice labels for profit and markup
added show invoice labels for fc profit and markup (margin)
missing lines added for total_fc calculation
calculate transport sum only from ordegroup transport charges
added order sum over group order transport charges. this can be different form the order's transport amount if groups are not charged with transport costs or if stock orders exist.
show total transport costs and sum of ordergroup charges for transport if they differ. for total, consider only the ordergroup transport charges.
@mjavurek mjavurek marked this pull request as draft November 13, 2024 12:14
if markup is used, show both differences without and with markup
@mjavurek
Copy link
Contributor Author

mjavurek commented Nov 13, 2024

Transport costs in invoices

I also noticed that actually, always the total transport costs are considered in balancing, even if the groups are not charged with the transport costs or if there is stock order and the foodcoop has to pay the transport costs for it. This is now considered by adding the transport costs over all ordergroups instead of taking the total transport costs.

In the following example, 10 € transport costs are divided on the order groups (including the stock order):
grafik

Now two transport costs are shown: the part of the ordergroups (6,24 €), and the total amount (10 €). Only the ordergroup part is considered in the total amount. In order to get a zero balance, the stock articles prices would have to be in creased to cover the transport costs for the stock articles (3,76 €).
grafik

@mjavurek mjavurek marked this pull request as ready for review November 13, 2024 13:52
…rders

if an invoice has only deliveries but nor orders, expected amount returned the invoice net-amount but not the deliveries. if an invoice has no orders and deliveries, expected_amount is now 0.
"Überschuss" drückt aus, dass die Einnahmen die Kosten übersteigen, ohne den kommerziellen oder profitorientierten Charakter anzudeuten, der mit "Gewinn" assoziiert wird.
"surplus" expresses that revenues exceed costs without implying the commercial or profit-oriented nature associated with the term "profit."
positive values are displayed green with extra +, negative in red
@mjavurek
Copy link
Contributor Author

mjavurek commented Nov 21, 2024

"new order balancing" view

In the finance / balance order view, the following modifications should make it more correct and transparent:
grafik

  • generally show amounts with markup only if markup > 0
  • in the order summary:
    • by now an invoice balance was shown, not considering any other invoice's orders or deliveries and therefore showing a wrong balance in these cases. therefore, the balances are removed from the summary and placed in the invoice field, considering the invoice balance instead of the order balance.
    • the stock order amount was added if > 0. stock order and group orders should sum up to the gross value.
    • transport costs are shown it there are some. The amount is split in group and FC amounts if a stock order exists.
  • in the invoice summary:
    • the numbers of orders (if > 1) and the numbers of deliveries (if > 0) are shown to make it clear why order and invoice amounts differ in these cases
    • the invoice balance is shown, replacing the not always correct order balance previously shown in the order summary
    • edit invoice is now a button, and a view invoice button is added.

For currency differences in balances, the helper function format_currency_difference() is introduced, showing negative values in red and positive values in green with an extra + sign.

If ordergroups are not charged with transport costs, the individual ordergroup transport costs were not actively set to 0. So if the ordergroups were charged before, the individual amounts are now cleared (before, they remained unchanged in this case).

@mjavurek
Copy link
Contributor Author

mjavurek commented Nov 21, 2024

order.profit was wrong for invoices with other orders and deliveries

There is still an issue with the order.profit() method, where order.profit = groups_sum - invoice.net_amount, and groups_sum is the sum of all group orders form this order. This is only correct if there is only one order and no delivery associated with the invoice. The resulting value is stored in order.foodcoop_result in order.close!() and displayed for each closed order in the finance/balancing list.

Example: if there are two orders

  • order 1: 10 €
  • order 2: 20 €
  • invoice for order 1 and 2: 30 €

The overall profit is 0. In contrast, order.profit is -20 € (= 10 € - 30 €) for order 1 and -10 € (= 20 € - 30 €) for order 2.

So basically, the overall profit should be split amongst all orders and deliveries. In my opinion, there is no perfect way to realize it, because it is generally an unsolvable problem without knowing details of the invoice about the parts of the invoice corresponding to the individual orders and deliveries. Example:

  • order 1: 10 €, part of invoice: 15 €: profit -5 €
  • order 2: 20 €, part of invoice: 20 €: profit: 0 €
  • invoice total: 35 €, overall profit: -5 €

Basically I see the following options (in brackets my estimation/opinion):

  1. divide the overall profit by the total number of orders:
    order.profit = invoice.profit / invoice.orders.count
    The number of deliveries is not considered in contrast to the number of orders because deliveries are not shown in the list of orders. Nevertheless, invoice.profit considers the amount of deliveries.
    (simple; all orders of the invoice have the same profit, which makes it more transparent that they belong together)
  2. split the overall profit according to the individual order or delivery amounts, like
    order.profit = invoice.profit / invoice.expected_amount * order.sum (more complicated, but still not more correct; looks more sophisticated and therefore implies that it is more correct, obscures that multiple orders of an invoice belong together)
  3. account the profit only to one order and set all other order and delivery profits to 0. This order could be e.g. the latest.
  4. account the invoice profit to all orders of the invoice; drawback: sum of order profits = number of invoice orders X invoice profit
  5. remove the display of an order profit, alternatively add profit to the invoice list (most correct solution)

I decided to implement nr. 1 as a compromise between a perfect solution and keeping changes small. Example for two orders per invoice, invoice surplus -1,18 € is divided equally on the 2 order surplus (each -0,59 €):

grafik

grafik

@mjavurek
Copy link
Contributor Author

@yksflip can you please have a look on this pull request and merge it?

@yksflip
Copy link
Member

yksflip commented Nov 25, 2024

Hey @mjavurek,

Thank you so much for your efforts! It's great to see your initiative on this one.

As you're probably aware, our development capacities are very limited at the moment. On the one hand, it's really great to have your contribution, but on the other hand, I can't promise to review and merge it very soon. That's why we established a roadmap and a monthly community call to organize the work (see Readme). If you can make it, it would be really cool to have you on the next call so we can discuss how to organize the work.

Recently, the Austrian team submitted #1073, which will require some time to review, and we want to try to limit the amount of conflicts. I can try to see if there will be conflicts with your contribution. If so, we might need to rework it to fit with it; if not, we might be able to get it through quickly.

Some general remarks: It would be great if you could squash your commits into only a few, more descriptive ones. That would make it easier to review already. Conventional Commits (conventionalcommits.org) might be a good starting point for this.

@mjavurek
Copy link
Contributor Author

mjavurek commented Nov 25, 2024

Thank you @yksflip for your response. I understand the difficulties, and I hope that despite of the difficulties, it will be possible to merge my pull request in the near future. I am quite sure that there will be conflicts in app/models/order.rb and app/models/invoice.rb, but it should be easy to resolve them.

I did not manage to squash my commits yet. I did all commits in the edit mode on the github website, unfortunately I am not familiar with Git CLI or Git Desktop, which seems to be necessary to be able to squash commits.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants