Fix #166 payment mail button

Prevents multiple form submits using a database lock and a rate-limit.

assets/templates/dining_lists/dining_slot_diners.html

@@ -150,17 +150,23 @@
                             <button type="submit" class="btn btn-block btn-primary" form="statsForm">
                                 Update User Stats
                             </button>
-                            {% if dining_list.payment_link %}
                             <form method="post" class="mt-3"
                                   action="{% url 'slot_inform_payment' day=date.day month=date.month year=date.year identifier=dining_list.association.slug %}">
                                 {% csrf_token %}
-                                <button type="submit" class="btn btn-block btn-secondary" onclick="this.disabled = true;">
-                                    <div class="badge badge-info" style="position: center">new</div>
-                                    <span class="">Send payment mail <i class="fas fa-envelope"></i></span>
+                                <button type="submit"
+                                        class="btn btn-block btn-secondary"
+                                        {% if not dining_list.payment_link %}disabled{% endif %}>
+                                    <i class="fas fa-envelope"></i> Send payment mail
                                 </button>
-                                <small>Sends a mail to all who have not yet paid with a reminder</small>
+                                <small>
+                                    {% if dining_list.payment_link %}
+                                        Sends a mail to all who have not yet paid with a reminder
+                                    {% else %}
+                                        <a href="{% url 'slot_change' day=date.day month=date.month year=date.year identifier=dining_list.association.slug %}">
+                                            Enter a payment link</a> to send a payment mail
+                                    {% endif %}
+                                </small>
                             </form>
-                            {% endif %}
                         {% endif %}
                     </td>
                     <td><div></div></td>
@@ -184,20 +190,23 @@
                 <button type="submit" class="btn btn-block btn-primary" form="statsForm">
                     Update User Stats
                 </button>
-                {% if dining_list.payment_link %}
-                    <form method="post" class="mt-3"
-                          action="{% url 'slot_inform_payment' day=date.day month=date.month year=date.year identifier=dining_list.association.slug %}">
-                        {% csrf_token %}
-                        <button type="submit" class="btn btn-block btn-secondary" onclick="this.disabled = true;">
-                            <div class="badge badge-info" style="position: center">new</div>
-                            <span class="">Send payment mail <i class="fas fa-envelope"></i></span>
-                        </button>
-                        <small>Sends a mail to all who have not yet paid with a reminder</small>
-                        <small class="text-warning d-flex d-md-none">
-                            We are aware of an issue where this button does not work on some mobile devices in Chrome.
-                        </small>
-                    </form>
-                {% endif %}
+                <form method="post" class="mt-3"
+                      action="{% url 'slot_inform_payment' day=date.day month=date.month year=date.year identifier=dining_list.association.slug %}">
+                    {% csrf_token %}
+                    <button type="submit"
+                            class="btn btn-block btn-secondary"
+                            {% if not dining_list.payment_link %}disabled{% endif %}>
+                        <i class="fas fa-envelope"></i> Send payment mail
+                    </button>
+                    <small>
+                        {% if dining_list.payment_link %}
+                            Sends a mail to all who have not yet paid with a reminder
+                        {% else %}
+                            <a href="{% url 'slot_change' day=date.day month=date.month year=date.year identifier=dining_list.association.slug %}">
+                                Enter a payment link</a> to send a payment mail
+                        {% endif %}
+                    </small>
+                </form>
             {% endif %}
            {% if dining_list|can_add_others:user %}
                <a class="btn btn-primary btn-block" href="

assets/templates/mail/dining_payment_reminder/body.txt

@@ -8,7 +8,7 @@ By: {{dining_list|short_owners_string}}
 On behalf of: {{dining_list.association}}
 
 {% if is_reminder %}However, according to our administration, you have not yet paid for the meal itself.{%endif%}
-{{ reminder }} kindly asks you if you could pay{% if dining_list.dining_cost %}€ {{dining_list.dining_cost}} {% else %} your share{%endif%}.
+{{ reminder }} kindly asks you if you could pay {% if dining_list.dining_cost %}€{{dining_list.dining_cost}}{% else %}your share{%endif%}.
 {% if dining_list.payment_link %}
 You can do that here:
 {{ dining_list.payment_link }}

dining/forms.py

@@ -1,20 +1,24 @@
 import warnings
-from datetime import datetime
+from datetime import timedelta
 from decimal import Decimal, ROUND_UP
+from typing import List, Dict
 
 from dal_select2.widgets import ModelSelect2, ModelSelect2Multiple
 from django import forms
 from django.conf import settings
+from django.core import mail
+from django.core.mail import EmailMessage
 from django.core.validators import MinValueValidator
 from django.db import transaction
-from django.db.models import OuterRef, Exists
+from django.db.models import OuterRef, Exists, QuerySet
 from django.forms import ValidationError
 from django.utils import timezone
 
 from creditmanagement.models import Transaction, Account
-from dining.models import DiningList, DiningEntryUser, DiningEntryExternal, DiningComment, DiningEntry
+from dining.models import DiningList, DiningEntryUser, DiningEntryExternal, DiningComment, DiningEntry, \
+    PaymentReminderLock
 from general.forms import ConcurrenflictFormMixin
