Clean up tax rate match logic

[mamhoff]

I've been complaining already in #656 about the absolutely undecipherable bit of code there that removes some tax rates from the matching tax rates. This commit removes Spree::TaxRate#potentially_applicable and instead creates two arrays of rates: One for the current order's tax zone and one for the default tax zone. Then removes rates from the default tax zone rate array if they have the same tax category as any in the order tax zone rate array.

Only at the end the two arrays are merged.

While this is still not correct behaviour, it's at least more understandable and shows the intention of the old behaviour. It should also work with Ruby 2.3 if I understand the new #delete_if behaviour correctly.

Some comments moved.

[jhawthorn]

These lines might read cleaner as

rates_for_order_zone = all_rates.select{|rate| rate.zone.contains?(order_tax_zone) }
rates_for_default_zone = all_rates.select{|rate| rate.default_vat? }

[mamhoff]

I know, but that would mean iterating twice over all the tax rates in the system, and running the expensive contains? call double as often.

[mamhoff]

Whoops, you're right. This way I just run the contains? call twice pre iteration, nothing gained.

[jhawthorn]

Thank you @mamhoff, this is fair more approachable and understandable.

My favourite thing about this is that the previous remaining_rates behaviour varied based on the order of the rates (which was somewhat random) and is now fixed to do the (more) right thing. So this isn't even really a behaviour change.

👍

[mamhoff]

Amended for better readability as mentioned. Thank you @jhawthorn!

[jhawthorn]

Note to future self: We'll have to make a change log entry for the removal of potentially_applicable.

Clarke says 👍