fix(notifications): post-merge review fixes for PR #162#183
Merged
mostlyvirtual merged 3 commits intomasterfrom May 2, 2026
Merged
fix(notifications): post-merge review fixes for PR #162#183mostlyvirtual merged 3 commits intomasterfrom
mostlyvirtual merged 3 commits intomasterfrom
Conversation
Three surgical fixes from post-merge review of squash 8e6fb08. 1. process_unsubscribe: catch ValidationError on malformed UUID services.py:1465 — Django UUIDField.to_python raises django.core.exceptions.ValidationError (NOT ValueError) for non-UUID strings on the legacy ?token=... query-string path. The narrowed except (DoesNotExist, ValueError) tuple let ValidationError bubble to the outer except Exception and get logged as ERROR ("Failed to process token") instead of the intended WARNING ("Invalid or unknown token"). Add ValidationError to the tuple and correct the inline comment. New regression test asserts a malformed string returns False without raising. 2. EmailPreference.update_marketing_consent: preserve consent_date on withdrawal models.py:919 — withdrawal branch now writes update_fields=["marketing", "updated_at"] only. The grant branch retains update_fields=["marketing", "marketing_consent_date", "marketing_consent_source", "updated_at"]. Restores the pre-PR invariant that withdrawal preserves historical grant evidence (GDPR Art. 7 "demonstrate consent" — grant remained demonstrable for the period it was active). New invariant test asserts consent_date and consent_source survive a grant -> withdrawal cycle. 3. Audit-failure tests: use OperationalError instead of RuntimeError test_unsubscribe_tokens.py — RuntimeError does not exercise the production failure mode (DatabaseError inside an atomic block can leave the connection in InFailedSqlTransaction state). Switch both audit-failure tests to django.db.utils.OperationalError so the savepoint pattern is tested against the actual failure class. Out of scope (follow-up): - Audit duplication: customers/signals.py:114 already fires log_compliance_event on Customer.save() with marketing_consent change, while this PR adds log_simple_event with different event types. Two audit records per consent change. Resolution requires a design call on whether to drop one path or thread source attribution through the signal — tracked separately to keep this diff surgical. Tests: - 25/25 tests.notifications.test_unsubscribe_tokens (1 new) - 37/37 tests.notifications.test_email_service (1 new) - ruff/mypy clean on all four files Refs #162 Signed-off-by: Claudiu Ciungan <claudiu@captainpragmatic.com>
There was a problem hiding this comment.
Pull request overview
Applies post-merge fixes to the notifications unsubscribe/consent codepaths to improve correctness and GDPR auditability, and adjusts tests to exercise audit-failure behavior using a realistic DB exception type.
Changes:
- Treat malformed unsubscribe token UUIDs as “invalid token” (warning +
False) by catching DjangoValidationErrorinprocess_unsubscribe. - Update
EmailPreference.update_marketing_consent()so withdrawal no longer clearsmarketing_consent_date/marketing_consent_source, preserving historical grant evidence. - Update audit-failure tests to raise
OperationalError(a realDatabaseErrorsubclass) instead ofRuntimeError.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| services/platform/apps/notifications/services.py | Catch Django ValidationError during token lookup so malformed UUIDs are handled as invalid tokens rather than logged as processing failures. |
| services/platform/apps/notifications/models.py | Preserve consent grant metadata on withdrawal; only update consent date/source on grant. |
| services/platform/tests/notifications/test_unsubscribe_tokens.py | Add malformed-UUID test and use OperationalError to better simulate DB audit logging failures. |
| services/platform/tests/notifications/test_email_service.py | Add regression test ensuring withdrawal preserves consent date/source. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Addresses an independent post-merge review finding on PR #183: the signal handler _handle_marketing_consent_change calls AuditService.log_compliance_event without a savepoint. On real Postgres, a DatabaseError raised mid-SQL inside the audit call would leave the connection in InFailedSqlTransaction state, which catching the Python exception alone cannot recover from. The outer transaction.atomic() in process_unsubscribe / update_preferences would then be rolled back at exit, silently undoing the consent change while the service returned True — a GDPR Art. 7(3) violation. Changes - apps/customers/signals.py: wrap log_compliance_event in transaction.atomic() savepoint inside the existing try/except in _handle_marketing_consent_change. The other 6 unsavepointed audit calls in this file (data_processing_consent, contracts, payment methods, etc.) follow the same anti-pattern but are pre-existing and out of scope for this PR — the marketing path is the one connected to the PR #162 service-layer savepoint fix and was identified as inconsistent. - tests/notifications/test_unsubscribe_tokens.py: - test_malformed_uuid_rejected_as_invalid: strengthened to assert log level (WARNING, not ERROR) via assertLogs. Pre-fix and post-fix both returned False, so the previous assertion proved nothing — only the log level distinguishes the two states. - 2 new tests patch log_compliance_event (signal path) with OperationalError, verifying the consent change persists. These are honest about their SQLite limitation in the docstring: SQLite does not implement InFailedSqlTransaction state, so the test cannot empirically distinguish "savepoint present" from "savepoint absent" — falsified in this session by removing the savepoint and observing the test still pass. The savepoint is required for production Postgres correctness regardless. Tests - 27/27 tests.notifications.test_unsubscribe_tokens (3 new total) - 314/314 tests.customers (no regressions in signal handler) - ruff/mypy clean on both edited files Refs #162, #182, #183 Signed-off-by: Claudiu Ciungan <claudiu@captainpragmatic.com>
Extends the savepoint pattern from _handle_marketing_consent_change to
the other 6 AuditService.log_compliance_event call sites in
apps/customers/signals.py. All share the same anti-pattern: a Python
try/except around an audit call that, if it raises a DatabaseError on
real Postgres, leaves the connection in InFailedSqlTransaction state
and forces rollback of the outer transaction at exit.
Sites fixed
- Line 257: handle_tax_profile_change — Romanian tax registration audit
- Line 715: _handle_gdpr_consent_change — data_processing_consent
(GDPR Art. 7(3) — same severity as marketing consent)
- Line 775: _verify_romanian_company_compliance — missing CUI warning
- Line 799: _validate_romanian_cui — invalid CUI format
- Line 876: extended_payment_terms compliance audit
- Line 914: _verify_primary_address_compliance — Romanian address audit
Pattern at every site
try:
with transaction.atomic():
AuditService.log_compliance_event(compliance_request)
except Exception:
logger.exception("🔥 [Customer Signal] <context> audit failed")
Code AFTER the audit call still runs on audit failure (e.g.,
gdpr_consent_date update at line 727 — GDPR Art. 7(1) requires
recording when consent was given, this must run regardless of audit
outcome).
Also normalised the marketing_consent handler I patched in 33c06fb
to drop the early `return` after audit failure — for symmetry with
the other handlers, success-log fires regardless of audit outcome.
Tests
- 314/314 tests.customers (no regressions in any signal handler)
- 27/27 tests.notifications.test_unsubscribe_tokens
- ruff/mypy clean
The same SQLite test limitation noted in 33c06fb applies: SQLite
does not implement InFailedSqlTransaction state, so unit tests
cannot empirically distinguish "savepoint present" from "savepoint
absent" for these sites either. The savepoints are required for
production Postgres correctness.
Refs #162, #182, #183
Signed-off-by: Claudiu Ciungan <claudiu@captainpragmatic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Three surgical fixes from post-merge review of PR #162 (squash 8e6fb08):
ValidationErroron malformed UUID so legacy `?token=...` paths log WARNING ("invalid token") instead of ERROR ("failed to process")Out of scope: audit duplication via `customers/signals.py:114` + new service-layer call. Tracked in #182 (needs a design call).
Test plan
Refs #162, #182