Add compatibility with ActiveModel::Dirty#attribute_will_change!

[radicaled]

Callers use #attribute_will_change! to store the previous value of an attribute, before it is changed.

[radicaled]

This is necessary to be compatible with Devise 4.x, and possibly other frameworks moving to support Rails 5.

[nviennot]

Hi Arron :)

So from what I'm seeing: https://github.com/plataformatec/devise/blob/master/lib/devise/models/database_authenticatable.rb#L42 Devise calls this on the password field. But it's set on a instance variable: https://github.com/plataformatec/devise/blob/master/lib/devise/models/database_authenticatable.rb#L32 This is not a database field, so why should NoBrainer care about this?

Calling attribute_may_change() on an unknown database field may create problems. NoBrainer does not expect non-db fields to be called on attribute_may_change(). I'm not sure what the problems may be though (things can get funny on a model that results from a query with pluck() or without()).

NoBrainer doesn't need to be told when an attribute may change. it tracks changes well on its own (except when the user directly tampers with @_attributes but that's fine)

I suggest we make attribute_will_change!() a no-op.

[radicaled]

This is not a database field, so why should NoBrainer care about this?

It looks like Devise wants to notify the model that it is dirty whenever password is called. In that case, changed? and password_changed? both return true. I don't know if they're reserving this for future behavior or it fixes some random bug in the code as a side-effect, but it looks like they're expecting Devise adapters to implement ActiveRecord::Dirty's interface (and ActiveRecord::Dirty doesn't care whether an attribute is a database field or just a regular field). Without this behavior changed? and password_changed? aren't true for NoBrainer, so I don't think a no-op will behave the way Devise expects in the future.

I assumed this was a relatively safe change to make -- it didn't look like @_old_attribute_keys was called by any of the persistence code, only the dirty tracking change code, which is what Devise wants.

[nviennot]

• if changing the password changes the stored encrypted password (which it will), then changed? will return true in NoBrainer
• password_changed? is not implemented in ActiveRecord if the field is not a DB field. The following shows an interactive session while running the devise test suite on ActiveRecord:

~/devise () > u.password = "x"
=> "x"
~/devise () > u.changes
=> {
              "password" => [
        [0] nil,
        [1] "x"
    ],
    "encrypted_password" => [
        [0] "$2a$04$AVJWHeg4cFIJEpYCdGRPIerCma/zhm6rllI8RRYjNTDwLaeAXdWNC",
        [1] "$2a$04$FEF/pPFZQ8320zMvJ3nyw.d.fzEaTFlxUNFg8MLYylCZd/hbPGNtS"
    ]
}
~/devise () > u.password_changed?
NoMethodError: undefined method `password_changed?' for #<User:0x000000061b6650>

The only thing that Devise could do would be to call .changes to read the old password back. This is very unlikely to be an issue I'd think.

[radicaled]

Fair enough. I'm not sure why they're using it, then, but as long as it doesn't cause any problems with my application I'm okay with the method being a no-op. I can't update the PR tonight, but will tomorrow unless you get to it before then.

[nviennot]

cool :)