From c22e7133b68a2e89d009adc8a780455f2bc6e61f Mon Sep 17 00:00:00 2001
From: Nicolas Viennot <nicolas@viennot.biz>
Date: Sun, 19 Apr 2015 10:41:06 -0400
Subject: [PATCH] Removes the bang in confirm! and reset_password!

Closes #3412 and #3570.
---
 CHANGELOG.md                                  |  4 ++
 lib/devise/models/confirmable.rb              | 11 +++--
 lib/devise/models/recoverable.rb              | 11 +++--
 .../custom_registrations_controller_test.rb   |  2 +-
 test/controllers/passwords_controller_test.rb |  2 +-
 test/controllers/sessions_controller_test.rb  |  6 +--
 test/models/confirmable_test.rb               | 46 +++++++++----------
 test/models/lockable_test.rb                  | 12 ++---
 test/models/recoverable_test.rb               | 10 ++--
 test/support/integration.rb                   |  4 +-
 test/test_helpers_test.rb                     | 14 +++---
 11 files changed, 68 insertions(+), 54 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 18ef44b4..2bca470d 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -13,6 +13,10 @@
   * Controllers inheriting from any Devise core controller will now use appropriate translations.
     The i18n scope can be overridden in `translation_scope`.
 
+* deprecations
+  * `confirm!` has been deprecated in favor of `confirm`.
+  * `reset_password!` has been deprecated in favor of `reset_password`.
+
 ### 3.4.1 - 2014-10-29
 
 * enhancements
diff --git a/lib/devise/models/confirmable.rb b/lib/devise/models/confirmable.rb
index 07adbe48..5d0ce632 100644
--- a/lib/devise/models/confirmable.rb
+++ b/lib/devise/models/confirmable.rb
@@ -24,7 +24,7 @@ module Devise
     #
     # == Examples
     #
-    #   User.find(1).confirm!      # returns true unless it's already confirmed
+    #   User.find(1).confirm       # returns true unless it's already confirmed
     #   User.find(1).confirmed?    # true/false
     #   User.find(1).send_confirmation_instructions # manually send instructions
     #
@@ -56,7 +56,7 @@ module Devise
       # Confirm a user by setting it's confirmed_at to actual time. If the user
       # is already confirmed, add an error to email field. If the user is invalid
       # add errors
