Fix deleting invoices upon dataminimization (#3468)

* Fix deleting invoices upon dataminimization

* Remove outdated test

website/moneybirdsynchronization/signals.py

@@ -158,20 +158,23 @@ def mark_invoice_outdated(sender, instance, **kwargs):
 
 
 @suspendingreceiver(post_delete, sender="registrations.Renewal")
-@suspendingreceiver(
-    post_delete,
-    sender="registrations.Registration",
-)
-@suspendingreceiver(
-    post_delete,
-    sender="pizzas.FoodOrder",
-)
-@suspendingreceiver(
-    post_delete,
-    sender="events.EventRegistration",
-)
+@suspendingreceiver(post_delete, sender="registrations.Registration")
+@suspendingreceiver(post_delete, sender="pizzas.FoodOrder")
+@suspendingreceiver(post_delete, sender="events.EventRegistration")
+@suspendingreceiver(post_delete, sender="sales.Order")
 def post_renewal_delete(sender, instance, **kwargs):
-    """Mark the invoice for deletion if it exists, so that it will be deleted later."""
+    """When a payable is deleted, other than during data minimisation, delete the invoice.
+
+    When objects are deleted for data minimisation, we don't want to delete the
+    Moneybird invoice as well, because we are obligated to store those for 7 years.
+    """
+    # During data minimisation, deletions are marked with a flag. This is currently
+    # the case only for registrations and renewals. The other payables are not deleted
+    # for data minimisation, but bulk-updated to remove personally identifiable
+    # information. Those bulk updates do not trigger post_save signals.
+    if getattr(instance, "__deleting_for_dataminimisation", False):
+        return
+
     invoice = MoneybirdExternalInvoice.get_for_object(instance)
     if invoice:
         invoice.needs_deletion = True

website/moneybirdsynchronization/tests/test_signals.py

@@ -0,0 +1,76 @@
+from unittest import mock
+
+from django.test import TestCase, override_settings
+
+from freezegun import freeze_time
+
+from members.models import Member
+from moneybirdsynchronization.models import MoneybirdExternalInvoice
+from payments.models import Payment
+from payments.services import create_payment
+from registrations.models import Renewal
+from registrations.services import execute_data_minimisation
+
+
+# Each test method has a mock_api argument that is a MagicMock instance, replacing the
+# MoneybirdAPIService *class*. To check calls or set behaviour of a MoneybirdAPIService
+# *instance*, use `mock_api.return_value.<MoneybirdAPIService method>`.
+@mock.patch("moneybirdsynchronization.moneybird.MoneybirdAPIService", autospec=True)
+@override_settings(  # Settings needed to enable synchronization.
+    MONEYBIRD_START_DATE="2023-09-01",
+    MONEYBIRD_ADMINISTRATION_ID="123",
+    MONEYBIRD_API_KEY="foo",
+    MONEYBIRD_SYNC_ENABLED=True,
+    SUSPEND_SIGNALS=True,
+)
+class SignalsTest(TestCase):
+    fixtures = ["members.json"]
+
+    @classmethod
+    def setUpTestData(cls):
+        cls.member = Member.objects.get(pk=1)
+        cls.member2 = Member.objects.get(pk=2)
+        cls.member3 = Member.objects.get(pk=3)
+        cls.member4 = Member.objects.get(pk=4)
+
+    def test_dataminimisation_does_not_trigger_invoice_deletion(self, mock_api):
+        with freeze_time("2023-09-01"):
+            renewal = Renewal.objects.create(
+                member=self.member,
+                length=Renewal.MEMBERSHIP_YEAR,
+            )
+
+            create_payment(renewal, self.member, Payment.CASH)
+            renewal.refresh_from_db()
+            renewal.status = Renewal.STATUS_COMPLETED
+            renewal.save()
+
+            invoice1 = MoneybirdExternalInvoice.objects.create(
+                payable_object=renewal,
+                needs_synchronization=False,
+                moneybird_invoice_id="1",
+            )
+
+        with freeze_time("2023-11-01"):
+            with override_settings(SUSPEND_SIGNALS=False):
+                count_deleted = execute_data_minimisation()
+
+                self.assertGreaterEqual(count_deleted, 1)
+
+                # The invoice should not be scheduled for deletion.
+                invoice1.refresh_from_db()
+                self.assertFalse(invoice1.needs_deletion)
+                self.assertFalse(invoice1.needs_synchronization)
+
+            # Recreate the removed renewal.
+            with freeze_time("2023-09-01"):
+                renewal.status = Renewal.STATUS_COMPLETED
+                renewal.save()
+
+            with override_settings(SUSPEND_SIGNALS=False):
+                # But (bulk)-deleting outside of data minimisation should still delete it.
+                Renewal.objects.all().delete()
+
+                invoice1.refresh_from_db()
+                self.assertTrue(invoice1.needs_deletion)
+                self.assertFalse(invoice1.needs_synchronization)

website/registrations/services.py

@@ -7,7 +7,7 @@
 from django.contrib.admin.options import get_content_type_for_model
 from django.contrib.auth import get_user_model
 from django.db import transaction
-from django.db.models import Q, QuerySet
+from django.db.models import Q, QuerySet, Value
 from django.utils import timezone
 
 import members
@@ -412,14 +412,25 @@ def execute_data_minimisation(dry_run=False):
     """Delete completed or rejected registrations that were modified at least 31 days ago.
 
     :param dry_run: does not really remove data if True
-    :return: number of removed registrations
+    :return: number of removed objects.
     """
     deletion_period = timezone.now() - timezone.timedelta(days=31)
-    objects = Entry.objects.filter(
-        (Q(status=Entry.STATUS_COMPLETED) | Q(status=Entry.STATUS_REJECTED))
-        & Q(updated_at__lt=deletion_period)
+    registrations = Registration.objects.filter(
+        Q(status=Entry.STATUS_COMPLETED) | Q(status=Entry.STATUS_REJECTED),
+        updated_at__lt=deletion_period,
+    )
+    renewals = Renewal.objects.filter(
+        Q(status=Entry.STATUS_COMPLETED) | Q(status=Entry.STATUS_REJECTED),
+        updated_at__lt=deletion_period,
     )
 
     if dry_run:
-        return objects.count()
-    return objects.delete()[0]
+        return registrations.count() + renewals.count()  # pragma: no cover
+
+    # Mark that this deletion is for data minimisation so that it can be recognized
+    # in any post_delete signal handlers. This is used to prevent the deletion of
+    # Moneybird invoices.
+    registrations = registrations.annotate(__deleting_for_dataminimisation=Value(True))
+    renewals = renewals.annotate(__deleting_for_dataminimisation=Value(True))
+
+    return registrations.delete()[0] + renewals.delete()[0]

website/registrations/tests/test_services.py

@@ -507,41 +507,6 @@ def test_process_entry_save(self, send_renewal_email):
             self.assertIsNotNone(self.e3.membership)
             self.assertEqual(self.e3.status, Entry.STATUS_COMPLETED)
 
-    @freeze_time("2019-01-01")
-    def test_execute_data_minimisation(self):
-        with self.subTest("No processed entries"):
-            self.assertEqual(services.execute_data_minimisation(), 0)
-
-        with freeze_time("2018-09-01"):
-            self.e0.status = Entry.STATUS_COMPLETED
-            self.e0.save()
-
-        with self.subTest("Has processed entries when completed"):
-            self.assertEqual(services.execute_data_minimisation(), 1)
-
-        with freeze_time("2018-09-01"):
-            self.e0.status = Entry.STATUS_REJECTED
-            self.e0.save()
-
-        with self.subTest("Has processed entries when rejected"):
-            self.assertEqual(services.execute_data_minimisation(), 1)
-
-        with freeze_time("2018-09-01"):
-            self.e0.status = Entry.STATUS_COMPLETED
-            self.e0.save()
-
-        with self.subTest("Has processed entries when rejected with dry-run"):
-            self.assertEqual(services.execute_data_minimisation(True), 1)
-
-        self.e0.status = Entry.STATUS_COMPLETED
-        self.e0.save()
-
-        with self.subTest("No processed entries when inside 31 days"):
-            self.assertEqual(services.execute_data_minimisation(), 0)
-
-        self.e0.status = Entry.STATUS_REVIEW
-        self.e0.save()
-
     @freeze_time("2019-01-01")
     def test_accept_tpay_registration(self):
         self.e2.created_at = timezone.now()