fix(payment-methods): delete correct QR S3 keys, clean up replaced QR objects, return presigned URL

[dmeiser]

Intent

Fix three data-integrity issues in kernelworx payment-method QR code S3 handling (one focused PR, no broad refactors): Issue #137 (High): payment-method deletion deletes the wrong QR S3 key - _delete_qr_if_exists in src/utils/payment_methods.py reconstructs a slug-based key via delete_qr_from_s3(account_id, name) while uploads use UUID-based keys, so the S3 object is never deleted; fix by deleting the stored key with delete_qr_by_key(method_to_delete['qrCodeUrl']). Issue #138 (Medium): re-uploading a QR code orphans the previous S3 object - request_qr_upload generates a fresh UUID key each time; delete the previous qrCodeUrl object when confirming a new upload. Issue #139 (Medium): confirm_qr_upload returns a raw S3 key instead of the presigned GET URL its docstring promises; return a freshly minted presigned GET URL. Data contract decided: DynamoDB stores the raw S3 key, clients always receive presigned GET URLs (AppSync field resolver passes already-presigned values through). Existing tests must pass; focused regression tests were added that fail before the fix and pass after. Commit references Closes #137, closes #138, closes #139.

What Changed

• Fixed payment-method deletion to remove the actual stored UUID-based S3 key from qrCodeUrl instead of reconstructing a slug-derived key that never matched the uploaded object.
• Updated QR upload confirmation to delete the previous S3 object when qrCodeUrl is replaced, preventing orphaned objects on re-upload.
• Changed confirm_qr_upload to return a freshly generated presigned GET URL instead of the raw S3 key, matching its documented contract.

Risk Assessment

✅ Low: The change is a focused, well-bounded fix that correctly addresses all three stated issues: payment-method deletion now deletes the stored UUID-based S3 key, confirm_qr_upload cleans up the previous QR object on re-upload, and it returns a freshly minted presigned GET URL while keeping the raw key in DynamoDB. Regression tests cover the new behavior and edge cases (same-key confirmation), and existing tests were updated appropriately. No material bugs or breaking changes were found.

Testing

Ran the focused payment-method QR S3 unit tests plus a moto-backed end-to-end demo that exercises all three fixes. The demo passes on the target commit and fails on the base commit, showing the regression tests detect the original issues. All 136 payment-methods unit tests pass.

Evidence: End-to-end QR S3 regression demo script

"""
End-to-end demonstration of the QR S3 data-integrity fixes.

This script exercises the public backend interface (payment-method CRUD,
QR upload confirmation, and deletion) against moto mocks and asserts the
three contract behaviors:

1. Deleting a payment method removes the stored UUID-based S3 object, not a
   slug-derived key.
2. Confirming a replacement QR upload deletes the previous S3 object.
3. confirm_qr_upload returns a presigned GET URL, not a raw S3 key.
"""

import os
import sys

# Ensure the repository source is importable
REPO_ROOT = "/home/dm/.no-mistakes/worktrees/d353adefa548/01M0RJ2E7BSP28GFDYE5H2D6HE"
sys.path.insert(0, REPO_ROOT)

os.environ["AWS_ACCESS_KEY_ID"] = "testing"
os.environ["AWS_SECRET_ACCESS_KEY"] = "testing"
os.environ["AWS_SECURITY_TOKEN"] = "testing"
os.environ["AWS_SESSION_TOKEN"] = "testing"
os.environ["AWS_DEFAULT_REGION"] = "us-east-1"
os.environ["ACCOUNTS_TABLE_NAME"] = "kernelworx-accounts-ue1-dev"
os.environ["EXPORTS_BUCKET"] = "test-exports-bucket"

import boto3
from botocore.exceptions import ClientError
from moto import mock_aws

from src.handlers.payment_methods_handlers import confirm_qr_upload
from src.utils.dynamodb import tables
from src.utils.payment_methods import create_payment_method, delete_payment_method
from tests.unit.table_schemas import create_all_tables


ACCOUNT_ID = "acc-123-456"
ACCOUNT_ID_KEY = f"ACCOUNT#{ACCOUNT_ID}"
BUCKET_NAME = os.environ["EXPORTS_BUCKET"]


def assert_not_found(s3, key: str) -> None:
    with pytest_raises(ClientError) as exc:
        s3.head_object(Bucket=BUCKET_NAME, Key=key)
    assert exc.value.response["Error"]["Code"] == "404"


def pytest_raises(exc_type):
    """Tiny helper to avoid importing pytest just for raises."""
    import pytest

    return pytest.raises(exc_type)


