Fixed issue #1977 - Remove online notifications too if user is deleted.

.rubocop.yml

@@ -224,6 +224,7 @@ Lint/AmbiguousBlockAssociation:
   StyleGuide: '#syntax'
   Enabled: true
   Exclude:
+    - "spec/support/*.rb"
     - "**/*_spec.rb"
     - "**/*_examples.rb"
 

app/models/authorization.rb

@@ -82,7 +82,7 @@ class Authorization < ApplicationModel
       end
     end
 
-    Authorization.create(
+    Authorization.create!(
       user: user,
       uid: hash['uid'],
       username: hash['info']['nickname'] || hash['info']['username'] || hash['info']['name'] || hash['info']['email'] || hash['username'],

app/models/avatar.rb

@@ -79,7 +79,7 @@ add avatar by url
     end
 
     # add initial avatar
-    add_init_avatar(object_id, data[:o_id])
+    _add_init_avatar(object_id, data[:o_id])
 
     record = {
       o_id: data[:o_id],
@@ -315,9 +315,11 @@ return all avatars of an user
 
   avatars = Avatar.list('User', 123)
 
+  avatars = Avatar.list('User', 123, no_init_add_as_boolean) # per default true
+
 =end
 
-  def self.list(object_name, o_id)
+  def self.list(object_name, o_id, no_init_add_as_boolean = true)
     object_id = ObjectLookup.by_name(object_name)
     avatars = Avatar.where(
       object_lookup_id: object_id,
@@ -325,7 +327,9 @@ return all avatars of an user
     ).order('initial DESC, deletable ASC, created_at ASC, id DESC')
 
     # add initial avatar
-    add_init_avatar(object_id, o_id)
+    if no_init_add_as_boolean
+      _add_init_avatar(object_id, o_id)
+    end
 
     avatar_list = []
     avatars.each do |avatar|
@@ -392,7 +396,7 @@ returns:
     end
   end
 
-  def self.add_init_avatar(object_id, o_id)
+  def self._add_init_avatar(object_id, o_id)
 
     count = Avatar.where(
       object_lookup_id: object_id,
@@ -400,7 +404,10 @@ returns:
     ).count
     return if count.positive?
 
-    Avatar.create(
+    object_name = ObjectLookup.by_id(object_id)
+    return if !object_name.constantize.exists?(id: o_id)
+
+    Avatar.create!(
       o_id: o_id,
       object_lookup_id: object_id,
       default: true,

app/models/karma/user.rb

@@ -14,7 +14,7 @@ class Karma::User < ApplicationModel
       record.save
       return record
     end
-    Karma::User.create(
+    Karma::User.create!(
       user_id: user.id,
       level: level,
       score: score,

app/models/user.rb

@@ -24,7 +24,7 @@ class User < ApplicationModel
   before_update   :check_preferences_default, :validate_preferences, :validate_ooo, :reset_login_failed, :validate_agent_limit_by_attributes, :last_admin_check_by_attribute
   after_create    :avatar_for_email_check
   after_update    :avatar_for_email_check
-  before_destroy  :avatar_destroy, :user_device_destroy, :cit_caller_id_destroy, :task_destroy
+  before_destroy  :destroy_longer_required_objects
 
   store :preferences
 
@@ -1112,20 +1112,17 @@ raise 'Minimum one user need to have admin permissions'
     true
   end
 
-  def avatar_destroy
+  def destroy_longer_required_objects
+    Authorization.where(user_id: id).destroy_all
     Avatar.remove('User', id)
-  end
-
-  def user_device_destroy
-    UserDevice.remove(id)
-  end
-
-  def cit_caller_id_destroy
     Cti::CallerId.where(user_id: id).destroy_all
-  end
-
-  def task_destroy
     Taskbar.where(user_id: id).destroy_all
+    Karma::ActivityLog.where(user_id: id).destroy_all
+    Karma::User.where(user_id: id).destroy_all
+    OnlineNotification.where(user_id: id).destroy_all
+    RecentView.where(created_by_id: id).destroy_all
+    UserDevice.remove(id)
+    true
   end
 
   def ensure_password

db/migrate/20120101000001_create_base.rb

@@ -502,6 +502,7 @@ class CreateBase < ActiveRecord::Migration[4.2]
     add_index :online_notifications, [:seen]
     add_index :online_notifications, [:created_at]
     add_index :online_notifications, [:updated_at]
+    add_foreign_key :online_notifications, :users
     add_foreign_key :online_notifications, :users, column: :created_by_id
     add_foreign_key :online_notifications, :users, column: :updated_by_id
 

db/migrate/20180426134922_issue_1977_remove_invalid_user_foreign_keys.rb

@@ -0,0 +1,38 @@
+class Issue1977RemoveInvalidUserForeignKeys < ActiveRecord::Migration[5.1]
+  def change
+
+    # return if it's a new setup
+    return if !Setting.find_by(name: 'system_init_done')
+
+    # cleanup
+    OnlineNotification.joins('LEFT OUTER JOIN users ON online_notifications.user_id = users.id')
+                      .where('users.id IS NULL')
+                      .destroy_all
+
+    RecentView.joins('LEFT OUTER JOIN users ON recent_views.created_by_id = users.id')
+              .where('users.id IS NULL')
+              .destroy_all
+
+    Avatar.joins('LEFT OUTER JOIN users ON avatars.o_id = users.id')
+          .where('users.id IS NULL')
+          .where(
+            object_lookup_id: ObjectLookup.by_name('User')
+          )
+          .destroy_all
+
+    # add (possibly) missing foreign_key
+    foreign_keys = [
+      %i[online_notifications users],
+      [:recent_views, :users, column: :created_by_id]
+    ]
+
+    foreign_keys.each do |args|
+      begin
+        add_foreign_key(*args)
+      rescue ActiveRecord::StatementInvalid => e
+        Rails.logger.info "Can't add foreign_keys '#{args.inspect}'"
+        Rails.logger.info e
+      end
+    end
+  end
+end

spec/db/migrate/issue_1977_remove_invalid_user_foreign_keys_spec.rb

@@ -0,0 +1,81 @@
+require 'rails_helper'
+
+RSpec.describe Issue1977RemoveInvalidUserForeignKeys, type: :db_migration do
+
+  context 'no online_notifications foreign key' do
+    self.use_transactional_tests = false
+
+    let(:existing_user_id) { User.first.id }
+
+    context 'invalid User foreign key columns' do
+
+      it 'cleans up OnlineNotification#user_id' do
+        witout_foreign_key(:online_notifications, column: :user_id)
+
+        create(:online_notification, user_id: 1337)
+        valid = create(:online_notification, user_id: existing_user_id)
+
+        expect do
+          migrate
+        end.to change {
+          OnlineNotification.count
+        }.by(-1)
+
+        # cleanup since we disabled
+        # transactions for this tests
+        valid.destroy
+      end
+
+      it 'cleans up RecentView#created_by_id' do
+        witout_foreign_key(:online_notifications, column: :user_id)
+        witout_foreign_key(:recent_views, column: :created_by_id)
+
+        create(:recent_view, created_by_id: 1337)
+        valid = create(:recent_view, created_by_id: existing_user_id)
+
+        expect do
+          migrate
+        end.to change {
+          RecentView.count
+        }.by(-1)
+
+        # cleanup since we disabled
+        # transactions for this tests
+        valid.destroy
+      end
+
+      it 'cleans up Avatar#o_id' do
+        witout_foreign_key(:online_notifications, column: :user_id)
+
+        create(:avatar, object_lookup_id: ObjectLookup.by_name('User'), o_id: 1337)
+        valid_ticket = create(:avatar, object_lookup_id: ObjectLookup.by_name('Ticket'), o_id: 1337)
+        valid_user   = create(:avatar, object_lookup_id: ObjectLookup.by_name('User'), o_id: existing_user_id)
+
+        expect do
+          migrate
+        end.to change {
+          Avatar.count
+        }.by(-1)
+
+        # cleanup since we disabled
+        # transactions for this tests
+        valid_ticket.destroy
+        valid_user.destroy
+      end
+
+    end
+
+    it 'adds OnlineNotification#user_id foreign key' do
+      adds_foreign_key(:online_notifications, column: :user_id)
+    end
+  end
+
+  context 'no recent_views foreign key' do
+    self.use_transactional_tests = false
+
+    it 'adds RecentView#created_by_id foreign key' do
+      adds_foreign_key(:recent_views, column: :created_by_id)
+    end
+  end
+
+end

spec/factories/avatar.rb

@@ -0,0 +1,15 @@
+FactoryBot.define do
+  factory :avatar do
+    object_lookup_id { ObjectLookup.by_name('User') }
+    o_id 1
+    default true
+    deletable true
+    initial false
+    source 'init'
+    source_url nil
+    created_by_id 1
+    updated_by_id 1
+    created_at Time.zone.now
+    updated_at Time.zone.now
+  end
+end

spec/factories/online_notification.rb

@@ -1,7 +1,8 @@
 FactoryBot.define do
   factory :online_notification do
     object_lookup_id { ObjectLookup.by_name('Ticket') }
-    type_lookup_id { TypeLookup.by_name('Assigned to you') }
+    o_id 1
+    type_lookup_id { TypeLookup.by_name('updated') }
     seen false
     user_id 1
     created_by_id 1

spec/factories/recent_view.rb

@@ -0,0 +1,9 @@
+FactoryBot.define do
+  factory :recent_view do
+    recent_view_object_id { ObjectLookup.by_name('User') }
+    o_id 1
+    created_by_id 1
+    created_at Time.zone.now
+    updated_at Time.zone.now
+  end
+end

spec/support/db_migration.rb

@@ -14,7 +14,7 @@ module DbMigrationHelper
   #
   # @example
   #  migrate
-
+  #
   # @example
   #  migrate(:down)
   #
@@ -28,6 +28,79 @@ module DbMigrationHelper
     end
   end
 
+  # Provides a helper method to remove foreign_keys if exist.
+  # Make sure to define type: :db_migration in your RSpec.describe call
+  # and add `self.use_transactional_tests = false` to your context.
+  #
+  # ATTENTION: We do not use the same arguments as the internally
+  #            used methods since giving a table name
+  #            as a second argument as e.g.
+  #            `remove_foreign_key(:online_notifications, :users)`
+  #            doesn't remove the index at first execution on at least MySQL
+  #
+  # @param [Symbol] from_table the name of the table with the foreign_key column
+  # @param [Symbol] column the name of the foreign_key column
+  #
+  # @example
+  #  witout_foreign_key(:online_notifications, column: :user_id)
+  #
+  # @return [nil]
+  def witout_foreign_key(from_table, column:)
+    suppress_messages do
+      break if !foreign_key_exists?(from_table, column: column)
+      remove_foreign_key(from_table, column: column)
+    end
+  end
+
+  # Enables the usage of `ActiveRecord::Migration` methods.
+  #
+  # @see ActiveRecord::Migration
+  #
+  # @example
+  #  remove_foreign_key(:online_notifications, :users)
+  #
+  # @return [nil]
+  def method_missing(method, *args, &blk)
+    ActiveRecord::Migration.send(method, *args, &blk)
+  rescue NoMethodError
+    super
+  end
+
+  # Enables the usage of `ActiveRecord::Migration` methods.
+  #
+  # @see ActiveRecord::Migration
+  #
+  # @example
+  #  remove_foreign_key(:online_notifications, :users)
+  #
+  # @return [nil]
+  def respond_to_missing?(*)
+    true
+  end
+
+  # Provides a helper method to check if migration class adds a foreign key.
+  # Make sure to define type: :db_migration in your RSpec.describe call
+  # and add `self.use_transactional_tests = false` to your context.
+  #
+  # @param [Symbol] from_table the name of the table with the foreign_key column
+  # @param [Symbol] column the name of the foreign_key column
+  #
+  # @example
+  #  adds_foreign_key(:online_notifications, column: :user_id)
+  #
+  # @return [nil]
+  def adds_foreign_key(from_table, column:)
+    witout_foreign_key(from_table, column: column)
+
+    suppress_messages do
+      expect do
+        migrate
+      end.to change {
+        foreign_key_exists?(from_table, column: column)
+      }
+    end
+  end
+
   def self.included(base)
 
     # Execute in RSpec class context

test/unit/user_test.rb

@@ -1171,4 +1171,103 @@ class UserTest < ActiveSupport::TestCase
 
   end
 
+  test 'cleanup references on destroy' do
+    agent1 = User.create!(
+      login: "agent-cleanup_check-1#{name}@example.com",
+      firstname: 'vaild_agent_group_permission-1',
+      lastname: "Agent#{name}",
+      email: "agent-cleanup_check-1#{name}@example.com",
+      password: 'agentpw',
+      active: true,
+      roles: Role.where(name: 'Agent'),
+      groups: Group.all,
+      updated_by_id: 1,
+      created_by_id: 1,
+    )
+    agent1_id = agent1.id
+    assert_equal(1, Avatar.list('User', agent1_id).count)
+
+    UserDevice.add(
+      'Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_3) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/44.0.2403.107 Safari/537.36',
+      '91.115.248.231',
+      agent1_id,
+      'fingerprint1234',
+      'session',
+    )
+    assert_equal(1, UserDevice.where(user_id: agent1_id).count)
+
+    Karma::User.sync(agent1)
+    assert_equal(1, Karma::User.where(user_id: agent1_id).count)
+
+    OnlineNotification.add(
+      type:          'Assigned to you',
+      object:        'Ticket',
+      o_id:          1,
+      seen:          false,
+      user_id:       agent1_id,
+      created_by_id: 1,
+      updated_by_id: 1,
+      created_at:    Time.zone.now,
+      updated_at:    Time.zone.now,
+    )
+    assert_equal(1, OnlineNotification.where(user_id: agent1_id).count)
+
+    Authorization.create!(
+      user: agent1,
+      uid: '123',
+      username: '123',
+      provider: 'some',
+      token: 'token',
+      secret: 'secret',
+    )
+    assert_equal(1, Authorization.where(user_id: agent1_id).count)
+
+    Cti::CallerId.maybe_add(
+      caller_id: '49123456789',
+      comment: 'Hairdresser Bob Smith, San Francisco', #optional
+      level: 'maybe', # known|maybe
+      user_id: agent1_id, # optional
+      object: 'Ticket',
+      o_id: 1,
+    )
+    assert_equal(1, Cti::CallerId.where(user_id: agent1_id).count)
+
+    Taskbar.create!(
+      client_id: 123,
+      key: 'Ticket-1',
+      callback: 'TicketZoom',
+      params: {
+        id: 1,
+      },
+      state: {},
+      user_id: agent1_id,
+      prio: 1,
+      notify: false,
+    )
+    assert_equal(1, Taskbar.where(user_id: agent1_id).count)
+
+    ticket1 = Ticket.create!(
+      title: 'test 1234-1',
+      group: Group.lookup(name: 'Users'),
+      customer_id: 2,
+      owner_id: 2,
+      updated_by_id: 1,
+      created_by_id: 1,
+    )
+
+    RecentView.log(ticket1.class.to_s, ticket1.id, agent1)
+    assert_equal(1, RecentView.where(created_by_id: agent1_id).count)
+
+    agent1.destroy!
+
+    assert_equal(0, UserDevice.where(user_id: agent1_id).count)
+    assert_equal(0, Avatar.list('User', agent1_id, false).count)
+    assert_equal(0, Karma::User.where(user_id: agent1_id).count)
+    assert_equal(0, OnlineNotification.where(user_id: agent1_id).count)
+    assert_equal(0, Authorization.where(user_id: agent1_id).count)
+    assert_equal(0, Cti::CallerId.where(user_id: agent1_id).count)
+    assert_equal(0, Taskbar.where(user_id: agent1_id).count)
+    assert_equal(0, RecentView.where(created_by_id: agent1_id).count)
+  end
+
 end