spree-contrib / spree_active_shipping

Spree integration for Shopify's active_shipping gem.
http://guides.spreecommerce.org
134 stars 207 forks source link

Dimensional Weight shift UPS/FedEx will require dimensions reported on all products #199

Closed amandablum closed 9 years ago

amandablum commented 9 years ago

UPS, FedEx are eliminating weight based shipping in Q12015, and their APIs will accept dimensions for each product and do the algorithm for calculating dimensional weight on the entire package on their respective ends, IF Spree reports the dimensions of each product.

Active Shipping needs to be updated to report dimensions of each product for anyone offering actual shipping rates, rather than flat rate, table rate or freeship.

Is anyone working on this?

IrvinFan commented 9 years ago

@isaacfreeman @amandablum I don't think anyone is working on it at the moment. Would you like to tackle it?

amandablum commented 9 years ago

If no one else does, we'll have to. But.... seriously, how are more people not concerned about this?

JDutil commented 9 years ago

@amandablum SpreeActiveShipping needs to be updated to pass the package dimensions to the ActiveShipping gem, which already supports dimensions as you know: https://github.com/Shopify/active_shipping/blob/master/lib/active_shipping/shipping/package.rb#L83-L86

From what I see the places to do this would be here: https://github.com/spree-contrib/spree_active_shipping/blob/master/app/models/spree/calculator/shipping/active_shipping/base.rb#L225 https://github.com/spree-contrib/spree_active_shipping/blob/master/app/models/spree/calculator/shipping/active_shipping/base.rb#L232 https://github.com/spree-contrib/spree_active_shipping/blob/master/app/models/spree/calculator/shipping/active_shipping/base.rb#L236 https://github.com/spree-contrib/spree_active_shipping/blob/master/app/models/spree/calculator/shipping/active_shipping/base.rb#L240

It appears only the last place L240 is actually passing the dimensions, and that's when you've assigned ProductPackage's to the product: https://github.com/spree-contrib/spree_active_shipping/blob/master/app/models/spree/calculator/shipping/active_shipping/base.rb#L222 https://github.com/spree-contrib/spree_active_shipping/blob/master/app/models/spree/calculator/shipping/active_shipping/base.rb#L205 https://github.com/spree-contrib/spree_active_shipping/blob/master/app/models/spree/calculator/shipping/active_shipping/base.rb#L239-L24

So if you've created product package dimensions for your products this should already work, but I imagine that's not suitable in all scenarios. I believe if the other three lines are updated to use the variants dimensions then you would be sending the proper dimensions into ActiveShipping. I'm not sure what else if anything will need to be tweaked, but probably things related to whether it's imperial or metric.

The caveat to all this is that this library has been around forever, and has a bunch of cruft & legacy code that no one is really wanting to step up to do the work. Obviously people are too worried touching one thing might break another in unexpected ways as the test coverage isn't all that great, which makes me nervous to even offer anything up as I don't want to be blamed for breaking it. Especially since this is not an officially supported extension, and people will expect more and more from me personally when I don't even use this extension or have it as a daily responsibility to maintain.

With the limited time available to me I'm working on improving Spree Core not third party integrations. This isn't just us wanting to pass the buck on to the open source community. We just recognize that this isn't something 80% of our users need or want, and for us to stand as the gatekeepers to extensions like this isn't going to be productive for the community. What makes a strong open source community is a community that contributes back to projects it's not one that's completely funded and developed by a single company trying to do everything for everyone that's how you get the next Microsoft.

What Ryan and I much preferred working with when reimagining this extension was the Easypost API, because it was a simple implementation. After seeing how clean it can be done there I bet this extension could be simpler too if it was done from scratch with the lessons learned. For instance I believe all that you would need to do to get Easypost extension working with dimensional shipping is to add the dimensions to this line of code: https://github.com/radar/spree_easypost/blob/2-2-stable/app/models/spree/stock/estimator_decorator.rb#L56-L58 https://www.easypost.com/docs/api#parcels

The big difference with that implementation being that it's simply overriding the Spree::Stock::Estimator to make the calls rather than sticking it into calculators.

A vast majority of Spree users I know end up just doing either a flat rate shipping charge or free shipping altogether, and I don't think they even care about this change at all. Most people want to give their customers straight forward pricing or don't mind integrating a third party like Easypost even if it costs a few cents a package in order to save themselves these sorts of headaches that come from integrating with monolithic services like UPS/FedEx.

@IrvinFan I know your team has vested interest in maintaining this extension so any ideas you guys have on a better implementations would be great. I'm sure we can come up with something that gets through the transition at the end of the month to dimensions, but I think long run the best thing to do is start fresh with a clean implementation that people don't mind digging into to learn about.

futhr commented 9 years ago

For Europe there is a new kid on the block Shipbeat that follows the footsteps of Easypost, but they still early in the development phase and doesn't even have a public API yet. Maybe @kennethsvenningsen can enlighten us if they have any plans for Spree Commerce.

kennethsvenningsen commented 9 years ago

@futhr Thanks for the mention. We will definitely look into Spree Commerce. We believe it is an interesting platform, but we are not actively developing for it short term. Our service helps tackle some of the issues with package dimensions. It is possible to make a direct API integration us already now. We support carriers in Northern Europe today

jswencki commented 9 years ago

@JDutil @amandablum Our team (with @IrvinFan) is definitely interested in this 2015 change. As we get closer to that portion of implementation within our product we will contribute back anything we can do to improve it if not already addressed. @JDutil has made some good call outs in the code to help others jump ahead on this update if they want, much appreciated! Our team will reach out and post updates once we get there, but to answer the larger question, yes others are interested and hope we all can contribute to this to make it a quick win for everyone involved.

mleglise commented 9 years ago

Has anyone made progress on this since December? I'm about to start work on it if not.

IrvinFan commented 9 years ago

@mleglise Please go ahead. I don't think anobody is working on it.

mleglise commented 9 years ago

I've added a method convert_package_to_dimensions_array that, by default, continues the current behavior of not providing any dimensions.

When overridden, it can be tailored to the particular needs of the application.

https://github.com/mleglise/spree_active_shipping/commit/ff75287402996b402140f65309b3e58c81efb20a

JDutil commented 9 years ago

Nice I believe specs are passing again now if you want to submit a PR.

jspizziri commented 9 years ago

@JDutil, it looks like you were on top of this issue. a PR has been submitted, would you please give it a quick glance and make sure its behaving as it should?

JDutil commented 9 years ago

@jspizziri the PR looks fine to me, but I don't use or work on this extension anymore so I don't have time for it.

jspizziri commented 9 years ago

@JDutil, I understand, I'm just trying to go through all the old PR's and issues to clean them up. Since you were the last person to be in the know I wanted to check with you.

jspizziri commented 9 years ago

@mleglise what was the status of this?

mleglise commented 9 years ago

The method Spree::Calculator::Shipping::ActiveShipping::Base.convert_package_to_dimensions_array is available in branches 2-4-stable, 3-0-stable, and master. It's there to be overridden by individual projects that want to compute their own package dimensions. It's not a perfect fix for this problem, just a stop-gap hook. Seeing no other recent activity on this issue, I am comfortable with closing it until a better solution is presented.