Basket discounts: what the API actually accepts, and error responses that survive an unexpected body

AGENTS.md

@@ -48,6 +48,9 @@ had been believed:
 | `expiresAt` of the installment plans is a unix timestamp in seconds (`1735689599`, per the API reference) | It is **milliseconds** (13 digits). Reading it as seconds lands in the year 58608 and raises |
 | The installment plans response is documented with seconds throughout | Mixed: `expiresAt` in ms, transaction dates as `YYYY-MM-DD HH:MM:SS` strings, rate dates as `YYYY-MM-DD` |
 | One response uses one type per boolean | `card3ds` is a bool while `billingAddressRequired` is the string `"false"` — same response |
+| A discount can be sent as its own negative basket item (a `voucher` line) | Both schemas reject negative item amounts — `API.600.200.131`, plus `API.600.410.018` on v1. A discount belongs in `amountDiscount` (v1) or `amountDiscountPerUnitGross` (v3), positive |
+| The basket endpoint checks its own arithmetic | Only v3 does, to the cent (`API.600.410.062`). v1 accepts items that contradict `amountTotalGross`, and a charge does not compare the basket to the payment amount either — measured with Prepayment: amounts of 817.02, 726.24, 907.80 and 1.00 are all accepted against the same basket worth 817.02 |
+| Any `returnUrl` the API accepts is fine for local development | A gateway in front of the API refuses `localhost`, `127.0.0.1` and private IPs with a **403 and an nginx HTML page** — the API never sees the request. Hostnames that merely *resolve* to 127.0.0.1 (`lvh.me`, `localtest.me`, `127-0-0-1.nip.io`) pass, so the block is on the string, not the resolved address |
 
 ### How to verify
 
