From c82622913a7115ce86cd81f7aaa41998b12951a1 Mon Sep 17 00:00:00 2001 From: Rolf Schmidt Date: Fri, 5 May 2017 11:16:47 +0200 Subject: [PATCH] Fixed issue #214 - Delete tickets. --- .rubocop.yml | 14 ++ .../_ui_element/ticket_perform_action.coffee | 10 ++ app/models/concerns/has_karma_activity_log.rb | 21 +++ app/models/concerns/has_links.rb | 24 +++ .../concerns/has_online_notifications.rb | 21 +++ app/models/karma/activity_log.rb | 24 +++ app/models/link.rb | 73 +++++---- app/models/ticket.rb | 38 +++-- spec/factories/online_notification.rb | 12 ++ spec/factories/tag.rb | 7 + spec/factories/ticket/article.rb | 14 ++ spec/models/ticket_spec.rb | 142 ++++++++++++++++++ 12 files changed, 360 insertions(+), 40 deletions(-) create mode 100644 app/models/concerns/has_karma_activity_log.rb create mode 100644 app/models/concerns/has_links.rb create mode 100644 app/models/concerns/has_online_notifications.rb create mode 100644 spec/factories/online_notification.rb create mode 100644 spec/factories/tag.rb create mode 100644 spec/factories/ticket/article.rb diff --git a/.rubocop.yml b/.rubocop.yml index 347ed9964..1f7ed20d9 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -210,3 +210,17 @@ Performance/Casecmp: Description: 'Use `casecmp` rather than `downcase ==`.' Reference: 'https://github.com/JuanitoFatas/fast-ruby#stringcasecmp-vs-stringdowncase---code' Enabled: false + +# RSpec tests +Style/NumericPredicate: + Description: >- + Checks for the use of predicate- or comparison methods for + numeric comparisons. + StyleGuide: '#predicate-methods' + # This will change to a new method call which isn't guaranteed to be on the + # object. Switching these methods has to be done with knowledge of the types + # of the variables which rubocop doesn't have. + AutoCorrect: false + Enabled: true + Exclude: + - "**/*_spec.rb" \ No newline at end of file diff --git a/app/assets/javascripts/app/controllers/_ui_element/ticket_perform_action.coffee b/app/assets/javascripts/app/controllers/_ui_element/ticket_perform_action.coffee index fa8a00d79..e64d6edf4 100644 --- a/app/assets/javascripts/app/controllers/_ui_element/ticket_perform_action.coffee +++ b/app/assets/javascripts/app/controllers/_ui_element/ticket_perform_action.coffee @@ -32,6 +32,16 @@ class App.UiElement.ticket_perform_action config.operator = ['add', 'remove'] elements["#{groupKey}.#{config.name}"] = config + # add ticket deletion action + elements['ticket.action'] = + name: 'action' + display: 'Action' + tag: 'select' + null: false + translate: true + options: + delete: 'delete' + [defaults, groups, elements] @render: (attribute, params = {}) -> diff --git a/app/models/concerns/has_karma_activity_log.rb b/app/models/concerns/has_karma_activity_log.rb new file mode 100644 index 000000000..70768a309 --- /dev/null +++ b/app/models/concerns/has_karma_activity_log.rb @@ -0,0 +1,21 @@ +# Copyright (C) 2012-2016 Zammad Foundation, http://zammad-foundation.org/ +module HasKarmaActivityLog + extend ActiveSupport::Concern + + included do + before_destroy :karma_activity_log_destroy + end + +=begin + +delete object online notification list, will be executed automatically + + model = Model.find(123) + model.karma_activity_log_destroy + +=end + + def karma_activity_log_destroy + Karma::ActivityLog.remove(self.class.to_s, id) + end +end diff --git a/app/models/concerns/has_links.rb b/app/models/concerns/has_links.rb new file mode 100644 index 000000000..4a8e0dc5f --- /dev/null +++ b/app/models/concerns/has_links.rb @@ -0,0 +1,24 @@ +# Copyright (C) 2012-2016 Zammad Foundation, http://zammad-foundation.org/ +module HasLinks + extend ActiveSupport::Concern + + included do + before_destroy :links_destroy + end + +=begin + +delete object link list, will be executed automatically + + model = Model.find(123) + model.links_destroy + +=end + + def links_destroy + Link.remove_all( + link_object: self.class.to_s, + link_object_value: id, + ) + end +end diff --git a/app/models/concerns/has_online_notifications.rb b/app/models/concerns/has_online_notifications.rb new file mode 100644 index 000000000..cb2ee529f --- /dev/null +++ b/app/models/concerns/has_online_notifications.rb @@ -0,0 +1,21 @@ +# Copyright (C) 2012-2016 Zammad Foundation, http://zammad-foundation.org/ +module HasOnlineNotifications + extend ActiveSupport::Concern + + included do + before_destroy :online_notification_destroy + end + +=begin + +delete object online notification list, will be executed automatically + + model = Model.find(123) + model.online_notification_destroy + +=end + + def online_notification_destroy + OnlineNotification.remove(self.class.to_s, id) + end +end diff --git a/app/models/karma/activity_log.rb b/app/models/karma/activity_log.rb index 40ef9ba81..a97a379e8 100644 --- a/app/models/karma/activity_log.rb +++ b/app/models/karma/activity_log.rb @@ -5,6 +5,14 @@ class Karma::ActivityLog < ApplicationModel self.table_name = 'karma_activity_logs' +=begin + +add karma activity log of an object + + Karma::ActivityLog.add('ticket create', User.find(1), 'Ticket', 123) + +=end + def self.add(action, user, object, o_id, force = false) activity = Karma::Activity.lookup(name: action) @@ -47,6 +55,22 @@ class Karma::ActivityLog < ApplicationModel true end +=begin + +remove whole karma activity log of an object + + Karma::ActivityLog.remove('Ticket', 123) + +=end + + def self.remove(object_name, o_id) + object_id = ObjectLookup.by_name(object_name) + Karma::ActivityLog.where( + object_lookup_id: object_id, + o_id: o_id, + ).destroy_all + end + def self.latest(user, limit = 12) result = [] logs = Karma::ActivityLog.where(user_id: user.id).order(id: :desc).limit(limit) diff --git a/app/models/link.rb b/app/models/link.rb index c3b714cbc..01186d59e 100644 --- a/app/models/link.rb +++ b/app/models/link.rb @@ -4,6 +4,8 @@ class Link < ApplicationModel belongs_to :link_type, class_name: 'Link::Type' belongs_to :link_object, class_name: 'Link::Object' + after_destroy :touch_link_references + @map = { 'normal' => 'normal', 'parent' => 'child', @@ -131,52 +133,65 @@ class Link < ApplicationModel linktype = link_type_get(name: data[:link_type]) data[:link_type_id] = linktype.id end - links = Link.where( + Link.where( link_type_id: data[:link_type_id], link_object_source_id: data[:link_object_source_id], link_object_source_value: data[:link_object_source_value], link_object_target_id: data[:link_object_target_id], link_object_target_value: data[:link_object_target_value] - ) - - # touch references - links.each { |link| - link.destroy - touch_reference_by_params( - object: Link::Object.lookup(id: link.link_object_source_id).name, - o_id: link.link_object_source_value, - ) - touch_reference_by_params( - object: Link::Object.lookup(id: link.link_object_target_id).name, - o_id: link.link_object_target_value, - ) - } + ).destroy_all # from the other site if data.key?(:link_type) linktype = link_type_get(name: @map[ data[:link_type] ]) data[:link_type_id] = linktype.id end - links = Link.where( + + Link.where( link_type_id: data[:link_type_id], link_object_target_id: data[:link_object_source_id], link_object_target_value: data[:link_object_source_value], link_object_source_id: data[:link_object_target_id], link_object_source_value: data[:link_object_target_value] - ) + ).destroy_all + end - # touch references - links.each { |link| - link.destroy - touch_reference_by_params( - object: Link::Object.lookup(id: link.link_object_source_id).name, - o_id: link.link_object_source_value, - ) - touch_reference_by_params( - object: Link::Object.lookup(id: link.link_object_target_id).name, - o_id: link.link_object_target_value, - ) - } +=begin + + Link.remove_all( + link_object: 'Ticket', + link_object_value: 6, + ) + +=end + + def self.remove_all(data) + if data.key?(:link_object) + linkobject = link_object_get(name: data[:link_object]) + data[:link_object_id] = linkobject.id + end + + Link.where( + link_object_target_id: data[:link_object_id], + link_object_target_value: data[:link_object_value], + ).destroy_all + Link.where( + link_object_source_id: data[:link_object_id], + link_object_source_value: data[:link_object_value], + ).destroy_all + + true + end + + def touch_link_references + Link.touch_reference_by_params( + object: Link::Object.lookup(id: link_object_source_id).name, + o_id: link_object_source_value, + ) + Link.touch_reference_by_params( + object: Link::Object.lookup(id: link_object_target_id).name, + o_id: link_object_target_value, + ) end def self.link_type_get(data) diff --git a/app/models/ticket.rb b/app/models/ticket.rb index a95f5467d..d34688823 100644 --- a/app/models/ticket.rb +++ b/app/models/ticket.rb @@ -7,6 +7,9 @@ class Ticket < ApplicationModel include HasHistory include HasTags include HasSearchIndexBackend + include HasOnlineNotifications + include HasKarmaActivityLog + include HasLinks include Ticket::Escalation include Ticket::Subject @@ -23,7 +26,6 @@ class Ticket < ApplicationModel after_create :check_escalation_update before_update :check_defaults, :check_title, :reset_pending_time after_update :check_escalation_update - before_destroy :destroy_dependencies validates :group_id, presence: true @@ -55,7 +57,7 @@ class Ticket < ApplicationModel :preferences belongs_to :group, class_name: 'Group' - has_many :articles, class_name: 'Ticket::Article', after_add: :cache_update, after_remove: :cache_update + has_many :articles, class_name: 'Ticket::Article', after_add: :cache_update, after_remove: :cache_update, dependent: :destroy has_many :ticket_time_accounting, class_name: 'Ticket::TimeAccounting', dependent: :destroy belongs_to :organization, class_name: 'Organization' belongs_to :state, class_name: 'Ticket::State' @@ -748,6 +750,19 @@ perform changes on ticket def perform_changes(perform, perform_origin, item = nil, current_user_id = nil) logger.debug "Perform #{perform_origin} #{perform.inspect} on Ticket.find(#{id})" + + # if the configuration contains the deletion of the ticket then + # we skip all other ticket changes because they does not matter + if perform['ticket.action'].present? && perform['ticket.action']['value'] == 'delete' + perform.each do |key, _value| + (object_name, attribute) = key.split('.', 2) + next if object_name != 'ticket' + next if attribute == 'action' + + perform.delete(key) + end + end + changed = false perform.each do |key, value| (object_name, attribute) = key.split('.', 2) @@ -906,6 +921,16 @@ perform changes on ticket next end + # delete ticket + if key == 'ticket.action' + next if value['value'].blank? + next if value['value'] != 'delete' + + destroy + + next + end + # lookup pre_condition if value['pre_condition'] if value['pre_condition'] =~ /^not_set/ @@ -1046,15 +1071,6 @@ result true end - def destroy_dependencies - - # delete articles - articles.destroy_all - - # destroy online notifications - OnlineNotification.remove(self.class.to_s, id) - end - def set_default_state return if state_id diff --git a/spec/factories/online_notification.rb b/spec/factories/online_notification.rb new file mode 100644 index 000000000..37ef435f9 --- /dev/null +++ b/spec/factories/online_notification.rb @@ -0,0 +1,12 @@ +FactoryGirl.define do + factory :online_notification do + object_lookup_id { ObjectLookup.by_name('Ticket') } + type_lookup_id { TypeLookup.by_name('Assigned to you') } + seen false + user_id 1 + 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/tag.rb b/spec/factories/tag.rb new file mode 100644 index 000000000..04e00c15d --- /dev/null +++ b/spec/factories/tag.rb @@ -0,0 +1,7 @@ +FactoryGirl.define do + factory :tag do + tag_object_id { Tag::Object.lookup_by_name_and_create('Ticket').id } + tag_item_id { Tag::Item.lookup_by_name_and_create('blub').id } + created_by_id 1 + end +end diff --git a/spec/factories/ticket/article.rb b/spec/factories/ticket/article.rb new file mode 100644 index 000000000..be26f9c3f --- /dev/null +++ b/spec/factories/ticket/article.rb @@ -0,0 +1,14 @@ +FactoryGirl.define do + factory :ticket_article, class: Ticket::Article do + from 'factory-customer-1@example.com' + to 'factory-customer-1@example.com' + subject 'factory article' + message_id 'factory@id_com_1' + body 'some message 123' + internal false + sender { Ticket::Article::Sender.find_by(name: 'Customer') } + type { Ticket::Article::Type.find_by(name: 'email') } + updated_by_id 1 + created_by_id 1 + end +end diff --git a/spec/models/ticket_spec.rb b/spec/models/ticket_spec.rb index 0c3cf3006..3ef97f67e 100644 --- a/spec/models/ticket_spec.rb +++ b/spec/models/ticket_spec.rb @@ -31,5 +31,147 @@ RSpec.describe Ticket do expect(check_ticket_ids).to match_array(expected_ticket_ids) end + end + + describe '.destroy' do + + it 'deletes all related objects before destroy' do + ApplicationHandleInfo.current = 'application_server' + + source_ticket = create(:ticket) + + # create some links + important_ticket1 = create(:ticket) + important_ticket2 = create(:ticket) + important_ticket3 = create(:ticket) + + # create some articles + create(:ticket_article, ticket_id: source_ticket.id) + create(:ticket_article, ticket_id: source_ticket.id) + create(:ticket_article, ticket_id: source_ticket.id) + + create(:link, link_object_source_value: source_ticket.id, link_object_target_value: important_ticket1.id) + create(:link, link_object_source_value: important_ticket2.id, link_object_target_value: source_ticket.id) + create(:link, link_object_source_value: source_ticket.id, link_object_target_value: important_ticket3.id) + + create(:online_notification, o_id: source_ticket.id) + create(:tag, o_id: source_ticket.id) + + Observer::Transaction.commit + Scheduler.worker(true) + + # get before destroy + activities = ActivityStream.where( + activity_stream_object_id: ObjectLookup.by_name('Ticket'), + o_id: source_ticket.id, + ) + links = Link.list( + link_object: 'Ticket', + link_object_value: source_ticket.id + ) + articles = Ticket::Article.where(ticket_id: source_ticket.id) + history = History.list('Ticket', source_ticket.id, nil, true) + karma_log = Karma::ActivityLog.where( + object_lookup_id: ObjectLookup.by_name('Ticket'), + o_id: source_ticket.id, + ) + online_notifications = OnlineNotification.where( + object_lookup_id: ObjectLookup.by_name('Ticket'), + o_id: source_ticket.id, + ) + recent_views = OnlineNotification.where( + object_lookup_id: ObjectLookup.by_name('Ticket'), + o_id: source_ticket.id, + ) + tags = Tag.tag_list( + object: 'Ticket', + o_id: source_ticket.id, + ) + + # check before destroy + expect(activities.count).to be >= 0 + expect(links.count).to be >= 0 + expect(articles.count).to be >= 0 + expect(history[:list].count).to be >= 0 + expect(karma_log.count).to be >= 0 + expect(online_notifications.count).to be >= 0 + expect(recent_views.count).to be >= 0 + expect(tags.count).to be >= 0 + + # destroy ticket + source_ticket.destroy + + # get after destroy + activities = ActivityStream.where( + activity_stream_object_id: ObjectLookup.by_name('Ticket'), + o_id: source_ticket.id, + ) + links = Link.list( + link_object: 'Ticket', + link_object_value: source_ticket.id + ) + articles = Ticket::Article.where(ticket_id: source_ticket.id) + history = History.list('Ticket', source_ticket.id, nil, true) + karma_log = Karma::ActivityLog.where( + object_lookup_id: ObjectLookup.by_name('Ticket'), + o_id: source_ticket.id, + ) + online_notifications = OnlineNotification.where( + object_lookup_id: ObjectLookup.by_name('Ticket'), + o_id: source_ticket.id, + ) + recent_views = OnlineNotification.where( + object_lookup_id: ObjectLookup.by_name('Ticket'), + o_id: source_ticket.id, + ) + tags = Tag.tag_list( + object: 'Ticket', + o_id: source_ticket.id, + ) + + # check after destroy + expect(activities.count).to be == 0 + expect(links.count).to be == 0 + expect(articles.count).to be == 0 + expect(history[:list].count).to be == 0 + expect(karma_log.count).to be == 0 + expect(online_notifications.count).to be == 0 + expect(recent_views.count).to be == 0 + expect(tags.count).to be == 0 + + end + + end + + describe '.perform_changes' do + + it 'performes a ticket state change on a ticket' do + source_ticket = create(:ticket) + + changes = { + 'ticket.state_id' => { 'value' => Ticket::State.lookup(name: 'closed').id.to_s }, + } + + source_ticket.perform_changes(changes, 'trigger', source_ticket, User.find(1)) + source_ticket.reload + + expect(source_ticket.state.name).to eq('closed') + end + + it 'performes a ticket deletion on a ticket' do + source_ticket = create(:ticket) + + changes = { + 'ticket.state_id' => { 'value' => Ticket::State.lookup(name: 'closed').id.to_s }, + 'ticket.action' => { 'value' => 'delete' }, + } + + source_ticket.perform_changes(changes, 'trigger', source_ticket, User.find(1)) + ticket_with_source_ids = Ticket.where(id: source_ticket.id) + expect(ticket_with_source_ids).to match_array([]) + end + + end + end