Allow orders with different shipping categories

[aitbw]

Description

This PR allows users to submit orders with items that belong to different shipping categories.

This issue is present from Solidus v2.6 through v2.8, as well as in master

Once merged:

• Closes Cannot order items with different shipping categories #2998
• Closes Unable to calculate shipping rates for the selected items #3315

See also:

• add inverse_of option to association solidusio-contrib/solidus_multi_domain#99
• Fix associations solidusio-contrib/solidus_multi_domain#105

Checklist:

• I have followed Pull Request guidelines
• I have added a detailed description into each commit message
• I have updated Guides and README accordingly to this change (if needed)
• I have added tests to cover this change (if needed)

[jacobherrington]

Nice specs. I think this is a good idea! Thanks @aitbw 👍

[kennyadsl]

Just left a question.

[kennyadsl]

What's the difference here? Shouldn't they return the same thing?

[aitbw]

They should; I checked the generated SQL on the sandbox and the output was the same for both calls, but somehow, the result is not the same. (at least, not in this scenario.)

I will investigate further to understand what's going on here.

[aitbw]

@kennyadsl I added this:

puts store.shipping_method_ids.inspect

to the scope and found that store.shipping_method_ids is returning just one shipping method ID, whereas store.shipping_methods.ids is returning all the IDs for the shipping methods associated to the store.

Can't understand why this is happening, as the generated SQL is exactly the same for both, and some unit tests I created for this work as expected with either. Maybe there's a bug present in Rails (as mentioned first by @afdev82 in here) or something else we're missing out.

[kennyadsl]

Trying this out and it returns just one shipping method ID also for:

ActiveRecord::Base.uncached { store.shipping_method_ids } # => [3]

in the test you wrote.

Also, the query seems a bit too much for what it should do:

(0.6ms)
SELECT DISTINCT "spree_shipping_methods"."id"
FROM "spree_shipping_methods"
LEFT OUTER JOIN "spree_shipping_method_stock_locations" ON "spree_shipping_methods"."id" = "spree_shipping_method_stock_locations"."shipping_method_id"
INNER JOIN "spree_store_shipping_methods" ON "spree_shipping_methods"."id" = "spree_store_shipping_methods"."shipping_method_id"
WHERE "spree_shipping_methods"."deleted_at" IS NULL
  AND "spree_shipping_methods"."id" IN
    (SELECT "spree_shipping_methods"."id"
     FROM "spree_shipping_methods"
     INNER JOIN "spree_shipping_method_categories" ON "spree_shipping_method_categories"."shipping_method_id" = "spree_shipping_methods"."id"
     WHERE "spree_shipping_methods"."deleted_at" IS NULL
       AND "spree_shipping_method_categories"."shipping_category_id" = ?
     GROUP BY spree_shipping_methods.id
     HAVING COUNT(DISTINCT "spree_shipping_method_categories"."id") = 1)
  AND ("spree_shipping_methods"."available_to_all" = 1
       OR "spree_shipping_method_stock_locations"."stock_location_id" = 1)
  AND "spree_store_shipping_methods"."store_id" = ? [["shipping_category_id", 2],
      ["store_id", 1]]

There are some suspect join with categories, which I'm not sure why are there and if they are needed here. Can you please investigate a little bit more?

[aitbw]

Yeah sure, let me have a further look at it

[spaghetticode]

@aitbw 👋hi Angel, did you have any chance to continue working on this?

[aitbw]

@spaghetticode, hello Andrea! I've been trying to find out why my proposed change works, but with no avail so far 😓

[cedum]

This seems to be fixed in Rails 6 with this commit:
rails/rails@17f2f30#diff-c47e1c26ae8a3d486119e0cc91f40a30
By applying this patch to Rails 5.2 store.shipping_method_ids starts to behave as expected.

In a scenario with two products with different shipping categories, we get two calls to this scope for each product: the first one seems correct, the second one instead returns the cached result from the previous one.
I'm quite ignorant about ActiveRecord internals, also the scope chaining here is extreme to be able to understand the behavior easily, however, it should be safe accepting the solution in this PR.

[kennyadsl]

@cedum nice finding!

@aitbw 👋👋👋can you please give us (@cedum or me) access to your solidus fork so we can rebase this PR against master? Or if you prefer and/or have time, please do that by yourself.

[aitbw]

@kennyadsl, hi Alberto! 👋 Done! ✔️

Removed the first commit based on @cedum's comment, but I can add it again if necessary.

[cedum]

hey @aitbw ! Thank you 🙏
It would be nice to have also the second commit since it fixes this scope for Rails 5.2 (the commit from the rails repo I linked has been merged only in rails 6 unfortunately)

[aitbw]

Ooooh ok ok, got it! @cedum, sorry about that 😅

EDIT: Done! ✔️

[ericsaupe]

Sounds like this fixes #3315 as well if you wanted to put that in the description

[kennyadsl]

The fix for this does not seem to have been backported on Rails 5.2, so I suppose we have no other solutions than merge this PR. Thanks @aitbw and @cedum!