You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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.
Before the has_many_inversing new Rails setting (rails/rails#34533 & rails/rails#37429), this initialization wasn't reflected in the inverse association Spree::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 through order.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.
The text was updated successfully, but these errors were encountered:
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.
The text was updated successfully, but these errors were encountered: