charge transport cost as well when accounting order (fix #861 after f7c7b56)

[twothreenine]

for me it worked now

Note: I think it could be useful to check if price != 0 before creating a transaction (to avoid empty transactions) but I left it out since this should be fixed quickly

[paroga]

can you add an unit test for it to spec/models/order_spec.rb?

[twothreenine]

sorry, I have no idea how that should look like (and about writing tests in general)

[paroga]

The idea of test is, that the test runner (in our case rspec) executes the code and checks if it results in the correct results. In your case it order_spec.rb sounds like a good place for the tests. To test your change you would need to create an order (e.g. let(:order) { create(:order, article_count: 4) }), user, ordergroup and grouporder. Then you would need to add transport costs to the (group)order and after closing the order (e.g. order.close!()) you need to check if created financial transaction of the corresponding ordergroup have been charged with the correct amount (including the transport costs).

[twothreenine]

Here's my first go at it.
I ran it a couple of times with randomized numbers and discovered that it fails for some. Then I ran it specifically for those numbers again (without any randomized values in the test itself) multiple times and weirdly, it sometimes succeeded and sometimes failed.

... bundle exec rspec spec/models/order_spec.rb -e "balancing"
transport: 5.84
price: 15.54
quantity: 17
tax_percent: 16
tax: 1.16
markup: 1.05
to be charged: -327.65
Run options: include {:full_description=>/balancing/}

Randomized with seed 38267
difference: 0.0
.

Finished in 31.7 seconds (files took 1 minute 41.4 seconds to load)
1 example, 0 failures

... bundle exec rspec spec/models/order_spec.rb -e "balancing"
transport: 5.84
price: 15.54
quantity: 17
tax_percent: 16
tax: 1.16
markup: 1.05
to be charged: -327.65
Run options: include {:full_description=>/balancing/}

Randomized with seed 35375
F

Failures:

  1) Order balancing charges correct amounts correct transactions created
     Failure/Error: expect(difference).to be <= 0.001

       expected: <= 0.001
            got:    0.2482e2
     # ./spec/models/order_spec.rb:193:in `block (3 levels) in <top (required)>'

Finished in 32.54 seconds (files took 1 minute 40.39 seconds to load)
1 example, 1 failure

Failed examples:

rspec ./spec/models/order_spec.rb:190 # Order balancing charges correct amounts correct transactions created

The difference was 0.2482e2 = 24.82, so not just a rounding issue ...

[paroga]

i have no clear answer why the test failed.

anyway: usually a test should only test one single point at a time. here we just want to make sure that the amount of the financial transaction is the same as the total of the group order. how total is calculated is not important here. checking that total returns the correct value should be done in a separate test (in group_order_spec.rb).

[paroga]

i think this file is not necessary

[twothreenine]

ok, since you wrote "including the transport costs," I thought the transaction amount should include at least one article's price, not just the transport cost.
Then I think we should rename this test to "balancing charges transport cost" or something like that

[paroga]

create :group_order_article, group_order: go, order_article: oa, quantity: 1 should create an article in the group order

[twothreenine]

but it doesn't have a price, so only the transport cost is charged

[paroga]

the price should be created via the factory at

foodsoft/spec/factories/article.rb

Line 6 in 007ff70

price { rand(0.1..26.0).round(2) }

you can see that the article has an price e.g. by printing it with puts "Price: #{oa.price.price}" in the before

[twothreenine]

I compared transport and total like this, they were the same (my change to transport should not have altered it? Somehow I could not print it otherwise)

[paroga]

now there should be an article in the result too, maybe @wvengen knows if there are already some fitting test helpers for that (to avoid the ugly code in before)?

[twothreenine]

Would you mind merging this for now and adding more tests later, if necessary?
I think it's urgent to fix this and, if possible, notify deployments/foodcoops who might be affected, so they can charge the missing transport costs manually. (I only noticed this by pure chance)