Make Splitter::Base.build_package and Shipment.to_package use Config::package_factory #39
Conversation
| def initialize(stock_location, order, splitters) end | ||
| end | ||
|
|
||
| packer.package_factory = CustomPackage |
There was a problem hiding this comment.
For some reason I couldn't mock package_factory with either
before { Spree::Config.package_factory = CustomPackage }
or
allow(Spree::Config).to receive(:package_factory) { CustomPackage }
I ended up making package_factory writable (attr_accessor instead of attr_reader above).
There was a problem hiding this comment.
could it be related to the fact that there is no ; between the initialize and the end in line 28 above? AFAIK that is not valid Ruby.
There was a problem hiding this comment.
This seems to work:
| packer.package_factory = CustomPackage | |
| allow(Spree::Config).to receive(:package_factory) { CustomPackage } |
There was a problem hiding this comment.
yeah, I must have been doing something wrong, it just works now...
@sauloperez the missing ; was not causing the problem but I added it anyway
luisramos0
changed the title
| def initialize(stock_location, order, splitters) end | ||
| end | ||
|
|
||
| packer.package_factory = CustomPackage |
There was a problem hiding this comment.
could it be related to the fact that there is no ; between the initialize and the end in line 28 above? AFAIK that is not valid Ruby.
core/app/models/spree/shipment.rb
Outdated
| @@ -222,7 +222,8 @@ def inventory_units_for(variant) | |||
| end | |||
|
|
|||
| def to_package | |||
| package = Stock::Package.new(stock_location, order) | |||
| package_factory = Spree::Stock::Coordinator.new(self).packer(stock_location).package_factory | |||
There was a problem hiding this comment.
Is there any reason why don't get the factory from Spree::Config.package_factory?
|
Good catch! I don't know how I missed this calls to the package class 🙈 |
| def initialize(stock_location, order, splitters) end | ||
| end | ||
|
|
||
| packer.package_factory = CustomPackage |
There was a problem hiding this comment.
This seems to work:
| packer.package_factory = CustomPackage | |
| allow(Spree::Config).to receive(:package_factory) { CustomPackage } |
luisramos0
force-pushed
the
2-0-fix-packer-bug
branch
from
3658c16
to
d4e4245
Compare
…instanciating Spree::Stock::Package This ensures the injected package factory is used when converting a shipment to a package
…e Packer This will make the splitter use the injected Package, not the default Spree Package class
luisramos0
force-pushed
the
2-0-fix-packer-bug
branch
from
d4e4245
to
aedf213
Compare
|
awesome, I incorporated your feedback, thanks guys! |
|
Perhaps you wanted to manually test it? |
|
it's fine, we test it on the ofn PR |
[Spree 2.1] Use latest version of spree which includes PR openfoodfoundation/spree#39
This PR fixes: openfoodfoundation/openfoodnetwork#3607
Description
This PR is related to coopdevs#6
We are replacing the usage of Spree::Stock::Package with a call to the package factory.
In both cases below, for OFN, the shipping methods were not being filtered by distributor, which is done in the OFN package implementation injected in package_factory.
Splitter::Base
The change in Splitter::Base makes all splitters use the injected Package, not the default Spree Package class when building the split packages. This was breaking all splitters that required the custom package factory.
This is related to openfoodfoundation/openfoodnetwork#3604
This has no impact on OFN after 3604 because this build_package method is called by all splitters except the Base splitter which is the only one that OFN will use for now.
Shipment.to_package
The change in Shipment.to_package uses the injected Package factory to convert every shipment to a package.
This was creating a bug in OFN during order.refresh_rates that happens when loading shipping_methods in the order edit page. See openfoodfoundation/openfoodnetwork#3607