From cfc78b72adac13b68169c3b744e6c759d74a6b25 Mon Sep 17 00:00:00 2001 From: Konrad du Plessis Date: Fri, 12 Jun 2026 17:41:20 +0200 Subject: [PATCH] fix: email failure after payment no longer shows a 500 to interactive callers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _send_payslip_email caught email errors, queued a warning toast, then re-raised unconditionally — but only batch_pay catches that re-raise. The Pay button (process_payment) and the three Pay-Immediately paths in add_adjustment called it bare, so an SMTP failure AFTER the payment committed replaced the warning with a 500 error page (the 28 May 2026 incident behaviour — admin can't tell the payment saved, may pay twice). Re-raise is now conditional on suppress_messages=True (the batch path, which counts failures in its summary); interactive callers get the warning toast as intended. Regression tests cover both sides. Co-Authored-By: Claude Fable 5 --- core/tests.py | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++ core/views.py | 22 +++++++++++------- 2 files changed, 78 insertions(+), 8 deletions(-) diff --git a/core/tests.py b/core/tests.py index cf49eb6..205cbd1 100644 --- a/core/tests.py +++ b/core/tests.py @@ -3,7 +3,9 @@ # determines, for each worker on a log, whether they were paid for it. import datetime +import json from decimal import Decimal +from unittest import mock from django.conf import settings from django.contrib.auth.models import User @@ -3679,3 +3681,65 @@ class ManagerSalariedPayUITests(TestCase): # guard test). self.assertNotIn('Salary Details', body) self.assertNotIn('Salary Amount:', body) + + +# ============================================================================= +# === AUDIT FIX #1 — EMAIL FAILURE AFTER PAYMENT MUST NOT 500 === +# Regression tests for the 28 May 2026 incident class: SMTP breaks, the +# payment commits, and the admin must see a warning toast — NOT an error +# page (which hides that the payment DID go through and invites a +# dangerous second click of Pay). +# ============================================================================= + +class PaymentEmailFailureTests(TestCase): + """Email failure after a successful payment: warn the admin, never crash.""" + + def setUp(self): + self.admin = User.objects.create_user( + username='emailfail_admin', password='x', is_staff=True) + self.client.force_login(self.admin) + self.project = Project.objects.create(name='EF Project') + self.worker = Worker.objects.create( + name='EF Worker', id_number='EF-1', monthly_salary=Decimal('8000')) + log = WorkLog.objects.create( + date=datetime.date(2026, 6, 1), project=self.project, + supervisor=self.admin) + log.workers.add(self.worker) + + def _broken_smtp(self): + # Simulate the 28 May 2026 state: every send() raises (bad creds / + # SMTP unreachable / empty DEFAULT_FROM_EMAIL). + return mock.patch( + 'django.core.mail.EmailMultiAlternatives.send', + side_effect=Exception('SMTP down')) + + def test_individual_payment_survives_email_failure(self): + with self._broken_smtp(): + resp = self.client.post( + reverse('process_payment', args=[self.worker.id]), follow=True) + # The payment was saved... + self.assertEqual( + PayrollRecord.objects.filter(worker=self.worker).count(), 1) + # ...and the admin landed back on a normal page (not a 500) + self.assertEqual(resp.status_code, 200) + msgs = [str(m) for m in resp.context['messages']] + self.assertTrue( + any('email delivery failed' in m for m in msgs), + f'expected an email-failure warning toast, got: {msgs}') + + def test_batch_pay_still_counts_email_failures(self): + # The batch path NEEDS the helper to re-raise internally so it can + # count failures — this guards the other side of the conditional. + with self._broken_smtp(): + resp = self.client.post( + reverse('batch_pay'), + data=json.dumps({'workers': [{'worker_id': self.worker.id}]}), + content_type='application/json', + follow=True) + self.assertEqual( + PayrollRecord.objects.filter(worker=self.worker).count(), 1) + self.assertEqual(resp.status_code, 200) + msgs = [str(m) for m in resp.context['messages']] + self.assertTrue( + any('1 email(s) failed to send' in m for m in msgs), + f'expected batch summary to count the email failure, got: {msgs}') diff --git a/core/views.py b/core/views.py index 06bdf48..602a1fb 100644 --- a/core/views.py +++ b/core/views.py @@ -3804,14 +3804,20 @@ def _send_payslip_email(request, worker, payroll_record, log_count, logs_amount, f'Payslip emailed successfully.' ) except Exception as e: - # Payment is saved — just warn that email failed - if not suppress_messages: - messages.warning( - request, - f'Payment of R {total_amount:,.2f} processed for {worker.name}, ' - f'but email delivery failed: {str(e)}' - ) - raise # Re-raise so batch_pay can count failures + # Payment is saved — the email is the only thing that failed. + # Batch mode (suppress_messages=True) re-raises so batch_pay can + # count failures in its summary. Interactive callers (the Pay + # button, Pay-Immediately adjustments) get a warning toast + # instead — a raise here would show the admin a 500 page and + # hide the fact that the payment DID go through (the 28 May + # 2026 incident behaviour). + if suppress_messages: + raise # batch_pay catches this and counts it + messages.warning( + request, + f'Payment of R {total_amount:,.2f} processed for {worker.name}, ' + f'but email delivery failed: {str(e)}' + ) else: # No SPARK_RECEIPT_EMAIL configured — just show success if not suppress_messages: