Fixed issue #1649 - Tickets are deleted but database is still the same size.

app/models/store.rb

@@ -47,7 +47,7 @@ returns
     data.delete('object')
 
     # store meta data
-    store = Store.create(data)
+    store = Store.create!(data)
 
     store
   end
@@ -123,17 +123,18 @@ remove one attachment from storage
 =end
 
   def self.remove_item(store_id)
-
     store   = Store.find(store_id)
     file_id = store.store_file_id
-    store.destroy
 
     # check backend for references
     files = Store.where(store_file_id: file_id)
-    return if files.count != 1
+    if files.count > 1 || files.first.id != store.id
-    return if files.first.id != store.id
+      store.destroy!
+      return true
+    end
 
-    Store::File.find(file_id).destroy
+    store.destroy!
+    Store::File.find(file_id).destroy!
   end
 
 =begin

app/models/ticket/article.rb

@@ -18,6 +18,7 @@ class Ticket::Article < ApplicationModel
   store         :preferences
   before_create :check_subject, :check_body, :check_message_id_md5
   before_update :check_subject, :check_body, :check_message_id_md5
+  after_destroy :store_delete
 
   sanitized_html :body
 
@@ -316,6 +317,18 @@ returns
     }
   end
 
+  # delete attachments and mails of article
+  def store_delete
+    Store.remove(
+      object: 'Ticket::Article',
+      o_id:   id,
+    )
+    Store.remove(
+      object: 'Ticket::Article::Mail',
+      o_id:   id,
+    )
+  end
+
   class Flag < ApplicationModel
   end
 

test/unit/store_test.rb

@@ -29,7 +29,7 @@ class StoreTest < ActiveSupport::TestCase
     assert(exists)
   end
 
