Simplify fetching the order to calculate rates for proposed shipments #4546
waiting-for-dev
started this conversation in
New Features or Ideas
Replies: 0 comments
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
-
I'm not super-familiar with the logic around this issue, so maybe I'm missing something. However, if my understanding is correct, this is a proposal for an internal simplification.
Spree::Stock::SimpleCoordinator#build_shipments
is meant to build shipments presented as an option to the user. To calculate their associated shipping rates, it delegates the shipments toSpree::Stock::Estimator
, through a previously generatedSpree:::Stock::Package
. This service needs to know the order instance those shipments would be assigned to. With the current implementation, this information is taken from the shipment itself as it's already associated with that order when it's initialized onSpree::Stock::Package
.Before the
has_many_inversing
new Rails setting (rails/rails#34533 & rails/rails#37429), this initialization wasn't reflected in the inverse associationSpree::Order#shipments,
but it's no longer the case. For this reason, after we have calculated the shipping rates, we need to remove the shipping instances throughorder.shipments = order.shipments - shipment
. This is part of what we have done in #4035.As an improvement, we could pass the order as a parameter to the estimator and avoid us depending on the complex AR associations cache. As users can provide both their own coordinator and estimator classes, we could have backward compatibility issues both from the caller and the callee. As a first step, we should keep the present behavior and provide the order anyway as an optional argument.
Beta Was this translation helpful? Give feedback.
All reactions