Fix missing nil attributes in SObject.update
#79
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
rferg
commented
Jan 8, 2024
| def prepare_for_update(id, attributes) | ||
| new(attributes.merge(id: id)).tap do |obj| | ||
| attributes.each do |name, value| | ||
| obj.public_send("#{name}_will_change!") if value.nil? |
There was a problem hiding this comment.
I think this could be generalized to:
obj.public_send("#{name}_will_change!") unless object.public_send("#{name}_changed?")in case there are other reasons that an attribute would be "unchanged". But this is more confusing and I think we should avoid those changed? calls if not really needed.
rferg
commented
Jan 8, 2024
| @@ -34,8 +34,24 @@ class << self | |||
| def_delegators :query, :not, :or, :where, :first, :last, :all, :find, :find!, :find_by, :find_by!, :sum, :count, :includes, :limit, :order, :select, :none | |||
| def_delegators :mapping, :table, :table_name, :custom_table?, :mappings | |||
|
|
|||
| def update(id, attributes) | |||
There was a problem hiding this comment.
I moved these up here so I could easily create a shared private class method (viz., prepare_for_update) and have them close together.
It's confusing that we have a class << self block and also a bunch of class methods defined outside of it with def self.method declarations. Might be better to migrate those into this block at some point.
michaelbatchelder
approved these changes
Jan 8, 2024
|
@rferg Please update the |
|
Thank you! |
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.
Previously, calling
SObject.update(id, a: nil)orSObject.update!(id, a: nil)would omit'A' => nilfrom the Restforce request. Consequently, the field would not be updated toNULLin Salesforce, despite the clear intention of the caller to do that.This is because
.updatecreates an instance, passing in the given attributes, and then calls#updateon it. That method only includes changed attributes in the request. And since a new instance's attribute isnilby default, setting it tonildoes not register it as changed.This PR fixes this by forcing any attributes given to
SObject.updateorSObject.update!withnilvalues to register a change on the model by using theActiveModel::Dirty#*_will_change!method.