Skip to content

Do not mark attribute as changed if value is the same#2

Closed
shioyama wants to merge 2 commits intomasterfrom
fix_dirty_set_unchanged_value_bug
Closed

Do not mark attribute as changed if value is the same#2
shioyama wants to merge 2 commits intomasterfrom
fix_dirty_set_unchanged_value_bug

Conversation

@shioyama
Copy link
Copy Markdown
Owner

@shioyama shioyama commented Mar 12, 2017

(Note: I'm starting to post bug fixes and important changes as pull requests to
make them more visible.)

Currently, for a model post with dirty tracking enabled, this is what happens:

post = Post.new
post.title        #=> nil
post.changed?     #=> false
post.changed      #=> []
post.changes      #=> {}

So far so good, but if we set the value to nil:

post.title = nil
post.changed?     #=> true

This is a bug, which this PR fixes. After the change:

post.title = nil
post.changed?   #=> false
post.title = "foo"
post.changed?   #=> true

Note: the change applies to both ActiveModel and Sequel modules.

@shioyama shioyama force-pushed the fix_dirty_set_unchanged_value_bug branch from f455079 to 11c2981 Compare March 12, 2017 05:00
@shioyama
Copy link
Copy Markdown
Owner Author

While working on this fix I noticed that there is a tricky relationship between dirty tracking and fallbacks. If I have a model Article with fallbacks enabled:

class Article
  include Mobility
  translates :title, dirty: true, fallbacks: { en: 'ja' }
end

and I set the value in Japanese, the value in English will fall through to this value:

article = Article.new
article.title
#=> nil
Mobility.with_locale(:ja) { article.title = "タイトル" }
article.title
#=> "タイトル"

In the dirty module, when we check the current value of the attribute in the current locale, we need to disable fallbacks, otherwise we will compare with the fallback value and mark a change when we set to the same blank value:

article.title = nil
article.changed
#=> ["title_ja", "title_en"]

We don't want this - i.e., while I may want to fall through to a different locale, I don't want to compare with the value in that locale when marking attributes in the current locale as having changed or not.

If instead we disable fallbacks (read(locale, options.merge(fallbacks: false))), then we get what (I think) you'd expect:

article.title = nil
article.changed
#=> ["title_ja"]

Only the value in Japanese has changed (from nil to タイトル).

However, when we do update the value, do we mark the change as a change from nil or as a change from the fallback value?

i.e.

article.title = "title"
article.changes
#=> { "title_ja" => [nil, "タイトル"], "title_en" => ["タイトル", "title"] }

Or should it be marked as a change from nil to the new value?

I have gone with the former (change from fallback value), becuase: a) to make it do otherwise would require hacking hte internals of Rails/Sequel, which I don't want to do, b) I think intuitively, this sort of makes sense.

@shioyama shioyama force-pushed the fix_dirty_set_unchanged_value_bug branch from 1f127a1 to e8aed90 Compare March 12, 2017 06:19
@shioyama shioyama closed this in 70344cb Mar 12, 2017
@shioyama shioyama deleted the fix_dirty_set_unchanged_value_bug branch March 12, 2017 06:59
@shioyama shioyama added the bug label Mar 12, 2017
timkrins added a commit to InfuseGroup/mobility that referenced this pull request May 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant