From ac742e32710199fdd7cb81f50895de8cbb316be0 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jos=C3=A9=20Valim?= <jose.valim@gmail.com>
Date: Wed, 10 Mar 2010 16:13:54 +0100
Subject: [PATCH] Clean up lockable and class methods API.

---
 .../devise/confirmations_controller.rb        |  2 +-
 .../devise/passwords_controller.rb            |  2 +-
 app/controllers/devise/unlocks_controller.rb  |  2 +-
 lib/devise/models/confirmable.rb              | 14 ++---
 lib/devise/models/lockable.rb                 | 42 ++++++--------
 lib/devise/models/recoverable.rb              |  2 +-
 test/integration/lockable_test.rb             |  4 +-
 test/mailers/unlock_instructions_test.rb      |  2 +-
 test/models/confirmable_test.rb               | 12 ++--
 test/models/lockable_test.rb                  | 58 +++++++++----------
 test/models/recoverable_test.rb               |  8 +--
 test/support/integration.rb                   |  2 +-
 12 files changed, 71 insertions(+), 79 deletions(-)

diff --git a/app/controllers/devise/confirmations_controller.rb b/app/controllers/devise/confirmations_controller.rb
index 8f94a1dc..1b0bc140 100644
--- a/app/controllers/devise/confirmations_controller.rb
+++ b/app/controllers/devise/confirmations_controller.rb
@@ -21,7 +21,7 @@ class Devise::ConfirmationsController < ApplicationController
 
   # GET /resource/confirmation?confirmation_token=abcdef
   def show
-    self.resource = resource_class.confirm!(:confirmation_token => params[:confirmation_token])
+    self.resource = resource_class.confirm_by_token(params[:confirmation_token])
 
     if resource.errors.empty?
       set_flash_message :notice, :confirmed
diff --git a/app/controllers/devise/passwords_controller.rb b/app/controllers/devise/passwords_controller.rb
index 20e4e7bc..29b99bf7 100644
--- a/app/controllers/devise/passwords_controller.rb
+++ b/app/controllers/devise/passwords_controller.rb
@@ -30,7 +30,7 @@ class Devise::PasswordsController < ApplicationController
 
   # PUT /resource/password
   def update
-    self.resource = resource_class.reset_password!(params[resource_name])
+    self.resource = resource_class.reset_password_by_token(params[resource_name])
 
     if resource.errors.empty?
       set_flash_message :notice, :updated
diff --git a/app/controllers/devise/unlocks_controller.rb b/app/controllers/devise/unlocks_controller.rb
index a13a1145..02e133ba 100644
--- a/app/controllers/devise/unlocks_controller.rb
+++ b/app/controllers/devise/unlocks_controller.rb
@@ -21,7 +21,7 @@ class Devise::UnlocksController < ApplicationController
 
   # GET /resource/unlock?unlock_token=abcdef
   def show
-    self.resource = resource_class.unlock!(:unlock_token => params[:unlock_token])
+    self.resource = resource_class.unlock_access_by_token(params[:unlock_token])
 
     if resource.errors.empty?
       set_flash_message :notice, :unlocked
diff --git a/lib/devise/models/confirmable.rb b/lib/devise/models/confirmable.rb
index bcda7e94..88bef8d3 100644
--- a/lib/devise/models/confirmable.rb
+++ b/lib/devise/models/confirmable.rb
@@ -60,7 +60,7 @@ module Devise
       # Remove confirmation date and send confirmation instructions, to ensure
       # after sending these instructions the user won't be able to sign in without
       # confirming it's account
-      def resend_confirmation!
+      def resend_confirmation_token
         unless_confirmed do
           generate_confirmation_token
           save(:validate => false)
@@ -78,11 +78,7 @@ module Devise
 
       # The message to be shown if the account is inactive.
       def inactive_message
-        if !confirmed?
-          :unconfirmed
-        else
-          super
-        end
+        !confirmed? ? :unconfirmed : super
       end
 
       # If you don't want confirmation to be sent on create, neither a code
@@ -148,7 +144,7 @@ module Devise
         # Options must contain the user email
         def send_confirmation_instructions(attributes={})
           confirmable = find_or_initialize_with_error_by(:email, attributes[:email], :not_found)
-          confirmable.resend_confirmation! unless confirmable.new_record?
+          confirmable.resend_confirmation_token unless confirmable.new_record?
           confirmable
         end
 
@@ -156,8 +152,8 @@ module Devise
         # If no user is found, returns a new user with an error.
         # If the user is already confirmed, create an error for the user
         # Options must have the confirmation_token
-        def confirm!(attributes={})
-          confirmable = find_or_initialize_with_error_by(:confirmation_token, attributes[:confirmation_token])        
+        def confirm_by_token(confirmation_token)
+          confirmable = find_or_initialize_with_error_by(:confirmation_token, confirmation_token)
           confirmable.confirm! unless confirmable.new_record?
           confirmable
         end
diff --git a/lib/devise/models/lockable.rb b/lib/devise/models/lockable.rb
index 5a7a9880..3212601b 100644
--- a/lib/devise/models/lockable.rb
+++ b/lib/devise/models/lockable.rb
@@ -22,23 +22,20 @@ module Devise
       include Devise::Models::Activatable
 
       # Lock an user setting it's locked_at to actual time.
-      def lock
+      def lock_access!
         self.locked_at = Time.now
+
         if unlock_strategy_enabled?(:email)
           generate_unlock_token
           send_unlock_instructions
         end
-      end
 
-      # Lock an user also saving the record.
-      def lock!
-        lock
         save(:validate => false)
       end
 
       # Unlock an user by cleaning locket_at and failed_attempts.
-      def unlock!
-        if_locked do
+      def unlock_access!
+        if_access_locked do
           self.locked_at = nil
           self.failed_attempts = 0
           self.unlock_token = nil
@@ -47,7 +44,7 @@ module Devise
       end
 
       # Verifies whether a user is locked or not.
-      def locked?
+      def access_locked?
         locked_at && !lock_expired?
       end
 
@@ -57,8 +54,8 @@ module Devise
       end
 
       # Resend the unlock instructions if the user is locked.
-      def resend_unlock!
-        if_locked do
+      def resend_unlock_token
+        if_access_locked do
           generate_unlock_token unless unlock_token.present?
           save(:validate => false)
           send_unlock_instructions
@@ -68,17 +65,13 @@ module Devise
       # Overwrites active? from Devise::Models::Activatable for locking purposes
       # by verifying whether an user is active to sign in or not based on locked?
       def active?
-        super && !locked?
+        super && !access_locked?
       end
 
       # Overwrites invalid_message from Devise::Models::Authenticatable to define
       # the correct reason for blocking the sign in.
       def inactive_message
-        if locked?
-          :locked
-        else
-          super
-        end
+        access_locked? ? :locked : super
       end
 
       # Overwrites valid_for_authentication? from Devise::Models::Authenticatable
@@ -89,7 +82,10 @@ module Devise
           self.failed_attempts = 0
         else
           self.failed_attempts += 1
-          lock if failed_attempts > self.class.maximum_attempts
+          if failed_attempts > self.class.maximum_attempts
+            lock_access! 
+            return false
+          end
         end
         save(:validate => false) if changed?
         result
@@ -113,8 +109,8 @@ module Devise
 
         # Checks whether the record is locked or not, yielding to the block
         # if it's locked, otherwise adds an error to email.
-        def if_locked
-          if locked?
+        def if_access_locked
+          if access_locked?
             yield
           else
             self.errors.add(:email, :not_locked)
@@ -134,7 +130,7 @@ module Devise
         # Options must contain the user email
         def send_unlock_instructions(attributes={})
          lockable = find_or_initialize_with_error_by(:email, attributes[:email], :not_found)
-         lockable.resend_unlock! unless lockable.new_record?
+         lockable.resend_unlock_token unless lockable.new_record?
          lockable
         end
 
@@ -142,9 +138,9 @@ module Devise
         # If no user is found, returns a new user with an error.
         # If the user is not locked, creates an error for the user
         # Options must have the unlock_token
-        def unlock!(attributes={})
-          lockable = find_or_initialize_with_error_by(:unlock_token, attributes[:unlock_token])
-          lockable.unlock! unless lockable.new_record?
+        def unlock_access_by_token(unlock_token)
+          lockable = find_or_initialize_with_error_by(:unlock_token, unlock_token)
+          lockable.unlock_access! unless lockable.new_record?
           lockable
         end
 
