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

1131 fix deprecation warning for dirty attributes for rails > 5 #1132

Conversation

Marinlemaignan
Copy link
Contributor

@Marinlemaignan Marinlemaignan commented Apr 4, 2018

Resolves #1131

encrypted_password_changed? &&
DeviseTokenAuth.remove_tokens_after_password_reset
else
saved_change_to_encrypted_password? &&
Copy link
Collaborator

Choose a reason for hiding this comment

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

@zachfeldman zachfeldman merged commit 3b39c1a into lynndylanhurley:master Apr 4, 2018
@zachfeldman
Copy link
Contributor

Thanks for the contribution @Marinlemaignan !

@dks17
Copy link
Contributor

dks17 commented Oct 17, 2018

    if Rails::VERSION::MAJOR <= 5
      encrypted_password_changed? &&
      DeviseTokenAuth.remove_tokens_after_password_reset
    else
      saved_change_to_encrypted_password? &&
      DeviseTokenAuth.remove_tokens_after_password_reset
    end

It looks like in the else block is dead code because of expression Rails::VERSION::MAJOR <= 5 always return true for supported version 4 and 5? Else block is executed when major rails version is 6 or higher.

@Marinlemaignan in the header of the issue you used strict comparison > 5. Did you test it against rails 6?

@MaicolBen what do you think?

@MaicolBen
Copy link
Collaborator

MaicolBen commented Nov 4, 2018

Yeah, I would change it to Rails::VERSION <= 5.0 , good discovery!

@dks17
Copy link
Contributor

dks17 commented Nov 5, 2018

  1. Method saved_change_to_attribute? was introduced in ActiveRecord since 5.1 version.
  2. Version checking should be put outside of the should_remove_tokens_after_password_reset? method. Like the code you linked above.
  3. Mongoid doesn't support saved_change_to_attribute? method. I think be better define the should_remove_tokens_after_password_reset? method into mongoid_support and active_record_support concerns. In active_record_support concern you may useActiveRecord constant for version checking (didn't check) or Devise constant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants