verbb / postie

A Craft Commerce shipping calculator plugin.
Other
12 stars 18 forks source link

Unnecessary Shipping Rate Calculations #29

Closed keyurshah closed 4 years ago

keyurshah commented 4 years ago

Description

Hello,

It seems that when a customer is logged in, it takes longer to add to cart.

Upon doing some logging, it seems that a call to get shipping rates is done while adding to cart, instead of waiting for the checkout shipping page.

I'm using a custom provider, so not sure if I am doing something wrong or if this is the intended behavior.

If it is intended, can we change it somehow as it will result in many unnecessary api calls and delay key site functions (add to cart, change cart quantities)

Thanks!

Steps to reproduce

  1. Add an item to cart
  2. Checkout and register an address with the current cart
  3. Go back to a product page and add a item to cart

Additional info

engram-design commented 4 years ago

This is due to how Commerce handles shipping methods and providers. As soon as there is a valid shipping address set on the cart, it'll return the shipping methods available to the cart.

For instance, when the cart has a customer that has an address (not really a guest, as an address doesn't persist), it'll try and fetch all available shipping methods. This calls Postie which in turn tries to fetch live rates for your providers.

I suppose this is useful in the case where you might like to show shipping rates on the product page.

Its something I've tackled in the past, but didn't really find a good solution. One option was to limit rate-fetching to a specific URL route, but I'm not sure if this would be too constricting. Caching was introduced so that rates are only fetched when the cart contents or address changes, but this doesn't help with add-to-cart.

I'll have a bit more of an investigate.

keyurshah commented 4 years ago

thanks for the update. that's what I was worried about.

Ideally craft could provide an option during add to cart. If the option is true, then a shipping recalculation would be performed. Otherwise in all cases it seems to be a huge performance hit.

thank you for looking into this!

keyurshah commented 4 years ago

Hey Josh,

Hope you’re well. Any luck on this? Do we need to create an issue with Commerce?

Thanks!

engram-design commented 4 years ago

No further progress (I haven't had the time to work on this).

If I altered behaviour so that you needed to send a specific POST param in order to fetch live rates, would that work?

For instance, you would need to do:

<form method="POST">
    <input type="hidden" name="action" value="commerce/cart/update-cart">
    <input type="hidden" name="fetchLiveRates" value="true">
</form>

When you wanted to fetch live rates. It would only then actually fetch things. You'd add this during checkout.

It does depend on your site though?

keyurshah commented 4 years ago

Hey no worries, I think that would work for me.

So as an optional config, postie will check if fetchLiveRates is available and then only update the rates.

This would probably go on the prior page, for example the shipping address page?

engram-design commented 4 years ago

Correct - it would be an opt-in config function as well - and yes, depending on what your templates look like - if you use Ajax to fetch shipping rates, or just plain form submitting. For the latter I would include it on the previous form. You might also like to include a "Calculate shipping" button on the page, but totally your call.

keyurshah commented 4 years ago

that sounds great! would greatly appreciate that. thank you very much!

keyurshah commented 4 years ago

Can the getSignature be made public as well

https://github.com/verbb/postie/blob/4a66c9a179a3fd6e3fbd918990c108c9d510e496/src/base/Provider.php#L295

so i can use the calculation to determine if i should add the fetchLiveRates parameter.

Thanks

engram-design commented 4 years ago

I can make getSignature public, but you should arguably never need to call this, as its already called by the fetchRates function. Once rates have been fetched, and if the cart or address hasn't changed, it won't keep fetching from the APIs. You'll want to be careful rolling your own rate-checking functionality.

Check out 2.2.0 where I've introduced manual rates fetching. Refer to the docs for specifics.

As a bonus, I've also greatly improved the speed of fetching the initial rates from providers. Hopefully you should notice a different straight away, regardless of using manual fetching.

So you mentioned using getSignature to check if you need to call fetchLiveRates - you shouldn't need to do any of that. If its a custom provider, you should be extending the Provider class, and because its protected, you've got access to it anyway.

Hope that helps.

keyurshah commented 4 years ago

Thank you so much!! Will try it out and let you know how it goes.

keyurshah commented 4 years ago

It seems I'm having some trouble.

I added the parameter to the address page and then checked out.

When the user selected a shipping method, the shipping method was not being added to the order.

So I then added the parameter to the shipping method page, and then it added the shipping method to the order.

But when completing payment, it did not save the shipping method with the order.

If i add the parameter to the payment form, then I get the following error

Something changed with the order before payment, please review your order and submit payment again.

I had to disable the parameter for now to get shipping to save to the order properly. I am using a custom provider, but the base logic is similar to the other providers.

Not sure how to think this through and where the underlying problem is. It seems without the parameter, the shippingMethodHandle is not saving to the order / gets removed.

Also, when reloading the shipping page, there were no shipping options returned. I understand the reason behind that is that it needs to be a post request. In that case, I guess i can run an ajax update.

Thanks!

engram-design commented 4 years ago

Out of interest, are you noticing performance improvements when adding items to the cart, with the setting turned off?

I believe I know the issue - the functionality for manually checking rates was firing off too early before checking for cached rate. This is now fixed in 2.2.1

keyurshah commented 4 years ago

ok, i'll try that out. the requests do seem slightly faster though even with the setting turned off.

keyurshah commented 4 years ago

Hi there!

It's almost working, but there's one more issue.

when I try to display the shipping method name after placing the order,

{{ order.shippingMethod.name() }}

I get the following error:

Impossible to access an attribute ("name") on a null variable.

Craft CMS offers order.shippingMethodHandle, but it seems not possible to get further attributes for order.shippingMethod once the order has been placed.


Also, when trying to send an email out, I get an error that a session cannot be set. Probably because the email is sent via queue (console). I think this line

https://github.com/verbb/postie/blob/1800d38c2d08718d608343c32776159a4fefebb2/src/base/Provider.php#L250

needs to be checked to see if it not a console request: ! isConsoleRequest()

https://www.yiiframework.com/doc/api/2.0/yii-base-request#$isConsoleRequest-detail

and same when the session is removed here

https://github.com/verbb/postie/blob/d9d3256441994d5bd279487d5414c6a2fae2006e/src/services/Service.php#L41

engram-design commented 4 years ago

Hmmm, this is because Commerce relies on its getAvailableShippingMethods() which fetches all shipping methods. The issue here is that fetching the shipping method means we need to return a price, therefore - API calls.

I'm uncomfortable returning methods and making API calls, even if you have the manualFetchRates on, as having it turned off will result in more API calls.

To get around this, I've added a case for completed order, which should never need live shipping rates, which will get around your issue.

I've also fixed the session issues all in 2.2.3