Conversation
There was a problem hiding this comment.
Pull request overview
Implements the unsubscribe hard-delete policy so that using the one-click unsubscribe link removes the subscriber row (rather than deactivating it), and updates user-facing policy text accordingly.
Changes:
- Replace the unsubscribe flow’s DB operation from “deactivate + wipe PII” to “hard-delete subscriber row by token”.
- Update unsubscribe and DB unit tests to reflect hard-delete semantics.
- Align privacy policy retention language with the new behavior and existing 7-day purge of inactive rows.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
immermatch/db.py |
Adds delete_subscriber_by_token() and uses DELETE on subscribers to hard-remove the subscriber row. |
immermatch/pages/unsubscribe.py |
Switches the page to call delete_subscriber_by_token() and keeps success/error messaging. |
tests/test_db.py |
Updates DB-layer tests to validate deletion-by-token behavior. |
tests/test_pages_unsubscribe.py |
Updates page tests to patch the new DB function for unsubscribe behavior. |
immermatch/pages/privacy.py |
Updates retention/unsubscribe copy to match immediate deletion on unsubscribe and 7-day deletion after expiry. |
docs/strategy/ROADMAP.md |
Marks the R6 unsubscribe hard-delete roadmap item as completed. |
| @@ -58,7 +58,7 @@ def test_valid_token_shows_success( | |||
| assert len(at.error) == 0 | |||
|
|
|||
| @patch.dict("os.environ", _FAKE_ENV, clear=False) | |||
| @patch("immermatch.db.deactivate_subscriber_by_token", return_value=True) | |||
| @patch("immermatch.db.delete_subscriber_by_token", return_value=True) | |||
| @patch("immermatch.db.get_admin_client", return_value=MagicMock()) | |||
| def test_deactivate_called_with_token( | |||
| self, | |||
| @@ -73,7 +73,7 @@ def test_deactivate_called_with_token( | |||
| assert args[1] == "my-unsubscribe-token" | |||
There was a problem hiding this comment.
In this test the patched function is now delete_subscriber_by_token, but the test name and mock parameter names still reference “deactivate”. Renaming them (e.g., test_delete_called_with_token, mock_delete_by_token) will keep intent clear and avoid confusion when reading failures.
| if exp_dt and datetime.now(timezone.utc) > exp_dt: | ||
| return False | ||
|
|
||
| success = deactivate_subscriber(client, sub["id"]) | ||
| if success: | ||
| delete_subscriber_data(client, sub["id"]) | ||
| return success | ||
| logger.info("Hard-deleting subscriber %s via unsubscribe token", sub["id"]) | ||
| result = client.table("subscribers").delete().eq("id", sub["id"]).execute() | ||
| return bool(result.data) |
There was a problem hiding this comment.
Hard-delete unsubscribe behavior: the function currently short-circuits earlier if the subscriber row is is_active=False, which means a matching token can fail to delete the row. If the policy is “unsubscribe = hard delete”, consider deleting the row whenever the token matches and is unexpired (regardless of is_active) so the flow can’t leave data behind in edge cases (e.g., rows manually marked inactive while retaining an unsubscribe token).
Closes #69