From 9aa5afbd6597ffebf7ed3ec1dd45e84e0e06b77f Mon Sep 17 00:00:00 2001 From: Martin Edenhofer Date: Wed, 25 Sep 2013 21:50:28 +0200 Subject: [PATCH] Moved to new model based history methodes. --- app/assets/stylesheets/zzz.css | 2 +- app/controllers/tickets_controller.rb | 2 +- app/models/application_model.rb | 20 +- .../application_model/history_log_base.rb | 64 +++++ app/models/history.rb | 11 + app/models/observer/history.rb | 59 +++-- app/models/ticket.rb | 3 +- app/models/ticket/article.rb | 1 + app/models/ticket/article/history_log.rb | 67 ++++++ app/models/ticket/escalation.rb | 2 +- app/models/ticket/history_log.rb | 65 ++++++ test/unit/history_test.rb | 219 +++++++++++++++--- 12 files changed, 440 insertions(+), 75 deletions(-) create mode 100644 app/models/application_model/history_log_base.rb create mode 100644 app/models/ticket/article/history_log.rb create mode 100644 app/models/ticket/history_log.rb diff --git a/app/assets/stylesheets/zzz.css b/app/assets/stylesheets/zzz.css index 76038834a..05ef79b7b 100644 --- a/app/assets/stylesheets/zzz.css +++ b/app/assets/stylesheets/zzz.css @@ -724,7 +724,7 @@ footer { .ticket-article { padding: 8px 0 6px 2px; - min-height: 110px; + min-height: 116px; margin: 2px 0; } .ticket-article-item { diff --git a/app/controllers/tickets_controller.rb b/app/controllers/tickets_controller.rb index 5bd83b48e..4dee5c2d2 100644 --- a/app/controllers/tickets_controller.rb +++ b/app/controllers/tickets_controller.rb @@ -130,7 +130,7 @@ class TicketsController < ApplicationController return if !ticket_permission( ticket ) # get history of ticket - history = History.list( 'Ticket', params[:id], 'Ticket::Article' ) + history = ticket.history_get # get related assets assets = ticket.assets({}) diff --git a/app/models/application_model.rb b/app/models/application_model.rb index c8601c1d2..b1782c518 100644 --- a/app/models/application_model.rb +++ b/app/models/application_model.rb @@ -1,12 +1,15 @@ # Copyright (C) 2013-2013 Zammad Foundation, http://zammad-foundation.org/ class ApplicationModel < ActiveRecord::Base + include ApplicationModel::HistoryLogBase + self.abstract_class = true before_create :check_attributes_protected, :cache_delete, :fill_up_user_create before_create :cache_delete, :fill_up_user_create before_update :cache_delete_before, :fill_up_user_update - before_destroy :cache_delete_before + before_destroy :cache_delete_before, :destroy_dependencies + after_create :cache_delete after_update :cache_delete after_destroy :cache_delete @@ -402,4 +405,19 @@ class OwnModel < ApplicationModel :data => { :id => self.id, :updated_at => self.updated_at } ) end + + private + +=begin + +destory object dependencies, will be executed automatically + +=end + + def destroy_dependencies + + # delete history + History.remove( self.class.to_s, self.id ) + end + end diff --git a/app/models/application_model/history_log_base.rb b/app/models/application_model/history_log_base.rb new file mode 100644 index 000000000..87e377c79 --- /dev/null +++ b/app/models/application_model/history_log_base.rb @@ -0,0 +1,64 @@ +# Copyright (C) 2012-2013 Zammad Foundation, http://zammad-foundation.org/ + +module ApplicationModel::HistoryLogBase + +=begin + +create history entry for this object + + organization = Organization.find(123) + result = organization.history_create( 'created', user_id ) + +returns + + result = true # false + +=end + + def history_create (type, user_id, data = {}) + data[:o_id] = self['id'] + data[:history_type] = type + data[:history_object] = self.class.name + data[:related_o_id] = nil + data[:related_history_object] = nil + data[:created_by_id] = user_id + History.add(data) + end + +=begin + +get history log for this object + + organization = Organization.find(123) + result = organization.history_get() + +returns + + result = [ + { + :history_type => 'created', + :history_object => 'Organization', + :created_by_id => 3, + :created_at => "2013-08-19 20:41:33", + }, + { + :history_type => 'updated', + :history_object => 'Organization', + :history_attribute => 'note', + :o_id => 1, + :id_to => nil, + :id_from => nil, + :value_from => "some note", + :value_to => "some other note", + :created_by_id => 3, + :created_at => "2013-08-19 20:41:33", + }, + ] + +=end + + def history_get + History.list( self.class.name, self['id'] ) + end + +end \ No newline at end of file diff --git a/app/models/history.rb b/app/models/history.rb index dddca9e01..581877682 100644 --- a/app/models/history.rb +++ b/app/models/history.rb @@ -267,6 +267,17 @@ return all histoy entries of an object return history_attribute end + private + +=begin + +nothing to do on destroying history entries + +=end + + def destroy_dependencies + end + class Object < ApplicationModel end diff --git a/app/models/observer/history.rb b/app/models/observer/history.rb index f22edd765..15ceff027 100644 --- a/app/models/observer/history.rb +++ b/app/models/observer/history.rb @@ -3,7 +3,7 @@ require 'history' class Observer::History < ActiveRecord::Observer - observe :ticket, :user, 'ticket::_article' + observe :ticket, 'ticket::_article', :user, :organization, :group def after_create(record) @@ -13,22 +13,17 @@ class Observer::History < ActiveRecord::Observer puts "HISTORY OBSERVER, object created #{ record.class.name }.find(#{ record.id })" # puts record.inspect - # if Ticket::Article has changed, remember ticket to be able - # to show article changes in ticket history - related_o_id = nil - related_history_object_id = nil - if record.class.name == 'Ticket::Article' - related_o_id = record.ticket_id - related_history_object = 'Ticket' + user_id = record.created_by_id || UserInfo.current_user_id || 1 + + # log activity stream + if record.respond_to?('history_create') + record.history_create( 'created', user_id ) + end + + # log activity stream + if record.respond_to?('activity_stream') + record.activity_stream( 'created', user_id ) end - History.add( - :o_id => record.id, - :history_type => 'created', - :history_object => record.class.name, - :related_o_id => related_o_id, - :related_history_object => related_history_object, - :created_by_id => record.created_by_id || UserInfo.current_user_id || 1 - ) end def before_update(record) @@ -83,6 +78,8 @@ class Observer::History < ActiveRecord::Observer :create_article_sender_id => true, } + user_id = record.created_by_id || UserInfo.current_user_id || 1 + history_logged = false diff.each do |key, value_ids| # do not log created_at and updated_at attributes @@ -137,28 +134,26 @@ class Observer::History < ActiveRecord::Observer attribute_name = attribute_name.scan(/^(.*)_id$/).first.first end - # if Ticket::Article has changed, remember ticket to be able - # to show article changes in ticket history - related_o_id = nil - related_history_object_id = nil - if record.class.name == 'Ticket::Article' - related_o_id = record.ticket_id - related_history_object_id = 'Ticket' - end - History.add( - :o_id => current.id, - :history_type => 'updated', - :history_object => record.class.name, + history_logged = true + + data = { :history_attribute => attribute_name, - :related_o_id => related_o_id, - :related_history_object => related_history_object_id, :value_from => value[0], :value_to => value[1], :id_from => value_ids[0], :id_to => value_ids[1], - :created_by_id => record['updated_by_id'] || UserInfo.current_user_id || 1 - ) + } + # log activity stream + if record.respond_to?('history_create') + record.history_create( 'updated', user_id, data ) + end + end + # log activity stream + if history_logged + if record.respond_to?('activity_stream') + record.activity_stream( 'updated', user_id ) + end end end diff --git a/app/models/ticket.rb b/app/models/ticket.rb index b27c4211c..b9a54a5a0 100644 --- a/app/models/ticket.rb +++ b/app/models/ticket.rb @@ -23,6 +23,7 @@ class Ticket < ApplicationModel include Ticket::Subject include Ticket::Permission include Ticket::Assets + include Ticket::HistoryLog extend Ticket::Search attr_accessor :callback_loop @@ -118,7 +119,7 @@ returns def destroy_dependencies # delete history - History.remove( 'Ticket', self.id ) + History.remove( self.class.to_s, self.id ) # delete articles self.articles.destroy_all diff --git a/app/models/ticket/article.rb b/app/models/ticket/article.rb index c1bf19b4a..cc2b0f2fa 100644 --- a/app/models/ticket/article.rb +++ b/app/models/ticket/article.rb @@ -2,6 +2,7 @@ class Ticket::Article < ApplicationModel include Ticket::Article::Assets + include Ticket::Article::HistoryLog after_create :attachment_check belongs_to :ticket diff --git a/app/models/ticket/article/history_log.rb b/app/models/ticket/article/history_log.rb new file mode 100644 index 000000000..3c4f6dbd4 --- /dev/null +++ b/app/models/ticket/article/history_log.rb @@ -0,0 +1,67 @@ +# Copyright (C) 2012-2013 Zammad Foundation, httpdata[://zammad-foundation.org/ + +module Ticket::Article::HistoryLog + +=begin + +create log activity for this article + + article = Ticket::Article.find(123) + result = article.history_create( 'created', user_id ) + +returns + + result = true # false + +=end + + def history_create (type, user_id, data = {}) + + # if Ticketdata[:data[:Article has changed, remember related ticket to be able + # to show article changes in ticket history + data[:o_id] = self['id'] + data[:history_type] = type + data[:history_object] = self.class.name + data[:related_o_id] = self['ticket_id'] + data[:related_history_object] = 'Ticket' + data[:created_by_id] = user_id + History.add(data) + end + +=begin + +get log activity for this article + + article = Ticket::Article.find(123) + result = article.history_get() + +returns + + result = [ + { + :history_type => 'created', + :history_object => 'Ticket::Article', + :created_by_id => 3, + :created_at => "2013-08-19 20:41:33", + }, + { + :history_type => 'updated', + :history_object => 'Ticket::Article', + :history_attribute => 'from', + :o_id => 1, + :id_to => nil, + :id_from => nil, + :value_from => "Some Body", + :value_to => "Some Body Else", + :created_by_id => 3, + :created_at => "2013-08-19 20:41:33", + }, + ] + +=end + + def history_get + History.list( self.class.name, self['id'] ) + end + +end \ No newline at end of file diff --git a/app/models/ticket/escalation.rb b/app/models/ticket/escalation.rb index 9ec408d39..004bd3392 100644 --- a/app/models/ticket/escalation.rb +++ b/app/models/ticket/escalation.rb @@ -212,7 +212,7 @@ def escalation_calculation_get_sla total_time_without_pending = 0 total_time = 0 #get history for ticket - history_list = History.list( 'Ticket', self.id ) + history_list = self.history_get #loop through hist. changes and get time last_state = nil diff --git a/app/models/ticket/history_log.rb b/app/models/ticket/history_log.rb new file mode 100644 index 000000000..40792e56f --- /dev/null +++ b/app/models/ticket/history_log.rb @@ -0,0 +1,65 @@ +# Copyright (C) 2012-2013 Zammad Foundation, http://zammad-foundation.org/ + +module Ticket::HistoryLog + +=begin + +create log activity for this ticket + + ticket = Ticket.find(123) + result = ticket.history_create( 'created', user_id ) + +returns + + result = true # false + +=end + + def history_create (type, user_id, data = {}) + + data[:o_id] = self['id'] + data[:history_type] = type + data[:history_object] = self.class.name + data[:related_o_id] = nil + data[:related_history_object] = nil + data[:created_by_id] = user_id + History.add(data) + end + +=begin + +get log activity for this ticket + + ticket = Ticket.find(123) + result = ticket.history_get() + +returns + + result = [ + { + :history_type => 'created', + :history_object => 'Ticket', + :created_by_id => 3, + :created_at => "2013-08-19 20:41:33", + }, + { + :history_type => 'updated', + :history_object => 'Ticket', + :history_attribute => 'ticket_priority', + :o_id => 1, + :id_to => 3, + :id_from => 2, + :value_from => "low", + :value_to => "high", + :created_by_id => 3, + :created_at => "2013-08-19 20:41:33", + }, + ] + +=end + + def history_get + History.list( self.class.name, self['id'], 'Ticket::Article' ) + end + +end \ No newline at end of file diff --git a/test/unit/history_test.rb b/test/unit/history_test.rb index 2f8a347ff..529586afe 100644 --- a/test/unit/history_test.rb +++ b/test/unit/history_test.rb @@ -161,44 +161,8 @@ class HistoryTest < ActiveSupport::TestCase # remember ticket tickets.push ticket - # get history - history_list = History.list( 'Ticket', ticket.id, 'Ticket::Article' ) -# puts history_list.inspect - test[:history_check].each { |check_item| -# puts '+++++++++++' -# puts check_item.inspect - match = false - history_list.each { |history_item| - next if match -# puts '--------' -# puts history_item.inspect - next if history_item.history_object.name != check_item[:history_object] - next if history_item.history_type.name != check_item[:history_type] - if check_item[:history_attribute] - next if check_item[:history_attribute] != history_item.history_attribute.name - end - match = true - if history_item.history_type.name == check_item[:history_type] - assert( true, "History type #{history_item.history_type.name} found!") - end - if check_item[:history_attribute] - assert_equal( check_item[:history_attribute], history_item.history_attribute.name, "check history attribute #{check_item[:history_attribute]}") - end - if check_item[:value_from] - assert_equal( check_item[:value_from], history_item.value_from, "check history :value_from #{history_item.value_from} ok") - end - if check_item[:value_to] - assert_equal( check_item[:value_to], history_item.value_to, "check history :value_to #{history_item.value_to} ok") - end - if check_item[:id_from] - assert_equal( check_item[:id_from], history_item.id_from, "check history :id_from #{history_item.id_from} ok") - end - if check_item[:id_to] - assert_equal( check_item[:id_to], history_item.id_to, "check history :id_to #{history_item.id_to} ok") - end - } - assert( match, "history check not matched! #{check_item.inspect}") - } + # check history + history_check( ticket.history_get, test[:history_check] ) } # delete tickets @@ -209,4 +173,183 @@ class HistoryTest < ActiveSupport::TestCase assert( !found, "Ticket destroyed") } end + + test 'user' do + tests = [ + + # test 1 + { + :user_create => { + :user => { + :login => 'some_login_test', + :firstname => 'Bob', + :lastname => 'Smith', + :email => 'somebody@example.com', + :updated_by_id => User.lookup( :login => 'nicole.braun@zammad.org' ).id, + :created_by_id => User.lookup( :login => 'nicole.braun@zammad.org' ).id, + }, + }, + :user_update => { + :user => { + :firstname => 'Bob', + :lastname => 'Master', + :email => 'master@example.com', }, + }, + :history_check => [ + { + :history_object => 'User', + :history_type => 'created', + }, + { + :history_object => 'User', + :history_type => 'updated', + :history_attribute => 'lastname', + :value_from => 'Smith', + :value_to => 'Master', + }, + ], + }, + + ] + users = [] + tests.each { |test| + + user = nil + + # user transaction + ActiveRecord::Base.transaction do + user = User.create( test[:user_create][:user]) + + assert_equal( user.class.to_s, 'User' ) + + # update user + if test[:user_update][:user] + user.update_attributes( test[:user_update][:user] ) + end + end + + # remember user + users.push user + + # check history + history_check( user.history_get, test[:history_check] ) + } + + # delete user + users.each { |user| + user_id = user.id + user.destroy + found = User.where( :id => user_id ).first + assert( !found, "User destroyed") + } + end + + test 'organization' do + tests = [ + + # test 1 + { + :organization_create => { + :organization => { + :name => 'Org äöüß', + :note => 'some note', + :updated_by_id => User.lookup( :login => 'nicole.braun@zammad.org' ).id, + :created_by_id => User.lookup( :login => 'nicole.braun@zammad.org' ).id, + }, + }, + :organization_update => { + :organization => { + :name => 'Org 123', + :note => 'some note', + }, + }, + :history_check => [ + { + :history_object => 'Organization', + :history_type => 'created', + }, + { + :history_object => 'Organization', + :history_type => 'updated', + :history_attribute => 'name', + :value_from => 'Org äöüß', + :value_to => 'Org 123', + }, + ], + }, + ] + organizations = [] + tests.each { |test| + + organization = nil + + # user transaction + ActiveRecord::Base.transaction do + organization = Organization.create( test[:organization_create][:organization]) + + assert_equal( organization.class.to_s, 'Organization' ) + + # update organization + if test[:organization_update][:organization] + organization.update_attributes( test[:organization_update][:organization] ) + end + end + + # remember user + organizations.push organization + + # check history + history_check( organization.history_get, test[:history_check] ) + } + + # delete user + organizations.each { |organization| + organization_id = organization.id + organization.destroy + found = Organization.where( :id => organization_id ).first + assert( !found, "Organization destroyed") + } + end + + + def history_check( history_list, history_check ) +# puts history_list.inspect + history_check.each { |check_item| +# puts '+++++++++++' +# puts check_item.inspect + match = false + history_list.each { |history_item| + next if match +# puts '--------' +# puts history_item.inspect +# puts history_item.history_object.name + next if history_item.history_object.name != check_item[:history_object] + next if history_item.history_type.name != check_item[:history_type] + if check_item[:history_attribute] + next if check_item[:history_attribute] != history_item.history_attribute.name + end + match = true + if history_item.history_type.name == check_item[:history_type] + assert( true, "History type #{history_item.history_type.name} found!") + end + if check_item[:history_attribute] + assert_equal( check_item[:history_attribute], history_item.history_attribute.name, "check history attribute #{check_item[:history_attribute]}") + end + if check_item[:value_from] + assert_equal( check_item[:value_from], history_item.value_from, "check history :value_from #{history_item.value_from} ok") + end + if check_item[:value_to] + assert_equal( check_item[:value_to], history_item.value_to, "check history :value_to #{history_item.value_to} ok") + end + if check_item[:id_from] + assert_equal( check_item[:id_from], history_item.id_from, "check history :id_from #{history_item.id_from} ok") + end + if check_item[:id_to] + assert_equal( check_item[:id_to], history_item.id_to, "check history :id_to #{history_item.id_to} ok") + end + } + assert( match, "history check not matched! #{check_item.inspect}") + } + end + end