Merge pull request #2030 from daviddavis/temp_1366666322

Moving before_destroy callbacks because of rails/rails#3458

Author: David Davis

app/models/kt_environment.rb

@@ -21,6 +21,10 @@ class KTEnvironment < ActiveRecord::Base
   include Ext::PermissionTagCleanup
   acts_as_reportable
 
+  # RAILS3458: before_destroys before associations. see http://tinyurl.com/rails3458
+  before_destroy :confirm_last_env
+  before_destroy :delete_default_view_version
+
   belongs_to :organization, :inverse_of => :environments
   has_many :activation_keys, :dependent => :destroy, :foreign_key => :environment_id
   has_and_belongs_to_many :priors, {:class_name => "KTEnvironment", :foreign_key => :environment_id,
@@ -68,8 +72,6 @@ def <<(*items)
   validates_with Validators::PathDescendentsValidator
 
   after_create :create_default_content_view_version
-  before_destroy :confirm_last_env
-  before_destroy :delete_default_view_version
 
   after_destroy :unset_users_with_default
    ERROR_CLASS_NAME = "Environment"

app/models/permission.rb

@@ -11,6 +11,8 @@
 # http://www.gnu.org/licenses/old-licenses/gpl-2.0.txt.
 
 class Permission < ActiveRecord::Base
+  before_destroy :check_locked # RAILS3458: must be before dependent associations http://tinyurl.com/rails3458
+
   belongs_to :resource_type
   belongs_to :organization
   belongs_to :role, :inverse_of => :permissions
@@ -24,11 +26,7 @@ class Permission < ActiveRecord::Base
   validates :name, :presence => true
   validates_with Validators::NonHtmlNameValidator, :attributes => :name
   validates_with Validators::KatelloDescriptionFormatValidator, :attributes => :description
-
   validates_uniqueness_of :name, :scope => [:organization_id, :role_id], :message => N_("Label has already been taken")
-
-  before_destroy :check_locked
-
   validates_with Validators::PermissionValidator
   validates_presence_of :resource_type
 

app/models/user.rb

@@ -29,6 +29,10 @@ class User < ActiveRecord::Base
   scope :hidden, where(:hidden => true)
   scope :visible, where(:hidden => false)
 
+  # RAILS3458: THIS CHECK MUST BE THE FIRST before_destroy AND
+  # PROCEED DEPENDENT ASSOCIATIONS tinyurl.com/rails3458
+  before_destroy :not_last_super_user?, :destroy_own_role
+
   has_many :roles_users
   has_many :roles, :through => :roles_users, :before_remove => :super_admin_check, :uniq => true, :extend => RolesPermissions::UserOwnRole
   validates_with Validators::OwnRolePresenceValidator, :attributes => :roles
@@ -61,8 +65,6 @@ class User < ActiveRecord::Base
   after_validation :setup_remote_id
   before_save   :hash_password, :setup_preferences
   after_save :create_or_update_default_system_registration_permission
-  # THIS CHECK MUST BE THE FIRST before_destroy
-  before_destroy :is_last_super_user?, :destroy_own_role
 
   # hash the password before creating or updateing the record
   def hash_password
@@ -75,7 +77,7 @@ def setup_preferences
     self.preferences = HashWithIndifferentAccess.new unless self.preferences
   end
 
-  def is_last_super_user?
+  def not_last_super_user?
     if !User.current.nil?
       if self.id == User.current.id
         self.errors.add(:base, _("Cannot delete currently logged user"))

test/models/user_test.rb

@@ -418,4 +418,18 @@ def test_default_env_removed
     assert_nil    @user.default_environment
   end
 
+  def test_before_destroy
+    # RAILS3458: Check that before_destroy callback is executed first http://tinyurl.com/rails3458
+    User.stubs(:current).returns(@user)
+    SearchFavorite.create!(:params => "abc",
+                           :path => "/garlic_naan",
+                           :user_id => @user.id
+                          )
+    @user.search_favorites.reload
+    refute_empty @user.search_favorites
+    refute @user.destroy
+
+    refute_empty @user.search_favorites
+  end
+
 end