-from general.mail_control import send_templated_mail
+from general.mail_control import construct_templated_mail
 from general.util import SelectWithDisabled
 from userdetails.models import Association, UserMembership, User
 
@@ -403,9 +407,10 @@ def save(self, *args, **kwargs):
 class SendReminderForm(forms.Form):
 
     def __init__(self, *args, dining_list: DiningList = None, **kwargs):
-        assert dining_list is not None
+        if dining_list is None:
+            raise ValueError("dining_list is required")
         self.dining_list = dining_list
-        super(SendReminderForm, self).__init__(*args, **kwargs)
+        super().__init__(*args, **kwargs)
 
     def clean(self):
         # Verify that there are people to inform
@@ -414,30 +419,87 @@ def clean(self):
         if self.dining_list.payment_link == "":
             raise ValidationError("There was no payment url defined", code="payment_url_missing")
 
-    def send_reminder(self, request=None):
-        assert request is not None
-        unpaid_user_entries = DiningEntryUser.objects.filter(dining_list=self.dining_list, has_paid=False)
-        unpaid_guest_entries = DiningEntryExternal.objects.filter(dining_list=self.dining_list, has_paid=False)
+    def get_user_recipients(self) -> QuerySet:
+        """Returns the users that need to pay themselves, excluding external entries.
 
-        is_reminder = datetime.now().date() > self.dining_list.date  # ?? Please explain non-trivial operations
+        Returns:
+            A QuerySet of User instances.
+        """
+        unpaid_user_entries = DiningEntryUser.objects.filter(dining_list=self.dining_list, has_paid=False)
+        return User.objects.filter(diningentry__in=unpaid_user_entries)
 
-        context = {'dining_list': self.dining_list, 'reminder': request.user, 'is_reminder': is_reminder}
+    def get_guest_recipients(self) -> Dict[User, List[str]]:
+        """Returns external diners who have not yet paid.
 
-        if unpaid_user_entries.count() > 0:
-            send_templated_mail('mail/dining_payment_reminder',
-                                User.objects.filter(diningentry__in=unpaid_user_entries),
-                                context=context,
-                                request=request)
+        Returns:
+            A dictionary from User to a list of guest names who were added by
+            the user.
+        """
+        unpaid_guest_entries = DiningEntryExternal.objects.filter(dining_list=self.dining_list, has_paid=False)
 
+        recipients = {}
         for user in User.objects.filter(diningentry__in=unpaid_guest_entries).distinct():
-            guests = []
-
-            for external_entry in unpaid_guest_entries.filter(user=user):
-                guests.append(external_entry.name)
+            recipients[user] = [e.name for e in unpaid_guest_entries.filter(user=user)]
+        return recipients
+
+    def construct_messages(self, request) -> List[EmailMessage]:
+        """Constructs the emails to send."""
+        messages = []
+
+        is_reminder = timezone.now().date() > self.dining_list.date
+
+        # Mail for internal diners
+        messages.extend(construct_templated_mail(
+            'mail/dining_payment_reminder',
+            self.get_user_recipients(),
+            context={'dining_list': self.dining_list, 'reminder': request.user, 'is_reminder': is_reminder},
+            request=request
+        ))
+
+        # Mail for external diners
+        for user, guests in self.get_guest_recipients().items():
+            messages.extend(construct_templated_mail(
+                'mail/dining_payment_reminder_external',
+                user,
+                context={
+                    'dining_list': self.dining_list,
+                    'reminder': request.user,
+                    'is_reminder': is_reminder,
+                    'guests': guests,
+                },
+                request=request
+            ))
+        return messages
+
+    def send_reminder(self, request, nowait=False) -> bool:
+        """Sends a reminder email to all non-paid diners on the dining list.
+
+        The sending is rate-limited to prevent multiple emails from being sent
+        simultaneously.
 
-            context["guests"] = guests
+        Args:
+            nowait: When True, raises DatabaseError instead of blocking when
+                the lock is held.
 
-            send_templated_mail('mail/dining_payment_reminder_external',
-                                user,
-                                context=context.copy(),
-                                request=request)
+        Returns:
+            True on success. False when a mail was already sent too recently
+            for this dining list.
+        """
+        # We use a critical section to prevent multiple emails from being sent
+        # simultaneously. The critical section is implemented using the
+        # database locking mechanism. However, SQLite does not support locking.
+        # You need to use PostgreSQL as database.
+        with transaction.atomic():
+            # Acquire a lock on the dining list row. This may block.
+            DiningList.objects.select_for_update(nowait=nowait).get(pk=self.dining_list.pk)
+            # Retrieve the row that stores the last sent time.
+            lock, _ = PaymentReminderLock.objects.get_or_create(dining_list=self.dining_list)
+            if lock.sent and timezone.now() - lock.sent < timedelta(seconds=30):
+                # A mail was sent too recently.
+                return False
+            else:
+                # Update the lock and send the emails.
+                lock.sent = timezone.now()
+                lock.save()
+                mail.get_connection().send_messages(self.construct_messages(request))
+                return True