diff --git a/lib/devise/models/recoverable.rb b/lib/devise/models/recoverable.rb
index a50cd1d2..fd80c665 100644
--- a/lib/devise/models/recoverable.rb
+++ b/lib/devise/models/recoverable.rb
@@ -65,7 +65,7 @@ module Devise
         # try saving the record. If not user is found, returns a new user
         # containing an error in reset_password_token attribute.
         # Attributes must contain reset_password_token, password and confirmation
-        def reset_password!(attributes={})
+        def reset_password_by_token(attributes={})
           recoverable = find_or_initialize_with_error_by(:reset_password_token, attributes[:reset_password_token])
           recoverable.reset_password!(attributes[:password], attributes[:password_confirmation]) unless recoverable.new_record?
           recoverable
diff --git a/test/integration/lockable_test.rb b/test/integration/lockable_test.rb
index e0211e3a..0bf7e26a 100644
--- a/test/integration/lockable_test.rb
+++ b/test/integration/lockable_test.rb
@@ -47,14 +47,14 @@ class LockTest < ActionController::IntegrationTest
 
   test "locked user should be able to unlock account" do
     user = create_user(:locked => true)
-    assert user.locked?
+    assert user.access_locked?
 
     visit_user_unlock_with_token(user.unlock_token)
 
     assert_template 'home/index'
     assert_contain 'Your account was successfully unlocked.'
 
-    assert_not user.reload.locked?
+    assert_not user.reload.access_locked?
   end
 
   test "sign in user automatically after unlocking it's account" do
diff --git a/test/mailers/unlock_instructions_test.rb b/test/mailers/unlock_instructions_test.rb
index 7923b745..1f1d46d7 100644
--- a/test/mailers/unlock_instructions_test.rb
+++ b/test/mailers/unlock_instructions_test.rb
@@ -10,7 +10,7 @@ class UnlockInstructionsTest < ActionMailer::TestCase
   def user
     @user ||= begin
       user = create_user
-      user.lock!
+      user.lock_access!
       user
     end
   end
diff --git a/test/models/confirmable_test.rb b/test/models/confirmable_test.rb
index 58efdc2d..eea24f0c 100644
--- a/test/models/confirmable_test.rb
+++ b/test/models/confirmable_test.rb
@@ -15,7 +15,7 @@ class ConfirmableTest < ActiveSupport::TestCase
     user = create_user
     3.times do
       token = user.confirmation_token
-      user.resend_confirmation!
+      user.resend_confirmation_token
       assert_not_equal token, user.confirmation_token
     end
   end
@@ -62,19 +62,19 @@ class ConfirmableTest < ActiveSupport::TestCase
 
   test 'should find and confirm an user automatically' do
     user = create_user
-    confirmed_user = User.confirm!(:confirmation_token => user.confirmation_token)
+    confirmed_user = User.confirm_by_token(user.confirmation_token)
     assert_equal confirmed_user, user
     assert user.reload.confirmed?
   end
 
   test 'should return a new record with errors when a invalid token is given' do
-    confirmed_user = User.confirm!(:confirmation_token => 'invalid_confirmation_token')
+    confirmed_user = User.confirm_by_token('invalid_confirmation_token')
     assert confirmed_user.new_record?
     assert_equal "is invalid", confirmed_user.errors[:confirmation_token].join
   end
 
   test 'should return a new record with errors when a blank token is given' do
-    confirmed_user = User.confirm!(:confirmation_token => '')
+    confirmed_user = User.confirm_by_token('')
     assert confirmed_user.new_record?
     assert_equal "can't be blank", confirmed_user.errors[:confirmation_token].join
   end
@@ -83,7 +83,7 @@ class ConfirmableTest < ActiveSupport::TestCase
     user = create_user
     user.confirmed_at = Time.now
     user.save
-    confirmed_user = User.confirm!(:confirmation_token => user.confirmation_token)
+    confirmed_user = User.confirm_by_token(user.confirmation_token)
     assert confirmed_user.confirmed?
     assert_equal "was already confirmed", confirmed_user.errors[:email].join
   end
@@ -173,7 +173,7 @@ class ConfirmableTest < ActiveSupport::TestCase
   test 'should not be able to send instructions if the user is already confirmed' do
     user = create_user
     user.confirm!
-    assert_not user.resend_confirmation!
+    assert_not user.resend_confirmation_token
     assert user.confirmed?
     assert_equal 'was already confirmed', user.errors[:email].join
   end
