packages(package) doesn't work as intended when item weights are zero
brchristian opened this issue · 3 comments
I've been looking closely at the logic of the packages(package) method, and I'm seeing some weird behavior when product weights are zero.
If a product weight is zero and max_weight is zero, then it will go into a package.
If a product weight is zero and there are other products that are weight > 0, then it will get lumped in with them into a package.
However, if there are only zero-weight products, then they will not get into a package due to the conditional in https://github.com/spree/spree_active_shipping/blob/master/app/models/spree/calculator/shipping/active_shipping/base.rb#L236
However, fixing this problem isn't as simple as removing that conditional, because we might end up with nil packages getting tacked on after the weights.each block.
I'll prepare a PR with my proposed solution, which is to set package_weight to nil before the block and use a conditional on weight != nil rather than on weight > 0.
I agree that packages logic is messed up, if you do a pr just make sure to take into consideration the weight conditions on services, some depend on nil or 0 values, nil means it shouldn't pass and 0 means there is no restriction.
I cleaned this logic a bit and removed what we felt was not doing much for us here
feel free to go through it and comment where needed, we use this in production and are getting good results,
As I'm looking into a PR, I'm realizing that this is even thornier than I thought originally.
For instance, if package_weight > max_weight, it just makes a package, which seems odd, as to my mind the point of a weight limit is to prevent packages over that limit.
I'll leave the above as an issue, and leave the PR to folks who know the code and its intention better than I do.
Agreed, I think the package weight restrictions should be enforced by splitters when creating shipments, and not this late when you already have the shipments and just need their rates.