@mock_aws
def demo():
    dynamodb = boto3.resource("dynamodb", region_name="us-east-1")
    create_all_tables(dynamodb)

    s3 = boto3.client("s3", region_name="us-east-1")
    s3.create_bucket(Bucket=BUCKET_NAME)

    # Seed account
    tables.accounts.put_item(
        Item={
            "accountId": ACCOUNT_ID_KEY,
            "email": "test@example.com",
            "givenName": "Test",
            "familyName": "User",
            "createdAt": "2026-01-01T00:00:00Z",
            "updatedAt": "2026-01-01T00:00:00Z",
        }
    )

    create_payment_method(ACCOUNT_ID, "Venmo")

    # ------------------------------------------------------------------
    # Issue #137: deletion deletes the stored UUID key, not a slug key
    # ------------------------------------------------------------------
    uuid_key = f"payment-qr-codes/{ACCOUNT_ID}/{'a' * 32}.png"
    slug_key = f"payment-qr-codes/{ACCOUNT_ID}/venmo.png"

    tables.accounts.put_item(
        Item={
            "accountId": ACCOUNT_ID_KEY,
            "preferences": {"paymentMethods": [{"name": "Venmo", "qrCodeUrl": uuid_key}]},
        }
    )
    s3.put_object(Bucket=BUCKET_NAME, Key=uuid_key, Body=b"qr-image")
    s3.put_object(Bucket=BUCKET_NAME, Key=slug_key, Body=b"decoy")

    delete_payment_method(ACCOUNT_ID, "Venmo")

    assert_not_found(s3, uuid_key)
    # The unrelated slug-key object must survive
    s3.head_object(Bucket=BUCKET_NAME, Key=slug_key)
    print("PASS: deletion removed the stored UUID key and left the slug decoy intact")

    # ------------------------------------------------------------------
    # Issues #138 & #139: re-upload orphans old object; confirm returns URL
    # ------------------------------------------------------------------
    create_payment_method(ACCOUNT_ID, "Venmo")

    old_key = f"payment-qr-codes/{ACCOUNT_ID}/{'b' * 32}.png"
    new_key = f"payment-qr-codes/{ACCOUNT_ID}/{'c' * 32}.png"

    s3.put_object(Bucket=BUCKET_NAME, Key=old_key, Body=b"old-qr")
    s3.put_object(Bucket=BUCKET_NAME, Key=new_key, Body=b"new-qr")

    # Simulate previous upload already recorded in DynamoDB
    response = tables.accounts.get_item(Key={"accountId": ACCOUNT_ID_KEY})
    preferences = response["Item"].get("preferences", {})
    methods = list(preferences.get("paymentMethods", []))
    for method in methods:
        if method["name"] == "Venmo":
            method["qrCodeUrl"] = old_key
    preferences["paymentMethods"] = methods
    tables.accounts.update_item(
        Key={"accountId": ACCOUNT_ID_KEY},
        UpdateExpression="SET preferences = :prefs",
        ExpressionAttributeValues={":prefs": preferences},
    )

    result = confirm_qr_upload(
        {
            "identity": {"sub": ACCOUNT_ID},
            "arguments": {"paymentMethodName": "Venmo", "s3Key": new_key},
        },
        None,
    )

    # Old object must be gone
    assert_not_found(s3, old_key)
    # New object must remain
    s3.head_object(Bucket=BUCKET_NAME, Key=new_key)

    # Stored value is still the raw S3 key (data contract)
    response = tables.accounts.get_item(Key={"accountId": ACCOUNT_ID_KEY})
    venmo = next(m for m in response["Item"]["preferences"]["paymentMethods"] if m["name"] == "Venmo")
    assert venmo["qrCodeUrl"] == new_key, "DynamoDB should store the raw S3 key"

    # Returned value must be a presigned GET URL
    url = result["qrCodeUrl"]
    assert url.startswith("http"), f"Expected presigned URL to start with http, got {url!r}"
    assert new_key in url, f"Expected URL to contain the S3 key, got {url!r}"
    assert "Signature=" in url, f"Expected presigned URL to contain Signature=, got {url!r}"

    print("PASS: re-upload deleted the old S3 object and confirm returned a presigned GET URL")
    print("Demo complete.")


if __name__ == "__main__":
    demo()

Evidence: Demo output on target commit (passes)

