ActiveModel::Dirty support not working

[aidandamerell]

The attribute_was method does not appear to return the old value from an encrypted value:

class Person < ApplicationRecord
	attr_encrypted :email, key: "Some key"
	before_update :check

	private

	def check
		puts "Old: #{self.email_was}"
		puts "New: #{self.email}"
	end
end

This can be seen in the console:

2.5.3 :006 > Person.first.email
 => "email@email.com"
2.5.3 :006 > Person.first.update(email: "another@email.com")
Old: 
New: another@email.com
=> true

[klappradla]

I ran into a similar problem when using attr_encrypted in combination with the bind_index gem: the way attr_encryptor defines the setter for the to be encrypted attribute, it only assigns the instance variable, not the attribute the way rails would do it using []= or the protected alternative write_attributes alternative.

Doing so, the changes object of the record won't be populated and therefore the attribute does not appear to have been changed.

This is the respective code in attr_encrypted:

attr_encrypted/lib/attr_encrypted.rb

Line 166 in 11df93a

define_method("#{attribute}=") do |value|

(here for quicker reading:)

define_method("#{attribute}=") do |value|
  send("#{encrypted_attribute_name}=", encrypt(attribute, value))
  instance_variable_set("@#{attribute}", value)
end

If I adapt this to closer resemble what Rails' internal attribute= method does, it could look like this:

define_method("#{attribute}=") do |value|
  send("#{encrypted_attribute_name}=", encrypt(attribute, value))
  send("[#{attribute}]=", value)
  instance_variable_set("@#{attribute}", value)
end

This setter implementation would actually assign the attribute. Changes to the attribute will appear in the object's changes hash and things like <attribute>.changed? would work same as for "normal" non-encrypted attributes. But I'm not sure if there's not more to it (did not yet dive into the details for ActiveModel::Dirty)...

I wasn't sure if this is actually intended by the authors 🤔
If not, I'd be happy to craft out a PR to change this, but this may also already be addressed in #337 💚

[surjay]

Any traction on this?