Skip to content
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

Add dirty methods for store accessors #19333

Merged
merged 2 commits into from Mar 31, 2019

Conversation

Projects
None yet
6 participants
@palkan
Copy link
Contributor

commented Mar 14, 2015

Example:

class User < AR:Base
  store_accessor :settings, :color
end

u = User.new
u.color_changed? #=> false

u.color = 'red-n-white'
# Or
u.settings['color'] = 'red-n-white'

u.color_changed? #=> true
u.color_was #=> nil
u.color_change #=> [nil, 'red-n-white']

Also, force Model.stored_attributes[:store] array to contain string keys.

@palkan palkan force-pushed the palkan:dirty-store branch 2 times, most recently from 29d7752 Jun 18, 2015

@palkan

This comment has been minimized.

Copy link
Contributor Author

commented Jun 18, 2015

@rafaelfranca @sgrif Any feedback on that?

@sgrif

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2015

I think that this falls into the bucket of "we should just promote store accessors to proper attributes".

@palkan

This comment has been minimized.

Copy link
Contributor Author

commented Jun 18, 2015

@sgrif Interesting. Is anyone struggling with that?

@palkan palkan force-pushed the palkan:dirty-store branch to 37da3c2 Dec 1, 2015

@palkan

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2017

Just found myself struggling with the absence of accessors dirty methods. Maybe, we should revamp this PR? It's pretty simple but useful change, IMO.

/cc @rafaelfranca @matthewd

@palkan

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2019

Years have passed. I'm still missing this 😢

Maybe, Rails 6 is a good place for this feature? @kaspth WDYT?

Show resolved Hide resolved activerecord/test/cases/store_test.rb Outdated
define_method("#{key}_changed?") do
return false unless attribute_changed?(store_attribute)
prev_store, new_store = changes[store_attribute]
prev_store.try(:[], key) != new_store.try(:[], key)

This comment has been minimized.

Copy link
@kaspth

kaspth Mar 20, 2019

Member

Let's go with &.dig(key) instead of the try.

Show resolved Hide resolved activerecord/test/cases/adapters/postgresql/hstore_test.rb Outdated
Show resolved Hide resolved activerecord/test/cases/store_test.rb Outdated
Show resolved Hide resolved activerecord/lib/active_record/store.rb Outdated
# u.color_changed? # => true
# u.color_was # => 'black'
# u.color_change # => ['black', 'red']
#
# # Add additional accessors to an existing store through store_accessor
# class SuperUser < User
# store_accessor :settings, :privileges, :servants

This comment has been minimized.

Copy link
@kaspth

kaspth Mar 20, 2019

Member

These'll also get change tracking too, right?

This comment has been minimized.

Copy link
@palkan

palkan Mar 25, 2019

Author Contributor

Could you please explain, what did you mean here?

This comment has been minimized.

Copy link
@kaspth

kaspth Mar 31, 2019

Member

It's basically what you demonstrate in your PR description. I'm just asking if store_accessor :color also adds color_changed? etc. or if we missed it and only store :settings, accessors: %i( color ) would get it.

@palkan palkan force-pushed the palkan:dirty-store branch 2 times, most recently from 65698c8 to 619c038 Mar 25, 2019

@palkan palkan force-pushed the palkan:dirty-store branch from 619c038 to 61a39ff Mar 25, 2019

@palkan palkan force-pushed the palkan:dirty-store branch from 62cfea4 to b574d28 Mar 25, 2019

@palkan

This comment has been minimized.

Copy link
Contributor Author

commented Mar 25, 2019

@kaspth Hey! Thanks for the feedback!

I've addressed all the comments and actualized the PR by adding saved changes helpers as well.

@kaspth kaspth merged commit ba4e74e into rails:master Mar 31, 2019

3 checks passed

buildkite/rails Build #59858 passed (18 minutes, 5 seconds)
Details
codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.