fix: email failure after payment no longer shows a 500 to interactive callers
_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 <noreply@anthropic.com>
This commit is contained in:
parent
14ab8d0f76
commit
cfc78b72ad
@ -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}')
|
||||
|
||||
@ -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:
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user