Tax Categories on Line Items respect updates to Variant and Product Tax Categories

[harmonymjb]

Summary

This PR was initiated in response to this reported issue:
Fixes #4683

We discovered that the LineItem's tax_category_id is currently being set in a before_validation in the LineItem model.
This means that the tax_category_id on the LineItem is set only once, when the LineItem is validated on the first save.

Upon investigation, we found that the tax_category_id was added on LineItems for performance reasons long in Spree's past:
spree/spree#3481 (This PR doesn't display the correct commits, but in the comments the associated commits are linked)

We wanted to be cognizent of performance here, but additionally attempt to fix the issue, so benchmarks and methodology are included below.

Fixing the issue

The underlying issue here has to do with setting the tax_category_id only once when the LineItem is created.
This means that changing the Tax Category on a Variant or Product will only have an effect on new Line Items created after the change.
If the associated Tax Category is soft deleted, the Line Item will be considered by the adjuster to have no Tax Category, no matter which Tax Categories exist on the associated Variant or Product.

To solve this issue, we decided to ensure that the Variant or Product's Tax Categories are considered when computing taxes.
The tax_category_id on the LineItem model still exists, and it's value is set at the same time that adjustments are created. This means that the tax_category_id that is present on a LineItem
always points to the Tax Category that was used to create the tax adjustments that exist for the LineItem.
If the Variant's Tax Category is changed, the next time tax adjustments are computed for the LineItem (i.e. during recalculate), the LineItems tax_category_id will be updated to reflect the
new relevant Tax Category.

If the Tax Category associated with some LineItems is soft-deleted, taxes will not be calculated based on this soft-deleted Tax Category, but will defer to the variant_tax_category_id.
On the other hand if taxes are not recalculated for that LineItem, the LineItem's tax_category_id will still point to that record and will represent the Tax Category that was used when taxes were calculated. These records can then be retrieved with .with_discarded if needed.

Benchmarking

I made temporary changes to our order_updater_spec tax_recalculation context:

        before do
          50.times do
            create(:line_item, order:, variant: create(:variant, tax_category:), price: 10)
          end
        end

        it 'benchmarks recalculate' do
          Benchmark.bm { |x| x.report { 50.times { order.recalculate } } }
        end 

And I ran the test 10 times before and after the changes:

On main:

    user     system      total        real
    1.519667   0.015154   1.534821 (  1.633398)
    1.539250   0.014212   1.553462 (  1.656476)
    1.486230   0.023042   1.509272 (  1.609028)
    1.496671   0.024544   1.521215 (  1.621268)
    1.517082   0.018463   1.535545 (  1.619166)
    1.515210   0.019308   1.534518 (  1.605963)
    1.496039   0.024313   1.520352 (  1.614227)
    1.498017   0.026294   1.524311 (  1.621422)
    1.522568   0.018466   1.541034 (  1.685498)
    1.474430   0.020689   1.495119 (  1.591637)
    avg real (  1.625808)
    avg real per recalculate (  0.032516)

After Changes (this branch):

   user     system      total        real
   1.598846   0.020576   1.619422 (  1.725181)
   1.575519   0.020300   1.595819 (  1.700659)
   1.584723   0.020872   1.605595 (  1.710245)
   1.607924   0.014587   1.622511 (  1.698217)
   1.556669   0.025032   1.581701 (  1.684801)
   1.581550   0.025173   1.606723 (  1.714748)
   1.608484   0.018360   1.626844 (  1.731001)
   1.596435   0.020785   1.617220 (  1.723850)
   1.610086   0.022443   1.632529 (  1.742005)
   1.606265   0.032092   1.638357 (  1.726858)
   avg real (  1.715757)
   avg real per recalculate (  0.034315)

An average increase of 1.799ms per recalculate call on orders with 50 Line Items.

Checklist

Check out our PR guidelines for more details.

The following are mandatory for all PRs:

• I have written a thorough PR description.
• I have kept my commits small and atomic.
• I have used clear, explanatory commit messages.

The following are not always needed:

• 📖 I have updated the README to account for my changes.
• 📑 I have documented new code with YARD.
• 🛣️ I have opened a PR to update the guides.
• ✅ I have added automated tests to cover my changes.
• 📸 I have attached screenshots to demo visual changes.

[jarednorman]

Not every day that you see a correct use of try. I'd consider adding a comment (saying basically what you say in the PR). As time goes to infinity, somebody is eventually going to think this was a mistake.

[tvdeyen]

Indeed and mentioned in the commit message as well. Really well done @harmonymjb and @adammathys

[kennyadsl]

Thanks, great PR!