From 629c561c285fd99579b63fbd99342e7c8926813e Mon Sep 17 00:00:00 2001 From: Martin Edenhofer Date: Tue, 18 Dec 2018 08:33:54 +0100 Subject: [PATCH] Fixed issue #2405 - Unable to load Zammad in web browser, because of online notification of ticket which was already deleted in the meantime. --- app/models/activity_stream.rb | 3 ++ app/models/application_model.rb | 1 + .../has_exists_check_by_object_and_id.rb | 35 ++++++++++++ app/models/online_notification.rb | 3 ++ app/models/recent_view.rb | 8 +-- test/unit/online_notifiaction_test.rb | 54 ++++++++++++++++--- 6 files changed, 94 insertions(+), 10 deletions(-) create mode 100644 app/models/application_model/has_exists_check_by_object_and_id.rb diff --git a/app/models/activity_stream.rb b/app/models/activity_stream.rb index 26b57f853dee..9a2ed922a778 100644 --- a/app/models/activity_stream.rb +++ b/app/models/activity_stream.rb @@ -49,6 +49,9 @@ def self.add(data) permission_id = permission.id end + # check if object for online notification exists + exists_by_object_and_id?(data[:object], data[:o_id]) + # check newest entry - is needed result = ActivityStream.where( o_id: data[:o_id], diff --git a/app/models/application_model.rb b/app/models/application_model.rb index cc60a294f651..309d0784a462 100644 --- a/app/models/application_model.rb +++ b/app/models/application_model.rb @@ -18,6 +18,7 @@ class ApplicationModel < ActiveRecord::Base include ApplicationModel::ChecksImport include ApplicationModel::CanTouchReferences include ApplicationModel::CanQueryCaseInsensitiveWhereOrSql + include ApplicationModel::HasExistsCheckByObjectAndId self.abstract_class = true end diff --git a/app/models/application_model/has_exists_check_by_object_and_id.rb b/app/models/application_model/has_exists_check_by_object_and_id.rb new file mode 100644 index 000000000000..88f0f5245a13 --- /dev/null +++ b/app/models/application_model/has_exists_check_by_object_and_id.rb @@ -0,0 +1,35 @@ +# Copyright (C) 2012-2016 Zammad Foundation, http://zammad-foundation.org/ +module ApplicationModel::HasExistsCheckByObjectAndId + extend ActiveSupport::Concern + + class_methods do + +=begin + +verify if referenced object exists + + success = Model.exists_by_object_and_id('Ticket', 123) + +returns + + # true or will raise an exception + +=end + + def exists_by_object_and_id?(object, o_id) + + begin + local_class = object.constantize + rescue => e + raise "Unable for get an instance of '#{object}': #{e.inspect}" + end + if !local_class.exists?(o_id) + raise ActiveRecord::RecordNotFound, "Unable for find reference object '#{object}.exists?(#{o_id})!" + end + + true + end + + end + +end diff --git a/app/models/online_notification.rb b/app/models/online_notification.rb index 0192ebfaf11c..e0cd562aa803 100644 --- a/app/models/online_notification.rb +++ b/app/models/online_notification.rb @@ -41,6 +41,9 @@ def self.add(data) object_id = ObjectLookup.by_name(data[:object]) end + # check if object for online notification exists + exists_by_object_and_id?(data[:object], data[:o_id]) + record = { o_id: data[:o_id], object_lookup_id: object_id, diff --git a/app/models/recent_view.rb b/app/models/recent_view.rb index d66ea3d8071a..019ea85ae617 100644 --- a/app/models/recent_view.rb +++ b/app/models/recent_view.rb @@ -15,9 +15,11 @@ class RecentView < ApplicationModel def self.log(object, o_id, user) return if !access(object, o_id, user) - RecentView.create(o_id: o_id, - recent_view_object_id: ObjectLookup.by_name(object), - created_by_id: user.id) + exists_by_object_and_id?(object, o_id) + + RecentView.create!(o_id: o_id, + recent_view_object_id: ObjectLookup.by_name(object), + created_by_id: user.id) end def self.log_destroy(requested_object, requested_object_id) diff --git a/test/unit/online_notifiaction_test.rb b/test/unit/online_notifiaction_test.rb index 3bac011f18f5..717454280a20 100644 --- a/test/unit/online_notifiaction_test.rb +++ b/test/unit/online_notifiaction_test.rb @@ -538,10 +538,22 @@ class OnlineNotificationTest < ActiveSupport::TestCase end test 'cleanup check' do + + ticket1 = Ticket.create( + group: @group, + customer_id: @customer_user.id, + owner_id: User.lookup(login: '-').id, + title: 'Unit Test 1 (äöüß)!', + state_id: Ticket::State.lookup(name: 'closed').id, + priority_id: Ticket::Priority.lookup(name: '2 normal').id, + updated_by_id: @agent_user1.id, + created_by_id: @agent_user1.id, + ) + online_notification1 = OnlineNotification.add( type: 'create', object: 'Ticket', - o_id: 123, + o_id: ticket1.id, seen: false, user_id: @agent_user1.id, created_by_id: 1, @@ -552,7 +564,7 @@ class OnlineNotificationTest < ActiveSupport::TestCase online_notification2 = OnlineNotification.add( type: 'create', object: 'Ticket', - o_id: 123, + o_id: ticket1.id, seen: true, user_id: @agent_user1.id, created_by_id: 1, @@ -563,7 +575,7 @@ class OnlineNotificationTest < ActiveSupport::TestCase online_notification3 = OnlineNotification.add( type: 'create', object: 'Ticket', - o_id: 123, + o_id: ticket1.id, seen: false, user_id: @agent_user1.id, created_by_id: 1, @@ -574,7 +586,7 @@ class OnlineNotificationTest < ActiveSupport::TestCase online_notification4 = OnlineNotification.add( type: 'create', object: 'Ticket', - o_id: 123, + o_id: ticket1.id, seen: true, user_id: @agent_user1.id, created_by_id: @agent_user1.id, @@ -585,7 +597,7 @@ class OnlineNotificationTest < ActiveSupport::TestCase online_notification5 = OnlineNotification.add( type: 'create', object: 'Ticket', - o_id: 123, + o_id: ticket1.id, seen: true, user_id: @agent_user1.id, created_by_id: @agent_user2.id, @@ -596,7 +608,7 @@ class OnlineNotificationTest < ActiveSupport::TestCase online_notification6 = OnlineNotification.add( type: 'create', object: 'Ticket', - o_id: 123, + o_id: ticket1.id, seen: true, user_id: @agent_user1.id, created_by_id: @agent_user1.id, @@ -607,7 +619,7 @@ class OnlineNotificationTest < ActiveSupport::TestCase online_notification7 = OnlineNotification.add( type: 'create', object: 'Ticket', - o_id: 123, + o_id: ticket1.id, seen: true, user_id: @agent_user1.id, created_by_id: @agent_user2.id, @@ -628,4 +640,32 @@ class OnlineNotificationTest < ActiveSupport::TestCase OnlineNotification.destroy_all end + test 'not existing object ref' do + assert_raises(RuntimeError) do + OnlineNotification.add( + type: 'create', + object: 'TicketNotExisting', + o_id: 123, + seen: false, + user_id: @agent_user1.id, + created_by_id: 1, + updated_by_id: 1, + created_at: Time.zone.now - 10.months, + updated_at: Time.zone.now - 10.months, + ) + end + assert_raises(ActiveRecord::RecordNotFound) do + OnlineNotification.add( + type: 'create', + object: 'Ticket', + o_id: 123, + seen: false, + user_id: @agent_user1.id, + created_by_id: 1, + updated_by_id: 1, + created_at: Time.zone.now - 10.months, + updated_at: Time.zone.now - 10.months, + ) + end + end end