diff --git a/test/models/lockable_test.rb b/test/models/lockable_test.rb
index a761b422..a32e83dc 100644
--- a/test/models/lockable_test.rb
+++ b/test/models/lockable_test.rb
@@ -18,14 +18,14 @@ class LockableTest < ActiveSupport::TestCase
     user = create_user
     attempts = Devise.maximum_attempts + 1
     attempts.times { authenticated_user = User.authenticate(:email => user.email, :password => "anotherpassword") }
-    assert user.reload.locked?
+    assert user.reload.access_locked?
   end
 
   test "should respect maximum attempts configuration" do
     user = create_user
     swap Devise, :maximum_attempts => 2 do
       3.times { authenticated_user = User.authenticate(:email => user.email, :password => "anotherpassword") }
-      assert user.reload.locked?
+      assert user.reload.access_locked?
     end
   end
 
@@ -39,26 +39,26 @@ class LockableTest < ActiveSupport::TestCase
 
   test "should verify wheter a user is locked or not" do
     user = create_user
-    assert_not user.locked?
-    user.lock!
-    assert user.locked?
+    assert_not user.access_locked?
+    user.lock_access!
+    assert user.access_locked?
   end
 
   test "active? should be the opposite of locked?" do
     user = create_user
     user.confirm!
     assert user.active?
-    user.lock!
+    user.lock_access!
     assert_not user.active?
   end
 
   test "should unlock an user by cleaning locked_at, falied_attempts and unlock_token" do
     user = create_user
-    user.lock!
+    user.lock_access!
     assert_not_nil user.reload.locked_at
     assert_not_nil user.reload.unlock_token
 
-    user.unlock!
+    user.unlock_access!
     assert_nil user.reload.locked_at
     assert_nil user.reload.unlock_token
     assert 0, user.reload.failed_attempts
@@ -66,12 +66,12 @@ class LockableTest < ActiveSupport::TestCase
 
   test 'should not unlock an unlocked user' do
     user = create_user
-    assert_not user.unlock!
+    assert_not user.unlock_access!
     assert_match "was not locked", user.errors[:email].join
   end
 
   test "new user should not be locked and should have zero failed_attempts" do
-    assert_not new_user.locked?
+    assert_not new_user.access_locked?
     assert_equal 0, create_user.failed_attempts
   end
 
@@ -79,10 +79,10 @@ class LockableTest < ActiveSupport::TestCase
     swap Devise, :unlock_in => 3.hours do
       user = new_user
       user.locked_at = 2.hours.ago
-      assert user.locked?
+      assert user.access_locked?
 
       Devise.unlock_in = 1.hour
-      assert_not user.locked?
+      assert_not user.access_locked?
     end
   end
 
@@ -90,14 +90,14 @@ class LockableTest < ActiveSupport::TestCase
     swap Devise, :unlock_strategy => :email do
       user = new_user
       user.locked_at = 2.hours.ago
-      assert user.locked?
+      assert user.access_locked?
     end
   end
 
   test "should set unlock_token when locking" do
     user = create_user
     assert_nil user.unlock_token
-    user.lock!
+    user.lock_access!
     assert_not_nil user.unlock_token
   end
 
@@ -106,7 +106,7 @@ class LockableTest < ActiveSupport::TestCase
     user.lock!
     3.times do
       token = user.unlock_token
-      user.resend_unlock!
+      user.resend_unlock_token
       assert_equal token, user.unlock_token
     end
   end
@@ -115,7 +115,7 @@ class LockableTest < ActiveSupport::TestCase
     unlock_tokens = []
     3.times do
       user = create_user
-      user.lock!
+      user.lock_access!
       token = user.unlock_token
       assert !unlock_tokens.include?(token)
       unlock_tokens << token
@@ -125,7 +125,7 @@ class LockableTest < ActiveSupport::TestCase
   test "should not generate unlock_token when :email is not an unlock strategy" do
     swap Devise, :unlock_strategy => :time do
       user = create_user
-      user.lock!
+      user.lock_access!
       assert_nil user.unlock_token
     end
   end
@@ -134,7 +134,7 @@ class LockableTest < ActiveSupport::TestCase
     swap Devise, :unlock_strategy => :email do
       user = create_user
       assert_email_sent do
-        user.lock!
+        user.lock_access!
       end
     end
   end
