Merge branch 'fix/race-condition-registration-save' into 'master'

Fix race condition in registration approve/save

Closes #841

See merge request !1196

website/registrations/admin.py

@@ -130,7 +130,7 @@ class RegistrationAdmin(admin.ModelAdmin):
     def reject_selected(self, request, queryset):
         """Reject the selected entries"""
         if request.user.has_perm('registrations.review_entries'):
-            rows_updated = services.reject_entries(queryset)
+            rows_updated = services.reject_entries(request.user.pk, queryset)
             _show_message(
                 self, request, rows_updated,
                 message=_("Successfully rejected %(count)d %(items)s."),
@@ -142,7 +142,7 @@ class RegistrationAdmin(admin.ModelAdmin):
     def accept_selected(self, request, queryset):
         """Accept the selected entries"""
         if request.user.has_perm('registrations.review_entries'):
-            rows_updated = services.accept_entries(queryset)
+            rows_updated = services.accept_entries(request.user.pk, queryset)
             _show_message(
                 self, request, rows_updated,
                 message=_("Successfully accepted %(count)d %(items)s."),
@@ -155,6 +155,12 @@ class RegistrationAdmin(admin.ModelAdmin):
         """Does the user have the review permission?"""
         return request.user.has_perm('registrations.review_entries')
 
+    def save_model(self, request, obj, form, change):
+        if not (obj.status == Entry.STATUS_REJECTED or
+                obj.status == Entry.STATUS_ACCEPTED or
+                obj.status == Entry.STATUS_COMPLETED):
+            super().save_model(request, obj, form, change)
+
 
 @admin.register(Renewal)
 class RenewalAdmin(RegistrationAdmin):
@@ -180,6 +186,7 @@ class RenewalAdmin(RegistrationAdmin):
                             'member',)
                     }),
     )
+    actions = RegistrationAdmin.actions
 
     def get_readonly_fields(self, request, obj=None):
         """Make all fields read-only and add member if needed"""

website/registrations/models.py

@@ -102,12 +102,7 @@ class Entry(models.Model):
         try:
             return self.registration.__str__()
         except Registration.DoesNotExist:
-            pass
-        try:
             return self.renewal.__str__()
-        except Renewal.DoesNotExist:
-            pass
-        return _("Registration entry")
 
     class Meta:
         verbose_name = _('entry')

website/registrations/services.py

@@ -3,6 +3,8 @@ import string
 import unicodedata
 
 from django.conf import settings
+from django.contrib.admin.models import LogEntry, CHANGE
+from django.contrib.admin.options import get_content_type_for_model
 from django.contrib.auth import get_user_model
 from django.db.models import Q
 from django.utils import timezone
@@ -75,7 +77,7 @@ def confirm_entry(queryset):
     return rows_updated
 
 
-def reject_entries(queryset):
+def reject_entries(user_id, queryset):
     """
     Reject all entries in the queryset
 
@@ -90,18 +92,32 @@ def reject_entries(queryset):
                                    updated_at=timezone.now())
 
     for entry in entries:
+        log_obj = None
+
         try:
             emails.send_registration_rejected_message(entry.registration)
+            log_obj = entry.registration
         except Registration.DoesNotExist:
             try:
                 emails.send_renewal_rejected_message(entry.renewal)
+                log_obj = entry.renewal
             except Renewal.DoesNotExist:
                 pass
 
+        if log_obj:
+            LogEntry.objects.log_action(
+                user_id=user_id,
+                content_type_id=get_content_type_for_model(log_obj).pk,
+                object_id=log_obj.pk,
+                object_repr=str(log_obj),
+                action_flag=CHANGE,
+                change_message='Changed status to rejected',
+            )
+
     return rows_updated
 
 
-def accept_entries(queryset):
+def accept_entries(user_id, queryset):
     """
     Accept all entries in the queryset
 
@@ -124,6 +140,8 @@ def accept_entries(queryset):
         entry.updated_at = timezone.now()
         entry.payment = _create_payment_for_entry(entry)
 