-  test 'store attachment' do
+  test 'store attachment and move it between backends' do
     files = [
       {
         data: 'hello world',
@@ -54,7 +54,7 @@ class StoreTest < ActiveSupport::TestCase
     ]
 
     files.each do |file|
-      sha = Digest::SHA256.hexdigest( file[:data] )
+      sha = Digest::SHA256.hexdigest(file[:data])
 
       # add attachments
       store = Store.add(
@@ -75,23 +75,23 @@ class StoreTest < ActiveSupport::TestCase
       assert attachments
 
       # sha check
-      sha_new = Digest::SHA256.hexdigest( attachments[0].content )
+      sha_new = Digest::SHA256.hexdigest(attachments[0].content)
-      assert_equal( sha, sha_new,  "check file #{file[:filename]}")
+      assert_equal(sha, sha_new,  "check file #{file[:filename]}")
 
       # filename check
-      assert_equal( file[:filename], attachments[0].filename )
+      assert_equal(file[:filename], attachments[0].filename)
 
       # provider check
-      assert_equal( 'DB', attachments[0].provider )
+      assert_equal('DB', attachments[0].provider)
     end
 
     success = Store::File.verify
     assert success, 'verify ok'
 
-    Store::File.move( 'DB', 'File' )
+    Store::File.move('DB', 'File')
 
     files.each do |file|
-      sha = Digest::SHA256.hexdigest( file[:data] )
+      sha = Digest::SHA256.hexdigest(file[:data])
 
       # get list of attachments
       attachments = Store.list(
@@ -101,54 +101,55 @@ class StoreTest < ActiveSupport::TestCase
       assert attachments
 
       # sha check
-      sha_new = Digest::SHA256.hexdigest( attachments[0].content )
+      sha_new = Digest::SHA256.hexdigest(attachments[0].content)
-      assert_equal( sha, sha_new,  "check file #{file[:filename]}")
+      assert_equal(sha, sha_new,  "check file #{file[:filename]}")
 
       # filename check
-      assert_equal( file[:filename], attachments[0].filename )
+      assert_equal(file[:filename], attachments[0].filename)
 
       # provider check
-      assert_equal( 'File', attachments[0].provider )
+      assert_equal('File', attachments[0].provider)
     end
 
     success = Store::File.verify
     assert success, 'verify ok'
 
-    Store::File.move( 'File', 'DB' )
+    Store::File.move('File', 'DB')
 
     files.each do |file|
-      sha = Digest::SHA256.hexdigest( file[:data] )
+      sha = Digest::SHA256.hexdigest(file[:data])
 
       # get list of attachments
       attachments = Store.list(
         object: 'Test',
         o_id: file[:o_id],
       )
-      assert attachments
+      assert(attachments)
+      assert_equal(attachments.count, 1)
 
       # sha check
-      sha_new = Digest::SHA256.hexdigest( attachments[0].content )
+      sha_new = Digest::SHA256.hexdigest(attachments[0].content)
-      assert_equal( sha, sha_new,  "check file #{file[:filename]}")
+      assert_equal(sha, sha_new,  "check file #{file[:filename]}")
 
       # filename check
-      assert_equal( file[:filename], attachments[0].filename )
+      assert_equal(file[:filename], attachments[0].filename)
 
       # provider check
-      assert_equal( 'DB', attachments[0].provider )
+      assert_equal('DB', attachments[0].provider)
 
       # delete attachments
       success = Store.remove(
         object: 'Test',
         o_id: file[:o_id],
       )
-      assert success
+      assert(success)
 
       # check attachments again
       attachments = Store.list(
         object: 'Test',
         o_id: file[:o_id],
       )
-      assert !attachments[0]
+      assert_not(attachments[0])
     end
   end
 end

test/unit/ticket_article_store_empty.rb

@@ -0,0 +1,75 @@
+# encoding: utf-8
+require 'test_helper'
+
+class TicketArticleStoreEmpty < ActiveSupport::TestCase
+
+  test 'check if attachments are deleted after ticket is deleted' do
+
+    current_count = Store.count
+    current_file_count = Store::File.count
+    current_backend_count = Store::Provider::DB.count
+
+    email_raw_string = IO.binread('test/fixtures/mail1.box')
+    ticket, article, user, mail = Channel::EmailParser.new.process({}, email_raw_string)
+
+    next_count = Store.count
+    next_file_count = Store::File.count
+    next_backend_count = Store::Provider::DB.count
+
+    assert_equal(current_count, next_count - 2)
+    assert_equal(current_file_count, next_file_count - 2)
+    assert_equal(current_backend_count, next_backend_count - 2)
+
+    ticket.destroy!
+
+    after_count = Store.count
+    after_file_count = Store::File.count
+    after_backend_count = Store::Provider::DB.count
+
+    assert_equal(current_count, after_count)
+    assert_equal(current_file_count, after_file_count)
+    assert_equal(current_backend_count, after_backend_count)
+
+  end
+
+  test 'check if attachments are deleted after ticket same ticket 2 times is deleted' do
+
+    current_count = Store.count
+    current_file_count = Store::File.count
+    current_backend_count = Store::Provider::DB.count
+
+    email_raw_string = IO.binread('test/fixtures/mail1.box')
+    ticket1, article1, user1, mail1 = Channel::EmailParser.new.process({}, email_raw_string)
+    ticket2, article2, user2, mail2 = Channel::EmailParser.new.process({}, email_raw_string)
+
+    next_count = Store.count
+    next_file_count = Store::File.count
+    next_backend_count = Store::Provider::DB.count
+
+    assert_equal(current_count, next_count - 4)
+    assert_equal(current_file_count, next_file_count - 2)
+    assert_equal(current_backend_count, next_backend_count - 2)
+
+    ticket1.destroy!
+
+    next_count = Store.count
+    next_file_count = Store::File.count
+    next_backend_count = Store::Provider::DB.count
+
+    assert_equal(current_count, next_count - 2)
+    assert_equal(current_file_count, next_file_count - 2)
+    assert_equal(current_backend_count, next_backend_count - 2)
+
+    ticket2.destroy!
+
+    after_count = Store.count
+    after_file_count = Store::File.count
+    after_backend_count = Store::Provider::DB.count
+
+    assert_equal(current_count, after_count)
+    assert_equal(current_file_count, after_file_count)
+    assert_equal(current_backend_count, after_backend_count)
+
+  end
+
+end