From 8c14a8659a523ab9043450e9fc7827f3b1facf99 Mon Sep 17 00:00:00 2001
From: Raul Riera <rieraraul@gmail.com>
Date: Tue, 18 Aug 2026 10:48:58 -0400
Subject: [PATCH] fix(models): halt destroy callbacks for datebooks, doctors
 and admin users

Rails 5+ ignores a false return from before_destroy; these guards were
inert, so datebooks and doctors with appointments and admin users could
be destroyed. Use errors.add + throw :abort, reinstate the commented
regression test, and clear appointments in the doctors empty-state test
setup so its destroy_all still empties the practice.
---
 app/models/datebook.rb                       |  4 ++--
 app/models/doctor.rb                         |  4 ++--
 app/models/user.rb                           |  4 ++--
 test/functional/datebooks_controller_test.rb | 12 ++++++------
 test/functional/doctors_controller_test.rb   |  1 +
 test/unit/models/datebook_test.rb            | 10 ++++++++++
 test/unit/models/doctor_test.rb              | 10 ++++++++++
 test/unit/models/user_test.rb                | 10 ++++++++++
 8 files changed, 43 insertions(+), 12 deletions(-)

diff --git a/app/models/datebook.rb b/app/models/datebook.rb
index 934ef5bc..e55b7e79 100644
--- a/app/models/datebook.rb
+++ b/app/models/datebook.rb
@@ -33,7 +33,7 @@ def is_deleteable
   def check_if_is_deleteable
     return if is_deleteable
 
-    errors[:base] << I18n.t('errors.messages.has_appointments')
-    false
+    errors.add(:base, I18n.t('errors.messages.has_appointments'))
+    throw :abort
   end
 end
diff --git a/app/models/doctor.rb b/app/models/doctor.rb
index 12aa1841..3ffb719a 100644
--- a/app/models/doctor.rb
+++ b/app/models/doctor.rb
@@ -53,7 +53,7 @@ def ciphered_feed_url
   def check_if_is_deleteable
     return if is_deleteable
 
-    errors[:base] << I18n.t('errors.messages.has_appointments_or_treatments')
-    false
+    errors.add(:base, I18n.t('errors.messages.has_appointments_or_treatments'))
+    throw :abort
   end
 end
diff --git a/app/models/user.rb b/app/models/user.rb
index 212178ce..fc6ac2ba 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -87,8 +87,8 @@ def validate_password?
   def check_if_admin
     return unless is_admin?
 
-    errors[:base] << I18n.t('errors.messages.unauthorised')
-    false
+    errors.add(:base, I18n.t('errors.messages.unauthorised'))
+    throw :abort
   end
 
   def set_admin_role_for_first_user
diff --git a/test/functional/datebooks_controller_test.rb b/test/functional/datebooks_controller_test.rb
index d9bd198d..70059935 100644
--- a/test/functional/datebooks_controller_test.rb
+++ b/test/functional/datebooks_controller_test.rb
@@ -63,11 +63,11 @@ class DatebooksControllerTest < ActionController::TestCase
     assert_redirected_to datebooks_url
   end
 
-  # test "should not destroy datebook with appointments" do
-  #   assert_no_difference('Datebook.count') do
-  #     delete :destroy, params: {id: datebooks(:playa_del_carmen).to_param}
-  #   end
+  test 'should not destroy datebook with appointments' do
+    assert_no_difference('Datebook.count') do
+      delete :destroy, params: { id: datebooks(:playa_del_carmen).to_param }
+    end
 
-  #   assert_redirected_to datebooks_url
-  # end
+    assert_redirected_to datebooks_url
+  end
 end
diff --git a/test/functional/doctors_controller_test.rb b/test/functional/doctors_controller_test.rb
index 8c7ce1ae..4950cf47 100644
--- a/test/functional/doctors_controller_test.rb
+++ b/test/functional/doctors_controller_test.rb
@@ -20,6 +20,7 @@ class DoctorsControllerTest < ActionController::TestCase
 
   test 'shows empty state cta when practice has no doctors' do
     practice = practices(:complete)
+    Appointment.where(doctor_id: Doctor.with_practice(practice.id)).delete_all
     Doctor.with_practice(practice.id).destroy_all
     practice.update_columns(doctors_count: 0)
 
diff --git a/test/unit/models/datebook_test.rb b/test/unit/models/datebook_test.rb
index c54038cc..b40e4978 100644
--- a/test/unit/models/datebook_test.rb
+++ b/test/unit/models/datebook_test.rb
@@ -43,6 +43,16 @@ class DatebookTest < ActiveSupport::TestCase
     assert_equal I18n.t('errors.messages.less_than_or_equal_to', count: 23), datebook.errors[:ends_at].join('; ')
   end
 
+  test 'datebook with appointments cannot be destroyed' do
+    datebook = datebooks(:playa_del_carmen)
+
+    assert_no_difference 'Datebook.count' do
+      assert_not datebook.destroy
+    end
+
+    assert datebook.errors[:base].any?
+  end
+
   test 'datebook name should be less than 100 chars' do
     datebook = Datebook.new(name: 'Lorem ipsum dolor sit amet, consectetur adipiscing elit. Donec vehicula arcu ante, nec eleifend ipsum. Proin vestibulum nisi sit amet diam mattis tempor.')
 
diff --git a/test/unit/models/doctor_test.rb b/test/unit/models/doctor_test.rb
index 5ccca258..0aca5502 100644
--- a/test/unit/models/doctor_test.rb
+++ b/test/unit/models/doctor_test.rb
@@ -80,6 +80,16 @@ class DoctorTest < ActiveSupport::TestCase
     assert_equal doctor.initials, 'RR'
   end
 
+  test 'doctor with appointments cannot be destroyed' do
+    doctor = doctors(:rebecca)
+
+    assert_no_difference 'Doctor.count' do
+      assert_not doctor.destroy
+    end
+
+    assert doctor.errors[:base].any?
+  end
+
   test 'destroying doctor purges profile picture attachment' do
     doctor = Doctor.create!(
       practice: practices(:complete),
diff --git a/test/unit/models/user_test.rb b/test/unit/models/user_test.rb
index 962717f4..49851281 100644
--- a/test/unit/models/user_test.rb
+++ b/test/unit/models/user_test.rb
@@ -171,4 +171,14 @@ class UserTest < ActiveSupport::TestCase
     user.firstname = 'Super'
     assert user.save
   end
+
+  test 'admin user cannot be destroyed' do
+    admin = users(:founder)
+
+    assert_no_difference 'User.count' do
+      assert_not admin.destroy
+    end
+
+    assert admin.errors[:base].any?
+  end
 end