solidusio / solidus

🛒 Solidus, the open-source eCommerce framework for industry trailblazers.
https://solidus.io
Other
4.96k stars 1.28k forks source link

Tax categories that are still in use can be soft deleted, creating silent failures #4683

Open tmtrademarked opened 1 year ago

tmtrademarked commented 1 year ago

Tax categories which are still in use by products or line items are allowed to be deleted. This creates a silent failure mode when attempting to update the taxes for any order referring to these objects.

Solidus Version: 3.2.2

To Reproduce

  1. Create a tax category and associated rate, assign it to a product, and create an order with that product.
  2. Now create a new tax category, and update the rate to have only the new category and not the old.
  3. Delete the old tax category via the UI.
  4. Update taxes on the order, and notice that no taxes are calculated - and all existing taxes are cleared.

Current behavior The code which selects the rates for the line item will return no available rates: https://github.com/solidusio/solidus/blob/425c82f2e4716d01e4058f447ee9539ec5ab8dde/core/app/models/spree/tax/tax_helpers.rb#L11

This will result in the behavior in the bug where we clear the taxes on the order.

Expected behavior There's two things we could do to make this more obvious:

gsmendoza commented 1 year ago

Hi @tmtrademarked ! I tried to test this but I wasn't able to reproduce Step 4: Update taxes on the order. Please see the video below. Am I missing something?

https://www.loom.com/share/ec029cc36b39472d98308c8df9a7a1bb

tmtrademarked commented 1 year ago

Hey @gsmendoza - thanks for the detailed test video! That was super helpful. I think the missing piece here is that you have to cause taxes to be recalculated on the order. That's what I meant by "update taxes on the order".

So you can trigger this by starting a reimbursement for an item, for instance - or I think by simply doing order.recalculate in a rails console. At that point, it should cause the taxes to be zeroed out on the line item.

gsmendoza commented 1 year ago

@tmtrademarked I've done the following:

  1. Create a tax category and associated rate, assign it to a product, and create an order with that product.
  2. Now create a new tax category, and update the rate to have only the new category and not the old.
  3. Assign the product to the new category.
  4. Delete the old tax category via the UI.
  5. Update taxes on the order.

I've confirmed that even if I assigned the product to the new category before deleting the old category, the tax rate is no longer applied to the order.

I'll check with the team what should be the expected behavior for this issue :+1:

kennyadsl commented 1 year ago

@gsmendoza did you already bring this to the team's attention?

gsmendoza commented 1 year ago

@kennyadsl Yes, I brought it up in slack and created a ticket for it.