[RFC] Rename order.update! to order.recalculate #2072
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
tvdeyen
approved these changes
Jul 7, 2017
core/app/models/spree/order.rb
Outdated
| updater.update | ||
| end | ||
|
|
||
| alias_method :update!, :recalculate |
jhawthorn
force-pushed
the
order_recalculate
branch
from
f3b6b57
to
a1dc526
Compare
mamhoff
approved these changes
Jul 8, 2017
kennyadsl
approved these changes
Jul 9, 2017
|
I am also on board with this. |
bbuchalter
approved these changes
Jul 10, 2017
jordan-brough
approved these changes
Jul 10, 2017
ericsaupe
approved these changes
Jul 10, 2017
|
|
|
Thanks for the feedback everyone. This was a lot less contentious than I expected. Merging now. As the 2.3 branch has been forked, this will be in our future 2.4.0 release. |
This was referenced Jul 12, 2017
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.
I expect this to be contentious, and it definitely introduces some work for users upgrading, which I'm always cautious of.
ActiveRecord::Basehas anupdate!(attributes)which as of Rails 4.0 is the recommended way to update the attributes on a model. The PR introducing it describes the olderupdate_attributes!as "soft-deprecated".Because we've overridden
update!, we can't use that method to update the the attributes on order (asupdate!does for every other model), and have to useupdate_attributes!.This isn't great because Rails devs (as well as documentation) are going to expect
update!to work and might not even be aware ofupdate_attributes!. This is doubly confusing because we have access to ActiveRecord'sorder.update, and one really expects that to do approximately the same thing as the!version.This PR adds
Order#recalculate, which does exactly whatOrder#update!did, invoking theOrderUpdater.Order#update!will now, if given attributes, will behave like AR'supdate!. If given no attributes it will emit a deprecation warning and callrecalculate.If we decide to do this, we should probably do the same for
Adjustment#update!andShipment#update!