From 9e20b7d995db354a40608f9ff8caba5bbb941577 Mon Sep 17 00:00:00 2001 From: Martin Edenhofer Date: Tue, 4 Jun 2013 14:52:56 +0200 Subject: [PATCH 1/2] Improved API, renamed methodes. --- app/controllers/tickets_controller.rb | 2 +- app/models/history.rb | 16 ++++++----- app/models/observer/history.rb | 6 ++-- app/models/observer/tag/ticket_history.rb | 8 ++---- .../ticket/article/communicate_email.rb | 5 ++-- app/models/observer/ticket/notification.rb | 6 ++-- app/models/ticket.rb | 4 +-- lib/import/otrs.rb | 28 +++++++++---------- test/unit/history_test.rb | 10 +++---- 9 files changed, 42 insertions(+), 43 deletions(-) diff --git a/app/controllers/tickets_controller.rb b/app/controllers/tickets_controller.rb index 0843c62ea..d10ca9ee7 100644 --- a/app/controllers/tickets_controller.rb +++ b/app/controllers/tickets_controller.rb @@ -147,7 +147,7 @@ class TicketsController < ApplicationController return if !ticket_permission( ticket ) # get history of ticket - history = History.history_list( 'Ticket', params[:id], 'Ticket::Article' ) + history = History.list( 'Ticket', params[:id], 'Ticket::Article' ) # get related users users = {} diff --git a/app/models/history.rb b/app/models/history.rb index 592f080d9..dbf684789 100644 --- a/app/models/history.rb +++ b/app/models/history.rb @@ -10,7 +10,7 @@ class History < ApplicationModel @@cache_object = {} @@cache_attribute = {} - def self.history_create(data) + def self.add(data) # lookups if data[:history_type] @@ -61,13 +61,15 @@ class History < ApplicationModel end end - def self.history_destroy( requested_object, requested_object_id ) - History.where( :history_object_id => History::Object.where( :name => requested_object ) ). - where( :o_id => requested_object_id ). - destroy_all + def self.remove( requested_object, requested_object_id ) + history_object = History::Object.where( :name => requested_object ).first + History.where( + :history_object_id => history_object.id, + :o_id => requested_object_id, + ).destroy_all end - def self.history_list( requested_object, requested_object_id, related_history_object = nil ) + def self.list( requested_object, requested_object_id, related_history_object = nil ) if !related_history_object history_object = self.history_object_lookup( requested_object ) history = History.where( :history_object_id => history_object.id ). @@ -179,7 +181,7 @@ class History < ApplicationModel if item['history_object'] == 'User' users[ item['o_id'] ] = User.user_data_full( item['o_id'] ) end - + # load users if !users[ item['created_by_id'] ] users[ item['created_by_id'] ] = User.user_data_full( item['created_by_id'] ) diff --git a/app/models/observer/history.rb b/app/models/observer/history.rb index ba5801775..dfd210295 100644 --- a/app/models/observer/history.rb +++ b/app/models/observer/history.rb @@ -19,7 +19,7 @@ class Observer::History < ActiveRecord::Observer related_o_id = record.ticket_id related_history_object = 'Ticket' end - History.history_create( + History.add( :o_id => record.id, :history_type => 'created', :history_object => record.class.name, @@ -143,7 +143,7 @@ class Observer::History < ActiveRecord::Observer related_o_id = record.ticket_id related_history_object_id = 'Ticket' end - History.history_create( + History.add( :o_id => current.id, :history_type => 'updated', :history_object => record.class.name, @@ -169,4 +169,4 @@ class Observer::History < ActiveRecord::Observer end h end -end \ No newline at end of file +end diff --git a/app/models/observer/tag/ticket_history.rb b/app/models/observer/tag/ticket_history.rb index 39647c42a..dfbc4e89a 100644 --- a/app/models/observer/tag/ticket_history.rb +++ b/app/models/observer/tag/ticket_history.rb @@ -9,13 +9,12 @@ class Observer::Tag::TicketHistory < ActiveRecord::Observer return true if record.tag_object.name != 'Ticket'; # add ticket history - History.history_create( + History.add( :o_id => record.o_id, :history_type => 'added', :history_object => 'Ticket', :history_attribute => 'Tag', :value_from => record.tag_item.name, - :created_by_id => record.created_by_id || UserInfo.current_user_id || 1 ) end def after_destroy(record) @@ -24,13 +23,12 @@ class Observer::Tag::TicketHistory < ActiveRecord::Observer return true if record.tag_object.name != 'Ticket'; # add ticket history - History.history_create( + History.add( :o_id => record.o_id, :history_type => 'removed', :history_object => 'Ticket', :history_attribute => 'Tag', :value_from => record.tag_item.name, - :created_by_id => record.created_by_id || UserInfo.current_user_id || 1 ) end -end \ No newline at end of file +end diff --git a/app/models/observer/ticket/article/communicate_email.rb b/app/models/observer/ticket/article/communicate_email.rb index 1023b2d77..7b32cd689 100644 --- a/app/models/observer/ticket/article/communicate_email.rb +++ b/app/models/observer/ticket/article/communicate_email.rb @@ -55,7 +55,7 @@ class Observer::Ticket::Article::CommunicateEmail < ActiveRecord::Observer end } if recipient_list != '' - History.history_create( + History.add( :o_id => record.id, :history_type => 'email', :history_object => 'Ticket::Article', @@ -63,8 +63,7 @@ class Observer::Ticket::Article::CommunicateEmail < ActiveRecord::Observer :related_history_object => 'Ticket', :value_from => record.subject, :value_to => recipient_list, - :created_by_id => record.created_by_id, ) end end -end \ No newline at end of file +end diff --git a/app/models/observer/ticket/notification.rb b/app/models/observer/ticket/notification.rb index f07afc9da..750e38c7f 100644 --- a/app/models/observer/ticket/notification.rb +++ b/app/models/observer/ticket/notification.rb @@ -24,7 +24,7 @@ class Observer::Ticket::Notification < ActiveRecord::Observer ticket = article.ticket elsif event[:name] == 'Ticket' ticket = Ticket.lookup( :id => event[:id] ) - + # next if ticket is already deleted next if !ticket @@ -235,7 +235,7 @@ From: #{article.from} # add history record if recipient_list != '' - History.history_create( + History.add( :o_id => ticket.id, :history_type => 'notification', :history_object => 'Ticket', @@ -299,4 +299,4 @@ From: #{article.from} # puts @a.inspect # AuditTrail.new(record, "UPDATED") end -end \ No newline at end of file +end diff --git a/app/models/ticket.rb b/app/models/ticket.rb index 613f4265a..e2f8c3d8e 100644 --- a/app/models/ticket.rb +++ b/app/models/ticket.rb @@ -695,7 +695,7 @@ class Ticket < ApplicationModel def destroy_dependencies # delete history - History.history_destroy( 'Ticket', self.id ) + History.remove( 'Ticket', self.id ) # delete articles self.articles.destroy_all @@ -704,4 +704,4 @@ class Ticket < ApplicationModel class Number end -end \ No newline at end of file +end diff --git a/lib/import/otrs.rb b/lib/import/otrs.rb index 1179ee0da..ed7ceb6d9 100644 --- a/lib/import/otrs.rb +++ b/lib/import/otrs.rb @@ -206,7 +206,7 @@ module Import::OTRS result = json(response) self._ticket_result(result) end - + def self._ticket_result(result) # puts result.inspect map = { @@ -309,7 +309,7 @@ module Import::OTRS end record['Articles'].each { |article| - + # get article values article_new = { :created_by_id => 1, @@ -342,10 +342,10 @@ module Import::OTRS rescue display_name = article_new[:from] end - + # do extra decoding because we needed to use field.value display_name = Mail::Field.new( 'X-From', display_name ).to_s - + roles = Role.lookup( :name => 'Customer' ) user = User.create( :login => email, @@ -361,7 +361,7 @@ module Import::OTRS end article_new[:created_by_id] = user.id end - + if article_new[:ticket_article_sender] == 'customer' article_new[:ticket_article_sender_id] = Ticket::Article::Sender.lookup( :name => 'Customer' ).id article_new.delete( :ticket_article_sender ) @@ -374,7 +374,7 @@ module Import::OTRS article_new[:ticket_article_sender_id] = Ticket::Article::Sender.lookup( :name => 'System' ).id article_new.delete( :ticket_article_sender ) end - + if article_new[:ticket_article_type] == 'email-external' article_new[:ticket_article_type_id] = Ticket::Article::Type.lookup( :name => 'email' ).id article_new[:internal] = false @@ -410,14 +410,14 @@ module Import::OTRS article.id = article_new[:id] article.save end - + } - + record['History'].each { |history| # puts '-------' # puts history.inspect if history['HistoryType'] == 'NewTicket' - History.history_create( + History.add( :id => history['HistoryID'], :o_id => history['TicketID'], :history_type => 'created', @@ -444,7 +444,7 @@ module Import::OTRS end end # puts "STATE UPDATE (#{history['HistoryID']}): -> #{from}->#{to}" - History.history_create( + History.add( :id => history['HistoryID'], :o_id => history['TicketID'], :history_type => 'updated', @@ -469,7 +469,7 @@ module Import::OTRS to = $3 to_id = $4 end - History.history_create( + History.add( :id => history['HistoryID'], :o_id => history['TicketID'], :history_type => 'updated', @@ -494,7 +494,7 @@ module Import::OTRS to = $3 to_id = $4 end - History.history_create( + History.add( :id => history['HistoryID'], :o_id => history['TicketID'], :history_type => 'updated', @@ -509,7 +509,7 @@ module Import::OTRS ) end if history['ArticleID'] && history['ArticleID'] != 0 - History.history_create( + History.add( :id => history['HistoryID'], :o_id => history['ArticleID'], :history_type => 'created', @@ -688,7 +688,7 @@ module Import::OTRS :UserLastname => :lastname, # :UserTitle => :UserLogin => :login, - :UserPw => :password, + :UserPw => :password, }; result.each { |user| diff --git a/test/unit/history_test.rb b/test/unit/history_test.rb index 6e8623115..35f2ce47a 100644 --- a/test/unit/history_test.rb +++ b/test/unit/history_test.rb @@ -1,6 +1,6 @@ # encoding: utf-8 require 'test_helper' - + class HistoryTest < ActiveSupport::TestCase test 'ticket' do tests = [ @@ -154,15 +154,15 @@ class HistoryTest < ActiveSupport::TestCase article.update_attributes( test[:ticket_update][:article] ) end end - - # execute ticket events + + # execute ticket events Observer::Ticket::Notification.transaction # remember ticket tickets.push ticket # get history - history_list = History.history_list( 'Ticket', ticket.id, 'Ticket::Article' ) + history_list = History.list( 'Ticket', ticket.id, 'Ticket::Article' ) puts history_list.inspect test[:history_check].each { |check_item| # puts '+++++++++++' @@ -207,4 +207,4 @@ class HistoryTest < ActiveSupport::TestCase assert( !found, "Ticket destroyed") } end -end \ No newline at end of file +end From d1c69a6170833d4da485a2378dbc65fb7b9b941f Mon Sep 17 00:00:00 2001 From: Martin Edenhofer Date: Tue, 4 Jun 2013 20:11:28 +0200 Subject: [PATCH 2/2] Moved strip of meta data for small JSON stream from model to controller. History.list will return real objects now. --- app/controllers/tickets_controller.rb | 37 ++++++++++++++++++++--- app/models/history.rb | 31 +------------------ test/unit/history_test.rb | 43 ++++++++++++--------------- 3 files changed, 53 insertions(+), 58 deletions(-) diff --git a/app/controllers/tickets_controller.rb b/app/controllers/tickets_controller.rb index d10ca9ee7..2d1a940fb 100644 --- a/app/controllers/tickets_controller.rb +++ b/app/controllers/tickets_controller.rb @@ -153,13 +153,42 @@ class TicketsController < ApplicationController users = {} users[ ticket.owner_id ] = User.user_data_full( ticket.owner_id ) users[ ticket.customer_id ] = User.user_data_full( ticket.customer_id ) + + # get only needed data to send to browser + history_list = [] history.each do |item| - users[ item['created_by_id'] ] = User.user_data_full( item['created_by_id'] ) + users[ item[:created_by_id] ] = User.user_data_full( item[:created_by_id] ) + item_tmp = item.attributes if item['history_object'] == 'Ticket::Article' - item['type'] = 'Article ' + item['type'].to_s + item_temp['type'] = 'Article ' + item['type'].to_s else - item['type'] = 'Ticket ' + item['type'].to_s + item_tmp['type'] = 'Ticket ' + item['type'].to_s end + item_tmp['history_type'] = item.history_type.name + item_tmp['history_object'] = item.history_object.name + if item.history_attribute + item_tmp['history_attribute'] = item.history_attribute.name + end + item_tmp.delete( 'history_attribute_id' ) + item_tmp.delete( 'history_object_id' ) + item_tmp.delete( 'history_type_id' ) + item_tmp.delete( 'o_id' ) + item_tmp.delete( 'updated_at' ) + if item_tmp['id_to'] == nil && item_tmp['id_from'] == nil + item_tmp.delete( 'id_to' ) + item_tmp.delete( 'id_from' ) + end + if item_tmp['value_to'] == nil && item_tmp['value_from'] == nil + item_tmp.delete( 'value_to' ) + item_tmp.delete( 'value_from' ) + end + if item_tmp['related_history_object_id'] == nil + item_tmp.delete( 'related_history_object_id' ) + end + if item_tmp['related_o_id'] == nil + item_tmp.delete( 'related_o_id' ) + end + history_list.push item_tmp end # fetch meta relations @@ -171,7 +200,7 @@ class TicketsController < ApplicationController render :json => { :ticket => ticket, :users => users, - :history => history, + :history => history_list, :history_objects => history_objects, :history_types => history_types, :history_attributes => history_attributes diff --git a/app/models/history.rb b/app/models/history.rb index dbf684789..03f6281c8 100644 --- a/app/models/history.rb +++ b/app/models/history.rb @@ -90,36 +90,7 @@ class History < ApplicationModel order('created_at ASC, id ASC') end - list = [] - history.each { |item| - item_tmp = item.attributes - item_tmp['history_type'] = item.history_type.name - item_tmp['history_object'] = item.history_object.name - if item.history_attribute - item_tmp['history_attribute'] = item.history_attribute.name - end - item_tmp.delete( 'history_attribute_id' ) - item_tmp.delete( 'history_object_id' ) - item_tmp.delete( 'history_type_id' ) - item_tmp.delete( 'o_id' ) - item_tmp.delete( 'updated_at' ) - if item_tmp['id_to'] == nil && item_tmp['id_from'] == nil - item_tmp.delete( 'id_to' ) - item_tmp.delete( 'id_from' ) - end - if item_tmp['value_to'] == nil && item_tmp['value_from'] == nil - item_tmp.delete( 'value_to' ) - item_tmp.delete( 'value_from' ) - end - if item_tmp['related_history_object_id'] == nil - item_tmp.delete( 'related_history_object_id' ) - end - if item_tmp['related_o_id'] == nil - item_tmp.delete( 'related_o_id' ) - end - list.push item_tmp - } - return list + return history end def self.activity_stream( user, limit = 10 ) diff --git a/test/unit/history_test.rb b/test/unit/history_test.rb index 35f2ce47a..e3298afe9 100644 --- a/test/unit/history_test.rb +++ b/test/unit/history_test.rb @@ -163,39 +163,34 @@ class HistoryTest < ActiveSupport::TestCase # 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 + test[:history_check].each { |check| match = false - history_list.each { |history_item| + history_list.each { |history| next if match -# puts '--------' -# puts history_item.inspect - next if history_item['history_object'] != check_item[:history_object] - next if history_item['history_type'] != check_item[:history_type] - next if check_item[:history_attribute] != history_item['history_attribute'] + next if history.history_object.name != check[:history_object] + next if history.history_type.name != check[:history_type] + next if check[:history_attribute] && history.history_attribute.name != check[:history_attribute] match = true - if history_item['history_type'] == check_item[:history_type] - assert( true, "History type #{history_item['history_type']} found!") + if history.history_type.name == check[:history_type] + assert( true, "History type #{history.history_type.name} found!") end - if check_item[:history_attribute] - assert_equal( check_item[:history_attribute], history_item['history_attribute'], "check history attribute #{check_item[:history_attribute]}") + if check[:history_attribute] + assert_equal( check[:history_attribute], history.history_attribute.name, "check history attribute #{check[: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") + if check[:value_from] + assert_equal( check[:value_from], history.value_from, "check history :value_from #{history.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") + if check[:value_to] + assert_equal( check[:value_to], history.value_to, "check history :value_to #{history.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") + if check[:id_from] + assert_equal( check[:id_from], history.id_from, "check history :id_from #{history.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") + if check[:id_to] + assert_equal( check[:id_to], history.id_to, "check history :id_to #{history.id_to} ok") end } - assert( match, "history check not matched! #{check_item.inspect}") + assert( match, "history check not matched! #{check.inspect}") } } @@ -204,7 +199,7 @@ class HistoryTest < ActiveSupport::TestCase ticket_id = ticket.id ticket.destroy found = Ticket.where( :id => ticket_id ).first - assert( !found, "Ticket destroyed") + assert( !found, 'Ticket destroyed') } end end