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

Support ActiveRecord 5.2 #337

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -18,16 +18,21 @@ env:
- ACTIVERECORD=4.2.0
- ACTIVERECORD=5.0.0
- ACTIVERECORD=5.1.1
- ACTIVERECORD=5.2.0
matrix:
exclude:
- rvm: 2.0
env: ACTIVERECORD=5.0.0
- rvm: 2.0
env: ACTIVERECORD=5.1.1
- rvm: 2.0
env: ACTIVERECORD=5.2.0
- rvm: 2.1
env: ACTIVERECORD=5.0.0
- rvm: 2.1
env: ACTIVERECORD=5.1.1
- rvm: 2.1
env: ACTIVERECORD=5.2.0
- rvm: 2.4.0
env: ACTIVERECORD=3.0.0
- rvm: 2.4.0
Expand All @@ -52,6 +57,8 @@ matrix:
env: ACTIVERECORD=5.0.0
- rvm: rbx
env: ACTIVERECORD=5.1.1
- rvm: rbx
env: ACTIVERECORD=5.2.0
allow_failures:
- rvm: rbx
fast_finish: true
Expand Down
3 changes: 2 additions & 1 deletion attr_encrypted.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ Gem::Specification.new do |s|
s.add_development_dependency('rake')
s.add_development_dependency('minitest')
s.add_development_dependency('sequel')
s.add_development_dependency('pry-byebug')
if RUBY_VERSION < '2.1.0'
s.add_development_dependency('nokogiri', '< 1.7.0')
s.add_development_dependency('public_suffix', '< 3.0.0')
Expand All @@ -50,7 +51,7 @@ Gem::Specification.new do |s|
s.add_development_dependency('activerecord-jdbcsqlite3-adapter')
s.add_development_dependency('jdbc-sqlite3', '< 3.8.7') # 3.8.7 is nice and broke
else
s.add_development_dependency('sqlite3')
s.add_development_dependency('sqlite3', '~> 1.3.0', '>= 1.3.6')
end
s.add_development_dependency('dm-sqlite-adapter')
s.add_development_dependency('simplecov')
Expand Down
16 changes: 15 additions & 1 deletion lib/attr_encrypted/adapters/active_record.rb
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ def attr_encrypted(*attrs)
end
else
define_method("#{attr}_changed?") do
attribute_changed?(attr)
attribute_changed?(attr)
end
end

Expand All @@ -75,6 +75,20 @@ def attr_encrypted(*attrs)
end

define_method("#{attr}_with_dirtiness=") do |value|
##
# In ActiveRecord 5.2+, due to changes to the way virtual
# attributes are handled, @attributes[attr].value is nil which
# breaks attribute_was. Setting it here returns us to the expected
# behavior.
if ::ActiveRecord::VERSION::STRING >= "5.2"
# This is needed support attribute_was before a record has
# been saved
set_attribute_was(attr, __send__(attr)) if value != __send__(attr)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works well with ActiveRecord 5.2.
However, in ActiveRecord 6.0.0, private method set_attribute_was was removed. rails/rails@6b0a9de

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how we should proceed with a fix that only works in a single release. My first thought was to allow this to merge, and for Rails 6 we create a new major release. That will also give us the opportunity to finally deprecate some old code and potentially do some major cleanup of the test suite.

Any thoughts on how we can move forward? Is my suggestion the best course or are there better ideas?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just update the pull request with the code snippet bellow
#337 (comment)

# This is needed to support attribute_was after a record has
# been saved
@attributes.write_from_user(attr.to_s, value) if value != __send__(attr)
end
##
attribute_will_change!(attr) if value != __send__(attr)
__send__("#{attr}_without_dirtiness=", value)
end
Expand Down
27 changes: 26 additions & 1 deletion test/active_record_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,6 @@ class Address < ActiveRecord::Base
end

class ActiveRecordTest < Minitest::Test

def setup
drop_all_tables
create_tables
Expand Down Expand Up @@ -221,6 +220,32 @@ def test_attribute_was_works_when_options_for_old_encrypted_value_are_different_
assert_equal pw.reverse, account.password
end

# ActiveRecord 5.2 specific methods
if ::ActiveRecord::VERSION::STRING >= "5.2"
def test_should_create_will_save_change_to_predicate
person = Person.create!(email: '[email protected]')
refute person.will_save_change_to_email?
person.email = '[email protected]'
refute person.will_save_change_to_email?
person.email = '[email protected]'
assert person.will_save_change_to_email?
end

def test_should_create_saved_change_to_predicate
person = Person.create!(email: '[email protected]')
assert person.saved_change_to_email?
person.reload
person.email = '[email protected]'
refute person.saved_change_to_email?
person.email = nil
refute person.saved_change_to_email?
person.email = '[email protected]'
refute person.saved_change_to_email?
person.save
assert person.saved_change_to_email?
end
end

if ::ActiveRecord::VERSION::STRING > "4.0"
def test_should_assign_attributes
@user = UserWithProtectedAttribute.new(login: 'login', is_admin: false)
Expand Down
3 changes: 2 additions & 1 deletion test/test_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@

require 'simplecov'
require 'simplecov-rcov'
require "codeclimate-test-reporter"
require 'codeclimate-test-reporter'
require 'pry-byebug'

SimpleCov.formatter = SimpleCov::Formatter::MultiFormatter.new(
[
Expand Down