Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify fetching the order to calculate rates for proposed shipments #4046

Closed
waiting-for-dev opened this issue May 4, 2021 · 0 comments
Closed

Comments

@waiting-for-dev
Copy link
Contributor

waiting-for-dev commented May 4, 2021

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 (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.

@solidusio solidusio locked and limited conversation to collaborators Sep 5, 2022
@waiting-for-dev waiting-for-dev converted this issue into discussion #4546 Sep 5, 2022

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant