diff --git a/website/moneybirdsynchronization/signals.py b/website/moneybirdsynchronization/signals.py index 66f54da5c..dc7c6f637 100644 --- a/website/moneybirdsynchronization/signals.py +++ b/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 diff --git a/website/moneybirdsynchronization/tests/test_signals.py b/website/moneybirdsynchronization/tests/test_signals.py new file mode 100644 index 000000000..f52e3e717 --- /dev/null +++ b/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.`. +@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) diff --git a/website/registrations/services.py b/website/registrations/services.py index 8525657ec..50902ccd3 100644 --- a/website/registrations/services.py +++ b/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] diff --git a/website/registrations/tests/test_services.py b/website/registrations/tests/test_services.py index 3461a7f07..ff6373f2c 100644 --- a/website/registrations/tests/test_services.py +++ b/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()