Errors cached as empty results creates checkout flow issue #38

Closed
j15e opened this Issue Oct 2, 2012 · 4 comments

Comments

Projects
None yet
2 participants
Contributor

j15e commented Oct 2, 2012

In the base class, an empty cache entry is created in the rescue block to prevent constant re-lookups but this seems to create an important issue : when I resubmit my form I do not always get the error again and I get an empty list of shipping options.

Am I missing something? It also allows customer without shipping option due to an error to complete checkout & payment without any shipping fee.

Contributor

jumph4x commented Oct 3, 2012

I believe the basic idea is as follows:

  1. If the inputs are the same, the output should be the same, so the empty output hash is appropriate
  2. If, however, anything changed: address, order items or qty, the @cache_key will change also and will be a cache miss, creating a new (hopefully) non-empty cache entry

The checkout flow is a separate issue. I would begin by hardcoding an empty hash output and play around that in dev environment.

Contributor

j15e commented Oct 3, 2012

Problem is the cache seems to only be set in the rescue block (in the case the carier has a response error)

def retrieve_rates(origin, destination, packages)
  begin
    response = carrier.find_rates(origin, destination, packages)
    # turn this beastly array into a nice little hash
    rate_hash = Hash[*response.rates.collect { |rate| [rate.service_name, rate.price] }.flatten]
    return rate_hash
  # ====== This is a rescue block
  rescue ActiveMerchant::ActiveMerchantError => e 

    if [ActiveMerchant::ResponseError, ActiveMerchant::Shipping::ResponseError].include? e.class
      params = e.response.params
      if params.has_key?("Response") && params["Response"].has_key?("Error") && params["Response"]["Error"].has_key?("ErrorDescription")
        message = params["Response"]["Error"]["ErrorDescription"]
      else
        message = e.message
      end
    else
      message = e.to_s
    end
    # ====== This is the only place where cache is set, inside the rescue block
    Rails.cache.write @cache_key, {} #write empty hash to cache to prevent constant re-lookups

    raise Spree::ShippingError.new("#{I18n.t(:shipping_error)}: #{message}")
  end

end
Contributor

j15e commented Oct 3, 2012

Ok sorry, I didn't get that the the cache key was written by the block in Base here :

          retrieve_rates_result = Rails.cache.fetch(cache_key(order)) do
            order_packages = packages(order)
            if order_packages.empty?
              {}
            else
              retrieve_rates(origin, destination, order_packages)
            end
          end

So the cache key is not only set in the rescue block, but it should be handled in a way that the error will be provided to the user again when hitting cache.

@j15e j15e added a commit to hooktstudios/spree_active_shipping that referenced this issue Oct 3, 2012

@j15e j15e Cache responses errors too (see spree-contrib/spree_active_shipping#38)
Otherwise the error is only shown on first user attempt
4c14e0c

j15e referenced this issue Oct 3, 2012

Closed

Cache fixes #40

Contributor

j15e commented Oct 3, 2012

Closing this one, see #40

j15e closed this Oct 3, 2012

@j15e j15e added a commit to hooktstudios/spree_active_shipping that referenced this issue Oct 3, 2012

@j15e j15e Cache responses errors (see spree-contrib/spree_active_shipping#38)
Otherwise the error is only shown on first user attempt
15d008b

j15e referenced this issue Oct 3, 2012

Closed

Cache fixes #42

@j15e j15e added a commit to hooktstudios/spree_active_shipping that referenced this issue Oct 4, 2012

@j15e j15e Cache responses errors (see spree-contrib/spree_active_shipping#38)
Otherwise the error is only shown on first user attempt
682bd9f

j15e referenced this issue Oct 4, 2012

Merged

Cache fixes 3 #44

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment