filter available shipping methods based on tags

[andrewpbrett]

What? Why?

Closes #5950

I was able to reproduce the issue by setting up a single shipping method for a shop and then tagging it "member". When checking out as a guest, the checkout screen showed no available shipping methods and the red error that @emilyjeanrogers circled in the issue's screenshot. Despite not selecting a shipping method, you can complete the checkout.

I stepped through the state machine transitions on checkout and noticed that after the transition to delivery, a shipping method was added to the order. I believe it was happening in the create_proposed_shipments method; that method calls the packages method which (eventually) uses the shipping methods method on each package.

Currently that method returns all the shipping methods for a distributor, even if there are tag rules that should apply. I looked at the available_shipping_methods method in EnterprisesHelper and used a similar approach to apply the appropriate tags.

With this PR, checkout now (correctly) fails with the error message: "We are unable to calculate shipping rates for the selected items." if a shipping method has not been selected.

While this first commit (93c1ea6) fixes the issue on the backend; there are a couple of additional improvements to consider:

• We could warn the shop owner when they create a tag rule that they should have an additional shipping method that's available to all customers (i.e. no tag rules on it), or require that all shops have a shipping method with no tags.
• We could have a better message on the front end. Currently the Shipping section of the checkout form is considered valid even if there's no shipping method selected, and the checkout page loads (with the ugly red error!) even if no shipping methods are injected. We could catch the error earlier and alert the shop owner that a customer wasn't able to check out because of tag rules.

Separately, in diving into this issue I found two other potential issues:

1. A similar thing can happen with payment methods - if there are rules that exclude all payment methods from showing up, a user can load the checkout page without knowing anything is wrong... they cannot actually checkout, however, so this would be a front-end improvement.

2. Deleting the last available shipping method for an enterprise doesn't warn you that it didn't work.

What should we test?

Checking out as a customer (guest or otherwise) at a shop where no shipping methods are available to you based on tag rules.

This PR also cleans up some code in Stock::Package and so we need to do some additional testing around that area. From Luis: We need to test everything related to shipping methods in the checkout process as well as shipping methods in the admin edit order page ... for example, checkout ship methods of different zones and categories. and change ship method in admin edit order page and verify the list of ship methods is correct.

Release notes

Fixed a bug where customers were assigned an incorrect shipping method at checkout if there were no shipping methods available based on tag rules.

Changelog Category: Fixed

Discourse thread

Dependencies

Documentation updates

[luisramos0]

Great catch Andy!
I think we must find a way to remove the copied logic from enterprise_helper. Otherwise we are making what's already badly structured a bit worse.

[luisramos0]

I am moving to in dev until feedback is addressed 👍

[andrewpbrett]

@luisramos0 this now uses DistributorShippingMethod service and the tests are back to green. I have a few things left on the todo list since there's a lot of potential for cleanup/refactoring here per the Boy Scout rule:

• Right now there's a parameter for whether we want to use the display_on_checkout scope; this could potentially be removed? Need to understand better what that scope is doing and where it's used.
• Look into removing shipping_categories from package.rb; it's currently referenced in one spec (package_spec.rb:161), but it doesn't make sense to me why we'd need it in a method.
• Functional spec to test that if no shipping method is selected, the order is not created. I opted for unit tests on the DistributorShippingMethods service instead.

[luisramos0]

nice, looking good.