dining/migrations/0021_create_paymentreminderlock.py

@@ -0,0 +1,21 @@
+# Generated by Django 3.2.13 on 2023-01-16 14:13
+
+from django.db import migrations, models
+import django.db.models.deletion
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('dining', '0020_delete_userdiningsettings'),
+    ]
+
+    operations = [
+        migrations.CreateModel(
+            name='PaymentReminderLock',
+            fields=[
+                ('dining_list', models.OneToOneField(on_delete=django.db.models.deletion.CASCADE, primary_key=True, serialize=False, to='dining.dininglist')),
+                ('sent', models.DateTimeField(null=True)),
+            ],
+        ),
+    ]

dining/models.py

@@ -249,3 +249,15 @@ class DiningDayAnnouncement(models.Model):
 
     def __str__(self):
         return self.title
+
+
+class PaymentReminderLock(models.Model):
+    """Database table to prevent multiple payment reminder emails.
+
+    This table stores temporary data and can safely be removed or recreated.
+
+    See SlotPaymentView.
+    """
+    # We use primary_key=True to prevent an unnecessary auto id column.
+    dining_list = models.OneToOneField(DiningList, on_delete=models.CASCADE, primary_key=True)  # Key
+    sent = models.DateTimeField(null=True)  # Value

dining/templatetags/dining_tags.py

@@ -131,15 +131,13 @@ def short_owners_string(dining_list: DiningList) -> str:
     Returns:
         Names of the owners separated by ',' and 'and'
     """
-    owners = dining_list.owners.all()
+    owners = list(dining_list.owners.all())
 
     if len(owners) > 1:
-        names = [o.first_name for o in owners]
+        first_names = [o.first_name for o in owners]
+        # Join by comma and 'and'
+        return '{} and {}'.format(', '.join(first_names[:-1]), first_names[-1])
+    elif len(owners) == 1:
+        return owners[0].get_full_name()
     else:
-        names = [o.get_full_name() for o in owners]
-
-    if len(names) >= 2:
-        # Join all but last names by ',' and put 'and' in front of last name
-        commaseparated = ', '.join(names[0:-1])
-        return '{} and {}'.format(commaseparated, names[-1])
-    return names[0]
+        return ''

dining/tests/test_forms.py

@@ -615,19 +615,21 @@ def test_invalid_missing_payment_link(self):
         self.dining_list.save()
         self.assert_form_has_error({}, code='payment_url_missing')
 
-    @patch('dining.forms.send_templated_mail')
-    def test_no_unpaid_users_sending(self, mock_mail):
-        """Asserts that when all users have paid, no mails are send to remind people."""
-        DiningEntryUser.objects.update(has_paid=True)
-        DiningEntryExternal.objects.all().delete()
-        form = self.build_form({})
-        request = HttpRequest()
-        request.user = User.objects.get(id=1)
-        form.send_reminder(request=request)
-
-        mock_mail.assert_not_called()
-
-    @patch('dining.forms.send_templated_mail')
+    # This is unnecessary. It is perfectly fine to call send_templated_mail with 0 recipients.
+
+    # @patch('dining.forms.construct_templated_mail')
+    # def test_no_unpaid_users_sending(self, mock_mail):
+    #     """Asserts that when all users have paid, no mails are send to remind people."""
+    #     DiningEntryUser.objects.update(has_paid=True)
+    #     DiningEntryExternal.objects.all().delete()
+    #     form = self.build_form({})
+    #     request = HttpRequest()
+    #     request.user = User.objects.get(id=1)
+    #     form.send_reminder(request=request)
+    #
+    #     mock_mail.assert_not_called()
+
+    @patch('dining.forms.construct_templated_mail')
     def test_mail_sending_users(self, mock_mail):
         form = self.assert_form_valid({})
         request = HttpRequest()
@@ -647,7 +649,7 @@ def test_mail_sending_users(self, mock_mail):
         self.assertEqual(calls[0]['context']['dining_list'], self.dining_list)
         self.assertEqual(calls[0]['context']['reminder'], User.objects.get(id=1))
 
-    @patch('dining.forms.send_templated_mail')
+    @patch('dining.forms.construct_templated_mail')
     def test_mail_sending_external(self, mock_mail):
         """Tests handling of messaging for external guests added by a certain user."""
         form = self.assert_form_valid({})

dining/tests/testformsdiningentry.py → dining/tests/test_forms_diningentry.py

@@ -91,7 +91,7 @@ def test_balance_too_low(self):
 
     def test_balance_too_low_exception(self):
         # Make user member of association with exception
-        assoc = Association.objects.create(slug='ankie4president', name='PvdD', has_min_exception=True)
+        assoc = Association.objects.create(slug='assoc', name='Association', has_min_exception=True)
         UserMembership.objects.create(related_user=self.user2, association=assoc, is_verified=True,
                                       verified_on=timezone.now())
         Transaction.objects.create(source=self.user2.account,