@@ -143,42 +143,42 @@ class LockableTest < ActiveSupport::TestCase
     swap Devise, :unlock_strategy => :time do
       user = create_user
       assert_email_not_sent do
-        user.lock!
+        user.lock_access!
       end
     end
   end
 
   test 'should find and unlock an user automatically' do
     user = create_user
-    user.lock!
-    locked_user = User.unlock!(:unlock_token => user.unlock_token)
+    user.lock_access!
+    locked_user = User.unlock_access_by_token(user.unlock_token)
     assert_equal locked_user, user
-    assert_not user.reload.locked?
+    assert_not user.reload.access_locked?
   end
 
   test 'should return a new record with errors when a invalid token is given' do
-    locked_user = User.unlock!(:unlock_token => 'invalid_token')
+    locked_user = User.unlock_access_by_token('invalid_token')
     assert locked_user.new_record?
     assert_equal "is invalid", locked_user.errors[:unlock_token].join
   end
 
   test 'should return a new record with errors when a blank token is given' do
-    locked_user = User.unlock!(:unlock_token => '')
+    locked_user = User.unlock_access_by_token('')
     assert locked_user.new_record?
     assert_equal "can't be blank", locked_user.errors[:unlock_token].join
   end
 
   test 'should authenticate a unlocked user' do
     user = create_user
-    user.lock!
-    user.unlock!
+    user.lock_access!
+    user.unlock_access!
     authenticated_user = User.authenticate(:email => user.email, :password => user.password)
     assert_equal authenticated_user, user
   end
 
   test 'should find a user to send unlock instructions' do
     user = create_user
-    user.lock!
+    user.lock_access!
     unlock_user = User.send_unlock_instructions(:email => user.email)
     assert_equal unlock_user, user
   end
@@ -195,8 +195,8 @@ class LockableTest < ActiveSupport::TestCase
 
   test 'should not be able to send instructions if the user is not locked' do
     user = create_user
-    assert_not user.resend_unlock!
-    assert_not user.locked?
+    assert_not user.resend_unlock_token
+    assert_not user.access_locked?
     assert_equal 'was not locked', user.errors[:email].join
   end
 
diff --git a/test/models/recoverable_test.rb b/test/models/recoverable_test.rb
index 82a56f50..42646d1b 100644
--- a/test/models/recoverable_test.rb
+++ b/test/models/recoverable_test.rb
@@ -104,18 +104,18 @@ class RecoverableTest < ActiveSupport::TestCase
     user = create_user
     user.send :generate_reset_password_token!
 
-    reset_password_user = User.reset_password!(:reset_password_token => user.reset_password_token)
+    reset_password_user = User.reset_password_by_token(:reset_password_token => user.reset_password_token)
     assert_equal reset_password_user, user
   end
 
   test 'should a new record with errors if no reset_password_token is found' do
-    reset_password_user = User.reset_password!(:reset_password_token => 'invalid_token')
+    reset_password_user = User.reset_password_by_token(:reset_password_token => 'invalid_token')
     assert reset_password_user.new_record?
     assert_equal "is invalid", reset_password_user.errors[:reset_password_token].join
   end
 
   test 'should a new record with errors if reset_password_token is blank' do
-    reset_password_user = User.reset_password!(:reset_password_token => '')
+    reset_password_user = User.reset_password_by_token(:reset_password_token => '')
     assert reset_password_user.new_record?
     assert_match "can't be blank", reset_password_user.errors[:reset_password_token].join
   end
@@ -125,7 +125,7 @@ class RecoverableTest < ActiveSupport::TestCase
     old_password = user.password
     user.send :generate_reset_password_token!
 
-    reset_password_user = User.reset_password!(
+    reset_password_user = User.reset_password_by_token(
       :reset_password_token => user.reset_password_token,
       :password => 'new_password',
       :password_confirmation => 'new_password'
diff --git a/test/support/integration.rb b/test/support/integration.rb
index 6607874b..62a0b544 100644
--- a/test/support/integration.rb
+++ b/test/support/integration.rb
@@ -14,7 +14,7 @@ class ActionController::IntegrationTest
         :created_at => Time.now.utc
       )
       user.confirm! unless options[:confirm] == false
-      user.lock! if options[:locked] == true
+      user.lock_access! if options[:locked] == true
       user
     end
   end