Avoid methods other than update! on OrderUpdater #1689
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jhawthorn
force-pushed
the
order_update_private
branch
from
cc5f02f to
6edc4c3
Compare
jhawthorn
changed the title
gmacdougall
reviewed
Jan 25, 2017
They were previously public. They're still a little too intertwined to be made private immediately. For example, in |
|
Thanks @jhawthorn I confused myself with the diffs. It was all the other stuff that became private, not those becoming public. |
|
Indeed great! |
Instead of using a (soon to be) private method in OrderUpdater, this method now does what is asked of it. Tests adjustments one-by-one to ensure that they remain eligible.
jhawthorn
force-pushed
the
order_update_private
branch
from
ee4b50d to
0944bf6
Compare
Previously refresh_rates was called from OrderUpdater on completed Orders. However, refresh_rates has no effect on completed orders, so this was pointless.
I don't think this is necessary, and has just been kept around. We should never get an unsaved shipment here. Even if we did, it would have unexpected behaviour as it would be saved later.
Previously, this was only called on completed orders. It's a simple and fast update, so we might as well perform it on every single update. This allows us to remove the need for Order#set_shipments_cost
jhawthorn
force-pushed
the
order_update_private
branch
from
0944bf6 to
aa1738d
Compare
This is now done as part of order.update! and is unnecessary
Previously OrderUpdateAttributes would call set_shipment_cost just in case any shipments had their selected shipping rate changed. set_shipment_cost would set the Shipment's cost from the shipping rate, and would update the order total, but wouldn't properly recalculate taxes or promotions. This commit removes the call to set_shipment_cost. Instead, order.update! should be called after OrderUpdateAttributes. This is done everywhere OrderUpdateAttributes is used in Solidus itself, and was already required (though not explicitly) for correct behaviour.
jhawthorn
force-pushed
the
order_update_private
branch
from
aa1738d to
e5fce46
Compare
4 tasks
elia
added a commit
to elia/solidus
that referenced
this pull request
May 10, 2019
After update! => recalculate rename. ref: - solidusio#1689 - solidusio#2072
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.