• display_on_checkout is needed there, it's an option in the methods to hide them from the checkout page. you can leave it as is. looks good to me.
• I think shipping_categories can be removed and that sepc (the build will also tell us if it's ok to remove it 👍 )

I think we need to extend the testing notes of the PR to include tests validating the changes in package.rb - I'd say we need to test everything related to shipping methods in the checkout process as well as shipping methods in the admin edit order page ... for example, checkout ship methods of different zones and categories. and change ship method in admin edit order page and verify the list of ship methods is correct.

[andrewpbrett]

That one spec that's failing is passing locally for me, I think rebuilding will get it back to green :)

[Matt-Yorkley]

Nice 😄

[filipefurtad0]

Hey @andrewpbrett ,

great catch indeed. Thanks for reproducing this error. I've done this as well in two different ways:

1. by using a tag to hide the all shipping methods set for a given shop:

Editing the order in the backoffice shows that the hidden (and only) payment method was indeed being used for processing the order:

2. by setting all shipping methods as "backoffice only". This is a different issue, since it does not really depend on tags. It seems related, as the error in checkout page is the same: {{ fieldErrors('order.shipping_method_id') }}. So I'll report here for now, opening an issue (should it persist after this PR). In this case, checkout is possible as well. However, looking at the order (as admin) reveals that no shipping method was selected, despite successful checkout:

In this situation, and despite the methods being set to backofice, it does not seem possible to select one. Trying to ship this order leads to an error:

This also happens for orders placed in the backoffice: in summary, setting all shipping methods as backoffice only seems to hide all of them, from the backoffice as well. Maybe this was introduced by #5392. If one enters customer details first and then chooses the product, then the shipping methods appear. This was quite tricky to track down, but it's indeed the current behavior.

After staging your PR, both these scenarios (1, and 2) were checked and a general sanity test around shipping methods functionality (3).

1. if all shipping methods are hidden using tags, the error is still visible on the checkout page, but checkout is not possible:

All good here - awesome! 🎉

2. Checkout appears to be still possible if all shipping methods as set "backoffice only" and thus hidden from /checkout. In this case, the order cannot be shipped as the shipping address cannot be set in the backoffice:

But again: skipping the "Order Details" and filling out "Customer Details" first, allows to access all payment methods set to "Backoffice" only.

3. Sanity check
   i) checkout process was verified. Nothing unusual, shipping methods appear and have the associated fees with them. Checking out proceed as expected.

ii) checkout shipping methods of different zones and categories - found no changes, I think we are good here. The category of the product can differ from the shipping category; the zone of the shipment can differ as well from the zone of delivery.

iii) change ship method in admin edit order page and verify the list of ship methods is correct
The list in the backoffice now appears to take tags into account now. So, if a shipping method is hidden from /checkout for a given customer using tags, then this particular shipping method does not appear in the backoffice, when placing an order. I'm pretty sure this is new, I checked on master branch as well. So, usually, hiding a shipping method from the /checkout does not hide it when placing backoffice orderd. I am wondering whether it is an intended change - requesting feedback on this one.

If it is in fact intended, then maybe we would need to change this wording, as it currently only refers to "checkout":

The PR is good to go, moving to Ready to Go.

For documentations sake and as a summary, I'm adding further notes, as there are issues which should to be opened, from this testing. I point out that these are not introduced by this PR:

• found that issue concerning creating a backoffice order, details see here
• cannot delete shipping methods used in previous orders (this refers perhaps to the point 2. you mention on your notes @andrewpbrett) : if a shippming method is freshly created but never used on checkout/orders then deletion works (trash-bin logo, on admin/shipping_methods). Used shipping methods cannot be deleted in this way.
• setting all shipping methods as "backoffice only" hides all of them when placing backoffice orders as well; if all but one shipping methods are set to "backoffice only", then they appear as usual.

[luisramos0]

if a shipping method is hidden from /checkout for a given customer using tags, then this particular shipping method does not appear in the backoffice, when placing an order.

yes Filipe, I didnt think about that case, with the change in Package done here:

The admin order edit page will not allow the manager to add ship methods that the customer doesn't have access to through tags... this is a loss of functionality... and I think it's a bug. Even if there are tags hiding the ship method from checkout, the ship method should appear on admin order edit page so that the manager, if they want, can override the tag rules...

It's a product decision to accept this as is or to prevent this PR from being merged as is. I am not sure where to move this PR to but I want to move it away from ready to go to avoid someone accidentally just merging it... let's keep it in Test Ready until this decision is made? I think @RachL or @lin-d-hop would be able to make a call here?

In terms of solution for it, the view in admin orders uses order.shipment.shipping_rates.backend, but with this PR this is now getting the tag rules filters. The code that builds the ship rates is the Package code that is inside the Order workflow and that is the same code used in the checkout part that we just fixed by adding the tag rules... we need to split this and make it work differently for frontoffice and backoffice. It's not obvious but there must be an easy way out of it.

[RachL]

I guess the workaround is for the admin to tag themselves? Correct?

[lin-d-hop]

Oof this is a tough one.

Very impressed by the whole team on this issue! 😍

To be sure I understand:
Currently:
an enterprise can add ANY shipping method to a shopper when they place an order via the backoffice
With this change:
an enterprise will not be able to add a shipping method to a customer if the customer is not tagged with the same tag as the shipping method.

Will the instance manager (super admin) be able to add the shipping method? Or no?

