From 164bf0068b21d1f4d365db4d14dab3743022750d Mon Sep 17 00:00:00 2001
From: Rolf Schmidt <rolf.schmidt@zammad.com>
Date: Fri, 23 Oct 2020 13:53:34 +0200
Subject: [PATCH] Fixes #2620 - Deleting articles with accounted time to not
 affect accounted time.

---
 app/models/ticket/article.rb               |  7 ++++-
 app/models/ticket/time_accounting.rb       | 25 +++++++--------
 spec/models/ticket/time_accounting_spec.rb | 36 ++++++++++++++++++++++
 3 files changed, 53 insertions(+), 15 deletions(-)

diff --git a/app/models/ticket/article.rb b/app/models/ticket/article.rb
index e0b24c27c..898ae18f6 100644
--- a/app/models/ticket/article.rb
+++ b/app/models/ticket/article.rb
@@ -22,7 +22,7 @@ class Ticket::Article < ApplicationModel
   before_save   :touch_ticket_if_needed
   before_create :check_subject, :check_body, :check_message_id_md5
   before_update :check_subject, :check_body, :check_message_id_md5
-  after_destroy :store_delete
+  after_destroy :store_delete, :update_time_units
 
   store :preferences
 
@@ -339,6 +339,11 @@ returns
     )
   end
 
+  # recalculate time accounting
+  def update_time_units
+    Ticket::TimeAccounting.update_ticket(ticket)
+  end
+
   def touch_ticket_if_needed
     return if !internal_changed?
 
diff --git a/app/models/ticket/time_accounting.rb b/app/models/ticket/time_accounting.rb
index 0cbc30cb2..4e53a4ea0 100644
--- a/app/models/ticket/time_accounting.rb
+++ b/app/models/ticket/time_accounting.rb
@@ -4,25 +4,22 @@ class Ticket::TimeAccounting < ApplicationModel
   belongs_to :ticket, optional: true
   belongs_to :ticket_article, class_name: 'Ticket::Article', inverse_of: :ticket_time_accounting, optional: true
 
-  after_create :ticket_time_unit_update
-  after_update :ticket_time_unit_update
+  after_create :update_time_units
+  after_update :update_time_units
 
-  def ticket_time_unit_update
-    exists = false
-    time_units = 0
-    Ticket::TimeAccounting.where(ticket_id: ticket_id).each do |record|
-      time_units += record.time_unit
-      exists = true
-    end
-    return false if exists == false
+  def update_time_units
+    self.class.update_ticket(ticket)
+  end
 
-    ticket = Ticket.lookup(id: ticket_id)
-    return false if !ticket
-    return false if ticket.time_unit == time_units
+  def self.update_ticket(ticket)
+    time_units = total(ticket)
+    return if ticket.time_unit.to_d == time_units
 
     ticket.time_unit = time_units
     ticket.save!
-    true
   end
 
+  def self.total(ticket)
+    ticket.ticket_time_accounting.sum(:time_unit)
+  end
 end
diff --git a/spec/models/ticket/time_accounting_spec.rb b/spec/models/ticket/time_accounting_spec.rb
index ea339387d..4c3e024ec 100644
--- a/spec/models/ticket/time_accounting_spec.rb
+++ b/spec/models/ticket/time_accounting_spec.rb
@@ -22,6 +22,42 @@ RSpec.describe Ticket::TimeAccounting, type: :model do
             .to change(described_class, :count).by(-1)
         end
       end
+
+      context 'when recalculating articles' do
+        let(:ticket)   { create(:ticket) }
+        let(:article1) { create(:ticket_article, ticket: ticket) }
+        let(:article2) { create(:ticket_article, ticket: ticket) }
+
+        it 'one article' do
+          time_accounting = create(:'ticket/time_accounting', ticket: ticket, ticket_article: article1)
+
+          expect(ticket.reload.time_unit).to eq(time_accounting.time_unit)
+        end
+
+        it 'multiple article' do
+          time_accounting1 = create(:'ticket/time_accounting', ticket: ticket, ticket_article: article1, time_unit: 5.5)
+          time_accounting2 = create(:'ticket/time_accounting', ticket: ticket, ticket_article: article2, time_unit: 10.5)
+
+          expect(ticket.reload.time_unit).to eq(time_accounting1.time_unit + time_accounting2.time_unit)
+        end
+
+        it 'destroy article' do
+          time_accounting1 = create(:'ticket/time_accounting', ticket: ticket, ticket_article: article1, time_unit: 5.5)
+          create(:'ticket/time_accounting', ticket: ticket, ticket_article: article2, time_unit: 10.5)
+          article2.destroy
+
+          expect(ticket.reload.time_unit).to eq(time_accounting1.time_unit)
+        end
+
+        it 'destroy all articles' do
+          create(:'ticket/time_accounting', ticket: ticket, ticket_article: article1, time_unit: 5.5)
+          create(:'ticket/time_accounting', ticket: ticket, ticket_article: article2, time_unit: 10.5)
+          article1.destroy
+          article2.destroy
+
+          expect(ticket.reload.time_unit).to eq(0)
+        end
+      end
     end
   end
 end