What happened here is that we had recently migrate...
# supportt
Tom Wilson
10/19/2022, 7:22 PMWhat happened here is that we had recently migrated our tax categories. And it turns out that even if you update the tax category on a variant, it seems like the line item may continue to refer to the old tax category. That's probably desirable for many reasons, but in our case, we deleted the old tax categories, which means that tax calculation was silently failing. Maybe there needs to be some additional validation?
c
Chris Todorov
10/19/2022, 9:52 PMI am curious about what you’re using for tax calculations, is it an external service through a Solidus extension, or the Solidus tax system. It’s unfortunate that this failed silently during the returns process.
t
Tom Wilson
10/19/2022, 9:54 PMThis is using Avatax via
solidus_avatax_certifiedc
Chris Todorov
10/19/2022, 10:04 PMYeah, that makes sense in some way, I would take a look at the extension and see if that is a feature in the way it works or possibly the Avatax API is not returning taxes because of the missing category.
Chris Todorov
10/19/2022, 10:05 PMIt’s possible that the bug here is that we are allowing hard-deletion of categories that are already referenced by line items. Maybe those need to be soft deleted or we need to prevent deletion altogether.
Chris Todorov
10/19/2022, 10:06 PMI am curious though, did the line items end up with a
nulltax_category_idt
Tom Wilson
10/19/2022, 10:06 PMIt doesn't even end up getting into Avatax or the extension - I think it's in solidus core. One sec, let me see if I can find the line again.
Tom Wilson
10/19/2022, 10:07 PMNope, they have a
tax_category_id👍🏼 1
Tom Wilson
10/19/2022, 10:08 PMThis line ends up returning no rates: https://github.com/solidusio/solidus/blob/425c82f2e4716d01e4058f447ee9539ec5ab8dde/core/app/models/spree/tax/tax_helpers.rb#L11
Tom Wilson
10/19/2022, 10:08 PMWhich makes sense, because no tax category exists that refers to these soft-deleted categories.
Tom Wilson
10/19/2022, 10:09 PMSo I think maybe the bug is that we shouldn't be able to soft-delete the category while line items reference it?
c
Chris Todorov
10/19/2022, 10:10 PMYeah, that’s one option for sure.
Chris Todorov
10/19/2022, 10:11 PMI think this is worth opening an issue for at least to get some understanding in what the suggested process for replacing tax categories is.
Chris Todorov
10/19/2022, 10:11 PMUn-scoping the records to include soft-deleted ones might not be the best option if you’ve already updated the tax rate to not reference the deleted categories.
t
Tom Wilson
10/19/2022, 10:12 PM👍 - I can open an issue in the repo with some details tomorrow! Thanks for the thoughts, @Chris Todorov!
👍🏼 1
k
kennyadsl
10/20/2022, 7:22 AM
What about creating a new tax rate instead of changing the tax category (from the soft-deleted to the new one) in the existing tax rate? If I get it correctly, that would have allowed the existing line item to still be connected with the right tax category that belongs to a tax rate. Of course, that might need to soft-delete the obsolete tax rate as well to work, otherwise there will be an extra tax rate when taxes are calculated + we’d need to be sure that completed orders can take into account soft-deleted tax categories and rates.
t
Tom Wilson
10/20/2022, 2:12 PMWe could have done that, perhaps, but we didn't. 🙂 I think my only point here is that while the behavior of storing the tax category in the line item makes perfect sense, it can create this unexpected edge case when deleting categories that maybe should have better validation.
Tom Wilson
10/20/2022, 2:21 PMFiled an issue as https://github.com/solidusio/solidus/issues/4683 for further discussion!
k
kennyadsl
10/24/2022, 7:24 AM
We could have done that, perhaps, but we didn’t.You are right, I was focused on how to prevent that in the future, but here we are still troubleshooting how to fix the issue that you are having because of this problem.