diff --git a/.rubocop.yml b/.rubocop.yml index 293770aa7..cdd47ae2f 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -224,6 +224,7 @@ Lint/AmbiguousBlockAssociation: StyleGuide: '#syntax' Enabled: true Exclude: + - "spec/support/*.rb" - "**/*_spec.rb" - "**/*_examples.rb" diff --git a/app/assets/javascripts/app/controllers/ticket_zoom/sidebar_article_attachments.coffee b/app/assets/javascripts/app/controllers/ticket_zoom/sidebar_article_attachments.coffee index 1dbac63ea..e478e2d17 100644 --- a/app/assets/javascripts/app/controllers/ticket_zoom/sidebar_article_attachments.coffee +++ b/app/assets/javascripts/app/controllers/ticket_zoom/sidebar_article_attachments.coffee @@ -13,15 +13,16 @@ class SidebarArticleAttachments extends App.Controller showObjects: (el) => @el = el - if !@ticket || _.isEmpty(@ticket.article_ids) + if _.isEmpty(@ticket) || _.isEmpty(@ticket.article_ids) @el.html("
#{App.i18n.translateInline('none')}
") return html = '' for ticket_article_id, index in @ticket.article_ids - article = App.TicketArticle.find(ticket_article_id) - attachments = App.TicketArticle.contentAttachments(article) - if !_.isEmpty(attachments) - html = App.view('ticket_zoom/sidebar_article_attachment')(article: article, attachments: attachments) + html + if App.TicketArticle.exists(ticket_article_id) + article = App.TicketArticle.find(ticket_article_id) + attachments = App.TicketArticle.contentAttachments(article) + if !_.isEmpty(attachments) + html = App.view('ticket_zoom/sidebar_article_attachment')(article: article, attachments: attachments) + html @el.html(html) @el.delegate('.js-attachments img', 'click', (e) => @imageView(e) diff --git a/app/assets/javascripts/app/lib/base/jquery.textmodule.js b/app/assets/javascripts/app/lib/base/jquery.textmodule.js index b40749fea..4a2891050 100644 --- a/app/assets/javascripts/app/lib/base/jquery.textmodule.js +++ b/app/assets/javascripts/app/lib/base/jquery.textmodule.js @@ -344,7 +344,7 @@ Plugin.prototype.onEntryClick = function(event) { event.preventDefault() - var id = $(event.target).data('id') + var id = $(event.currentTarget).data('id') this.take(id) } diff --git a/app/assets/javascripts/app/models/ticket_article.coffee b/app/assets/javascripts/app/models/ticket_article.coffee index 604ba4485..7c3523ace 100644 --- a/app/assets/javascripts/app/models/ticket_article.coffee +++ b/app/assets/javascripts/app/models/ticket_article.coffee @@ -49,6 +49,7 @@ class App.TicketArticle extends App.Model return "Unknow action for (#{@objectDisplayName()}/#{item.type}), extend activityMessage() of model." @contentAttachments: (article) -> + return [] if !article return [] if !article.attachments attachments = [] for attachment in article.attachments diff --git a/app/models/authorization.rb b/app/models/authorization.rb index 7098b0929..4b8075f0b 100644 --- a/app/models/authorization.rb +++ b/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'], diff --git a/app/models/avatar.rb b/app/models/avatar.rb index 3600288ae..c109cc931 100644 --- a/app/models/avatar.rb +++ b/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, diff --git a/app/models/karma/user.rb b/app/models/karma/user.rb index 627984492..b276f45e3 100644 --- a/app/models/karma/user.rb +++ b/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, diff --git a/app/models/organization/assets.rb b/app/models/organization/assets.rb index 81636cabc..71b438387 100644 --- a/app/models/organization/assets.rb +++ b/app/models/organization/assets.rb @@ -47,7 +47,7 @@ returns local_attributes['member_ids'] = local_attributes['member_ids'].sort[0, 100] end local_attributes['member_ids'].each do |local_user_id| - next if data[ app_model_user ][ local_user_id ] + next if data[ app_model_user ] && data[ app_model_user ][ local_user_id ] user = User.lookup(id: local_user_id) next if !user data = user.assets(data) diff --git a/app/models/user.rb b/app/models/user.rb index 4c2ec6ff7..a6924976d 100644 --- a/app/models/user.rb +++ b/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 diff --git a/db/migrate/20120101000001_create_base.rb b/db/migrate/20120101000001_create_base.rb index 01834da3c..c1085a7ef 100644 --- a/db/migrate/20120101000001_create_base.rb +++ b/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 diff --git a/db/migrate/20180426134922_issue_1977_remove_invalid_user_foreign_keys.rb b/db/migrate/20180426134922_issue_1977_remove_invalid_user_foreign_keys.rb new file mode 100644 index 000000000..d431c98af --- /dev/null +++ b/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 diff --git a/spec/db/migrate/issue_1977_remove_invalid_user_foreign_keys_spec.rb b/spec/db/migrate/issue_1977_remove_invalid_user_foreign_keys_spec.rb new file mode 100644 index 000000000..48c1c46ff --- /dev/null +++ b/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 diff --git a/spec/factories/avatar.rb b/spec/factories/avatar.rb new file mode 100644 index 000000000..a3ab598e7 --- /dev/null +++ b/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 diff --git a/spec/factories/online_notification.rb b/spec/factories/online_notification.rb index 00e43c1d0..7b7bf169f 100644 --- a/spec/factories/online_notification.rb +++ b/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 diff --git a/spec/factories/recent_view.rb b/spec/factories/recent_view.rb new file mode 100644 index 000000000..85742bc24 --- /dev/null +++ b/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 diff --git a/spec/support/db_migration.rb b/spec/support/db_migration.rb index 24154c428..49f9e87af 100644 --- a/spec/support/db_migration.rb +++ b/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 diff --git a/test/unit/user_test.rb b/test/unit/user_test.rb index 5e9b9b383..3eda56389 100644 --- a/test/unit/user_test.rb +++ b/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