Fixed issue #2420 - Unable to show ticket history.

app/controllers/organizations_controller.rb

@@ -314,10 +314,7 @@ curl http://localhost/api/v1/organization/{id} -v -u #{login}:#{password} -H "Co
     organization = Organization.find(params[:id])
 
     # get history of organization
-    history = organization.history_get(true)
-
-    # return result
-    render json: history
+    render json: organization.history_get(true)
   end
 
   # @path    [GET] /organizations/import_example

app/controllers/tickets_controller.rb

@@ -294,10 +294,7 @@ class TicketsController < ApplicationController
     access!(ticket, 'read')
 
     # get history of ticket
-    history = ticket.history_get(true)
-
-    # return result
-    render json: history
+    render json: ticket.history_get(true)
   end
 
   # GET /api/v1/ticket_related/1

app/controllers/users_controller.rb

@@ -502,10 +502,7 @@ class UsersController < ApplicationController
     user = User.find(params[:id])
 
     # get history of user
-    history = user.history_get(true)
-
-    # return result
-    render json: history
+    render json: user.history_get(true)
   end
 
 =begin

app/models/concerns/has_history.rb

@@ -204,12 +204,14 @@ returns
 =end
 
   def history_get(fulldata = false)
+    relation_object = self.class.instance_variable_get(:@history_relation_object) || nil
+
     if !fulldata
-      return History.list(self.class.name, self['id'])
+      return History.list(self.class.name, self['id'], relation_object)
     end
 
     # get related objects
-    history = History.list(self.class.name, self['id'], nil, true)
+    history = History.list(self.class.name, self['id'], relation_object, true)
     history[:list].each do |item|
       record = Kernel.const_get(item['object']).find(item['o_id'])
 
@@ -242,5 +244,21 @@ end
     def history_attributes_ignored(*attributes)
       @history_attributes_ignored = attributes
     end
+
+=begin
+
+serve methode to ignore model attributes in historization
+
+class Model < ApplicationModel
+  include HasHistory
+  history_relation_object 'Some::Relation::Object'
+end
+
+=end
+
+    def history_relation_object(attribute)
+      @history_relation_object = attribute
+    end
+
   end
 end

app/models/history.rb

@@ -40,19 +40,19 @@ add a new history entry for an object
     return if Setting.get('import_mode') && !data[:id]
 
     # lookups
-    if data[:history_type]
+    if data[:history_type].present?
       history_type = type_lookup(data[:history_type])
     end
-    if data[:history_object]
+    if data[:history_object].present?
       history_object = object_lookup(data[:history_object])
     end
     related_history_object_id = nil
-    if data[:related_history_object]
+    if data[:related_history_object].present?
       related_history_object = object_lookup(data[:related_history_object])
       related_history_object_id = related_history_object.id
     end
     history_attribute_id = nil
-    if data[:history_attribute]
+    if data[:history_attribute].present?
       history_attribute = attribute_lookup(data[:history_attribute])
       history_attribute_id = history_attribute.id
     end
@@ -80,11 +80,11 @@ add a new history entry for an object
     if history_record
       history_record.update!(record)
     else
-      record_new = History.create(record)
+      record_new = History.create!(record)
       if record[:id]
         record_new.id = record[:id]
       end
-      record_new.save
+      record_new.save!
     end
   end
 
@@ -148,7 +148,7 @@ returns
 =end
 
   def self.list(requested_object, requested_object_id, related_history_object = nil, assets = nil)
-    if !related_history_object
+    if related_history_object.blank?
       history_object = object_lookup(requested_object)
       history = History.where(history_object_id: history_object.id)
                        .where(o_id: requested_object_id)
@@ -220,14 +220,10 @@ returns
   def self.type_lookup(name)
     # lookup
     history_type = History::Type.lookup(name: name)
-    if history_type
-      return history_type
-    end
+    return history_type if history_type
 
     # create
-    History::Type.create(
-      name: name
-    )
+    History::Type.create!(name: name)
   end
 
   def self.object_lookup_id(id)
@@ -237,14 +233,10 @@ returns
   def self.object_lookup(name)
     # lookup
     history_object = History::Object.lookup(name: name)
-    if history_object
-      return history_object
-    end
+    return history_object if history_object
 
     # create
-    History::Object.create(
-      name: name
-    )
+    History::Object.create!(name: name)
   end
 
   def self.attribute_lookup_id(id)
@@ -254,14 +246,10 @@ returns
   def self.attribute_lookup(name)
     # lookup
     history_attribute = History::Attribute.lookup(name: name)
-    if history_attribute
-      return history_attribute
-    end
+    return history_attribute if history_attribute
 
     # create
-    History::Attribute.create(
-      name: name
-    )
+    History::Attribute.create!(name: name)
   end
 
   class Object < ApplicationModel

app/models/ticket.rb

@@ -53,6 +53,8 @@ class Ticket < ApplicationModel
                              :article_count,
                              :preferences
 