+        log_obj = None
+
         try:
             if entry.registration.username is None:
                 entry.registration.username = _generate_username(
@@ -131,20 +149,32 @@ def accept_entries(queryset):
                 entry.registration.save()
             emails.send_registration_accepted_message(entry.registration,
                                                       entry.payment)
+            log_obj = entry.registration
         except Registration.DoesNotExist:
             try:
                 emails.send_renewal_accepted_message(entry.renewal,
                                                      entry.payment)
+                log_obj = entry.renewal
             except Renewal.DoesNotExist:
                 pass
 
+        if log_obj:
+            LogEntry.objects.log_action(
+                user_id=user_id,
+                content_type_id=get_content_type_for_model(log_obj).pk,
+                object_id=log_obj.pk,
+                object_repr=str(log_obj),
+                action_flag=CHANGE,
+                change_message='Change status to approved',
+            )
+
         entry.save()
         updated_entries.append(entry.pk)
 
     return len(updated_entries)
 
 
-def revert_entry(entry):
+def revert_entry(user_id, entry):
     """
     Revert status of entry to review so that it can be corrected
 
@@ -161,6 +191,28 @@ def revert_entry(entry):
     if payment is not None:
         payment.delete()
 
+    log_obj = None
+
+    try:
+        log_obj = entry.registration
+    except Registration.DoesNotExist:
+        try:
+            log_obj = entry.renewal
+        except Renewal.DoesNotExist:
+            pass
+
+    print(log_obj)
+
+    if log_obj:
+        LogEntry.objects.log_action(
+            user_id=user_id,
+            content_type_id=get_content_type_for_model(log_obj).pk,
+            object_id=log_obj.pk,
+            object_repr=str(log_obj),
+            action_flag=CHANGE,
+            change_message='Revert status to review',
+        )
+
 
 def _create_payment_for_entry(entry):
     """

website/registrations/tests/test_admin.py

@@ -11,6 +11,7 @@ from django.utils.translation import ugettext_lazy as _
 
 from members.models import Member
 from payments.models import Payment
+from payments.widgets import PaymentWidget
 from registrations import admin
 from registrations.models import Entry, Registration, Renewal
 
@@ -22,6 +23,7 @@ def _get_mock_request(perms=None):
     mock_request = HttpRequest()
     mock_request.META = mock.Mock(return_value={})
     mock_request.user = mock.MagicMock()
+    mock_request.user.pk = 1
     mock_request.user.is_superuser = False
     mock_request.user.user_permissions = perms
     mock_request.user.has_perm = lambda x: x in perms
@@ -166,7 +168,7 @@ class RegistrationAdminTest(TestCase):
         request = _get_mock_request(['registrations.review_entries'])
         self.admin.accept_selected(request, queryset)
 
-        accept_entries.assert_called_once_with(queryset)
+        accept_entries.assert_called_once_with(1, queryset)
 
         request._messages.add.assert_called_once_with(
             messages.SUCCESS,
@@ -189,7 +191,7 @@ class RegistrationAdminTest(TestCase):
 
         request = _get_mock_request(['registrations.review_entries'])
         self.admin.reject_selected(request, queryset)
-        reject_entries.assert_called_once_with(queryset)
+        reject_entries.assert_called_once_with(1, queryset)
 
         request._messages.add.assert_called_once_with(
             messages.SUCCESS,
@@ -300,6 +302,50 @@ class RegistrationAdminTest(TestCase):
         reg.payment = None
         self.assertEqual(self.admin.payment_status(reg), '-')
 
+    def test_formfield_for_dbfield(self):
+        with self.subTest('Payment field'):
+            field = self.admin.formfield_for_dbfield(
+                Registration._meta.get_field('payment'), request=None)
+            self.assertIsInstance(field.widget, PaymentWidget)
+        with self.subTest('Other field'):
+            field = self.admin.formfield_for_dbfield(
+                Registration._meta.get_field('first_name'), request=None)
+            self.assertNotIsInstance(field.widget, PaymentWidget)
+            self.assertIsNotNone(field.widget)
+
+    def test_save_model(self):
+        reg = Registration(
+            status=Registration.STATUS_REVIEW,
+            birthday=timezone.now()
+        )
+
+        with self.subTest('Status review saves'):
+            reg.first_name = 'Test1'
+            self.admin.save_model({}, reg, None, True)
+            self.assertTrue(
+                Registration.objects.filter(first_name='Test1').exists())
+
+        with self.subTest('Status accepted, no save'):
+            reg.first_name = 'Test2'
+            reg.status = Registration.STATUS_ACCEPTED
+            self.admin.save_model({}, reg, None, True)
+            self.assertFalse(
+                Registration.objects.filter(first_name='Test2').exists())
+
+        with self.subTest('Status reject, no save'):
+            reg.first_name = 'Test2'
+            reg.status = Registration.STATUS_REJECTED
+            self.admin.save_model({}, reg, None, True)
+            self.assertFalse(
+                Registration.objects.filter(first_name='Test2').exists())
+
+        with self.subTest('Status completed, no save'):
+            reg.first_name = 'Test2'
+            reg.status = Registration.STATUS_COMPLETED
+            self.admin.save_model({}, reg, None, True)
+            self.assertFalse(
+                Registration.objects.filter(first_name='Test2').exists())
+
 
 class RenewalAdminTest(TestCase):
 
@@ -321,7 +367,7 @@ class RenewalAdminTest(TestCase):
         request = _get_mock_request(['registrations.review_entries'])
         self.admin.accept_selected(request, queryset)
 
-        accept_entries.assert_called_once_with(queryset)
+        accept_entries.assert_called_once_with(1, queryset)
 
         request._messages.add.assert_called_once_with(
             messages.SUCCESS,
@@ -344,7 +390,7 @@ class RenewalAdminTest(TestCase):
 
         request = _get_mock_request(['registrations.review_entries'])
         self.admin.reject_selected(request, queryset)
-        reject_entries.assert_called_once_with(queryset)
+        reject_entries.assert_called_once_with(1, queryset)
 
         request._messages.add.assert_called_once_with(
             messages.SUCCESS,

website/registrations/tests/test_models.py

@@ -202,17 +202,20 @@ class RegistrationTest(TestCase):
             membership_type=Membership.MEMBER,
         )
 
-        self.assertEqual(len(mail.outbox), 1)
-        confirm_url = '{}{}'.format('https://thalia.nu',
-                                    reverse('registrations:confirm-email',
-                                            args=[registration.pk]))
-        self.assertTrue(confirm_url in mail.outbox[0].body)
-        mail.outbox.clear()
-
-        registration.status = Entry.STATUS_REVIEW
-        registration.save()
-
-        self.assertEqual(len(mail.outbox), 0)
+        with self.subTest(f'Create confirmation mail'
+                          f'on save with status confirm'):
+            self.assertEqual(len(mail.outbox), 1)
+            confirm_url = '{}{}'.format('https://thalia.nu',
+                                        reverse('registrations:confirm-email',
+                                                args=[registration.pk]))
+            self.assertTrue(confirm_url in mail.outbox[0].body)
+            mail.outbox.clear()
+
+        with self.subTest('No emails in outbox when status is not confirm'):
+            registration.status = Entry.STATUS_REVIEW
+            registration.save()
+
+            self.assertEqual(len(mail.outbox), 0)
 
 
 class RenewalTest(TestCase):

website/registrations/tests/test_services.py

@@ -169,7 +169,7 @@ class ServicesTest(TestCase):
         self.e3.status = Entry.STATUS_REVIEW
         self.e3.save()
 
-        rows_updated = services.reject_entries(Entry.objects.all())
+        rows_updated = services.reject_entries(1, Entry.objects.all())
 
         self.assertEqual(rows_updated, 3)
         self.assertEqual(Entry.objects
@@ -182,7 +182,7 @@ class ServicesTest(TestCase):
         self.e3.status = Entry.STATUS_REVIEW
         self.e3.save()
 
-        rows_updated = services.accept_entries(Entry.objects.all())
+        rows_updated = services.accept_entries(1, Entry.objects.all())
 
         self.e2.refresh_from_db()
         self.e3.refresh_from_db()
@@ -204,7 +204,7 @@ class ServicesTest(TestCase):
         self.e3.status = Entry.STATUS_REVIEW
         self.e3.save()
 
-        rows_updated = services.accept_entries(Entry.objects.all())
+        rows_updated = services.accept_entries(1, Entry.objects.all())
 
         self.e2.refresh_from_db()
         self.assertEqual(self.e2.username, 'manual_username')
@@ -223,7 +223,7 @@ class ServicesTest(TestCase):
         self.e3.status = Entry.STATUS_REVIEW
         self.e3.save()
 
-        rows_updated = services.accept_entries(Entry.objects.all())
+        rows_updated = services.accept_entries(1, Entry.objects.all())
 
         self.assertEqual(rows_updated, 0)
         self.assertEqual(Entry.objects.filter(
@@ -236,7 +236,7 @@ class ServicesTest(TestCase):
             self.e2.payment = services._create_payment_for_entry(self.e2)
             self.e2.save()
 
-            services.revert_entry(self.e2)
+            services.revert_entry(1, self.e2)
 
             self.e2.refresh_from_db()
 
@@ -244,21 +244,32 @@ class ServicesTest(TestCase):
             self.assertIsNone(self.e2.payment)
 
         with self.subTest("Revert rejected entry"):
-            self.e2.status = Entry.STATUS_REJECTED
-            self.e2.save()
+            self.e3.status = Entry.STATUS_REJECTED
+            self.e3.save()
 
-            services.revert_entry(self.e2)
+            services.revert_entry(1, self.e3)
 
-            self.e2.refresh_from_db()
+            self.e3.refresh_from_db()
 
-            self.assertEqual(self.e2.status, Entry.STATUS_REVIEW)
+            self.assertEqual(self.e3.status, Entry.STATUS_REVIEW)
+
+        with self.subTest("Revert another rejected entry"):
+            self.e0.status = Entry.STATUS_REJECTED
+            self.e0.payment = services._create_payment_for_entry(self.e0)
+            self.e0.save()
+
+            services.revert_entry(1, self.e0)
+
+            self.e0.refresh_from_db()
+
+            self.assertEqual(self.e0.status, Entry.STATUS_REVIEW)
 
         with self.subTest("Does not revert completed entry"):
             self.e2.status = Entry.STATUS_COMPLETED
             self.e2.payment = services._create_payment_for_entry(self.e2)
             self.e2.save()
 
-            services.revert_entry(self.e2)
+            services.revert_entry(1, self.e0)
 
             self.e2.refresh_from_db()
 

website/registrations/tests/test_views.py

@@ -138,7 +138,7 @@ class EntryAdminViewTest(TestCase):
                     '/admin/registrations/%s/%s/change/' % (type, entry.pk)
                 )
 
-                accept_entries.assert_called_once_with(entry_qs)
+                accept_entries.assert_called_once_with(1, entry_qs)
                 self.assertFalse(reject_entries.called)
 
                 request._messages.add.assert_called_once_with(
@@ -192,7 +192,7 @@ class EntryAdminViewTest(TestCase):
                     '/admin/registrations/%s/%s/change/' % (type, entry.pk)
                 )
 
-                reject_entries.assert_called_once_with(entry_qs)
+                reject_entries.assert_called_once_with(1, entry_qs)
                 self.assertFalse(accept_entries.called)
 
                 request._messages.add.assert_called_once_with(
@@ -265,7 +265,7 @@ class EntryAdminViewTest(TestCase):
                     '/admin/registrations/%s/%s/change/' % (type, entry.pk)
                 )
 
-                revert.assert_called_once_with(entry.entry_ptr)
+                revert.assert_called_once_with(1, entry.entry_ptr)
 
     @mock.patch('registrations.models.Entry.objects.filter')
     def test_post_not_exists(self, qs_mock):

website/registrations/views.py

@@ -56,14 +56,14 @@ class EntryAdminView(View):
                 messages.error(request, _('Could not accept %s. '
                                           'Username is not unique.') %
                                model_ngettext(entry, 1))
-            elif services.accept_entries(entry_qs) > 0:
+            elif services.accept_entries(request.user.pk, entry_qs) > 0:
                 messages.success(request, _('Successfully accepted %s.') %
                                  model_ngettext(entry, 1))
             else:
                 messages.error(request, _('Could not accept %s.') %
                                model_ngettext(entry, 1))
         elif action == 'reject':
-            if services.reject_entries(entry_qs) > 0:
+            if services.reject_entries(request.user.pk, entry_qs) > 0:
                 messages.success(request, _('Successfully rejected %s.') %
                                  model_ngettext(entry, 1))
             else:
@@ -75,7 +75,7 @@ class EntryAdminView(View):
             except Registration.DoesNotExist:
                 pass
         elif action == 'revert':
-            services.revert_entry(entry)
+            services.revert_entry(request.user.pk, entry)
 
         if entry_qs.filter(renewal=None).exists():
             content_type = ContentType.objects.get_for_model(Registration)