shopinvader / odoo-shopinvader

Odoo Modules. Sorry Magento, Shopinvader is coming
GNU Affero General Public License v3.0
121 stars 104 forks source link

Extremely slow performance when switching shipping methods (Redundant rate_shipment requests) #896

Closed dcoleman-lulzbot closed 10 months ago

dcoleman-lulzbot commented 3 years ago

We have been using ShopInvader in production for over a month and we have noticed that we are consistently getting long wait times when switching between different shipping methods in the checkout process. (I should mention this is in the context of a website that is based on shopinvader-template)

From my observation this is caused by naively re-requesting shipping estimates way more often than should be needed. The shipping rates are never meaningfully retained. ShopInvader goes all the way back to the carriers' external APIs any time a list of the carriers is needed.

When I change my carrier from the website, I observe the following API calls to Odoo: 2021-02-24 15:10:26,881 59 INFO prod werkzeug: x.x.x.x - - [24/Feb/2021 15:10:26] "POST /shopinvader/cart/apply_delivery_method HTTP/1.1" 200 - 331 0.222 5.058 2021-02-24 15:10:33,249 57 INFO prod werkzeug: x.x.x.x - - [24/Feb/2021 15:10:33] "GET /shopinvader/delivery_carrier?target=current_cart HTTP/1.1" 200 - 83 0.048 6.142

Added together, the user is waiting almost 12 seconds just waiting for their carrier change to be processed, and that is after the shipping estimate has already been run the first time.

I believe both of these API calls are problematic.

apply_delivery_method

delivery_carrier

This endpoint is used every time the "delivery" cart step's web page is loaded. The problem with this approach is that displaying the shipping estimates is directly tied to sending requests to get the estimate). This means that any time you include {% erp get 'delivery_carrier' as delivery_carriers with target: 'current_cart' %} you are not only retrieving the estimates, you are causing them to be re-requested from the carriers' APIs. So, in the case of the user selecting a different carrier, the process goes as follows:

  1. User loads the deliveries page, which calls delivery_carrier to both get the estimates from the carriers and return them to Locomotive
  2. User clicks a different shipping method, and the HTML form sends a POST request. Once the POST request is done, the browser is redirected back to the delivery page, which means...
  3. delivery_carrier is called again upon the page reloading, which kicks off another round of requests to the carriers for estimates.

Since the shipping estimate is a very expensive operation (5+ seconds in our environment), sending requests to UPS (or whoever the carrier is) over and over absolutely kills performance, and it is very noticeable to the end user. It seems this issue arises from the shipping rates being completely transitive, never being cached for later.

Ideally, the shipping rates should only be fetched once, and no further estimates should be needed unless the user changes the contents of their cart.

JordiBForgeFlow commented 3 years ago

I believe that we should store the shipping charges in the sale order every time the request for shipping charges is sent, together with the shipping conditions that led to this rate (as a hash). Then next time a request is sent the hash is recomputed and compared. If it is the same, no new request is sent out.

simahawk commented 3 years ago

I believe that we should store the shipping charges in the sale order every time the request for shipping charges is sent, together with the shipping conditions that led to this rate (as a hash). Then next time a request is sent the hash is recomputed and compared. If it is the same, no new request is sent out.

I've done something similar here for images. Definitely a game changer :)

github-actions[bot] commented 11 months ago

There hasn't been any activity on this issue in the past 6 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days. If you want this issue to never become stale, please ask a PSC member to apply the "no stale" label.