Extract the packaging code into a service object #21
Conversation
fredericboivin
force-pushed
the
extract_service
branch
4 times, most recently
from
8ea8376 to
6fc9252
Compare
|
|
||
| attr_reader :shipping_calculator | ||
|
|
||
| def_instance_delegator :shipping_calculator, :max_weight_for_country |
There was a problem hiding this comment.
Is there any reason why we can't initialise the PackageBuilder with the max_weight_for_county so it doesn't have to be coupled with the calculator?
fredericboivin
force-pushed
the
extract_service
branch
3 times, most recently
from
d7947c8 to
9ede5c5
Compare
| @@ -15,11 +15,26 @@ def process(package) | |||
| packages(package) | |||
| end | |||
|
|
|||
| # Configuration | |||
| def units | |||
There was a problem hiding this comment.
Are any of these going to be part of the public interface of the PackageBuilder? If not, i'd prefer if they were private.
There was a problem hiding this comment.
To my knowledge, they should not be used elsewhere, so good catch
|
Last commit breaks the tests since the behavior is different, currently working on adding spec to the package_builder (that I will amend later) so we can kill/salvage the weight_limits_spec and kill the weight-related code in the base calculator |
| active_shipping_packages = [] | ||
| weights = convert_package_to_weights_array(solidus_package) | ||
|
|
||
| ci_without_boxes = [] |
There was a problem hiding this comment.
I would prefer if we didn't introduce a new name for product_packages here as boxes. We also generally avoid abbreviating things in names. It isn't immediately clear what ci is when reading this code.
There was a problem hiding this comment.
Makes sense, fixed it
fredericboivin
force-pushed
the
extract_service
branch
3 times, most recently
from
79ccc7f to
77c8682
Compare
|
@jhawthorn : I'd really appreciate if you could take a look and provide some feedback if you have the time (outside of style issues that I will rubocop and the stuff Chris already mentionned) |
fredericboivin
force-pushed
the
extract_service
branch
6 times, most recently
from
7743361 to
92239bc
Compare
fredericboivin
force-pushed
the
extract_service
branch
from
8302093 to
dfa4673
Compare
fredericboivin
changed the title
|
Should be ready for another round of review if build is green |
fredericboivin
force-pushed
the
extract_service
branch
from
f0c4c12 to
a5ba92b
Compare
|
Tests are broken in general now on master because of Solidus PR#1138 Going to fix this in another PR |
|
Will need to be rebased on #29 |
|
#29 is merged so we can rebase this to get the tests 🍏 again! |
fredericboivin
force-pushed
the
extract_service
branch
2 times, most recently
from
10f7d64 to
f0cc228
Compare
|
This guy has been around for a while so I think it's time we take another look over it and merge 🎉 |
|
@dangerdogz When you have a chance can you rebase this on top of master and push up so it can run specs again against new Solidus master and v2.0 branch. |
fredericboivin
force-pushed
the
extract_service
branch
from
f0cc228 to
c02d365
Compare
Extract the packaging related code into a new service object. Goal is for it to be an extension point which can support various calculators related to different metrics. For now the code/logic is exactly the same.
Extract methods for quickly building inventory units and content items into PackageHelper so it can be reused between spec
Centralize the definition and use of the config values so it can be easily updated and usage is the same everywhere
Add comments to the method and simplify the logic. Compute it once then reuse the result throughout the process
Delete the calculator::weight namespace and move the PackageBuilder to the default Spree::
Use the methods provided by ContentItem to avoid chaining associations to get variant_weight + ContentItem. ContentItem also has a quantity of one so we don't need the convoluted logic around fitting a max quantity of item into a package.
packages method was accessing the properties of ProductPackages through an array index which is unreliable, opted to use the object itself instead of an array of numbers and access the data through the properties. Also remove the hardcoded imperial units
Rename variables so it's easier to understand what is coming from Solidus and what is returned by ActiveShipping
Previously, product with associated product packages were computed twice (once based on weight and added in the general packages) and once for each product_packages. This commit aims to split the generation so product are added once based on the existence of associated product_packages
It's full of holes and it should be replace by package_builder_spec instead.
The code was extracted to package_builder.rb
Before, the call to max_weight_for_country was delegated to the calculator but the max_weight was computed by the package builder. It is out of scope for the PackageBuilder since the max_weight is determined at the calculator level. This commit remove the delegation and make it a parameter of the process function. Also included : small tweaks to the calculator to compute and send the parameters and changes to the test suite.
Find indentation and various style issues
fredericboivin
force-pushed
the
extract_service
branch
from
c02d365 to
23fb102
Compare
|
Rebased |
Logic related to building the ActiveShipping::Packages needed for the find rates is extracted into a service object.
The goal would be for it to be an extension point where different "packager" could be used. Code is the same for now, it's a straight extraction so no refactoring yet
Max weight is delegated to the calculator through Forwardable since max_weight_for_country is a protected method.
Next step would be to determine how to better handle the max_weight since it's determined on the calculator level and is currently riddled with bugs
Note : build is currently failing because USPS is returning 503