@@ -182,25 +185,84 @@ from its source, the docstring says so and why — `PaymentTypes` drops the `UNK
 the Java SDK uses as a parsing fallback. Deviating silently is how `Action` came to be missing
 a type in the first place.
 
-**Amounts are rounded to four decimals on serialisation** (`unzer.utils.roundAmount`). The API
-takes `Decimal{10,4}`, and float arithmetic does not cooperate: `12.3 - 10.0 - 2.3` is
-`8.88e-16`, which `json.dumps` writes in scientific notation — not a number the API accepts.
-Amounts come back from the API as *strings* with four decimals (`"5.5500"`).
+**Basket-level amounts are rounded to four decimals on serialisation**
+(`unzer.utils.roundAmount`). The API takes `Decimal{10,4}`, and float arithmetic does not
+cooperate: `12.3 - 10.0 - 2.3` is `8.88e-16`, which `json.dumps` writes in scientific
+notation — not a number the API accepts. Amounts come back from the API as *strings* with
+four decimals (`"5.5500"`).
+
+`BasketItem.serialize` does **not** do this — measured, an item goes out as
+`0.30000000000000004` next to a basket total rounded to `0.3`. That is an inconsistency,
+not a design decision (#18), and it matters most where v3 reconciles the total against the
+items to the cent.
 
 **`Basket` intentionally supports two incompatible schemas.** v1 uses `amountTotalGross`, v3
 uses `totalValueGross`; setting the latter switches the basket to the v3 endpoint. Do not
 "clean this up" into one schema.
 
 Which version a payment method needs is a recurring source of confusion, and the
-documentation is not reliable here:
+documentation is not reliable here. Measured, the answer so far is that none of them
+*needs* a particular one — what the column records is which schemas were seen to work,
+not a requirement:
 
 | Method | Verified in practice | What the docs claim |
 |---|---|---|
-| `paylater-installment` | v3 | v3 |
-| `klarna` | **v1 works** — in production use via viur-shop | v2 |
+| `paylater-installment` | **both** — sandbox authorize succeeds on v1 and on v3 | v3 |
+| `klarna` | **both** — v1 in production use via viur-shop, v3 measured | v2 |
 | `paylater-invoice` | not verified | v2 |
 | everything else | v1 | v1 |
 
+**Do not mix the schemas within one basket** — and note that only one direction fails
+loudly. Measured: a v3 item in a v1 basket is accepted with a 201 and every item amount
+stored as `0.0000`, while the basket-level total survives, so the basket looks plausible
+and every line is worth nothing. A v1 item in a v3 basket is refused with
+`API.600.410.051`. `Basket.isV3()` and `BasketItem.isV3()` decide independently and
+nothing checks that they agree (#16). A basket is also only readable through the schema it
+was created with — `API.600.410.024` otherwise — and the ids differ visibly: v1 gives
+`s-bsk-72`, v3 a UUID.
+
+**A basket can only be used once.** A second charge against the same `basketId` is refused with `API.330.200.152 "Resources: basket was used."`, so a retry after a failed authorize needs a new basket.
+
+**Discounts belong in a discount field, not in a negative line item.** The obvious
+shape — one item per article plus a `voucher` item carrying the negative discount, the
+grosses adding up to the order total — is refused by both schemas
+(`API.600.200.131 "Amount … has to be positive"`, and `API.600.410.018` on v1). Send the
+discount as a positive value instead:
+
+| | v1 | v3 |
+|---|---|---|
+| Field | `amountDiscount` — per line or per unit is **undecidable, and so far inconsequential**: nothing measured reads the value back out (see below) | `amountDiscountPerUnitGross`, **per unit** |
+| Item amount | `amountGross` stays the pre-discount gross; the API stores both untouched | `amountPerUnitGross` minus the discount must stay positive |
+| Total | not checked at all | `totalValueGross == sum((amountPerUnitGross - amountDiscountPerUnitGross) * quantity)`, exact to the cent (`API.600.410.062`) |
+| `vat` per item | optional | mandatory (`API.600.410.052`) |
+
+So a discount that exceeds a single line has to be spread over several items in v3,
+while v1 swallows it. v3 names the offending line in `API.600.410.064` ("Basket item i1
+'amountDiscountPerUnitGross' does not equal to 'amountPerUnitGross'") — but only if the
+basket total stays positive; otherwise the negative total is refused first with the
+generic `API.600.200.131`, which says nothing about the item. The v1 tolerance is not a
+licence: a charge does not compare the basket to the payment amount, but the methods
+that forward the basket to a partner system may. `tests/sandbox/test_live_api.py::TestBasket` holds all of this as executable evidence,
+except the v1 per-line/per-unit question above.
+
+**Nothing measured consumes `amountDiscount`.** The question was chased down the whole
+chain with an item of `quantity=3`, gross 300.00 and `amountDiscount=10.00`, where the
+two readings differ by 20.00:
+
+* the v1 basket endpoint stores it and reconciles nothing;
+* a charge does not compare the basket to the amount at all;
+* Klarna's checkout lists the item by title only and takes its total from the authorize
+  `amount` — 290.00 and 270.00 were both accepted and both displayed as sent;
+* Unzer's Hosted Payment Page renders the line as `3x T-Shirt € 300,00`, i.e. plain
+  `amountGross` with the discount nowhere, and its total likewise comes from the request,
+  not from the basket.
+
+So `amountDiscount` is stored and forwarded but not evaluated by anything reachable from
+here, and the per-line/per-unit distinction has no observable effect. Worth knowing for
+two reasons: the value cannot be used to make a basket "add up" for the customer, and a
+line reduced by a discount is shown at its **undiscounted** gross next to a lower total,
+with no label explaining the difference.
+
 Basket v2 is not implemented (`Basket.apiVersion` only returns `v1` or `v3`), and so far
 nothing has needed it. v2 and v3 share one schema, so if a method ever does require v2, the v3
 model can build the body — only the endpoint version differs.
@@ -304,5 +366,9 @@ minimal dicts.
   affects parsing.
 - Errors can arrive with a 2xx status code and an `errors` list in the body — Unzer documents
   this explicitly. Do not treat HTTP 200 as success without checking `isError`.
+- Not every 4xx body is JSON. The gateway in front of the API answers with an HTML page,
+  so `_request` builds an `ErrorResponse` from the status and the raw text instead of
+  parsing it — a bare `JSONDecodeError` from inside the SDK hides both and reads like an
+  SDK bug. The known trigger is a `returnUrl` on localhost or a private IP.
 - `logger.debug` output contains full request payloads, including IBANs and dates of birth.
   Do not add payload logging above DEBUG level.

src/unzer/client.py

@@ -204,7 +204,29 @@ def _request(self, url: str, method: str,
                 continue
             logger.debug("Client error")
             logger.debug("Response[%s %s]: %r", r.status_code, r.reason, r.text)
-            errorResponse = ErrorResponse.fromDict(r.json())
+            # Not every 4xx comes from the API itself, and a body the SDK cannot map
+            # onto its error model used to escape as a bare exception from inside
+            # `fromDict` -- hiding the status code and the body, so the caller could
+            # not tell a refused request from an SDK bug. The two causes are kept
+            # apart because they need different things done about them.
+            try:
+                data = r.json()
+            except ValueError:
+                # Measured: a gateway in front of the API refuses a returnUrl on
+                # localhost or a private IP with a 403 and an nginx HTML page.
+                errorResponse = ErrorResponse(
+                    f"HTTP {r.status_code} {r.reason} with a non-JSON body: {r.text[:200]!r}")
+            else:
+                try:
+                    errorResponse = ErrorResponse.fromDict(data)
+                except (KeyError, TypeError, ValueError):
+                    # Valid JSON, but not this API's error envelope -- the shape an
+                    # API gateway or WAF sends (`{"message": "Forbidden"}`), and the
+                    # shape `fromDict` produces for a timestamp it cannot parse.
+                    logger.exception(f"Cannot read {data!r} as an ErrorResponse")
+                    errorResponse = ErrorResponse(
+                        f"HTTP {r.status_code} {r.reason} with a body the error schema "
+                        f"does not fit: {r.text[:200]!r}")
             errorResponse.statusCode = r.status_code
             errorResponse.srcResponse = r
             raise errorResponse

src/unzer/model/basket.py

@@ -13,9 +13,17 @@ class Basket(BaseModel):
     with gross amounts, otherwise the v1 endpoint with :attr:`amountTotalGross` is used.
     The basket items follow the same rule on their own
     (see :class:`unzer.model.BasketItem`), so don't mix the schemas within one basket.
+    Measured: a v3 item in a v1 basket is accepted with a 201 and every item amount
+    stored as ``0.0000``, silently, while a v1 item in a v3 basket is refused with
+    ``API.600.410.051``. A basket is also only readable through the schema it was
+    created with, ``API.600.410.024`` otherwise.
 
-    The Pay later payment methods (e.g. :class:`unzer.model.PaylaterInstallment`)
-    require the v3 schema.
+    Both schemas are accepted by every payment method measured so far -- a sandbox
+    authorize with Klarna and with :class:`unzer.model.PaylaterInstallment` succeeds
+    on either. The documentation claims the Pay later methods require v3; the API does
+    not enforce it. What does differ is validation: v3 reconciles
+    :attr:`totalValueGross` against the items to the cent (``API.600.410.062``),
+    v1 checks nothing at all.
 
     Note that the v2 and v3 endpoints share the same schema, so the newer v3 is used here.
     """

src/unzer/model/basketItem.py

@@ -47,7 +47,8 @@ def __init__(
         :param amountDiscount: (optional) (v1) Discount amount for the basket item
             (multiplied by the :attr:`quantity`) format: float
         :type amountDiscount: float
-        :param vat: (optional) Integer Vat value for the basket item in percent (0-100) format: int32
+        :param vat: (optional in v1, mandatory in v3 -- ``API.600.410.052``) Integer
+            Vat value for the basket item in percent (0-100) format: int32
         :type vat: int
         :param amountGross: (optional) (v1) Gross amount (= amountNet + amountVat) in the specified currency.
             Equals amountNet if vat value is 0 format: float

src/unzer/model/error.py

@@ -1,5 +1,8 @@
-import datetime
 import logging
+import typing as t
+from datetime import datetime as dt
+
+from ..utils import parseDateTime
 
 logger = logging.getLogger("unzer-sdk").getChild(__name__)
 
@@ -14,7 +17,13 @@ class Error:
     .. seealso:: https://docs.unzer.com/server-side-integration/api-basics/error-handling/
     """
 
-    def __init__(self, code, merchantMessage, customerMessage, **kwargs):
+    def __init__(
+            self,
+            code: str | None = None,
+            merchantMessage: str | None = None,
+            customerMessage: str | None = None,
+            **kwargs: t.Any,
+    ) -> None:
         self.code = code
         self.merchantMessage = merchantMessage
         self.customerMessage = customerMessage
@@ -77,19 +86,51 @@ def __init__(
             logger.warning("ErrorResponse got additional unhandled data: %r", kwargs)
 
     @classmethod
-    def fromDict(cls, data, message="Unzer Error"):
+    def fromDict(cls, data: t.Any, message: str = "Unzer Error") -> "ErrorResponse":
+        """Build an ErrorResponse from a decoded API error body.
+
+        Only ``errors`` is treated as required, because it is what identifies the
+        body as this API's error envelope and it is the one part a caller acts on --
+        `UnzerClient.createOrUpdateCustomer` branches on ``errors[0].code``. Anything
+        else missing or unreadable costs that field alone, never the list: an error
+        that cannot be reported is worse than one reported without its timestamp.
+
+        :param data: The decoded body.
+        :param message: The exception message.
+        :raises ValueError: If ``data`` is not an error envelope, so the caller can
+            tell "the API refused this" from "something else answered".
+        :return: The error response.
+        """
+        if not isinstance(data, dict) or "errors" not in data:
+            raise ValueError(f"Not an API error envelope: {data!r}")
         return cls(
             message,
-            timestamp=datetime.datetime.strptime(data["timestamp"], "%Y-%m-%d %H:%M:%S"),
-            url=data["url"],
-            errors=[Error(**error) for error in data["errors"]],
+            timestamp=cls._parseTimestamp(data.get("timestamp")),
+            url=data.get("url"),
+            errors=[Error(**error) for error in data.get("errors") or []],
             errorId=data.get("id"),
             traceId=data.get("traceId"),
             isError=data.get("isError"),
             isPending=data.get("isPending"),
             isSuccess=data.get("isSuccess"),
         )
 
+    @staticmethod
+    def _parseTimestamp(value: str | dt | None) -> dt | None:
+        """Read the error timestamp, tolerating a format the SDK does not know.
+
+        The API is known to use two formats and has been seen with others; losing the
+        error codes over the one field nobody branches on is not a trade worth making.
+
+        :param value: The raw ``timestamp`` value.
+        :return: The parsed timestamp, or ``None`` if it cannot be read.
+        """
+        try:
+            return parseDateTime(value)
+        except (TypeError, ValueError):
+            logger.warning(f"Cannot parse the error timestamp {value!r}")
+            return None
+
     def __repr__(self):
         return (
             f"{self.__class__.__module__}.{self.__class__.__name__}("