solidus icon indicating copy to clipboard operation
solidus copied to clipboard

Simplify fetching the order to calculate rates for proposed shipments

Open waiting-for-dev opened this issue 4 years ago • 0 comments

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 to Spree::Stock::Estimator, through a previously generated Spree:::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 on Spree::Stock::Package.

Before the has_many_inversing new Rails setting (https://github.com/rails/rails/pull/34533 & https://github.com/rails/rails/pull/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.

waiting-for-dev avatar May 04 '21 14:05 waiting-for-dev