Remove tax refunds

[mamhoff]

This takes inspiration from #2194 - if we can remove that migratory task because we assume people have run it when they were on 1.3 or 1.4, we must also be able to remove all the code paths around tax "refunds" that are only run when a default tax zone is present.

For a bit of background: In versions of Solidus pre-1.3, a "default tax zone" existed. If that zone had a VAT-style tax rate, all prices in the system were assumed to include VAT, making it necessary to refund that via a Franken-adjustment.

We have sinced moved to a system where we have country-dependent prices, and the default tax zone doesn't exist anymore or mean anything.

I'll rebase on master once #2194 is merged.

[jasonfb]

I'm really curious, what's a "Franken-adjustment"? I couldn't find that term on Google (but lots of references to American Senator Al Franken and tax reform). Is than an accounting term?

Or do you just mean like "frankenstein," like a "big messy thing?"

[mamhoff]

jasonfb: It's a reference to Frankenstein the movie, and yeah, it's a messy thing. It's an included tax adjustment (which by definition does not change the line item's amount) with a negative amount, which - because of its negative amount - is counted as additional, changing the line item's amount. If that sounds confusing to you, that's because it is.

[jhawthorn]

Is this going to do the right thing with this removed? If so, the assignment on the previous line should be removed to make it more obvious that it is just being returned.

[mamhoff]

Yeah, it will. And also, yes the assignment is unnecessary. I'm not sure we need to ship this entire method any longer, either, as it we don't do taxation on orders since Spree 2.2.

[mamhoff]

Amended!

[tvdeyen]

Could we reduce the number of methods here or are these semi-public interfaces? In terms of private, but widely used by extensions/shops?

[mamhoff]

We can reduce them. I chose to keep applicable_rates, because that's what's being used in calculators, and remove rates_for_order. Thank you!

[tvdeyen]

Thanks, but I think we can remove even more.

applicable_rates is never called outside of this helper. The calculators only use rates_for_item.

If we also move sum_of_included_tax_rates into Spree::Calculator::DefaultTax (the only place where this is called) we can reduce the module to:

module Spree
  module Tax
    module TaxHelpers
      private

      def rates_for_item(item)
        rates = Spree::TaxRate.for_address(item.order.tax_address)
        rates.select do |rate|
          rate.tax_categories.map(&:id).include?(item.tax_category_id)
        end
      end
    end
  end
end

WDYT?

[tvdeyen]

And maybe we should rename the method to something meaningful then

tax_rates_for(item) maybe?

[mamhoff]

I did not realize the module is only called via rates_for_item by now, good catch! However, I did not change the name of the method. While it's not grammatically perfect, it conveys what the method does, and this way I don't have to change the calling code.

I kept the ivar caching, but removed the two superfluous methods.

[tvdeyen]

Perfect! Thanks

[cbrunsdon]

Looks good to me, looks like all the code removed isn't anything we'd ever want people to run. (aka the old tax system)