+  history_relation_object 'Ticket::Article'
+
   belongs_to    :group
   belongs_to    :organization
   has_many      :articles,               class_name: 'Ticket::Article', after_add: :cache_update, after_remove: :cache_update, dependent: :destroy, inverse_of: :ticket
@@ -878,7 +880,7 @@ perform changes on ticket
         next if value['value'].blank?
         next if value['value'] != 'delete'
 
-        logger.debug { "Deleted ticket from #{perform_origin} #{perform.inspect} Ticket.find(#{id})" }
+        logger.info { "Deleted ticket from #{perform_origin} #{perform.inspect} Ticket.find(#{id})" }
         destroy!
         next
       end
@@ -1158,27 +1160,6 @@ result
     Ticket::Article.where(ticket_id: id).order(:created_at, :id)
   end
 
-  def history_get(fulldata = false)
-    list = History.list(self.class.name, self['id'], 'Ticket::Article')
-    return list if !fulldata
-
-    # get related objects
-    assets = {}
-    list.each do |item|
-      record = Kernel.const_get(item['object']).find(item['o_id'])
-      assets = record.assets(assets)
-
-      if item['related_object']
-        record = Kernel.const_get(item['related_object']).find( item['related_o_id'])
-        assets = record.assets(assets)
-      end
-    end
-    {
-      history: list,
-      assets:  assets,
-    }
-  end
-
   private
 
   def check_generate

lib/notification_factory/mailer.rb

@@ -210,7 +210,7 @@ retunes
 =end
 
   def self.already_sent?(ticket, recipient, type)
-    result = ticket.history_get()
+    result = ticket.history_get
     count  = 0
     result.each do |item|
       next if item['type'] != 'notification'

spec/requests/organization_spec.rb

@@ -452,6 +452,22 @@ RSpec.describe 'Organization', type: :request, searchindex: true do
 
     end
 
+    it 'does organization history' do
+      organization1 = create(
+        :organization,
+        name: 'some org',
+      )
+
+      authenticated_as(agent_user)
+      get "/api/v1/organizations/history/#{organization1.id}", params: {}, as: :json
+      expect(response).to have_http_status(200)
+      expect(json_response).to be_a_kind_of(Hash)
+      expect(json_response['history'].class).to eq(Array)
+      expect(json_response['assets'].class).to eq(Hash)
+      expect(json_response['assets']['Ticket']).to be_nil
+      expect(json_response['assets']['Organization'][organization1.id.to_s]).not_to be_nil
+    end
+
     it 'does csv example - customer no access' do
       authenticated_as(customer_user)
       get '/api/v1/organizations/import_example', params: {}, as: :json

spec/requests/ticket_spec.rb

@@ -2078,6 +2078,30 @@ RSpec.describe 'Ticket', type: :request do
       expect(json_response).to be_a_kind_of(Hash)
       expect(json_response['tickets']).to eq([ticket2.id, ticket1.id])
     end
+
+    it 'does ticket history ' do
+      ticket1 = create(
+        :ticket,
+        title:       'some title',
+        group:       ticket_group,
+        customer_id: customer_user.id,
+      )
+      create(
+        :ticket_article,
+        type:      Ticket::Article::Type.lookup(name: 'note'),
+        sender:    Ticket::Article::Sender.lookup(name: 'Customer'),
+        ticket_id: ticket1.id,
+      )
+
+      authenticated_as(agent_user)
+      get "/api/v1/ticket_history/#{ticket1.id}", params: {}, as: :json
+      expect(response).to have_http_status(200)
+      expect(json_response).to be_a_kind_of(Hash)
+      expect(json_response['history'].class).to eq(Array)
+      expect(json_response['assets'].class).to eq(Hash)
+      expect(json_response['assets']['User'][customer_user.id.to_s]).not_to be_nil
+      expect(json_response['assets']['Ticket'][ticket1.id.to_s]).not_to be_nil
+    end
   end
 
   describe 'stats' do

spec/requests/user_spec.rb

@@ -919,6 +919,25 @@ RSpec.describe 'User', type: :request, searchindex: true do
       user2.destroy!
     end
 
+    it 'does user history' do
+      user1 = create(
+        :customer_user,
+        login:     'history@example.com',
+        firstname: 'History',
+        lastname:  'Customer1',
+        email:     'history@example.com',
+      )
+
+      authenticated_as(agent_user)
+      get "/api/v1/users/history/#{user1.id}", params: {}, as: :json
+      expect(response).to have_http_status(200)
+      expect(json_response).to be_a_kind_of(Hash)
+      expect(json_response['history'].class).to eq(Array)
+      expect(json_response['assets'].class).to eq(Hash)
+      expect(json_response['assets']['Ticket']).to be_nil
+      expect(json_response['assets']['User'][user1.id.to_s]).not_to be_nil
+    end
+
     it 'does user search sortable' do
       firstname = "user_search_sortable #{rand(999_999_999)}"
 

