Tax adjustments are not destroyed properly if there is more than one type of adjustable for relevant_items

[AdnanTheExcellent]

Using solidus 1.2.

I am having problem with taxes not destroying tax adjustments properly when Spree::TaxRate.adjust(order_tax_zone, items) is called and items consists of an array of different class types.

For example, our store taxes shipping (since we upcharge). if "items" input contained all our line items and shipments in the order, then this line of code fails:

https://github.com/solidusio/solidus/blob/v1.2/core/app/models/spree/tax_rate.rb#L90

Lets say my relevant items contains a LineItem and a Shipment and i call line 90, this is the SQL query that active record returns:

[16] pry(Spree::TaxRate)> Spree::Adjustment.where(adjustable: relevant_items)
  CACHE (0.1ms)  SELECT `spree_adjustments`.* FROM `spree_adjustments` WHERE `spree_adjustments`.**`adjustable_type` = 'Spree::LineItem'** AND `spree_adjustments`.`adjustable_id` IN (1071079807, 1056546687)

As you can see in the SQL query, it is only looking for line items. Thus, it only returns the tax adjustment for the line item. if i called this line instead:

[17] pry(Spree::TaxRate)> Spree::Adjustment.where(adjustable_id: relevant_items.map(&:id))
  CACHE (0.2ms)  SELECT `spree_adjustments`.* FROM `spree_adjustments` WHERE `spree_adjustments`.`adjustable_id` IN (1071079807, 1056546687)

then i get back the tax adjustments for the line item and the shipment.

[mamhoff]

Wow, that's a bug I didn't even know existed. This is fixed on the current master branch though.

[mamhoff]

The line you're proposing is good in most cases, but will destroy adjustments you might not want destroyed, unfortunately. (Example: You have a promo adjustment with ID 1 and a line item adjustment with ID 1, your code would destroy both...)

[AdnanTheExcellent]

it still needs the .tax.destroy_all at the end. the code i proposed was to just show the initial sql query before the .tax for fetching the adjustments and how the two were returning different results. With the .tax it shouldnt destroy any other type of adjustments.

If you changed line 90 to:

Spree::Adjustment.where(adjustable_id: relevant_items.map(&:id)).tax.destroy_all

it should work fine

[mamhoff]

That will still not be foolproof. You would possibly be deleting shipment
or line item tax adjustments on another order. You can provide a PR that
actually fixes that issue on Solidus 1.2 or upgrade to master.
Am 25.02.2016 20:58 schrieb "Adnan Abdulally" [email protected]:

it still needs the .tax.destroy_all at the end. the code i proposed was
to just show the initial sql query before the .tax for fetching the
adjustments

—
Reply to this email directly or view it on GitHub
#909 (comment).

[AdnanTheExcellent]

Ah ok, i see what you're saying now.

This should do the trick:

relevant_items.map(&:adjustments).map(&:tax).flatten.destroy_all

[mamhoff]

That would work. Please submit a pull request as it does fix a bug.
However, be aware that it also foregoes the efficiency of the query we had
before: now you create items.length SQL queries.
Am 25.02.2016 23:50 schrieb "Adnan Abdulally" [email protected]:

Ah ok, i see what you're saying now.

This should do the trick:

relevant_items.map(&:adjustments).map(&:tax).flatten.destroy_all

—
Reply to this email directly or view it on GitHub
#909 (comment).

[AdnanTheExcellent]

True, but master does the same thing now that they extracted the code to Spree::Tax::ItemAdjuster. It only does 1 object at a time so i would have to loop over each item and do items.length SQL queries.

https://github.com/solidusio/solidus/blob/master/core/app/models/spree/tax/item_adjuster.rb#L28

[mamhoff]

I think this has been resolved in both master and Solidus 1.2.