-      def confirm!(args={})
+      def confirm(args={})
         pending_any_confirmation do
           if confirmation_period_expired?
             self.errors.add(:email, :confirmation_period_expired,
@@ -82,6 +82,11 @@ module Devise
         end
       end
 
+      def confirm!(args={})
+        ActiveSupport::Deprecation.warn "confirm! is deprecated in favor of confirm"
+        confirm(args)
+      end
+
       # Verifies whether a user is confirmed or not
       def confirmed?
         !!confirmed_at
@@ -284,7 +289,7 @@ module Devise
           confirmation_token = Devise.token_generator.digest(self, :confirmation_token, confirmation_token)
 
           confirmable = find_or_initialize_with_error_by(:confirmation_token, confirmation_token)
-          confirmable.confirm! if confirmable.persisted?
+          confirmable.confirm if confirmable.persisted?
           confirmable.confirmation_token = original_token
           confirmable
         end
diff --git a/lib/devise/models/recoverable.rb b/lib/devise/models/recoverable.rb
index 3dce0d7c..58194174 100644
--- a/lib/devise/models/recoverable.rb
+++ b/lib/devise/models/recoverable.rb
@@ -14,7 +14,7 @@ module Devise
     # == Examples
     #
     #   # resets the user password and save the record, true if valid passwords are given, otherwise false
-    #   User.find(1).reset_password!('password123', 'password123')
+    #   User.find(1).reset_password('password123', 'password123')
     #
     #   # only resets the user password, without saving the record
     #   user = User.find(1)
@@ -32,7 +32,7 @@ module Devise
 
       # Update password saving the record and clearing token. Returns true if
       # the passwords are valid and the record was saved, false otherwise.
-      def reset_password!(new_password, new_password_confirmation)
+      def reset_password(new_password, new_password_confirmation)
         self.password = new_password
         self.password_confirmation = new_password_confirmation
 
@@ -44,6 +44,11 @@ module Devise
         save
       end
 
+      def reset_password!(new_password, new_password_confirmation)
+        ActiveSupport::Deprecation.warn "reset_password! is deprecated in favor of reset_password"
+        reset_password(new_password, new_password_confirmation)
+      end
+
       # Resets reset password token and send reset password instructions by email.
       # Returns the token sent in the e-mail.
       def send_reset_password_instructions
@@ -142,7 +147,7 @@ module Devise
 
           if recoverable.persisted?
             if recoverable.reset_password_period_valid?
-              recoverable.reset_password!(attributes[:password], attributes[:password_confirmation])
+              recoverable.reset_password(attributes[:password], attributes[:password_confirmation])
             else
               recoverable.errors.add(:reset_password_token, :expired)
             end
diff --git a/test/controllers/custom_registrations_controller_test.rb b/test/controllers/custom_registrations_controller_test.rb
index 10e81e48..0421ad03 100644
--- a/test/controllers/custom_registrations_controller_test.rb
+++ b/test/controllers/custom_registrations_controller_test.rb
@@ -8,7 +8,7 @@ class CustomRegistrationsControllerTest < ActionController::TestCase
   setup do
     request.env["devise.mapping"] = Devise.mappings[:user]
     @password = 'password'
-    @user = create_user(password: @password, password_confirmation: @password).tap(&:confirm!)
+    @user = create_user(password: @password, password_confirmation: @password).tap(&:confirm)
   end
 
   test "yield resource to block on create success" do
diff --git a/test/controllers/passwords_controller_test.rb b/test/controllers/passwords_controller_test.rb
index 3c225cbf..383724b8 100644
--- a/test/controllers/passwords_controller_test.rb
+++ b/test/controllers/passwords_controller_test.rb
@@ -6,7 +6,7 @@ class PasswordsControllerTest < ActionController::TestCase
 
   setup do
     request.env["devise.mapping"] = Devise.mappings[:user]
-    @user = create_user.tap(&:confirm!)
+    @user = create_user.tap(&:confirm)
     @raw  = @user.send_reset_password_instructions
   end
 
diff --git a/test/controllers/sessions_controller_test.rb b/test/controllers/sessions_controller_test.rb
index f82d1e9d..d931bf83 100644
--- a/test/controllers/sessions_controller_test.rb
+++ b/test/controllers/sessions_controller_test.rb
@@ -36,7 +36,7 @@ class SessionsControllerTest < ActionController::TestCase
     request.session["user_return_to"] = 'foo.bar'
 
     user = create_user
-    user.confirm!
+    user.confirm
     post :create, user: {
       email: user.email,
       password: user.password
@@ -50,7 +50,7 @@ class SessionsControllerTest < ActionController::TestCase
     request.session["user_return_to"] = 'foo.bar'
 
     user = create_user
-    user.confirm!
+    user.confirm
     post :create, format: 'json', user: {
       email: user.email,
       password: user.password
@@ -72,7 +72,7 @@ class SessionsControllerTest < ActionController::TestCase
   test "#destroy doesn't set the flash if the requested format is not navigational" do
     request.env["devise.mapping"] = Devise.mappings[:user]
     user = create_user
-    user.confirm!
+    user.confirm
     post :create, format: 'json', user: {
       email: user.email,
       password: user.password
diff --git a/test/models/confirmable_test.rb b/test/models/confirmable_test.rb
index c4ce4e55..fa3a226b 100644
--- a/test/models/confirmable_test.rb
+++ b/test/models/confirmable_test.rb
@@ -23,7 +23,7 @@ class ConfirmableTest < ActiveSupport::TestCase
   test 'should confirm a user by updating confirmed at' do
     user = create_user
     assert_nil user.confirmed_at
-    assert user.confirm!
+    assert user.confirm
     assert_not_nil user.confirmed_at
   end
 
@@ -31,16 +31,16 @@ class ConfirmableTest < ActiveSupport::TestCase
     assert_not new_user.confirmed?
     user = create_user
     assert_not user.confirmed?
-    user.confirm!
+    user.confirm
     assert user.confirmed?
   end
 
   test 'should not confirm a user already confirmed' do
     user = create_user
-    assert user.confirm!
+    assert user.confirm
     assert_blank user.errors[:email]
 
-    assert_not user.confirm!
+    assert_not user.confirm
     assert_equal "was already confirmed, please try signing in", user.errors[:email].join
   end
 
@@ -169,7 +169,7 @@ class ConfirmableTest < ActiveSupport::TestCase
   test 'should not reset confirmation status or token when updating email' do
     user = create_user
     original_token = user.confirmation_token
-    user.confirm!
+    user.confirm
     user.email = 'new_test@example.com'
     user.save!
 
@@ -180,7 +180,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!
+    user.confirm
     assert_not user.resend_confirmation_instructions
     assert user.confirmed?
     assert_equal 'was already confirmed, please try signing in', user.errors[:email].join
@@ -215,7 +215,7 @@ class ConfirmableTest < ActiveSupport::TestCase
     assert_not user.confirmed?
     assert_not user.active_for_authentication?
 
-    user.confirm!
+    user.confirm
     assert user.confirmed?
     assert user.active_for_authentication?
   end
@@ -305,25 +305,25 @@ class ConfirmableTest < ActiveSupport::TestCase
       self.username = self.username.to_s + 'updated'
     end
     old = user.username
-    assert user.confirm!
+    assert user.confirm
     assert_not_equal user.username, old
   end
 
   test 'should not call after_confirmation if not confirmed' do
     user = create_user
-    assert user.confirm!
+    assert user.confirm
     user.define_singleton_method :after_confirmation do
       self.username = self.username.to_s + 'updated'
     end
     old = user.username
-    assert_not user.confirm!
+    assert_not user.confirm
     assert_equal user.username, old
   end
 
   test 'should always perform validations upon confirm when ensure valid true' do
     admin = create_admin
     admin.stubs(:valid?).returns(false)
-    assert_not admin.confirm!(ensure_valid: true)
+    assert_not admin.confirm(ensure_valid: true)
   end
 end
 
@@ -331,12 +331,12 @@ class ReconfirmableTest < ActiveSupport::TestCase
   test 'should not worry about validations on confirm even with reconfirmable' do
     admin = create_admin
     admin.reset_password_token = "a"
-    assert admin.confirm!
+    assert admin.confirm
   end
 
   test 'should generate confirmation token after changing email' do
     admin = create_admin
-    assert admin.confirm!
+    assert admin.confirm
     residual_token = admin.confirmation_token
     assert admin.update_attributes(email: 'new_test@example.com')
     assert_not_equal residual_token, admin.confirmation_token
@@ -345,7 +345,7 @@ class ReconfirmableTest < ActiveSupport::TestCase
   test 'should not regenerate confirmation token or require reconfirmation if skipping reconfirmation after changing email' do
     admin = create_admin
     original_token = admin.confirmation_token
-    assert admin.confirm!
+    assert admin.confirm
     admin.skip_reconfirmation!
     assert admin.update_attributes(email: 'new_test@example.com')
     assert admin.confirmed?
@@ -364,7 +364,7 @@ class ReconfirmableTest < ActiveSupport::TestCase
 
   test 'should regenerate confirmation token after changing email' do
     admin = create_admin
-    assert admin.confirm!
+    assert admin.confirm
     assert admin.update_attributes(email: 'old_test@example.com')
     token = admin.confirmation_token
     assert admin.update_attributes(email: 'new_test@example.com')
@@ -373,7 +373,7 @@ class ReconfirmableTest < ActiveSupport::TestCase
 
   test 'should send confirmation instructions by email after changing email' do
     admin = create_admin
-    assert admin.confirm!
+    assert admin.confirm
     assert_email_sent "new_test@example.com" do
       assert admin.update_attributes(email: 'new_test@example.com')
     end
@@ -382,7 +382,7 @@ class ReconfirmableTest < ActiveSupport::TestCase
 
   test 'should not send confirmation by email after changing password' do
     admin = create_admin
-    assert admin.confirm!
+    assert admin.confirm
     assert_email_not_sent do
       assert admin.update_attributes(password: 'newpass', password_confirmation: 'newpass')
     end
@@ -390,7 +390,7 @@ class ReconfirmableTest < ActiveSupport::TestCase
 
   test 'should not send confirmation by email after changing to a blank email' do
     admin = create_admin
-    assert admin.confirm!
+    assert admin.confirm
     assert_email_not_sent do
       admin.email = ''
       admin.save(validate: false)
@@ -399,23 +399,23 @@ class ReconfirmableTest < ActiveSupport::TestCase
 
   test 'should stay confirmed when email is changed' do
     admin = create_admin
-    assert admin.confirm!
+    assert admin.confirm
     assert admin.update_attributes(email: 'new_test@example.com')
     assert admin.confirmed?
   end
 
   test 'should update email only when it is confirmed' do
     admin = create_admin
-    assert admin.confirm!
+    assert admin.confirm
     assert admin.update_attributes(email: 'new_test@example.com')
     assert_not_equal 'new_test@example.com', admin.email
-    assert admin.confirm!
+    assert admin.confirm
     assert_equal 'new_test@example.com', admin.email
   end
 
   test 'should not allow admin to get past confirmation email by resubmitting their new address' do
     admin = create_admin
-    assert admin.confirm!
+    assert admin.confirm
     assert admin.update_attributes(email: 'new_test@example.com')
     assert_not_equal 'new_test@example.com', admin.email
     assert admin.update_attributes(email: 'new_test@example.com')
@@ -424,7 +424,7 @@ class ReconfirmableTest < ActiveSupport::TestCase
 
   test 'should find a admin by send confirmation instructions with unconfirmed_email' do
     admin = create_admin
-    assert admin.confirm!
+    assert admin.confirm
     assert admin.update_attributes(email: 'new_test@example.com')
     confirmation_admin = Admin.send_confirmation_instructions(email: admin.unconfirmed_email)
     assert_equal confirmation_admin, admin
diff --git a/test/models/lockable_test.rb b/test/models/lockable_test.rb
index 3f7284ca..8acbf3cf 100644
--- a/test/models/lockable_test.rb
+++ b/test/models/lockable_test.rb
@@ -7,7 +7,7 @@ class LockableTest < ActiveSupport::TestCase
 
   test "should respect maximum attempts configuration" do
     user = create_user
-    user.confirm!
+    user.confirm
     swap Devise, maximum_attempts: 2 do
       2.times { user.valid_for_authentication?{ false } }
       assert user.reload.access_locked?
@@ -16,7 +16,7 @@ class LockableTest < ActiveSupport::TestCase
 
   test "should increment failed_attempts on successfull validation if the user is already locked" do
     user = create_user
-    user.confirm!
+    user.confirm
 
     swap Devise, maximum_attempts: 2 do
       2.times { user.valid_for_authentication?{ false } }
@@ -29,7 +29,7 @@ class LockableTest < ActiveSupport::TestCase
 
   test "should not touch failed_attempts if lock_strategy is none" do
     user = create_user
-    user.confirm!
+    user.confirm
     swap Devise, lock_strategy: :none, maximum_attempts: 2 do
       3.times { user.valid_for_authentication?{ false } }
       assert !user.access_locked?
@@ -53,7 +53,7 @@ class LockableTest < ActiveSupport::TestCase
 
   test "active_for_authentication? should be the opposite of locked?" do
     user = create_user
-    user.confirm!
+    user.confirm
     assert user.active_for_authentication?
     user.lock_access!
     assert_not user.active_for_authentication?
@@ -230,7 +230,7 @@ class LockableTest < ActiveSupport::TestCase
   test 'should unlock account if lock has expired and increase attempts on failure' do
     swap Devise, unlock_in: 1.minute do
       user = create_user
-      user.confirm!
+      user.confirm
 
       user.failed_attempts = 2
       user.locked_at = 2.minutes.ago
@@ -243,7 +243,7 @@ class LockableTest < ActiveSupport::TestCase
   test 'should unlock account if lock has expired on success' do
     swap Devise, unlock_in: 1.minute do
       user = create_user
-      user.confirm!
+      user.confirm
 
       user.failed_attempts = 2
       user.locked_at = 2.minutes.ago
diff --git a/test/models/recoverable_test.rb b/test/models/recoverable_test.rb
index 95534d82..51c1348d 100644
--- a/test/models/recoverable_test.rb
+++ b/test/models/recoverable_test.rb
@@ -23,13 +23,13 @@ class RecoverableTest < ActiveSupport::TestCase
 
   test 'should reset password and password confirmation from params' do
     user = create_user
-    user.reset_password!('123456789', '987654321')
+    user.reset_password('123456789', '987654321')
     assert_equal '123456789', user.password
     assert_equal '987654321', user.password_confirmation
   end
 
   test 'should reset password and save the record' do
-    assert create_user.reset_password!('123456789', '123456789')
+    assert create_user.reset_password('123456789', '123456789')
   end
 
   test 'should clear reset password token while reseting the password' do
@@ -38,7 +38,7 @@ class RecoverableTest < ActiveSupport::TestCase
 
     user.send_reset_password_instructions
     assert_present user.reset_password_token
-    assert user.reset_password!('123456789', '123456789')
+    assert user.reset_password('123456789', '123456789')
     assert_nil user.reset_password_token
   end
 
@@ -46,14 +46,14 @@ class RecoverableTest < ActiveSupport::TestCase
     user = create_user
     user.send_reset_password_instructions
     assert_present user.reset_password_token
-    assert_not user.reset_password!('123456789', '987654321')
+    assert_not user.reset_password('123456789', '987654321')
     assert_present user.reset_password_token
   end
 
   test 'should not reset password with invalid data' do
     user = create_user
     user.stubs(:valid?).returns(false)
-    assert_not user.reset_password!('123456789', '987654321')
+    assert_not user.reset_password('123456789', '987654321')
   end
 
   test 'should reset reset password token and send instructions by email' do
diff --git a/test/support/integration.rb b/test/support/integration.rb
index 39111d52..ac140e86 100644
--- a/test/support/integration.rb
+++ b/test/support/integration.rb
@@ -15,7 +15,7 @@ class ActionDispatch::IntegrationTest
         created_at: Time.now.utc
       )
       user.update_attribute(:confirmation_sent_at, options[:confirmation_sent_at]) if options[:confirmation_sent_at]
-      user.confirm! unless options[:confirm] == false
+      user.confirm unless options[:confirm] == false
       user.lock_access! if options[:locked] == true
       user
     end
@@ -28,7 +28,7 @@ class ActionDispatch::IntegrationTest
         password: '123456', password_confirmation: '123456',
         active: options[:active]
       )
-      admin.confirm! unless options[:confirm] == false
+      admin.confirm unless options[:confirm] == false
       admin
     end
   end
diff --git a/test/test_helpers_test.rb b/test/test_helpers_test.rb
index 847258ab..2f02b173 100644
--- a/test/test_helpers_test.rb
+++ b/test/test_helpers_test.rb
@@ -34,7 +34,7 @@ class TestHelpersTest < ActionController::TestCase
 
   test "does not redirect with valid user" do
     user = create_user
-    user.confirm!
+    user.confirm
 
     sign_in user
     get :index
@@ -46,7 +46,7 @@ class TestHelpersTest < ActionController::TestCase
     assert_response :redirect
 
     user = create_user
-    user.confirm!
+    user.confirm
 
     sign_in user
     get :index
@@ -55,7 +55,7 @@ class TestHelpersTest < ActionController::TestCase
 
   test "redirects if valid user signed out" do
     user = create_user
-    user.confirm!
+    user.confirm
 
     sign_in user
     get :index
@@ -105,7 +105,7 @@ class TestHelpersTest < ActionController::TestCase
       end
 
       user = create_user
-      user.confirm!
+      user.confirm
       sign_in user
     ensure
       Warden::Manager._after_set_user.pop
@@ -118,7 +118,7 @@ class TestHelpersTest < ActionController::TestCase
         flunk "callback was called while it should not"
       end
       user = create_user
-      user.confirm!
+      user.confirm
 
       sign_in user
       sign_out user
@@ -146,7 +146,7 @@ class TestHelpersTest < ActionController::TestCase
 
   test "allows to sign in with different users" do
     first_user = create_user
-    first_user.confirm!
+    first_user.confirm
 
     sign_in first_user
     get :index
@@ -154,7 +154,7 @@ class TestHelpersTest < ActionController::TestCase
     sign_out first_user
 
     second_user = create_user
-    second_user.confirm!
+    second_user.confirm
 
     sign_in second_user
     get :index