Openfoodnetwork: Make shipping_method.zones work correctly

[sauloperez]

https://github.com/openfoodfoundation/openfoodnetwork/pull/1627/files

• [ ] Document EnterpriseHelper#available_shipping_methods
• [ ] Document EnterpriseHelper#available_payment_methods

[luisramos0]

The first step was to remove within zone as in #1627 . I do that with new #2652

I dont think I understand the checklist above:

• Document EnterpriseHelper#available_shipping_methods
• Document EnterpriseHelper#available_payment_methods

There are two parts to this:

• I don't think filtering shipping methods by zone will be done in EnterpriseHelper (this will filter methods by enterprise), spree must be doing it somewhere
• make sure the original customization works (if it is supposed to work)
  
  
  
  • do shipping methods need to appear before shipping address is selected?
  
  
  • if so, make sure that it is working after the upgrade
  
  

[sauloperez]

I don't understand the checklist either :trollface: I guess these methods were not clear to me.

[sauloperez]

do shipping methods need to appear before shipping address is selected?

that must be due to our single-step checkout page. Shall we create an issue for it to make sure it keeps working? I think we can close this one afterward.

[sigmundpetersen]

I don't know if this one should have been closed, but the connected PRs were merged. @luisramos0 could you reopen and put it were it belongs if it's not to be closed?

[luisramos0]

The simple approach is to keep this open until we:

make sure the original customization works (if it is supposed to work)

• do shipping methods need to appear before shipping address is selected?
• if so, make sure that is working after the upgrade

[luisramos0]

@sigmundpetersen 👍 removing myself from the assignee as this can be taken by anyone at some point in the future.

[luisramos0]

one of the things we need to do regarding shipping_method zones is to always pass zone tests on pickup shipping_methods.

One of the zone validations done on shipping_methods is in shipphing_method.include?
We can decorate this method with something like this:

app/models/spree/shipping_method_decorator.rb

+  def include_with_pickup?(address)
+    return true unless require_ship_address
+    include_without_pickup?(address)
+  end
+  alias_method_chain :include?, :pickup

[sauloperez]

We purposefully wanted to get rid of this kind of metaprogramming as it's not explicit and it goes out of hand real soon. That's why we started refactoring our customizations with extension hooks like https://github.com/openfoodfoundation/spree/pull/6

You can read the reasoning behind decision in the meeting notes

[luisramos0]

fair enough, are you giving this one a try with hooks?

This really looks like a case for class extension (class eval or not), OfnShippingMethod extends SpreeShippingMethod and implements include? in a bit more specific way.

[luisramos0]

Moving this to the main upgrade epic as it is not really related to multi shipments.

[luisramos0]

SM - shipping method
I gave this a try in v1 and on checkout:

• if user address is outside the SM's zone and SM is pickup, checkout works
• if user address is outside the SM's zone and SM is delivery, the checkout will only work on the second try (second click on checkout).

In v2, currently without any adaptation, the checkout works for both cases.

[sauloperez]

• if user address is outside the SM's zone and SM is delivery, the checkout will only work on the second try (second click on checkout).

Any clue why? sounds like a bug

• In v2, currently without any adaptation, the checkout works for both cases.

I remember working on the #within_zone method very long ago.

[luisramos0]

-- any clue why?
must be because the within_zone method is working, it needs to be verified.

Because we have a single step checkout all SMs are shown and the validation between ship address and SM zone can only be done when the user clicks on the checkout button.

• Ideally, we would have a user message saying "SM is not valid for your address, please update address or change SM".
• And additionally, SM zones would be indicated as part of the SM selection in the checkout page.

There's some thinking to be done here, but the fact is that this is not top priority and it's not blocking anything else because checkout just works all the time now (even if SM zone is not valid).

[luisramos0]

I am moving this out of the upgrade to 2.04., this is a detail that should not block the v2 release.
We will need to come back to this if we want shipping methods to work only inside specific address zones.