{"timestamp": "2026-08-24T00:24:51.493219+00:00", "level": "INFO", "message": "Created payment method", "correlationId": "cd20effe-5d60-4175-93a8-d3202030950a", "account_id": "acc-123-456", "name": "Venmo"}
{"timestamp": "2026-08-24T00:24:51.506665+00:00", "level": "INFO", "message": "Deleted QR code from S3", "correlationId": "66be2259-4cfd-4705-a811-908a53d29738", "s3_key": "payment-qr-codes/acc-123-456/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.png"}
{"timestamp": "2026-08-24T00:24:51.509084+00:00", "level": "INFO", "message": "Deleted payment method", "correlationId": "c9f040a6-1c02-433d-b9ab-b0ddbc1890ea", "account_id": "acc-123-456", "name": "Venmo"}
PASS: deletion removed the stored UUID key and left the slug decoy intact
{"timestamp": "2026-08-24T00:24:51.568694+00:00", "level": "INFO", "message": "Created payment method", "correlationId": "bf630fd4-bc2a-4263-93d4-e8131a1932cb", "account_id": "acc-123-456", "name": "Venmo"}
{"timestamp": "2026-08-24T00:24:51.588479+00:00", "level": "INFO", "message": "Deleted QR code from S3", "correlationId": "983c1c7e-8c22-4795-9d8c-40026b366c4d", "s3_key": "payment-qr-codes/acc-123-456/bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb.png"}
{"timestamp": "2026-08-24T00:24:51.591765+00:00", "level": "INFO", "message": "Generated GET URL", "correlationId": "1aa64c86-6a40-43e5-ba4a-5578d304e209", "account_id": "acc-123-456", "payment_method": "Venmo", "s3_key": "payment-qr-codes/acc-123-456/cccccccccccccccccccccccccccccccc.png"}
{"timestamp": "2026-08-24T00:24:51.591858+00:00", "level": "INFO", "message": "Confirmed QR code upload", "correlationId": "0b8ec7e9-3060-468f-9f44-8dd013e97e8f", "account_id": "acc-123-456", "payment_method": "Venmo", "s3_key": "payment-qr-codes/acc-123-456/cccccccccccccccccccccccccccccccc.png"}
PASS: re-upload deleted the old S3 object and confirm returned a presigned GET URL
Demo complete.

