Attach invoice line items so Stripe does not finalize at $0 paid (#34)

## Summary
- Closes #33
- Create the draft invoice first, then attach the line item to that invoice id before finalize. Stripe's current API defaults `pending_invoice_items_behavior` to `exclude`, so the old pending-item-then-create-invoice order produced an empty invoice. Finalizing a $0 invoice auto-marks it paid.
- If the finalized total does not match `amount_cents`, void the Stripe invoice and do not store `paid`. Ignore `invoice.paid` webhooks whose Stripe total is $0 when the local amount is non-zero.

## Test plan
- [ ] Create a one-off invoice for $2040.00 for a test customer
- [ ] Confirm the local invoice status is Open and the amount is $2040.00
- [ ] Open the Stripe hosted invoice and confirm the total is $2040.00 and status is Open, not Paid
- [ ] Confirm a real `invoice.paid` webhook still marks a non-zero invoice paid after the customer pays

Reviewed-on: #34

company_site/financial/stripe_billing.py

@@ -146,25 +146,23 @@ def ensure_stripe_customer(customer: BillingCustomer) -> str:
 
 
 def create_one_off_invoice(invoice: Invoice) -> Invoice:
-    """Create + finalize a Stripe Invoice; store hosted pay URL."""
+    """Create + finalize a Stripe Invoice; store hosted pay URL.
-    stripe_customer_id = ensure_stripe_customer(invoice.customer)
 
-    _stripe_request(
+    The line item is attached to the draft invoice. Stripe's current
-        "POST",
+    Invoices API defaults ``pending_invoice_items_behavior`` to
-        "/invoiceitems",
+    ``exclude``, so a pending item created beforehand is left off the
-        {
+    invoice. Finalizing that empty invoice yields a $0 total, and Stripe
-            "customer": stripe_customer_id,
+    marks any $0 invoice paid.
-            "amount": invoice.amount_cents,
+    """
-            "currency": invoice.currency,
+    stripe_customer_id = ensure_stripe_customer(invoice.customer)
-            "description": invoice.description,
-        },
-    )
 
     create_data: dict = {
         "customer": stripe_customer_id,
         "collection_method": "send_invoice",
-        "auto_advance": True,
+        # Finalize ourselves after the line item is attached. auto_advance
-        "metadata": {"local_invoice_id": str(invoice.pk)},
+        # would let Stripe finalize the still-empty draft on its own.
+        "auto_advance": False,
+        "pending_invoice_items_behavior": "exclude",
     }
     if invoice.due_date:
         due_dt = datetime.combine(invoice.due_date, datetime.min.time())
@@ -175,11 +173,40 @@ def create_one_off_invoice(invoice: Invoice) -> Invoice:
         create_data["days_until_due"] = 30
 
     stripe_invoice = _stripe_request("POST", "/invoices", create_data)
-    stripe_invoice = _stripe_request(
+    invoice_id = stripe_invoice["id"]
-        "POST", f"/invoices/{stripe_invoice['id']}/finalize", {}
+
+    _stripe_request(
+        "POST",
+        "/invoiceitems",
+        {
+            "customer": stripe_customer_id,
+            "invoice": invoice_id,
+            "amount": invoice.amount_cents,
+            "currency": invoice.currency,
+            "description": invoice.description,
+        },
     )
 
-    invoice.stripe_invoice_id = stripe_invoice["id"]
+    stripe_invoice = _stripe_request(
+        "POST", f"/invoices/{invoice_id}/finalize", {}
+    )
+
+    total = stripe_invoice.get("total")
+    if total != invoice.amount_cents:
+        _void_mismatched_invoice(invoice_id)
+        raise StripeAPIError(
+            f"Stripe invoice total was {total} cents, expected {invoice.amount_cents} cents."
+        )
+
+    # Metadata is set only after the total checks out, so an invoice.paid
+    # webhook for a $0 finalize cannot match this local row.
+    _stripe_request(
+        "POST",
+        f"/invoices/{invoice_id}",
+        {"metadata": {"local_invoice_id": str(invoice.pk)}},
+    )
+
+    invoice.stripe_invoice_id = invoice_id
     invoice.hosted_invoice_url = stripe_invoice.get("hosted_invoice_url") or ""
     invoice.invoice_pdf_url = stripe_invoice.get("invoice_pdf") or ""
     status = stripe_invoice.get("status") or Invoice.Status.OPEN
@@ -198,6 +225,16 @@ def create_one_off_invoice(invoice: Invoice) -> Invoice:
     return invoice
 
 
+def _void_mismatched_invoice(invoice_id: str) -> None:
+    """Void a finalized invoice whose total does not match the local amount."""
+    try:
+        _stripe_request("POST", f"/invoices/{invoice_id}/void", {})
+    except StripeAPIError:
+        logger.exception(
+            "Failed to void Stripe invoice %s after amount mismatch", invoice_id
+        )
+
+
 def create_subscription_checkout(
     subscription: RecurringSubscription,
     *,
@@ -314,7 +351,19 @@ def apply_stripe_invoice_event(stripe_invoice: dict) -> Invoice | None:
         return None
 
     status = stripe_invoice.get("status") or invoice.status
-    if status in dict(Invoice.Status.choices):
+    # A $0 Stripe invoice is auto-marked paid on finalize. Never copy that
+    # onto a local invoice that was supposed to charge money.
+    if (
+        status == Invoice.Status.PAID
+        and invoice.amount_cents > 0
+        and stripe_invoice.get("total") == 0
+    ):
+        logger.error(
+            "Refusing paid status for invoice %s: Stripe total is 0, local amount is %s cents",
+            invoice.pk,
+            invoice.amount_cents,
+        )
+    elif status in dict(Invoice.Status.choices):
         invoice.status = status
     invoice.hosted_invoice_url = (
         stripe_invoice.get("hosted_invoice_url") or invoice.hosted_invoice_url

company_site/financial/tests.py

@@ -1,5 +1,7 @@
+from unittest.mock import patch
+
 from django.contrib.auth.models import User
-from django.test import Client, TestCase
+from django.test import Client, TestCase, override_settings
 from django.urls import reverse
 
 from financial.models import (
@@ -216,3 +218,134 @@ class InvoiceAccessTests(TestCase):
         self.assertEqual(invoice.status, Invoice.Status.PAID)
         self.assertEqual(invoice.hosted_invoice_url, "https://pay.stripe.com/test")
 
+    def test_apply_invoice_webhook_ignores_zero_dollar_paid(self):
+        from financial.models import BillingCustomer, Invoice
+        from financial.stripe_billing import apply_stripe_invoice_event
+
+        customer = BillingCustomer.objects.create(
+            name="Acme", email="acme-zero@example.com"
+        )
+        invoice = Invoice.objects.create(
+            customer=customer,
+            description="Work",
+            amount_cents=204000,
+            stripe_invoice_id="in_zero",
+            status=Invoice.Status.DRAFT,
+        )
+        apply_stripe_invoice_event(
+            {
+                "id": "in_zero",
+                "status": "paid",
+                "total": 0,
+                "metadata": {"local_invoice_id": str(invoice.pk)},
+            }
+        )
+        invoice.refresh_from_db()
+        self.assertEqual(invoice.status, Invoice.Status.DRAFT)
+
+
+class CreateOneOffInvoiceTests(TestCase):
+    def setUp(self):
+        from financial.models import BillingCustomer, Invoice
+
+        self.customer = BillingCustomer.objects.create(
+            name="Acme", email="billing@example.com"
+        )
+        self.invoice = Invoice.objects.create(
+            customer=self.customer,
+            description="Site build",
+            amount_cents=204000,
+            status=Invoice.Status.DRAFT,
+        )
+
+    @override_settings(STRIPE_SECRET_KEY="sk_test_x")
+    @patch("financial.stripe_billing._stripe_request")
+    def test_line_item_attached_to_draft_before_finalize(self, stripe_request):
+        from financial.models import Invoice
+        from financial.stripe_billing import create_one_off_invoice
+
+        def side_effect(method, path, data=None):
+            if path == "/customers":
+                return {"id": "cus_123"}
+            if path == "/invoices":
+                return {"id": "in_123", "status": "draft"}
+            if path == "/invoiceitems":
+                return {"id": "ii_123"}
+            if path == "/invoices/in_123/finalize":
+                return {
+                    "id": "in_123",
+                    "status": "open",
+                    "total": 204000,
+                    "amount_due": 204000,
+                    "hosted_invoice_url": "https://pay.stripe.com/i/in_123",
+                    "invoice_pdf": "https://pay.stripe.com/i/in_123/pdf",
+                }
+            if path == "/invoices/in_123":
+                return {"id": "in_123"}
+            raise AssertionError(f"unexpected {method} {path}")
+
+        stripe_request.side_effect = side_effect
+        create_one_off_invoice(self.invoice)
+
+        paths = [call.args[1] for call in stripe_request.call_args_list]
+        self.assertEqual(
+            paths,
+            [
+                "/customers",
+                "/invoices",
+                "/invoiceitems",
+                "/invoices/in_123/finalize",
+                "/invoices/in_123",
+            ],
+        )
+        invoice_create = stripe_request.call_args_list[1].args[2]
+        self.assertEqual(invoice_create["pending_invoice_items_behavior"], "exclude")
+        self.assertNotIn("metadata", invoice_create)
+        item = stripe_request.call_args_list[2].args[2]
+        self.assertEqual(item["invoice"], "in_123")
+        self.assertEqual(item["amount"], 204000)
+
+        self.invoice.refresh_from_db()
+        self.assertEqual(self.invoice.status, Invoice.Status.OPEN)
+        self.assertEqual(self.invoice.amount_cents, 204000)
+        self.assertEqual(self.invoice.stripe_invoice_id, "in_123")
+        self.assertEqual(
+            self.invoice.hosted_invoice_url, "https://pay.stripe.com/i/in_123"
+        )
+
+    @override_settings(STRIPE_SECRET_KEY="sk_test_x")
+    @patch("financial.stripe_billing._stripe_request")
+    def test_zero_total_finalize_does_not_mark_paid(self, stripe_request):
+        from financial.models import Invoice
+        from financial.stripe_billing import StripeAPIError, create_one_off_invoice
+
+        def side_effect(method, path, data=None):
+            if path == "/customers":
+                return {"id": "cus_123"}
+            if path == "/invoices":
+                return {"id": "in_zero", "status": "draft"}
+            if path == "/invoiceitems":
+                return {"id": "ii_123"}
+            if path == "/invoices/in_zero/finalize":
+                return {
+                    "id": "in_zero",
+                    "status": "paid",
+                    "total": 0,
+                    "amount_due": 0,
+                }
+            if path == "/invoices/in_zero/void":
+                return {"id": "in_zero", "status": "void"}
+            raise AssertionError(f"unexpected {method} {path}")
+
+        stripe_request.side_effect = side_effect
+        with self.assertRaises(StripeAPIError):
+            create_one_off_invoice(self.invoice)
+
+        paths = [call.args[1] for call in stripe_request.call_args_list]
+        self.assertIn("/invoices/in_zero/void", paths)
+        self.assertNotIn("/invoices/in_zero", paths)
+        self.invoice.refresh_from_db()
+        self.assertEqual(self.invoice.status, Invoice.Status.DRAFT)
+        self.assertEqual(self.invoice.stripe_invoice_id, "")
+        self.assertEqual(self.invoice.amount_cents, 204000)
+