Fix deprecation warnings #26

olbrich · 2018-03-21T08:04:38Z

This branch does the following:

adds formalized testing framework for testing against rails 4.2 and 5.1 to help with ensuring backwards compatibility
refactors the PasswordArchiveable module to deal with changes in the rails dirty attribute handling in rails 5.1
Cleans up tests a bit
makes the test harness user include all of the various devise modules so we can check for interactions
updates supported ruby versions
adds some debugging tools and configurations to better expose deprecation warnings.

fixes #22

…icitly

…some cases). It's optional and only used by us for testing, so it doesn't show up in the gemspec

…heck for interactions with them.

…to make sure the created fixtures are valid with all the rules enforced

…nd a bug in newer versions

… simplify the code

…ere indirectly saving a user (by saving a dependent object) would trigger an activesupport deprecation warning.

…lly simplify the logic. Also adds a compatibility module to deal with API changes in the rails dirty attribute handling

… API for testing changed between 4.2 and 5.0

coveralls · 2018-03-21T08:11:58Z

Pull Request Test Coverage Report for Build 63

120 of 127 (94.49%) changed or added relevant lines in 19 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage decreased (-0.8%) to 89.394%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
lib/devise-security/models/compatibility.rb	9	11	81.82%
lib/devise-security/schema.rb	1	3	33.33%
lib/devise-security/models/password_archivable.rb	19	22	86.36%

Files with Coverage Reduction	New Missed Lines	%
lib/devise-security/models/password_archivable.rb	1	89.74%

Totals
Change from base Build 59:	-0.8%
Covered Lines:	767
Relevant Lines:	858

💛 - Coveralls

natebird

This is a great pull request. Thank you for your work. Because there are so many changes I'd like to have @oniofchaos look at it as well before merging.

natebird · 2018-03-21T12:54:51Z

test/dummy/config/environments/test.rb

@@ -24,4 +24,10 @@
  I18n.enforce_available_locales = false

  config.active_support.test_order = :sorted
+  config.log_level = :debug
+  if Rails.version < '5'
+    # TODO: this needs to be addressed


What needs to be addressed here?

If I don't set this config in rails 4.2, it will raise a deprecation warning. I'm not entirely sure there is more to do here or if that warning is just intended to warn people about about a change in behavior.

I don't think a TODO here is appropriate, as there's nothing to fix. Instead, a comment explaining why this is needed for older Rails would be good.

It was more of a reminder to myself that I forgot to remove.

dillonwelch

Looks like the test suite is bombing out?

dillonwelch · 2018-03-21T18:24:44Z

.gitignore

@@ -37,3 +37,4 @@ log
 test/tmp/*
 *.gem
 Gemfile.lock
+*.lock


What's this for?

Appraisal generates Gemfiles in the /gemfiles directory. We don't want to check in their lock files. The same way you don't check in Gemfile.lock files

dillonwelch · 2018-03-21T18:25:46Z

lib/devise-security/models/compatibility.rb

@@ -0,0 +1,22 @@
+module Devise
+  module Models
+    module Compatibility


dillonwelch · 2018-03-21T18:26:08Z

lib/devise-security/models/password_archivable.rb

    module PasswordArchivable
      extend ActiveSupport::Concern
+      include Devise::Models::DatabaseAuthenticatable


Can you alphabetize these?

dillonwelch · 2018-03-21T18:27:55Z

test/dummy/config/environments/test.rb

@@ -24,4 +24,10 @@
  I18n.enforce_available_locales = false

  config.active_support.test_order = :sorted
+  config.log_level = :debug
+  if Rails.version < '5'
+    # TODO: this needs to be addressed


I don't think a TODO here is appropriate, as there's nothing to fix. Instead, a comment explaining why this is needed for older Rails would be good.

natebird · 2018-03-21T20:24:02Z

@olbrich Looks good. Thank you!

I'll just note here that the test coverage only decreased because of the refactoring of the migration code. We should probably exclude some of those files in the future.

olbrich added 18 commits March 17, 2018 22:32

add appraisal gem so we can test against other versions of Rails expl…

b9c37bd

…icitly

add pry

8d83762

update ruby versions

6616373

allow rails 4.2 to fail for now

52ade00

rubocop changed configs again

0b3da25

update rails dependencies to include omniauth (which devise needs in …

c85863b

…some cases). It's optional and only used by us for testing, so it doesn't show up in the gemspec

add migrations to tests for expireable

ea7cc18

include all standard modules from devise to our test user so we can c…

94eb900

…heck for interactions with them.

add migrations to tests for standard devise modules and update tests …

c4c8fc6

…to make sure the created fixtures are valid with all the rules enforced

add some pry tools to help with debugging, lock minitest to work arou…

be667eb

…nd a bug in newer versions

refactor PasswordArchiveable to eliminate deprecation warnings and to…

771d59c

… simplify the code

add some tests and migrations (to support them) to expose an issue wh…

23f6509

…ere indirectly saving a user (by saving a dependent object) would trigger an activesupport deprecation warning.

refactor PasswordArchiveable to avoid deprecation warnings and genera…

1354f46

…lly simplify the logic. Also adds a compatibility module to deal with API changes in the rails dirty attribute handling

tweak SecureVaidateable to make sure it works properly with api changes.

65d8f66

test tweaks. Mostly to account for small differences in how the Rails…

aed38f0

… API for testing changed between 4.2 and 5.0

no longer expect failures for 4.2

d72a999

set development version to 2.2.9

41b15c5

include compatibility in secure_validateable

1f45757

natebird approved these changes Mar 21, 2018

View reviewed changes

natebird mentioned this pull request Mar 21, 2018

Cleanup from Rubocop suggestions #21

Merged

Merge branch 'master' into issue-22-warnings

106ef29

dillonwelch requested changes Mar 21, 2018

View reviewed changes

formatting and a test fix

f93f6b1

natebird merged commit 1efd42d into devise-security:master Mar 21, 2018

olbrich added this to the v0.12.0 milestone Mar 22, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix deprecation warnings #26

Fix deprecation warnings #26

olbrich commented Mar 21, 2018 •

edited

coveralls commented Mar 21, 2018 •

edited

natebird left a comment

natebird Mar 21, 2018

olbrich Mar 21, 2018

dillonwelch Mar 21, 2018

olbrich Mar 21, 2018

dillonwelch left a comment

dillonwelch Mar 21, 2018

olbrich Mar 21, 2018 •

edited

dillonwelch Mar 21, 2018

dillonwelch Mar 21, 2018

dillonwelch Mar 21, 2018

natebird commented Mar 21, 2018

Fix deprecation warnings #26

Fix deprecation warnings #26

Conversation

olbrich commented Mar 21, 2018 • edited

coveralls commented Mar 21, 2018 • edited

Pull Request Test Coverage Report for Build 63

💛 - Coveralls

natebird left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dillonwelch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olbrich Mar 21, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natebird commented Mar 21, 2018

olbrich commented Mar 21, 2018 •

edited

coveralls commented Mar 21, 2018 •

edited

olbrich Mar 21, 2018 •

edited