I'm trying to consider support load in thinking about this. The case that we are resolving is reasonably rare and essentially protecting against user error (if the user sets up no shipping methods that are available by default). I estimate that the case that we are creating will create a bigger support load than we are solving if users lose flexibility over shipping methods.

For that reason I would lean towards NOT merging as is. I don't feel strongly about this and would accept if others did feel this should be merged.

I hate to throw a spanner in the works - but would a more appropriate solution to this issue to be a check when adding tags that warns the hub if they create tag conditions that result in no shipping/payment methods available by default? I haven't thought this through yet. It might be terribly complex 🤔

[andrewpbrett]

@lin-d-hop your understanding is correct. Even a superadmin would be not be able to add the shipping method (I'm pretty sure). I also think your point about the frequency of these cases is a really good one. I'd also lean toward not merging as it is.

I have a potential solution in the works that would make all the shipping methods available to the backend and still apply the filters correctly on the frontend. Not sure exactly when it'll be ready to push up and have others take a look, but I'd say we can pause more discussion until it's there.

PS I did think about adding a validation or warning the admin when they're adding tags that they should/must have one shipping method that has no tag rules on it, but it seemed fairly complicated (and potentially confusing/annoying to people) so I wrote it as a "potential further enhancement" wayyy back up there in the original PR description. It would be really nice to do this but we might want some design thinking/inception around it.

[andrewpbrett]

Hrm, I may have messed that up, it's now showing all the commits from master as part of this PR.... uno momento.

Edit: all good now. Sorry about the force push :)

[luisramos0]

This is a good try but:

• I think it's too complicated, I dont think we need so much flexibility in so many methods (see comment about the possiblity to remove args from refresh_rates).
• I think it's incorrect in some details (see comment about some default values).

I must confess this is not straight forward and I did burn a few neurons processing this PR :-)
So, well done Andy for taking care of this!

[luisramos0]

I dont think this is correct, this step is also executed when you create an order in the backoffice and so we would want these checkout/tags args to be false in that case...

[andrewpbrett]

Ah okay that makes sense - I think I was focused on the case where we wanted to be able to change the shipping method on an existing order in the backend and have all methods available... forgot about creating one. I think we can still make it work, but definitely trickier.

[andrewpbrett]

I think this commit a36c2bf should address this.

[sauloperez]

At this point the shipping_methods is already an Array because the ActiveRecord::Relation fired a DB query on line 9 due to to_a. I'd be much better if we asked to remove duplicates to PostgreSQL which is much faster than asking Ruby to do it (and instantiate a bunch of bloated ActiveModel objects that are meant to be discarded). We could do that by using #distinct.

That, unfortunately, will only work when apply_tags == false when true shipping_methods will be an array at this point (making TagRuleApplication rely on AR relation is totally different topic) so we'll need to nest it within an else branch. I think it's worth it.

[mkllnk]

I'm really sorry to delay this further but I think that it still contains a regression, see below. I'm moving it to in-dev until the questions are resolved.

[mkllnk]

I'm not sure that this is correct. If I'm a super admin I see all shipping methods in the admin interface and in the checkout even if I shouldn't based on my current tags, right? That makes it impossible to test tags as admin user. It's also not good if you can't tell which methods another enterprise wants you to use. So this is a problem when a super admin account is also used for shopping.

This also doesn't allow enterprise users to add any shipping method of their own enterprise to orders created in the backoffice. I think that's what @lin-d-hop was after.

I would say:

• The front-office always applies filters to shipping methods.
• The back-office does not apply filters to shipping methods.

So this logic needs to be triggered by the controller. The model is not the right place for this logic. Spree's design decision to include all checkout logic in a state machine is making it very difficult here. It's not pretty but we can move this out of the model into the OrderWorkflow service. It already contains logic concerning shipping method selection and it would make sense to have it in one place. It can then be called with different options from the frontend and backend.

What do you think?

[mkllnk]

Really minor but I thought it might be worth sharing that I would found this easier to read like this:

Suggested change

	DistributorShippingMethods.shipping_methods(
	distributor: self, checkout: true, apply_tags: false
	).any? && payment_methods.available.any?
	checkout_shipping_methods = DistributorShippingMethods.shipping_methods(
	distributor: self, checkout: true, apply_tags: false
	)
	checkout_shipping_methods.any? && payment_methods.available.any?

[RachL]

@andrewpbrett do you still plan on finishing this PR? Otherwise I would close it and move the issue to all the things. It doesn't look like a bug prioritised?