Move destroy confirmation logic from model to controllerwinh-delete-account-modal

4 files changed, 91 insertions, 164 deletions

diff --git a/app/controllers/registrations_controller.rb b/app/controllers/registrations_controller.rb
index 1f3a936eb5c..d9142311b6f 100644
--- a/app/controllers/registrations_controller.rb
+++ b/app/controllers/registrations_controller.rb
@@ -25,29 +25,32 @@ class RegistrationsController < Devise::RegistrationsController
   end
 
   def destroy
-    begin
-      confirmation_params = params.permit(:username, :password)
-      current_user.delete_async(deleted_by: current_user, confirmation_params: confirmation_params)
-    rescue User::DeletionNotConfirmedError
-      alert = if current_user.confirm_deletion_with_password?
-                s_('Profiles|Invalid password')
-              else
-                s_('Profiles|Invalid username')
-              end
-
-      redirect_to profile_account_path, status: 303, alert: alert
-      return
+    if destroy_confirmation_valid?
+      current_user.delete_async(deleted_by: current_user)
+      session.try(:destroy)
+      redirect_to new_user_session_path, status: 303, notice: s_('Profiles|Account scheduled for removal.')
+    else
+      redirect_to profile_account_path, status: 303, alert: destroy_confirmation_failure_message
     end
+  end
+
+  protected
 
-    respond_to do |format|
-      format.html do
-        session.try(:destroy)
-        redirect_to new_user_session_path, status: 303, notice: s_('Profiles|Account scheduled for removal.')
-      end
+  def destroy_confirmation_valid?
+    if current_user.confirm_deletion_with_password?
+      current_user.valid_password?(params[:password])
+    else
+      current_user.username == params[:username]
     end
   end
 
-  protected
+  def destroy_confirmation_failure_message
+    if current_user.confirm_deletion_with_password?
+      s_('Profiles|Invalid password')
+    else
+      s_('Profiles|Invalid username')
+    end
+  end
 
   def build_resource(hash = nil)
     super
diff --git a/app/models/user.rb b/app/models/user.rb
index 7312d14969a..959738ba608 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -19,8 +19,6 @@ class User < ActiveRecord::Base
 
   DEFAULT_NOTIFICATION_LEVEL = :participating
 
-  DeletionNotConfirmedError = Class.new(StandardError)
-
   ignore_column :external_email
   ignore_column :email_provider
 
@@ -885,15 +883,7 @@ class User < ActiveRecord::Base
     system_hook_service.execute_hooks_for(self, :destroy)
   end
 
-  def delete_async(deleted_by:, params: {}, confirmation_params: {})
-    if deleted_by == self
-      if confirm_deletion_with_password?
-        raise DeletionNotConfirmedError unless valid_password?(confirmation_params[:password])
-      else
-        raise DeletionNotConfirmedError unless username == confirmation_params[:username]
-      end
-    end
-
+  def delete_async(deleted_by:, params: {})
     block if params[:hard_delete]
     DeleteUserWorker.perform_async(deleted_by.id, id, params)
   end
diff --git a/spec/controllers/registrations_controller_spec.rb b/spec/controllers/registrations_controller_spec.rb
index 4ce0a31002a..1d3ddfbd220 100644
--- a/spec/controllers/registrations_controller_spec.rb
+++ b/spec/controllers/registrations_controller_spec.rb
@@ -76,53 +76,67 @@ describe RegistrationsController do
       sign_in(user)
     end
 
-    context 'with confirmation error' do
-      before do
-        allow_any_instance_of(User).to receive(:delete_async).and_raise(User::DeletionNotConfirmedError)
-      end
+    def expect_failure(message)
+      expect(flash[:alert]).to eq(message)
+      expect(response.status).to eq(303)
+      expect(response).to redirect_to profile_account_path
+    end
 
-      context 'with password confirmation' do
-        before do
-          allow_any_instance_of(User).to receive(:confirm_deletion_with_password?).and_return(true)
-        end
+    def expect_password_failure
+      expect_failure('Invalid password')
+    end
 
-        it 'redirects with alert' do
-          post(:destroy)
+    def expect_username_failure
+      expect_failure('Invalid username')
+    end
 
-          expect(flash[:alert]).to eq 'Invalid password'
-          expect(response.status).to eq(303)
-          expect(response).to redirect_to profile_account_path
-        end
+    def expect_success
+      expect(flash[:notice]).to eq 'Account scheduled for removal.'
+      expect(response.status).to eq(303)
+      expect(response).to redirect_to new_user_session_path
+    end
+
+    context 'user requires password confirmation' do
+      it 'fails if password confirmation is not provided' do
+        post :destroy
+
+        expect_password_failure
       end
 
-      context 'without password confirmation' do
-        before do
-          allow_any_instance_of(User).to receive(:confirm_deletion_with_password?).and_return(false)
-        end
+      it 'fails if password confirmation is wrong' do
+        post :destroy, password: 'wrong password'
 
-        it 'redirects with alert' do
-          post(:destroy)
+        expect_password_failure
+      end
 
-          expect(flash[:alert]).to eq 'Invalid username'
-          expect(response.status).to eq(303)
-          expect(response).to redirect_to profile_account_path
-        end
+      it 'succeeds if password is confirmed' do
+        post :destroy, password: '12345678'
+
+        expect_success
       end
     end
 
-    context 'when user deletion was successful' do
-      let(:confirmation_params) { { username: 'Gabriel Shear', password: 'swordfish' } }
-
+    context 'user does not require password confirmation' do
       before do
-        expect_any_instance_of(User).to receive(:delete_async).with(deleted_by: user, confirmation_params: confirmation_params)
+        stub_application_setting(password_authentication_enabled: false)
+      end
+
+      it 'fails if username confirmation is not provided' do
+        post :destroy
+
+        expect_username_failure
+      end
+
+      it 'fails if username confirmation is wrong' do
+        post :destroy, username: 'wrong username'
+
+        expect_username_failure
       end
 
-      it 'redirects to login page' do
-        post(:destroy, **confirmation_params)
+      it 'succeeds if username is confirmed' do
+        post :destroy, username: user.username
 
-        expect(flash[:notice]).to eq 'Account scheduled for removal.'
-        expect(response.status).to eq(303)
-        expect(response).to redirect_to new_user_session_path
+        expect_success
       end
     end
   end
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index caace9a7d1d..995211845ce 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -2284,127 +2284,47 @@ describe User do
   end
 
   describe '#confirm_deletion_with_password?' do
-    let(:user) { create(:user) }
-
-    subject { user.confirm_deletion_with_password? }
+    where(
+      password_automatically_set: [true, false],
+      ldap_user: [true, false],
+      password_authentication_disabled: [true, false]
+    )
 
-    context 'with password authentication enabled' do
-      before do
-        allow(user).to receive(:allow_password_authentication?).once.and_return(true)
-      end
-
-      context 'with password automatically set' do
-        before do
-          allow(user).to receive(:password_automatically_set?).once.and_return(true)
-        end
+    with_them do
+      let!(:user) { create(:user, password_automatically_set: password_automatically_set) }
+      let!(:identity) { create(:identity, user: user) if ldap_user }
 
-        it { is_expected.to be false }
-      end
-
-      context 'with password automatically set' do
-        before do
-          allow(user).to receive(:password_automatically_set?).once.and_return(false)
-        end
-
-        it { is_expected.to be true }
-      end
-    end
+      # Only confirm deletion with password if all inputs are false
+      let(:expected) { !(password_automatically_set || ldap_user || password_authentication_disabled) }
 
-    context 'with password authentication disabled' do
       before do
-        allow(user).to receive(:allow_password_authentication?).once.and_return(false)
+        stub_application_setting(password_authentication_enabled: !password_authentication_disabled)
       end
 
-      context 'with password automatically set' do
-        before do
-          allow(user).to receive(:password_automatically_set?).once.and_return(true)
-        end
-
-        it { is_expected.to be false }
-      end
-
-      context 'with password automatically set' do
-        before do
-          allow(user).to receive(:password_automatically_set?).once.and_return(false)
-        end
-
-        it { is_expected.to be false }
+      it 'returns false unless all inputs are true' do
+        expect(user.confirm_deletion_with_password?).to eq(expected)
       end
     end
   end
 
   describe '#delete_async' do
     let(:user) { create(:user) }
-    let(:deleted_by) { user }
-    let(:params) { {} }
-    let(:confirmation_params) { { password: 'top secret' } }
+    let(:deleted_by) { create(:user) }
 
-    subject { user.delete_async(deleted_by: deleted_by, params: params, confirmation_params: confirmation_params) }
+    it 'blocks the user then schedules them for deletion if a hard delete is specified' do
+      expect(DeleteUserWorker).to receive(:perform_async).with(deleted_by.id, user.id, hard_delete: true)
 
-    context 'when triggered by same user' do
-      context 'with password confirmation' do
-        before do
-          expect(user).to receive(:confirm_deletion_with_password?).once.and_return(true)
-        end
+      user.delete_async(deleted_by: deleted_by, params: { hard_delete: true })
 
-        context 'with invalid password' do
-          before do
-            expect(user).to receive(:valid_password?).once.and_return(false)
-          end
-
-          it 'raises DeletionNotConfirmedError' do
-            expect { subject }.to raise_error(User::DeletionNotConfirmedError)
-          end
-        end
-
-        context 'with valid password' do
-          before do
-            expect(user).to receive(:valid_password?).once.with(confirmation_params[:password]).and_return(true)
-          end
-
-          it 'schedules user for deletion' do
-            expect(DeleteUserWorker).to receive(:perform_async).with(user.id, user.id, params)
-
-            subject
-          end
-        end
-      end
-
-      context 'without password confirmation' do
-        before do
-          expect(user).to receive(:confirm_deletion_with_password?).once.and_return(false)
-          expect(user).not_to receive(:valid_password?)
-        end
-
-        context 'with invalid username confirmation' do
-          let(:confirmation_params) { { username: "Ceci n'est pas une username" } }
-
-          it 'raises DeletionNotConfirmedError' do
-            expect { subject }.to raise_error(User::DeletionNotConfirmedError)
-          end
-        end
-
-        context 'with valid username confirmation' do
-          let(:confirmation_params) { { username: user.username } }
-
-          it 'schedules user for deletion' do
-            expect(DeleteUserWorker).to receive(:perform_async).with(user.id, user.id, params)
-
-            subject
-          end
-        end
-      end
+      expect(user).to be_blocked
     end
 
-    context 'when triggered by other user' do
-      let(:deleted_by) { create(:user) }
+    it 'schedules user for deletion without blocking them' do
+      expect(DeleteUserWorker).to receive(:perform_async).with(deleted_by.id, user.id, {})
 
-      it 'schedules user for deletion' do
-        expect(user).not_to receive(:confirm_deletion_with_password?)
-        expect(DeleteUserWorker).to receive(:perform_async).with(deleted_by.id, user.id, params)
+      user.delete_async(deleted_by: deleted_by)
 
-        subject
-      end
+      expect(user).not_to be_blocked
     end
   end
 end