Include adjustments with negative taxes on Sales Tax reports #5492

pacodelaluna · 2020-05-24T10:01:16Z

What? Why?

On Sales Tax reports, with the Tax Rates filter, you can see that the tax is not managed when an adjustment is negative. This was because we were not including negative tax rate in the calculation, so I just removed the scope applying this filter.

What should we test?

Add a negative adjustment on an order. Then go on /admin/reports/sales_tax, select the Tax Rates filter, then control that you can see the negative amount for the tax.

Release notes

Deal with negative tax amounts for adjustments on Sales Tax reports

Changelog Category: Changed

pacodelaluna · 2020-05-24T11:53:01Z

app/models/spree/order_decorator.rb

-       Spree::Money.new(tax_amount, currency: currency)]
-    end]
-  end
-


This method is not used anywhere.

luisramos0

Thanks for the PR François!

It's great to see some of this code go 🎉 price_adjustment_totals

I am not sure if this change is correct. You may be adding zero value entries in some reports? Or even in the invoices where these methods are used?
I think this change requires at least new unit tests, ideally feature specs would cover the report and the order invoices (the 2 parts that use this code).

I have an idea: you could extract this tax_adjustment_totals and tax_adjustments methods to a separate service.
You could move these two methods to a service under app/services called OrderTaxAdjustments
with only one public method OrderTaxAdjustments.totals
The method tax_adjustments can be private.
You would need to update app/helpers/checkout_helper.rb and the sales tax report to do
OrderTaxAdjustments.new(order).totals
instead of
order.tax_adjustment_totals

This would enable you to copy the related specs in spec/models/spree/order_spec.rb to a separate service unit test file and add more tests for the new cases (zero included tax and negative included tax).

I hope this makes sense to you.

luisramos0 · 2020-05-25T12:51:10Z

app/models/spree/order_decorator.rb

  end

  def tax_adjustment_totals
    tax_adjustments.each_with_object({}) do |adjustment, hash|
+      # No need of dealing with a missing Tax Rate if no tax setup
+      next if adjustment.included_tax.zero?


included_tax means that there is no included tax in the adjustment amount but some adjustments may have tax that is not included!?
in that case you would need to go forward in the method and still use the tax that is not included?

luisramos0 · 2020-05-25T13:00:33Z

app/models/spree/order_decorator.rb

@@ -305,12 +305,14 @@ def total_tax
  end

  def tax_adjustments
-    adjustments.with_tax +
-      line_items.includes(:adjustments).map { |li| li.adjustments.with_tax }.flatten
+    adjustments + price_adjustments


if you want to include negatives you can change the scope to include negatives
scope :with_tax, -> { where('spree_adjustments.included_tax > 0') }

but here you are just including all adjustments, including the ones with zero tax. that means the method becomes non-sense where tax_adjustments returns all adjustments.

I think you want to keep the method as tax_adjustment but exclude included_tax ZERO.

Yes, you are right, I was not thinking of this case, and it will also solve the comment you put above for adjustment.included_tax.zero? control. I will fix this, need to cross-check side effect related to this scope use.

pacodelaluna · 2020-05-28T08:21:50Z

@luisramos0 Thanks for your feedback, I will take a look at it this weekend, extract the logic into a service makes sense yes.

pacodelaluna · 2020-06-09T21:54:14Z

@luisramos0 Sorry I was busy lately but I have refactored the code within a service. I have changed the scope to now include also the negative tax values, there will be a side effect but I think it is a desired effect. Tell me if any issue.

luisramos0

nice, I think this is correct 👍

sauloperez · 2020-06-10T14:45:09Z

app/services/order_tax_adjustments_fetcher.rb