test/unit/history_test.rb

@@ -1,9 +1,9 @@
 require 'test_helper'
 
 class HistoryTest < ActiveSupport::TestCase
-  current_user = User.lookup(email: 'nicole.braun@zammad.org')
 
   test 'ticket' do
+    current_user = User.lookup(email: 'nicole.braun@zammad.org')
     tests = [
 
       # test 1
@@ -188,6 +188,7 @@ class HistoryTest < ActiveSupport::TestCase
   end
 
   test 'user' do
+    current_user = User.lookup(email: 'nicole.braun@zammad.org')
     name = rand(999_999)
     tests = [
 
@@ -275,6 +276,7 @@ class HistoryTest < ActiveSupport::TestCase
   end
 
   test 'organization' do
+    current_user = User.lookup(email: 'nicole.braun@zammad.org')
     tests = [
 
       # test 1
@@ -337,6 +339,57 @@ class HistoryTest < ActiveSupport::TestCase
     organizations.each(&:destroy!)
   end
 
+  test 'ticket assets' do
+    UserInfo.current_user_id = 1
+    agent1 = User.create!(
+      login:     'agent1@example.com',
+      firstname: 'agent',
+      lastname:  '1',
+      email:     'agent1@example.com',
+      password:  'agentpw',
+      active:    true,
+      roles:     Role.where(name: %w[Agent Admin]),
+      groups:    Group.all,
+    )
+    current_user = User.lookup(email: 'nicole.braun@zammad.org')
+    UserInfo.current_user_id = current_user.id
+
+    ticket = Ticket.create!(
+      title:       'test 1',
+      group:       Group.first,
+      customer_id: current_user.id,
+      state:       Ticket::State.lookup(name: 'new'),
+      priority:    Ticket::Priority.lookup(name: '2 normal'),
+    )
+    article = Ticket::Article.create!(
+      ticket_id:  ticket.id,
+      from:       'some_customer_com-1@example.com',
+      to:         'some_zammad_com-1@example.com',
+      subject:    'com test 1',
+      message_id: 'some@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'),
+    )
+
+    # verify if user of history record is in assets
+    UserInfo.current_user_id = agent1.id
+    ticket.state = Ticket::State.find_by(name: 'closed')
+    ticket.save!
+
+    # update updated_by (to not include agent1 in assets by ticket)
+    UserInfo.current_user_id = current_user.id
+    ticket.priority = Ticket::Priority.find_by(name: '3 high')
+    ticket.save!
+
+    history = ticket.history_get(true)
+    assert(history[:assets][:User][current_user.id])
+    assert(history[:assets][:User][agent1.id])
+    assert(history[:assets][:Ticket][ticket.id])
+    assert(history[:assets][:TicketArticle][article.id])
+  end
+
   def history_check(history_list, history_check)
     history_check.each do |check_item|
       match = false

test/unit/notification_factory_mailer_template_test.rb

@@ -8,7 +8,7 @@ class NotificationFactoryMailerTemplateTest < ActiveSupport::TestCase
 
     groups = Group.where(name: 'Users')
     roles  = Role.where(name: 'Agent')
-    agent1 = User.create_or_update(
+    agent1 = User.create!(
       login:         'notification-template-agent1@example.com',
       firstname:     'Notification<b>xxx</b>',
       lastname:      'Agent1<b>yyy</b>',
@@ -24,7 +24,7 @@ class NotificationFactoryMailerTemplateTest < ActiveSupport::TestCase
       created_by_id: 1,
     )
 
-    agent_current_user = User.create_or_update(
+    agent_current_user = User.create!(
       login:         'notification-template-current_user@example.com',
       firstname:     'Notification Current',
       lastname:      'User',

test/unit/notification_factory_mailer_test.rb

@@ -83,7 +83,7 @@ class NotificationFactoryMailerTest < ActiveSupport::TestCase
 
     groups = Group.all
     roles  = Role.where(name: 'Agent')
-    agent1 = User.create_or_update(
+    agent1 = User.create!(
       login:         'notification-settings-agent1@example.com',
       firstname:     'Notification<b>xxx</b>',
       lastname:      'Agent1',
@@ -96,7 +96,7 @@ class NotificationFactoryMailerTest < ActiveSupport::TestCase
       created_by_id: 1,
     )
 
-    agent2 = User.create_or_update(
+    agent2 = User.create!(
       login:         'notification-settings-agent2@example.com',
       firstname:     'Notification<b>xxx</b>',
       lastname:      'Agent2',
@@ -109,7 +109,7 @@ class NotificationFactoryMailerTest < ActiveSupport::TestCase
       created_by_id: 1,
     )
 
-    group_notification_setting = Group.create_or_update(
+    group_notification_setting = Group.create!(
       name:          'NotificationSetting',
       updated_by_id: 1,
       created_by_id: 1,