Refactor according to rodrigoflores

- Favor using update_attribute instead of constructor parameters in user
  factory for tests
- Test for accurate error message when confirmation token is expired
- Don't check twice whether the confirmation period is expired

lib/devise/models/confirmable.rb

@@ -30,7 +30,6 @@ module Devise
     #
     module Confirmable
       extend ActiveSupport::Concern
-      # TODO: is this a good idea?
       include ActionView::Helpers::DateHelper
 
       included do
@@ -159,30 +158,18 @@ module Devise
           confirmation_sent_at && confirmation_sent_at.utc >= self.class.allow_unconfirmed_access_for.ago
         end
 
-        # Checks if the user confirmation happens before the token becomes invalid
-        #
-        # Examples:
-        #
-        #   # expire_confirmation_token_after = 3.days and confirmation_sent_at = 2.days.ago
-        #   confirmation_period_expired?  # returns false
-        #
-        #   # expire_confirmation_token_after = 3.days and confirmation_sent_at = 4.days.ago
-        #   confirmation_period_expired?  # returns true
-        #
-        #   # expire_confirmation_token_after = nil
-        #   confirmation_period_expired?  # will always return false
-        #
-        def confirmation_period_expired?
-          self.class.expire_confirmation_token_after && (Time.now > self.confirmation_sent_at + self.class.expire_confirmation_token_after)
-        end
-
         # Checks whether the record requires any confirmation.
         def pending_any_confirmation
-          unless confirmation_period_expired? || (confirmed? && !pending_reconfirmation?)
+          @confirmation_period_expired = if @confirmation_period_expired.nil?
+                                           self.class.expire_confirmation_token_after && (Time.now > self.confirmation_sent_at + self.class.expire_confirmation_token_after )
+                                         else
+                                           @confirmation_period_expired
+                                         end
+
+          if (!confirmed? || pending_reconfirmation?) && !@confirmation_period_expired
             yield
           else
-            # TODO: cache this call or not?
+            if @confirmation_period_expired
-            if confirmation_period_expired?
               self.errors.add(:email, :confirmation_period_expired, period: time_ago_in_words(self.class.expire_confirmation_token_after.ago))
             else
               self.errors.add(:email, :already_confirmed)

test/integration/confirmable_test.rb

@@ -50,28 +50,22 @@ class ConfirmationTest < ActionController::IntegrationTest
     assert user.reload.confirmed?
   end
 
+  include ActionView::Helpers::DateHelper
   test 'user with valid confirmation token should not be able to confirm an account after the token has expired' do
     swap Devise, :expire_confirmation_token_after => 3.days do
-      # TODO: why is confirmation_sent_at not set correctly when passed to create_user?
-      # TODO: User.create! always sets confirmation_sent_at to Time.now
       user = create_user(:confirm => false, :confirmation_sent_at => 4.days.ago)
-      user.confirmation_sent_at = 4.days.ago
-      user.save
       assert_not user.confirmed?
       visit_user_confirmation_with_token(user.confirmation_token)
 
       assert_have_selector '#error_explanation'
-      assert_contain /needs to be confirmed within/
+      assert_contain /needs to be confirmed within 3 days/
       assert_not user.reload.confirmed?
     end
   end
 
   test 'user with valid confirmation token should be able to confirm an account before the token has expired' do
     swap Devise, :expire_confirmation_token_after => 3.days do
-      # TODO: why is confirmation_sent_at not set correctly when passed to create_user?
       user = create_user(:confirm => false, :confirmation_sent_at => 2.days.ago)
-      user.confirmation_sent_at = 2.days.ago
-      user.save
       assert_not user.confirmed?
       visit_user_confirmation_with_token(user.confirmation_token)
 

test/models/confirmable_test.rb

@@ -237,11 +237,8 @@ class ConfirmableTest < ActiveSupport::TestCase
   end
 
   def confirm_user_by_token_with_confirmation_sent_at(confirmation_sent_at)
-    # TODO: why is confirmation_sent_at not set correctly when passed to create_user?
-    # TODO: User.create! always sets confirmation_sent_at to Time.now
     user = create_user
-    user.confirmation_sent_at = confirmation_sent_at
+    user.update_attribute(:confirmation_sent_at, confirmation_sent_at)
-    user.save
     confirmed_user = User.confirm_by_token(user.confirmation_token)
     assert_equal confirmed_user, user
     user.reload.confirmed?

test/support/helpers.rb

@@ -26,8 +26,7 @@ class ActiveSupport::TestCase
     { :username => "usertest",
       :email => generate_unique_email,
       :password => '12345678',
-      :password_confirmation => '12345678',
+      :password_confirmation => '12345678' }.update(attributes)
-      :confirmation_sent_at => nil }.update(attributes)
   end
 
   def new_user(attributes={})

test/support/integration.rb

@@ -12,9 +12,9 @@ class ActionDispatch::IntegrationTest
         :email => options[:email] || 'user@test.com',
         :password => options[:password] || '12345678',
         :password_confirmation => options[:password] || '12345678',
-        :created_at => Time.now.utc,
+        :created_at => Time.now.utc
-        :confirmation_sent_at => options[:confirmation_sent_at]
       )
+      user.update_attribute(:confirmation_sent_at, options[:confirmation_sent_at]) if options[:confirmation_sent_at]
       user.confirm! unless options[:confirm] == false
       user.lock_access! if options[:locked] == true
       user