+  def all
+    order.adjustments.with_tax +
+      order.line_items.includes(:adjustments).map { |li|
+        li.adjustments.with_tax


Is there a reason why we can't merge the AR relation at line 24 with this with_tax scope? otherwise, we'll be causing N+1 again. I bet we can fetch both order and line item adjustments in a single query, which would be the best in term of performance.

sauloperez · 2020-06-10T14:49:14Z

app/services/order_tax_adjustments_fetcher.rb

+  def totals
+    all.each_with_object({}) do |adjustment, hash|
+      tax_rates_hash = tax_rates_hash(adjustment)
+      hash.update(tax_rates_hash) { |_tax_rate, amount1, amount2| amount1 + amount2 }


I'm being picky here but I'd lean towards #merge instead of #update for readability purposes. They are synonyms, right? I didn't even know #update existed and I had to look it up to tell whether it belonged to AR or Ruby. Feel free to dismiss though.

sauloperez

I'm moving this to Test Ready but I'm already checking whether we are affecting performance too much. I don't know yet if we made it worse or not. Meanwhile, we can check the behavior is right and prepare a patch later if needed.

sauloperez · 2020-06-11T10:14:45Z

it doesn't look good. Using PROFILE and assets compiled:

from master:

from this branch:

pacodelaluna · 2020-06-11T11:10:19Z

@sauloperez Oh ok, I am a bit surprised about this performance issue as I thought I was mostly moving code in the PR. I will take a deeper look this weekend, I saw that there was an other service, OrderAdjustmentsFetcher, I initially didn't want to add too much refactoring but it seems we really need it...

sauloperez · 2020-06-11T11:20:37Z

I'm pretty sure the problem is scoped within OrderTaxAdjustmentsFetcher#all. Surely can be refactored to a single query starting from Spree:Adjustment.

EDIT I'm not sure those numbers are totally accurate because I can't reproduce them exactly now but there's definitely an N+1 there.

sauloperez · 2020-06-12T11:36:09Z

@pacodelaluna I gave it a try and added a couple of commits. Take a look at them and let me know what you think. They remove the N+1 and effectively fetch the adjustments in one shot.

There is at least one more N+1 related to adjustments in this report but this PR brings already a better world. It'll be easier to tackle once this is shipped.

pacodelaluna · 2020-06-12T12:03:51Z

@sauloperez Great job! Thanks for your help on this!

luisramos0

nice one.

sigmundpetersen · 2020-06-16T11:51:49Z

The conflict on this branch needs to be resolved before testing. A rebase would be good?

pacodelaluna · 2020-06-16T12:02:52Z

@sigmundpetersen Just did the rebase, should be fine for testing.

luisramos0

yeah, the problem was that price_adjustments disappeared in spree 2.1
you could replace price_adjustments with the new order.line_item_adjustments
but that's out of scope here.

filipefurtad0 · 2020-06-16T18:34:40Z

Hey @pacodelaluna!

I checked/reproduced what @RachL described in the issue. So, placed order R006812641, which can be seen in Tax Rates report:

Below, the same order, after adjustment with the amount £-20.00 (£-3.33 included tax):

After this PR the report is rendered with the correct values:

Awesome, thanks Paco 🎉
Ready to go.

PS: I found that clicking on that order-hyperlink leads to /admin/orders/R006812641 -> which is neither related to this PR nor to the upgrade - checked on staging-es as well. Opening a separate issue.

pacodelaluna changed the title ~~Include adjustments without positive taxes~~ Include adjustments with negative taxes on Sales Tax reports May 24, 2020

pacodelaluna commented May 24, 2020

View reviewed changes

sigmundpetersen assigned pacodelaluna May 25, 2020

luisramos0 reviewed May 25, 2020

View reviewed changes

luisramos0 approved these changes Jun 9, 2020

View reviewed changes

sauloperez suggested changes Jun 10, 2020

View reviewed changes

sauloperez approved these changes Jun 11, 2020

View reviewed changes

sauloperez self-assigned this Jun 11, 2020

luisramos0 approved these changes Jun 12, 2020

View reviewed changes

filipefurtad0 mentioned this pull request Jun 16, 2020

Remove N+1 fetching payments in report #5595

Merged

François Turbelin and others added 5 commits June 16, 2020 14:01

Include adjustments without positive taxes

c729f64

Use OrderTaxAdjustmentsFetcher service

9abe41f

Cosmetics

b4952dc

Remove N+1 fetching order and li adjustments

00c9dd7

Refactor with Arel to perform a single query

ff4d7fb

pacodelaluna force-pushed the include-negative-tax-adjustments-in-sales-report branch from 6b8e278 to ff4d7fb Compare June 16, 2020 12:02

Readd price_adustments after bad rebase

6672cfd

luisramos0 approved these changes Jun 16, 2020

View reviewed changes

filipefurtad0 added the pr-staged-uk staging.openfoodnetwork.org.uk label Jun 16, 2020

filipefurtad0 self-assigned this Jun 16, 2020

filipefurtad0 removed the pr-staged-uk staging.openfoodnetwork.org.uk label Jun 16, 2020

luisramos0 merged commit 01e27dd into openfoodfoundation:master Jun 16, 2020

luisramos0 mentioned this pull request Jun 17, 2020

Move all Calculators from spree to OFN and out of the Spree namespace #5613

Merged

pacodelaluna deleted the include-negative-tax-adjustments-in-sales-report branch March 21, 2021 17:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Include adjustments with negative taxes on Sales Tax reports #5492

Include adjustments with negative taxes on Sales Tax reports #5492

pacodelaluna commented May 24, 2020

pacodelaluna May 24, 2020

luisramos0 left a comment •

edited

Loading

luisramos0 May 25, 2020

luisramos0 May 25, 2020 •

edited

Loading

pacodelaluna May 28, 2020

pacodelaluna commented May 28, 2020

pacodelaluna commented Jun 9, 2020

luisramos0 left a comment

sauloperez Jun 10, 2020

sauloperez Jun 10, 2020

sauloperez left a comment •

edited

Loading

sauloperez commented Jun 11, 2020 •

edited

Loading

pacodelaluna commented Jun 11, 2020 •

edited

Loading

sauloperez commented Jun 11, 2020 •

edited

Loading

sauloperez commented Jun 12, 2020 •

edited

Loading

pacodelaluna commented Jun 12, 2020

luisramos0 left a comment

sigmundpetersen commented Jun 16, 2020

pacodelaluna commented Jun 16, 2020

luisramos0 left a comment

filipefurtad0 commented Jun 16, 2020

Include adjustments with negative taxes on Sales Tax reports #5492

Include adjustments with negative taxes on Sales Tax reports #5492

Conversation

pacodelaluna commented May 24, 2020

What? Why?

What should we test?

Release notes

pacodelaluna May 24, 2020

Choose a reason for hiding this comment

luisramos0 left a comment • edited Loading

Choose a reason for hiding this comment

luisramos0 May 25, 2020

Choose a reason for hiding this comment

luisramos0 May 25, 2020 • edited Loading

Choose a reason for hiding this comment

pacodelaluna May 28, 2020

Choose a reason for hiding this comment

pacodelaluna commented May 28, 2020

pacodelaluna commented Jun 9, 2020

luisramos0 left a comment

Choose a reason for hiding this comment

sauloperez Jun 10, 2020

Choose a reason for hiding this comment

sauloperez Jun 10, 2020

Choose a reason for hiding this comment

sauloperez left a comment • edited Loading

Choose a reason for hiding this comment

sauloperez commented Jun 11, 2020 • edited Loading

pacodelaluna commented Jun 11, 2020 • edited Loading

sauloperez commented Jun 11, 2020 • edited Loading

sauloperez commented Jun 12, 2020 • edited Loading

pacodelaluna commented Jun 12, 2020

luisramos0 left a comment

Choose a reason for hiding this comment

sigmundpetersen commented Jun 16, 2020

pacodelaluna commented Jun 16, 2020

luisramos0 left a comment

Choose a reason for hiding this comment

filipefurtad0 commented Jun 16, 2020

luisramos0 left a comment •

edited

Loading

luisramos0 May 25, 2020 •

edited

Loading

sauloperez left a comment •

edited

Loading

sauloperez commented Jun 11, 2020 •

edited

Loading

pacodelaluna commented Jun 11, 2020 •

edited

Loading

sauloperez commented Jun 11, 2020 •

edited

Loading

sauloperez commented Jun 12, 2020 •

edited

Loading