Evidence: Demo output on base commit (fails on #137)

{"timestamp": "2026-08-24T00:24:57.998972+00:00", "level": "INFO", "message": "Created payment method", "correlationId": "151678c6-1eb4-4fc2-a774-e9683f9a2714", "account_id": "acc-123-456", "name": "Venmo"}
{"timestamp": "2026-08-24T00:24:58.012539+00:00", "level": "INFO", "message": "Deleted QR code from S3", "correlationId": "902edffb-a0ff-47b4-b076-d74534d456db", "account_id": "acc-123-456", "payment_method": "Venmo", "s3_key": "payment-qr-codes/acc-123-456/venmo.png"}
{"timestamp": "2026-08-24T00:24:58.013745+00:00", "level": "INFO", "message": "Deleted QR code from S3", "correlationId": "902edffb-a0ff-47b4-b076-d74534d456db", "account_id": "acc-123-456", "payment_method": "Venmo", "s3_key": "payment-qr-codes/acc-123-456/venmo.jpg"}
{"timestamp": "2026-08-24T00:24:58.014866+00:00", "level": "INFO", "message": "Deleted QR code from S3", "correlationId": "902edffb-a0ff-47b4-b076-d74534d456db", "account_id": "acc-123-456", "payment_method": "Venmo", "s3_key": "payment-qr-codes/acc-123-456/venmo.webp"}
{"timestamp": "2026-08-24T00:24:58.017549+00:00", "level": "INFO", "message": "Deleted payment method", "correlationId": "17fb6836-d1be-4757-8c0f-5e0f164188ae", "account_id": "acc-123-456", "name": "Venmo"}
Traceback (most recent call last):
  File "/tmp/no-mistakes-evidence/01M0RJ2E7BSP28GFDYE5H2D6HE/qr_s3_regression_demo.py", line 155, in <module>
    demo()
    ~~~~^^
  File "/home/dm/.no-mistakes/worktrees/d353adefa548/01M0RJ2E7BSP28GFDYE5H2D6HE/.venv/lib/python3.14/site-packages/moto/core/models.py", line 119, in wrapper
    result = func(*args, **kwargs)
  File "/tmp/no-mistakes-evidence/01M0RJ2E7BSP28GFDYE5H2D6HE/qr_s3_regression_demo.py", line 96, in demo
    assert_not_found(s3, uuid_key)
    ~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^
  File "/tmp/no-mistakes-evidence/01M0RJ2E7BSP28GFDYE5H2D6HE/qr_s3_regression_demo.py", line 45, in assert_not_found
    with pytest_raises(ClientError) as exc:
         ~~~~~~~~~~~~~^^^^^^^^^^^^^
  File "/home/dm/.no-mistakes/worktrees/d353adefa548/01M0RJ2E7BSP28GFDYE5H2D6HE/.venv/lib/python3.14/site-packages/_pytest/raises.py", line 710, in __exit__
    fail(f"DID NOT RAISE {self.expected_exceptions[0]!r}")
    ~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/dm/.no-mistakes/worktrees/d353adefa548/01M0RJ2E7BSP28GFDYE5H2D6HE/.venv/lib/python3.14/site-packages/_pytest/outcomes.py", line 163, in __call__
    raise Failed(msg=reason, pytrace=pytrace)
Failed: DID NOT RAISE <class 'botocore.exceptions.ClientError'>

Evidence: Focused pytest output

============================= test session starts ==============================
platform linux -- Python 3.14.7, pytest-9.0.3, pluggy-1.6.0 -- /home/dm/.no-mistakes/worktrees/d353adefa548/01M0RJ2E7BSP28GFDYE5H2D6HE/.venv/bin/python
cachedir: .pytest_cache
rootdir: /home/dm/.no-mistakes/worktrees/d353adefa548/01M0RJ2E7BSP28GFDYE5H2D6HE
configfile: pyproject.toml
plugins: asyncio-1.4.0, cov-7.1.0, playwright-0.8.0, base-url-2.1.0
asyncio: mode=Mode.STRICT, debug=False, asyncio_default_fixture_loop_scope=None, asyncio_default_test_loop_scope=function
collecting ... collected 12 items

tests/unit/test_payment_methods.py::TestDeletePaymentMethod::test_delete_method_with_uuid_qr_key PASSED [  8%]
tests/unit/test_payment_methods_handlers.py::TestConfirmQRUpload::test_confirm_upload_success PASSED [ 16%]
tests/unit/test_payment_methods_handlers.py::TestConfirmQRUpload::test_confirm_upload_deletes_replaced_qr_object PASSED [ 25%]
tests/unit/test_payment_methods_handlers.py::TestConfirmQRUpload::test_confirm_upload_same_key_keeps_object PASSED [ 33%]
tests/unit/test_payment_methods_handlers.py::TestConfirmQRUpload::test_confirm_upload_nonexistent_s3_object PASSED [ 41%]
tests/unit/test_payment_methods_handlers.py::TestConfirmQRUpload::test_confirm_upload_nonexistent_method PASSED [ 50%]
tests/unit/test_payment_methods_handlers.py::TestConfirmQRUpload::test_confirm_upload_account_not_exists PASSED [ 58%]
tests/unit/test_payment_methods_handlers.py::TestConfirmQRUpload::test_confirm_upload_unauthenticated PASSED [ 66%]
tests/unit/test_payment_methods_handlers.py::TestConfirmQRUpload::test_confirm_upload_empty_parameters PASSED [ 75%]
tests/unit/test_payment_methods_handlers.py::TestConfirmQRUpload::test_confirm_upload_wrong_account_s3_key PASSED [ 83%]
tests/unit/test_payment_methods_handlers.py::TestConfirmQRUpload::test_confirm_upload_malformed_s3_key PASSED [ 91%]
tests/unit/test_payment_methods_handlers.py::TestConfirmQRUpload::test_confirm_upload_detects_concurrent_modification PASSED [100%]

============================== 12 passed in 1.06s ==============================

Pipeline

Updates from git push no-mistakes

✅ **intent** - passed

✅ No issues found.

✅ **Rebase** - passed

✅ No issues found.

✅ **Review** - passed

✅ No issues found.

✅ **Test** - passed

✅ No issues found.

• uv run pytest tests/unit/test_payment_methods.py tests/unit/test_payment_methods_handlers.py -v --no-cov (136 tests passed)
• uv run python /tmp/no-mistakes-evidence/01M0RJ2E7BSP28GFDYE5H2D6HE/qr_s3_regression_demo.py on target commit (bcb1e741)
• git checkout 776cb419 then re-ran the same demo script on the base commit to verify it fails before the fix
• git checkout bcb1e741 restored the target commit after the base demo

✅ **Document** - passed

✅ No issues found.

✅ **Lint** - passed

✅ No issues found.

✅ **Push** - passed

✅ No issues found.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.