Fix doc cleanup signals, testing

src-ui/src/app/components/admin/trash/trash.component.spec.ts

@@ -1,22 +1,157 @@
 import { ComponentFixture, TestBed } from '@angular/core/testing'
 
 import { TrashComponent } from './trash.component'
+import { HttpClientTestingModule } from '@angular/common/http/testing'
+import { PageHeaderComponent } from '../../common/page-header/page-header.component'
+import {
+  NgbModal,
+  NgbPaginationModule,
+  NgbPopoverModule,
+} from '@ng-bootstrap/ng-bootstrap'
+import { NgxBootstrapIconsModule, allIcons } from 'ngx-bootstrap-icons'
+import { FormsModule, ReactiveFormsModule } from '@angular/forms'
+import { TrashService } from 'src/app/services/trash.service'
+import { of } from 'rxjs'
+import { ConfirmDialogComponent } from '../../common/confirm-dialog/confirm-dialog.component'
+import { By } from '@angular/platform-browser'
+
+const documentsInTrash = [
+  {
+    id: 1,
+    name: 'test1',
+    created: new Date('2023-03-01T10:26:03.093116Z'),
+    deleted_at: new Date('2023-03-01T10:26:03.093116Z'),
+  },
+  {
+    id: 2,
+    name: 'test2',
+    created: new Date('2023-03-01T10:26:03.093116Z'),
+    deleted_at: new Date('2023-03-01T10:26:03.093116Z'),
+  },
+]
 
 describe('TrashComponent', () => {
   let component: TrashComponent
   let fixture: ComponentFixture<TrashComponent>
+  let trashService: TrashService
+  let modalService: NgbModal
 
   beforeEach(async () => {
     await TestBed.configureTestingModule({
-      declarations: [TrashComponent],
+      declarations: [
+        TrashComponent,
+        PageHeaderComponent,
+        ConfirmDialogComponent,
+      ],
+      imports: [
+        HttpClientTestingModule,
+        FormsModule,
+        ReactiveFormsModule,
+        NgbPopoverModule,
+        NgbPaginationModule,
+        NgxBootstrapIconsModule.pick(allIcons),
+      ],
     }).compileComponents()
 
     fixture = TestBed.createComponent(TrashComponent)
+    trashService = TestBed.inject(TrashService)
+    modalService = TestBed.inject(NgbModal)
     component = fixture.componentInstance
     fixture.detectChanges()
   })
 
-  it('should create', () => {
+  it('should call correct service method on reload', () => {
-    expect(component).toBeTruthy()
+    const trashSpy = jest.spyOn(trashService, 'getTrash')
+    trashSpy.mockReturnValue(of(documentsInTrash))
+    component.reload()
+    expect(trashSpy).toHaveBeenCalled()
+    expect(component.documentsInTrash).toEqual(documentsInTrash)
+  })
+
+  it('should support delete document', () => {
+    const trashSpy = jest.spyOn(trashService, 'emptyTrash')
+    let modal
+    modalService.activeInstances.subscribe((instances) => {
+      modal = instances[0]
+    })
+    trashSpy.mockReturnValue(of('OK'))
+    component.delete(documentsInTrash[0])
+    expect(modal).toBeDefined()
+    modal.componentInstance.confirmClicked.next()
+    expect(trashSpy).toHaveBeenCalled()
+  })
+
+  it('should support empty trash', () => {
+    const trashSpy = jest.spyOn(trashService, 'emptyTrash')
+    let modal
+    modalService.activeInstances.subscribe((instances) => {
+      modal = instances[instances.length - 1]
+    })
+    trashSpy.mockReturnValue(of('OK'))
+    component.emptyTrash()
+    expect(modal).toBeDefined()
+    modal.componentInstance.confirmClicked.next()
+    expect(trashSpy).toHaveBeenCalled()
+    modal.close()
+    component.emptyTrash(new Set([1, 2]))
+    modal.componentInstance.confirmClicked.next()
+    expect(trashSpy).toHaveBeenCalledWith([1, 2])
+  })
+
+  it('should support restore document', () => {
+    const restoreSpy = jest.spyOn(trashService, 'restoreDocuments')
+    const reloadSpy = jest.spyOn(component, 'reload')
+    restoreSpy.mockReturnValue(of('OK'))
+    component.restore(documentsInTrash[0])
+    expect(restoreSpy).toHaveBeenCalledWith([documentsInTrash[0].id])
+    expect(reloadSpy).toHaveBeenCalled()
+  })
+
+  it('should support restore all documents', () => {
+    const restoreSpy = jest.spyOn(trashService, 'restoreDocuments')
+    const reloadSpy = jest.spyOn(component, 'reload')
+    restoreSpy.mockReturnValue(of('OK'))
+    component.restoreAll()
+    expect(restoreSpy).toHaveBeenCalled()
+    expect(reloadSpy).toHaveBeenCalled()
+    component.restoreAll(new Set([1, 2]))
+    expect(restoreSpy).toHaveBeenCalledWith([1, 2])
+  })
+
+  it('should support toggle all items in view', () => {
+    component.documentsInTrash = documentsInTrash
+    expect(component.selectedDocuments.size).toEqual(0)
+    const toggleAllSpy = jest.spyOn(component, 'toggleAll')
+    const checkButton = fixture.debugElement.queryAll(
+      By.css('input.form-check-input')
+    )[0]
+    checkButton.nativeElement.dispatchEvent(new Event('click'))
+    checkButton.nativeElement.checked = true
+    checkButton.nativeElement.dispatchEvent(new Event('click'))
+    expect(toggleAllSpy).toHaveBeenCalled()
+    expect(component.selectedDocuments.size).toEqual(documentsInTrash.length)
+  })
+
+  it('should support toggle item', () => {
+    component.selectedDocuments = new Set([1])
+    component.toggleSelected(documentsInTrash[0])
+    expect(component.selectedDocuments.size).toEqual(0)
+    component.toggleSelected(documentsInTrash[0])
+    expect(component.selectedDocuments.size).toEqual(1)
+  })
+
+  it('should support clear selection', () => {
+    component.selectedDocuments = new Set([1])
+    component.clearSelection()
+    expect(component.selectedDocuments.size).toEqual(0)
+  })
+
+  it('should correctly display days remaining', () => {
+    expect(component.getDaysRemaining(documentsInTrash[0])).toBeLessThan(0)
+    const tenDaysAgo = new Date()
+    tenDaysAgo.setDate(tenDaysAgo.getDate() - 10)
+    expect(
+      component.getDaysRemaining({ deleted_at: tenDaysAgo })
+    ).toBeGreaterThan(0) // 10 days ago but depends on month
   })
 })

src-ui/src/app/components/admin/trash/trash.component.ts

@@ -79,7 +79,7 @@ export class TrashComponent implements OnDestroy {
       .pipe(takeUntil(this.unsubscribeNotifier))
       .subscribe(() => {
         this.trashService
-          .emptyTrash(documents ? Array.from(documents) : [])
+          .emptyTrash(documents ? Array.from(documents) : null)
           .subscribe(() => {
             this.toastService.showInfo($localize`Document(s) deleted`)
             this.allToggled = false
@@ -95,9 +95,9 @@ export class TrashComponent implements OnDestroy {
     })
   }
 
-  restoreAll(objects: Set<number> = null) {
+  restoreAll(documents: Set<number> = null) {
     this.trashService
-      .restoreDocuments(objects ? Array.from(this.selectedDocuments) : [])
+      .restoreDocuments(documents ? Array.from(documents) : null)
       .subscribe(() => {
         this.toastService.showInfo($localize`Document(s) restored`)
         this.allToggled = false

src-ui/src/app/services/trash.service.spec.ts

@@ -1,16 +1,50 @@
 import { TestBed } from '@angular/core/testing'
 
 import { TrashService } from './trash.service'
+import {
+  HttpClientTestingModule,
+  HttpTestingController,
+} from '@angular/common/http/testing'
+import { environment } from 'src/environments/environment'
 
 describe('TrashServiceService', () => {
   let service: TrashService
+  let httpTestingController: HttpTestingController
 
   beforeEach(() => {
-    TestBed.configureTestingModule({})
+    TestBed.configureTestingModule({
+      imports: [HttpClientTestingModule],
+    })
     service = TestBed.inject(TrashService)
+    httpTestingController = TestBed.inject(HttpTestingController)
   })
 
-  it('should be created', () => {
+  it('should call correct endpoint for getTrash', () => {
-    expect(service).toBeTruthy()
+    service.getTrash().subscribe()
+    const req = httpTestingController.expectOne(
+      `${environment.apiBaseUrl}trash/`
+    )
+    expect(req.request.method).toEqual('GET')
+  })
+
+  it('should call correct endpoint for emptyTrash', () => {
+    service.emptyTrash().subscribe()
+    const req = httpTestingController.expectOne(
+      `${environment.apiBaseUrl}trash/`
+    )
+    expect(req.request.method).toEqual('POST')
+    expect(req.request.body).toEqual({ action: 'empty', documents: [] })
+  })
+
+  it('should call correct endpoint for restoreDocuments', () => {
+    service.restoreDocuments([1, 2, 3]).subscribe()
+    const req = httpTestingController.expectOne(
+      `${environment.apiBaseUrl}trash/`
+    )
+    expect(req.request.method).toEqual('POST')
+    expect(req.request.body).toEqual({
+      action: 'restore',
+      documents: [1, 2, 3],
+    })
   })
 })

src/documents/serialisers.py

@@ -1868,7 +1868,7 @@ class WorkflowSerializer(serializers.ModelSerializer):
 
 class TrashSerializer(SerializerWithPerms):
     documents = serializers.ListField(
-        required=True,
+        required=False,
         label="Documents",
         write_only=True,
         child=serializers.IntegerField(),

src/documents/signals/handlers.py

@@ -1,7 +1,6 @@
 import logging
 import os
 import shutil
-from datetime import timedelta
 from typing import Optional
 
 from celery import states
@@ -302,12 +301,8 @@ def set_storage_path(
             document.save(update_fields=("storage_path",))
 
 
-@receiver(models.signals.post_delete, sender=Document)
+# see empty_trash in documents/tasks.py for signal handling
-def cleanup_document_deletion(sender, instance, force=False, **kwargs):
+def cleanup_document_deletion(sender, instance, **kwargs):
-    if not force:
-        now = timezone.localtime(timezone.now())
-        if now - instance.deleted_at < timedelta(days=settings.EMPTY_TRASH_DELAY):
-            return
     with FileLock(settings.MEDIA_LOCK):
         if settings.TRASH_DIR:
             # Find a non-conflicting filename in case a document with the same

src/documents/tasks.py

@@ -11,8 +11,8 @@ import tqdm
 from celery import Task
 from celery import shared_task
 from django.conf import settings
+from django.db import models
 from django.db import transaction
-from django.db.models.signals import post_delete
 from django.db.models.signals import post_save
 from django.utils import timezone
 from filelock import FileLock
@@ -44,6 +44,7 @@ from documents.plugins.base import StopConsumeTaskError
 from documents.plugins.helpers import ProgressStatusOptions
 from documents.sanity_checker import SanityCheckFailedException
 from documents.signals import document_updated
+from documents.signals.handlers import cleanup_document_deletion
 
 if settings.AUDIT_LOG_ENABLED:
     import json
@@ -307,10 +308,11 @@ def empty_trash(doc_ids=None):
         if doc_ids is not None
         else Document.deleted_objects.filter(deleted_at__gt=cutoff)
     )
+
+    # Temporarily connect the cleanup handler (hard_delete calls delete)
+    models.signals.post_delete.connect(cleanup_document_deletion, sender=Document)
+
     for doc in documents:
-        doc.delete()
+        doc.hard_delete()
-        post_delete.send(
+
-            sender=Document,
+    models.signals.post_delete.disconnect(cleanup_document_deletion, sender=Document)
-            instance=doc,
-            force=True,
-        )

src/documents/tests/test_api_uisettings.py

@@ -40,6 +40,7 @@ class TestApiUiSettings(DirectoriesMixin, APITestCase):
                 "app_title": None,
                 "app_logo": None,
                 "auditlog_enabled": True,
+                "trash_delay": 30,
                 "update_checking": {
                     "backend_setting": "default",
                 },

src/documents/tests/test_document_model.py

@@ -10,6 +10,7 @@ from django.utils import timezone
 
 from documents.models import Correspondent
 from documents.models import Document
+from documents.tasks import empty_trash
 
 
 class TestDocument(TestCase):
@@ -43,10 +44,39 @@ class TestDocument(TestCase):
 
         with mock.patch("documents.signals.handlers.os.unlink") as mock_unlink:
             document.delete()
+            empty_trash([document.pk])
             mock_unlink.assert_any_call(file_path)
             mock_unlink.assert_any_call(thumb_path)
             self.assertEqual(mock_unlink.call_count, 2)
 
+    def test_document_soft_delete(self):
+        document = Document.objects.create(
+            correspondent=Correspondent.objects.create(name="Test0"),
+            title="Title",
+            content="content",
+            checksum="checksum",
+            mime_type="application/pdf",
+        )
+
+        file_path = document.source_path
+        thumb_path = document.thumbnail_path
+
+        Path(file_path).touch()
+        Path(thumb_path).touch()
+
+        with mock.patch("documents.signals.handlers.os.unlink") as mock_unlink:
+            document.delete()
+            self.assertEqual(mock_unlink.call_count, 0)
+
+            self.assertEqual(Document.objects.count(), 0)
+
+            document.restore(strict=False)
+            self.assertEqual(Document.objects.count(), 1)
+
+            document.delete()
+            empty_trash([document.pk])
+            self.assertEqual(mock_unlink.call_count, 2)
+
     def test_file_name(self):
         doc = Document(
             mime_type="application/pdf",

src/documents/tests/test_file_handling.py

@@ -19,6 +19,7 @@ from documents.models import Correspondent
 from documents.models import Document
 from documents.models import DocumentType
 from documents.models import StoragePath
+from documents.tasks import empty_trash
 from documents.tests.utils import DirectoriesMixin
 from documents.tests.utils import FileSystemAssertsMixin
 
@@ -169,6 +170,7 @@ class TestFileHandling(DirectoriesMixin, FileSystemAssertsMixin, TestCase):
 
         # Ensure that filename is properly generated
         document.filename = generate_filename(document)
+        document.save()
         self.assertEqual(document.filename, "none/none.pdf")
 
         create_source_path_directory(document.source_path)
@@ -176,6 +178,7 @@ class TestFileHandling(DirectoriesMixin, FileSystemAssertsMixin, TestCase):
 
         # Ensure file deletion after delete
         document.delete()
+        empty_trash([document.pk])
         self.assertIsNotFile(
             os.path.join(settings.ORIGINALS_DIR, "none", "none.pdf"),
         )
@@ -185,7 +188,7 @@ class TestFileHandling(DirectoriesMixin, FileSystemAssertsMixin, TestCase):
         FILENAME_FORMAT="{correspondent}/{correspondent}",
         TRASH_DIR=tempfile.mkdtemp(),
     )
-    def test_document_delete_trash(self):
+    def test_document_delete_trash_dir(self):
         document = Document()
         document.mime_type = "application/pdf"
         document.storage_type = Document.STORAGE_TYPE_UNENCRYPTED
@@ -193,6 +196,7 @@ class TestFileHandling(DirectoriesMixin, FileSystemAssertsMixin, TestCase):
 
         # Ensure that filename is properly generated
         document.filename = generate_filename(document)
+        document.save()
         self.assertEqual(document.filename, "none/none.pdf")
 
         create_source_path_directory(document.source_path)
@@ -201,6 +205,7 @@ class TestFileHandling(DirectoriesMixin, FileSystemAssertsMixin, TestCase):
         # Ensure file was moved to trash after delete
         self.assertIsNotFile(os.path.join(settings.TRASH_DIR, "none", "none.pdf"))
         document.delete()
+        empty_trash([document.pk])
         self.assertIsNotFile(
             os.path.join(settings.ORIGINALS_DIR, "none", "none.pdf"),
         )
@@ -214,9 +219,11 @@ class TestFileHandling(DirectoriesMixin, FileSystemAssertsMixin, TestCase):
         document.storage_type = Document.STORAGE_TYPE_UNENCRYPTED
         document.save()
         document.filename = generate_filename(document)
+        document.save()
         create_source_path_directory(document.source_path)
         Path(document.source_path).touch()
         document.delete()
+        empty_trash([document.pk])
         self.assertIsFile(os.path.join(settings.TRASH_DIR, "none_01.pdf"))
 
     @override_settings(FILENAME_FORMAT="{correspondent}/{correspondent}")
@@ -227,6 +234,7 @@ class TestFileHandling(DirectoriesMixin, FileSystemAssertsMixin, TestCase):
         document.save()
 
         document.delete()
+        empty_trash([document.pk])
 
     @override_settings(FILENAME_FORMAT="{correspondent}/{correspondent}")
     def test_directory_not_empty(self):
@@ -436,6 +444,7 @@ class TestFileHandling(DirectoriesMixin, FileSystemAssertsMixin, TestCase):
 
         # Ensure that filename is properly generated
         document.filename = generate_filename(document)
+        document.save()
         self.assertEqual(document.filename, "none/none/none.pdf")
         create_source_path_directory(document.source_path)
         Path(document.source_path).touch()
@@ -444,6 +453,7 @@ class TestFileHandling(DirectoriesMixin, FileSystemAssertsMixin, TestCase):
         self.assertIsDir(os.path.join(settings.ORIGINALS_DIR, "none/none"))
 
         document.delete()
+        empty_trash([document.pk])
 
         self.assertIsNotFile(
             os.path.join(settings.ORIGINALS_DIR, "none/none/none.pdf"),
@@ -550,6 +560,7 @@ class TestFileHandling(DirectoriesMixin, FileSystemAssertsMixin, TestCase):
         self.assertEqual(document2.filename, "qwe_01.pdf")
 
         document.delete()
+        empty_trash([document.pk])
 
         self.assertIsNotFile(document.source_path)
 
@@ -819,6 +830,7 @@ class TestFileHandlingWithArchive(DirectoriesMixin, FileSystemAssertsMixin, Test
         self.assertIsFile(doc.archive_path)
 
         doc.delete()
+        empty_trash([doc.pk])
 
         self.assertIsNotFile(original)
         self.assertIsNotFile(archive)
@@ -854,6 +866,7 @@ class TestFileHandlingWithArchive(DirectoriesMixin, FileSystemAssertsMixin, Test
         self.assertIsFile(doc2.source_path)
 
         doc2.delete()
+        empty_trash([doc2.pk])
 
         self.assertIsFile(doc1.source_path)
         self.assertIsFile(doc1.archive_path)

src/paperless/tests/test_settings.py

@@ -156,6 +156,7 @@ class TestCeleryScheduleParsing(TestCase):
     CLASSIFIER_EXPIRE_TIME = 59.0 * 60.0
     INDEX_EXPIRE_TIME = 23.0 * 60.0 * 60.0
     SANITY_EXPIRE_TIME = ((7.0 * 24.0) - 1.0) * 60.0 * 60.0
+    EMPTY_TRASH_EXPIRE_TIME = 23.0 * 60.0 * 60.0
 
     def test_schedule_configuration_default(self):
         """
@@ -190,6 +191,11 @@ class TestCeleryScheduleParsing(TestCase):
                     "schedule": crontab(minute=30, hour=0, day_of_week="sun"),
                     "options": {"expires": self.SANITY_EXPIRE_TIME},
                 },
+                "Empty trash": {
+                    "task": "documents.tasks.empty_trash",
+                    "schedule": crontab(minute=0, hour="1"),
+                    "options": {"expires": self.EMPTY_TRASH_EXPIRE_TIME},
+                },
             },
             schedule,
         )
@@ -232,6 +238,11 @@ class TestCeleryScheduleParsing(TestCase):
                     "schedule": crontab(minute=30, hour=0, day_of_week="sun"),
                     "options": {"expires": self.SANITY_EXPIRE_TIME},
                 },
+                "Empty trash": {
+                    "task": "documents.tasks.empty_trash",
+                    "schedule": crontab(minute=0, hour="1"),
+                    "options": {"expires": self.EMPTY_TRASH_EXPIRE_TIME},
+                },
             },
             schedule,
         )
@@ -266,6 +277,11 @@ class TestCeleryScheduleParsing(TestCase):
                     "schedule": crontab(minute=30, hour=0, day_of_week="sun"),
                     "options": {"expires": self.SANITY_EXPIRE_TIME},
                 },
+                "Empty trash": {
+                    "task": "documents.tasks.empty_trash",
+                    "schedule": crontab(minute=0, hour="1"),
+                    "options": {"expires": self.EMPTY_TRASH_EXPIRE_TIME},
+                },
             },
             schedule,
         )
@@ -286,6 +302,7 @@ class TestCeleryScheduleParsing(TestCase):
                 "PAPERLESS_TRAIN_TASK_CRON": "disable",
                 "PAPERLESS_SANITY_TASK_CRON": "disable",
                 "PAPERLESS_INDEX_TASK_CRON": "disable",
+                "PAPERLESS_EMPTY_TRASH_TASK_CRON": "disable",
             },
         ):
             schedule = _parse_beat_schedule()