Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ShippingError rescue is holding back other results #90

Closed
mrpollo opened this issue Jun 26, 2013 · 9 comments
Closed

ShippingError rescue is holding back other results #90

mrpollo opened this issue Jun 26, 2013 · 9 comments

Comments

@mrpollo
Copy link
Contributor

mrpollo commented Jun 26, 2013

Lets say you are shipping to Russia (from US) here is an address:

Snezhinsk, 456770, Russian Federation

My defined shipping providers are: USPS and FedEx ( all services from both )

When I call @order.rate_hash it raises the following ShippingError

Spree::ShippingError (Shipping Error: WARNING - 556: There are no valid services available. ):

the error comes from:
raise rates_result if rates_result.kind_of?(Spree::ShippingError)
models/spree/calculator/shipping/active_shipping/base.rb#L58

The problem is that FedEx does not ship to that location in Russia (confirmed with FedEx over the phone), USPS does, but @order.rate_hash never gets a chance to get the rates from USPS because raise error halts the operation sending the error back to you.

What is the best recommended way to deal with this situation?

@rterbush
Copy link
Contributor

I question if there is any value to raising an error here. Perhaps we should just return nil?

cc @cmar @LBRapid

@mrpollo
Copy link
Contributor Author

mrpollo commented Jun 26, 2013

@rterbush I think that would make sense but only if we handle the exception of really no methods available through any provider in @order.rate_hash in the order model here https://github.com/spree/spree/blob/1-3-stable/core/app/models/spree/order.rb#L425

else we would return either an array with nil values or an empty array by the next unless logic

does this make sense?

@rterbush
Copy link
Contributor

Agreed. We have recently encountered this in 2.0 and are dealing with it at the moment with the following:

https://gist.github.com/rterbush/5853985

Still waiting on the seniors to weigh in on the general issue.

@LBRapid
Copy link
Contributor

LBRapid commented Jun 26, 2013

@mrpollo I think that checking for available methods in the #rate_hash method of the Order model makes a lot of sense.

@rterbush I'm not super familiar with this extension these days but I think I understand the issue at hand and believe you are both on the right track. Let me know if you need anymore assistance.

@mrpollo
Copy link
Contributor Author

mrpollo commented Jun 27, 2013

@rterbush I'm submitting fixes for Spree 1-3, do you have it fixed on Spree 2 as well? maybe we can both send the PR's

mrpollo added a commit to mrpollo/spree_active_shipping that referenced this issue Jun 27, 2013
moved Error logic to the Order model on rate_hash
@rterbush
Copy link
Contributor

I will follow up with PRs on 2-0-stable and master.

@mrpollo
Copy link
Contributor Author

mrpollo commented Jun 27, 2013

Feedback wanted, need to raise an error inside @order.rate_hash but Spree::ShippingError i inside spree_active_shipping what should I be raising instead?

spree/spree#3268

@mrpollo
Copy link
Contributor Author

mrpollo commented Jun 27, 2013

never-mind on the feedback

rterbush added a commit that referenced this issue Jun 28, 2013
removed raise ShippingError from compute, fix #90
rterbush added a commit that referenced this issue Jun 28, 2013
remove raise ShippingError from compute, fix #90
rterbush added a commit that referenced this issue Jun 28, 2013
remove raise ShippingError from comput, fix #90
@mrpollo
Copy link
Contributor Author

mrpollo commented Jun 28, 2013

oh wow nice, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants