Skip to content

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

Merged
sveneberth merged 12 commits into
developfrom
fix/basket-discounts-and-non-json-errors
Sep 2, 2026
Merged

Basket discounts: what the API actually accepts, and error responses that survive an unexpected body#13
sveneberth merged 12 commits into
developfrom
fix/basket-discounts-and-non-json-errors

Conversation

@sveneberth

@sveneberth sveneberth commented Sep 1, 2026

Copy link
Copy Markdown
Member

Three findings from tracking down a failing Klarna checkout, all measured against the
sandbox.

How a discount has to reach a basket

Neither schema accepts a basket item with a negative amount — so a discount cannot be
sent as its own negative voucher line, which is the shape a consumer arrives at
naturally (one item per article, one for the voucher, the grosses adding up to the
order total). It belongs in a positive discount field of the item it reduces, and the
two schemas verify very different amounts of it:

v1 v3
negative item amounts API.600.200.131 + API.600.410.018 API.600.200.131
discount field amountDiscount (per line or per unit is unverified, see below) amountDiscountPerUnitGross, per unit
item amount amountGross stays the pre-discount gross amountPerUnitGross minus the discount must stay positive
total not checked at all exact to the cent, API.600.410.062
vat per item optional, stored as 0 mandatory, API.600.410.052
discount above the item accepted refused, API.600.410.064

TestBasket grows from 2 to 13 tests, one per row plus the per-unit semantics of the
v3 discount. AGENTS.md carries the same as prose, with the v1 tolerance marked as a
warning rather than a licence: a charge does not compare the basket to the payment
amount either, but a method that hands the basket to a partner system may well be
stricter.

Two things about the last row are worth stating precisely, because a first version of
it was wrong. The per-item cap only shows itself when the basket total stays positive:
with a single over-discounted line the total goes negative too and the API answers the
generic API.600.200.131 "Amount has to be positive", which the negative total alone
explains. With a second, larger item it names the line instead — API.600.410.064,
"Basket item i1 'amountDiscountPerUnitGross' does not equal to 'amountPerUnitGross'".
And the charge-does-not-check-the-basket claim is now backed by Prepayment rather than
iDEAL, which this repository states cannot be driven server-side: against one basket
worth 817.02, charges of 817.02, 726.24, 907.80 and 1.00 were all accepted and
booked at face value.

The two existing basket tests are fixed along the way: they passed type="goods", but
the parameter is kind. BaseModel.__init__ swallows unknown kwargs, so "type": ""
went out and no item type was ever exercised.

Error responses that survive an unexpected body

errorResponse = ErrorResponse.fromDict(r.json())   # ← raised JSONDecodeError

Not every 4xx comes from the API. A gateway in front of it answers with an HTML page,
and parsing that as JSON raised a bare JSONDecodeError from inside the SDK — losing
the status code and the body, and looking like an SDK bug rather than a refused
request.

Guarding that turned out to need three steps rather than one, because the first
attempt was both too narrow and too wide:

  • Too narrow. fromDict indexed timestamp, url and errors, so a 4xx whose
    body is JSON but is not this API's envelope raised KeyError/TypeError — not a
    ValueError. Measured, these all still escaped _request bare: {"message": "Forbidden"} (the canonical API-gateway body), null, [], "Forbidden".
  • Too wide. fromDict parsed the timestamp with one hard-coded format while the
    API is known to use two, and that ValueError landed in the same handler. A
    well-formed API error with a different format was reported as "non-JSON body" —
    while quoting JSON — and arrived with an empty errors list.

The second is the one that mattered: UnzerClient.createOrUpdateCustomer falls back to
updateCustomer on errors[0].code == "API.410.200.010", so an emptied list turns a
recoverable checkout into a failed one, silently. That path is exercised routinely in
production.

So _request now guards only r.json() and reports a body that decodes but does not
fit the schema separately, and ErrorResponse.fromDict treats only errors as
required — it is what identifies the body as an error envelope. Everything else
missing or unreadable costs that field alone: the timestamp goes through
utils.parseDateTime and falls back to None with a warning, and Error gained
defaults so an entry missing merchantMessage keeps its code. A body that is not an
envelope still raises, so callers keep the distinction between "the API refused this"
and "something else answered".

Measured across 2026-08-21 10:15:32, 21.08.2026 10:15:32, 2026-08-21T10:15:32,
2026-08-21 10:15:32.123 and a missing timestamp: the codes arrive in all five and the
createOrUpdateCustomer fallback fires again. Closes #14.

The trigger that surfaced all this is worth knowing for anyone developing locally:

returnUrl Result
http://localhost:8080/… 403, nginx HTML
http://127.0.0.1:8080/…, https://localhost:8080/… 403, nginx HTML
http://192.168.1.5:8080/… 403, nginx HTML
https://example.com/… API answers
http://lvh.me:8080/…, http://localtest.me:8080/…, http://127-0-0-1.nip.io:8080/… API answers

The gateway refuses localhost and private IP literals before the API sees the request.
Hostnames that merely resolve to 127.0.0.1 pass, so the check is on the string. A
rate limit is ruled out: localhost still gets a 403 right after a successful call
with a public host.

Documentation claims corrected by measurement

Three assertions in the docstrings and in AGENTS.md did not survive being checked:

  • The Pay later methods do not require v3. A sandbox authorize with
    PaylaterInstallment and with Klarna succeeds on a v1 basket carrying
    amountDiscount exactly as it does on v3, booking the requested amount unchanged.
    The Basket docstring and the schema table both claimed otherwise.
  • vat is mandatory in v3, not "optional" as BasketItem said.
  • Mixing the schemas fails silently in one direction. A v3 item in a v1 basket is
    accepted with a 201 and every item amount stored as 0.0000; the reverse is refused
    loudly with API.600.410.051. A basket is also only readable through the schema it
    was created with (API.600.410.024), and single use — API.330.200.152 on a second
    charge, so a retry after a failed authorize needs a new one. The silent-zeroing case
    is filed as A v3 item in a v1 basket is accepted and silently stored as 0.0000 #16, since enforcing it belongs in Basket.serialize.

The v1 amountDiscount semantics are now labelled unverified rather than asserted:
the table said per line, basketItem.py:47 says per unit, every v1 discount test uses
quantity=1 where the two are indistinguishable, and v1 reconciles nothing — so the
basket endpoint cannot settle it. Only the rendered basket or the partner system can.

Verification

  • pytest -m sandbox tests/sandbox/test_live_api.py::TestBasket — 13 passed against
    the real sandbox.
  • pytest — 291 passed, including the new mocked cases for the HTML body, the five
    foreign-JSON shapes, the five timestamp formats and the incomplete error entry.
  • pycodestyle --diff — clean.
  • One pre-existing sandbox test is red and untouched by this branch (verified by
    stashing): TestPaymentPage::test_redirect_url_points_at_the_sandbox expects sbx-
    in the redirect URL, the API now answers payment.test.unzer.com.

Filed rather than fixed here

#15 (the remaining _request branches: retry-exhausted statusCode == 0, the
unguarded and doubled r.json() on 2xx, <= 201 sending a 204 into the error branch,
r not reset after a transport failure), #16 (schema mix), #17 (BaseModel swallows
unknown kwargs — examples/03_installment_plans.py:57 still carries the type= bug
this branch fixed in the tests), #18 (BasketItem.serialize does not round its
amounts while Basket.serialize does).

Note on the target branch

Against develop, not main, although the commits are patch level: main carries
neither tests/ nor AGENTS.md yet — #8 through #12 are still only on develop, so
the files this branch changes do not exist there.

…hemas

Neither schema accepts a basket item with a negative amount, so a discount
cannot be sent as its own negative `voucher` line — the shape a consumer
arrives at naturally. It belongs in a positive discount field of the item it
reduces, and the two schemas differ in how much they verify:

| | v1 | v3 |
|---|---|---|
| negative item amounts | `API.600.200.131` + `API.600.410.018` | `API.600.200.131` |
| discount field | `amountDiscount`, per line | `amountDiscountPerUnitGross`, per unit |
| total | not checked at all | exact to the cent, `API.600.410.062` |
| `vat` per item | optional, stored as 0 | mandatory, `API.600.410.052` |
| discount above the item | accepted | refused, the item may not go negative |

`TestBasket` grows from 2 to 13 tests, one per row plus the per-unit semantics
of the v3 discount. All of it measured against the sandbox, none of it derived
from the documentation. `AGENTS.md` gets the same as prose.

Also fixes the two existing basket tests: they passed `type="goods"`, but the
parameter is `kind` — `BaseModel.__init__` swallows unknown kwargs, so `"type":
""` went out and no item type was ever exercised. A read-back assertion now
pins it down.
Not every 4xx comes from the API. A gateway in front of it answers with an HTML
error page, and `ErrorResponse.fromDict(r.json())` then raised a bare
`JSONDecodeError` from inside the SDK — losing the status code and the body, and
looking like an SDK bug rather than a refused request. The retry path already
guarded against this; the client-error path did not.

`_request` now falls back to an `ErrorResponse` carrying the status, the reason
and the start of the raw body, so `statusCode` and `srcResponse` stay usable.

Measured trigger, and the reason this surfaced: the gateway refuses any
`returnUrl` pointing at `localhost`, `127.0.0.1` or a private IP with a 403 and
an nginx page — which every local development server produces. Hostnames that
merely resolve to 127.0.0.1 pass, so the check is on the string.
@sveneberth sveneberth added bug Something isn't working documentation Improvements or additions to documentation labels Sep 1, 2026
@sveneberth

Copy link
Copy Markdown
Member Author

Review notes

Everything below was reproduced against this branch's src/ (mocked with responses), not derived from the docs. Where I could not measure something, it says so.

The direction is right and the basket tests are genuinely valuable. My concern is that the error-handling fix is mis-sized in both directions, and that the one branch it hardens is not the one that sees the most non-JSON bodies.

1. except ValueError is too wide — it silently eats real API errors

ErrorResponse.fromDict parses the timestamp with datetime.strptime(data["timestamp"], "%Y-%m-%d %H:%M:%S") (model/error.py:83), which raises ValueError too. Measured, with a well-formed error body that only differs in the timestamp format:

timestamp result
2026-08-21 10:15:32 ErrorResponse, 1 error — correct
21.08.2026 10:15:32 ErrorResponse, 0 errors, "…with a non-JSON body: '{\"id\": \"s-err-1\"…'"
2026-08-21T10:15:32 same
2026-08-21 10:15:32.123 same

So the message claims "non-JSON body" while quoting JSON, and errors is dropped. That is load-bearing: createOrUpdateCustomer (client.py:337) guards on if er.errors and er.statusCode == 400 and er.errors[0].code == "API.410.200.010" — with an empty list it re-raises instead of falling back to updateCustomer.

Note the SDK already owns a two-format parser, utils.parseDateTime, and payment.py:443, payment.py:723 and risk_check.py:52 all use it for the same field. ErrorResponse.fromDict is the only place that hard-codes one format, and it is now the one place whose failure is hidden.

Suggested: guard only the decode.

try:
    data = r.json()
except ValueError:
    errorResponse = ErrorResponse(...)
else:
    errorResponse = ErrorResponse.fromDict(data)

2. …and too narrow — a JSON body in a foreign shape still escapes

fromDict hard-indexes data["timestamp"], data["url"], data["errors"]. KeyError/TypeError are not ValueError, so they still leave _request bare — the exact failure class the commit message describes. Measured on a 403:

body escapes as
{"message": "Forbidden"} KeyError: 'timestamp'
null TypeError: 'NoneType' object is not subscriptable
[] TypeError: list indices must be integers…
"Forbidden" TypeError: string indices must be integers
valid envelope, error entry {"code": "X"} TypeError: Error.__init__() missing 2 required positional arguments

{"message": "Forbidden"} is the canonical AWS API Gateway 403; Cloudflare and Kong emit comparable shapes. The gateway in front of the API answers HTML today — nothing pins it there.

3. The retry-exhausted branch has the same defect, untouched

client.py:222-232 parses r.json() under its own except ValueError, then falls through to raise ErrorResponse("All request attempts failed", srcResponse=r) — which never sets statusCode. Measured with a 502 carrying an nginx page: statusCode == 0, no body, plus a full JSONDecodeError traceback at ERROR level. retryableMethods = ("GET", "HEAD"), and an HTML error page is at least as likely on 5xx as on 4xx, so this path probably sees the problem more often than the one that was fixed.

The two branches are now six near-identical lines with different behaviour. One _errorResponseFrom(r, message) helper called from both would fix this for free — and would also serve client.py:547 and client.py:650, which still call fromDict on a 2xx isError body. Worth noting that tests/fixtures/charge.json has none of timestamp/url/errors, and tests/test_client.py:112-116 injects them by hand to make that path assert.

4. r is never reset after a transport failure (pre-existing)

r = None is set once before the loop (client.py:171); the except (Timeout, ConnectionError) clause does continue without clearing it. A sequence 500 → timeout → timeout → timeout therefore builds the final ErrorResponse from attempt 1's response. That is worse than the empty statusCode above, because statusCode and srcResponse are populated and look trustworthy. One line in the except clause fixes it.

5. The 2xx path still parses unguarded — and twice

if 200 <= r.status_code <= 201:
    logger.debug("Response[%s %s]: %r", r.status_code, r.reason, r.json())
    return r.json()

Response.json() does not memoise, and the %-style deferral only defers formatting, not argument evaluation — so every successful call pays two decodes plus two json.loads, with DEBUG off. Bind it once.

Separately, <= 201 routes every other 2xx into the client-error branch. Measured: a 204 No Content now raises ErrorResponse("HTTP 204 No Content with a non-JSON body: ''") with statusCode = 204. Before the PR that was an obvious JSONDecodeError; now a successful deleteCustomer is indistinguishable from a real rejection. I did not verify whether the API currently answers 204 anywhere — the three DELETE methods read data["id"], so today they get a body — but the misclassification is structural.

6. The new diagnostic does not reach any consumer

The fallback puts everything in the exception message and leaves errors == []. But ErrorResponse.__repr__ (model/error.py:93) renders only url, errorId, traceId, errors — all empty here. Measured:

repr: unzer.model.error.ErrorResponse(url=None, errorId=None, traceId=None, errors=[])

%r is this SDK's own logging style (client.py:178/179/197/201/206), and the documented downstream consumer logs exactly that and then iterates .errors. README.md:141 and examples/03_installment_plans.py:85 do the same. So for a .errors-reading caller the new error carries less than the JSONDecodeError it replaces.

Two options, either is fine: synthesize one Error entry (code = the HTTP status, merchantMessage = the body excerpt) so a raised ErrorResponse never has an empty list; or include args[0] and statusCode in __repr__. Both would also give callers a way to tell "the gateway refused this" from "the API refused this", which today is only reachable by string-matching the message.

Minor, same line: r.text[:200] decodes the full body to keep 200 chars, and for a text/html body without a charset requests decodes as ISO-8859-1 per RFC 2616 — a UTF-8 gateway page comes out as mojibake. r.content[:200] avoids both.

7. BasketItem.serialize does not round, Basket.serialize does

This one matters because of the docs this PR adds. basket.py:92-97 wraps every basket amount in roundAmount; basketItem.py:116-125 wraps none. Measured:

basket amountTotalGross: 0.3
item   amountGross     : 0.30000000000000004
item   amountDiscount  : 8.881784197001252e-16

roundAmount's own docstring says the API truncates past four decimals and does not accept scientific notation. The new AGENTS.md row requires totalValueGross == sum((amountPerUnitGross - amountDiscountPerUnitGross) * quantity) exact to the cent, i.e. it asks consumers to do exactly the arithmetic whose residue is no longer scrubbed. Spreading a 90.78 discount over 7 units gives 12.968571428571428 on the wire against a rounded total.

Tests

  • test_v3_discount_must_not_exceed_the_unit_price does not isolate its claim. It sends totalValueGross=self.GROSS - 1000.0, i.e. a negative basket total, and asserts API.600.200.131 "Amount has to be positive" — the same code test_v3_rejects_negative_item_amounts asserts for a negative item. The negative total alone explains the rejection. If the API dropped the per-item cap tomorrow the test stays green and the AGENTS.md rule derived from it silently becomes false. Adding a second, larger item keeps the total positive so only the one line goes negative.
  • test_non_json_client_error_raises_error_response is tautological. assert "403" in str(error) and "403 Forbidden" in str(error) — the fixture body itself contains <title>403 Forbidden</title>, so both substrings match even if r.status_code, r.reason and the body excerpt were all dropped from the message. Also the first clause is implied by the second. assert str(error).startswith("HTTP 403 Forbidden") plus a body-only substring would actually pin the format. Nothing covers the [:200] truncation.
  • The two vat tests cannot show what they claim. serialize() emits every key of the chosen schema, so both send "vat": null — never an absent field. They prove "the API rejects an explicit null" and "v1 coerces null to 0", which is not the same as optional/mandatory.
  • goods_v1/goods_v3 are not "overridable per test". The defaults are passed as explicit keywords next to **overrides, so overriding one is a TypeError, not an override. Measured: amountDiscountPerUnitGross= works, vat=None and quantity=3 raise got multiple values for keyword argument. That is the mechanical reason five of the new tests build their item inline, and why the two pre-existing tests were edited rather than migrated. The repo already has the idiom — dict-merge with |, used at client.py:146 and basketItem.py:115/120: build the defaults as a dict and call BasketItem(**defaults | overrides).
  • Hard-coded orderIds. CONTRIBUTING.md:86 says "generate unique ids per run"; the 11 new baskets use fixed ids. The same file does it right 90 lines above (f"sdk-test-{uuid.uuid4().hex[:12]}", with a docstring explaining why), and uuid is already imported. Two pre-existing tests already deviate, so this extends rather than invents the deviation — but it also leaves eight permanent baskets on the account per run.
  • Three redundant round trips. createBasket ends with return self.getBasket(data["id"], api_version=…), so the returned Basket already is the read-back. Lines 296, 357 and 380 GET it again, same URL. Asserting on basket directly keeps the docstrings true.
  • The kind/type fix is not protected by CI. The only new assertion for it sits in the sandbox suite, which pyproject.toml (addopts = "-m 'not sandbox'") and the workflow both deselect. A two-line mocked test on BasketItem(kind="goods").serialize()["type"] would guard it where it runs.

Docs

  • examples/03_installment_plans.py:57 still has type="goods". The commit fixes the two tests and names the root cause, but leaves the only public example of building a basket item shipping "type": "". That is what a downstream user copies.
  • BaseModel.__init__ swallows unknown kwargs without a word (model/base.py:33), while Error.__init__ and ErrorResponse.__init__ both logger.warning on extras. Measured: BasketItem(type="goods", quantitiy=3) yields kind=None, "type": "" and quantity: None, silently. Given the camelCase→snake_case rename announced for 2.0, this will bite again. Mirroring the Error warning in BaseModel would have surfaced the test bug the day it was written (it needs BasketItem.fromDict/Basket.fromDict to pop the keys they deliberately pass through).
  • AGENTS.md:215 contradicts basketItem.py:47. The table says v1 amountDiscount is "per line"; the docstring says "Discount amount for the basket item (multiplied by the quantity)", which reads as per unit. Every v1 discount test uses quantity=1, where the two are indistinguishable — while the v3 side does get the discriminating test. Since v1 reconciles nothing, the basket endpoint cannot settle this; per AGENTS.md:87 it should either be measured downstream or marked unverified.
  • basketItem.py:50 says vat is (optional) unconditionally, which AGENTS.md:218 and the new test now contradict for v3. Every other schema-specific field in that docstring carries a (v1)/(v3) tag.
  • AGENTS.md:52 is in the "confirmed by real sandbox calls" table but is not covered by a test. The row ends "…and a charge does not compare the basket to the payment amount either". The test docstring it comes from says "(measured with iDEAL). That is not tested here", and the reason given (it would create a payment per run) is sound. What I could not resolve from the repo is how it was measured: both tests/sandbox/test_live_api.py:10-16 and AGENTS.md:130-135 state that iDEAL cannot be exercised server-side. A manually created typeId from the Payment Page would explain it and would not be a contradiction — but as written a reader cannot tell. Worth a sentence, because this is the row a consumer leans on to skip validating a basket against a charge amount.
  • The discount rules live only in AGENTS.md. Its own "Where documentation goes" table sends user-facing detail to README.md/docs/*.md; docs/payment-methods.md still describes basket construction with no discount guidance. The named downstream consumer is exactly the audience that models a voucher as a negative line item, and has no reason to open a file headed "Guidance for AI coding agents".

@sveneberth

Copy link
Copy Markdown
Member Author

Follow-up: sandbox measurements, and two corrections to my own review

I went and measured the things above that I had taken from a docstring. Two of my points were wrong, and the measurements turned up something the PR should probably cover.

Correction 1: the parseDateTime fix is not sufficient on its own

I suggested routing ErrorResponse.fromDict through utils.parseDateTime. Measured, that parser handles exactly two formats:

'2026-08-21 10:15:32'  -> datetime(2026, 8, 21, 10, 15, 32)
'21.08.2026 10:15:32'  -> datetime(2026, 8, 21, 10, 15, 32)
'2026-08-21T10:15:32'  -> ValueError
'2026-08-21'           -> ValueError

So it fixes the European format but not an ISO-T or date-only timestamp. The point stands — the swallowed ValueError is the real defect — but the fix needs parseDateTime and a try/except that only wraps the decode, not just the former.

Correction 2: "the Pay later methods require v3" is false

model/basket.py's docstring says so, and I repeated it. Measured against the sandbox, a v1 basket with amountDiscount is accepted end to end:

Paylater Installment, v1 + amountDiscount   AUTHORIZED  booked=817.02  success=True
Paylater Installment, v3 + amountDiscountPerUnitGross  AUTHORIZED  booked=817.02  success=True
Klarna,               v1 + amountDiscount   AUTHORIZED  booked=817.02  pending=True
Klarna,               v3                    AUTHORIZED  booked=817.02  pending=True

Real authorizes, not just risk checks. So the docstring overstates a requirement that the API does not enforce, and the Basket docstring is worth correcting per this repo's own rule.

Side note on AGENTS.md:130-135 and tests/sandbox/test_live_api.py:10-16, which both say Klarna "cannot be exercised server-side" because createPaymentType yields an empty resource the provider rejects with COR.800.400.160: the authorize above went through with pending=True and a redirect. I did not follow the redirect, so the rejection may still be waiting further down the flow — but the claim as written is too absolute for what I could reproduce.

New: a v3 payload sent to the v1 endpoint is accepted and silently zeroed

This is the one I would fix in this PR. isV3() keys off a single field, totalValueGross is not None, and that choice picks the endpoint. But the two schemas are only mostly enforced:

payload POST /v1/baskets POST /v3/baskets
v1 shape 201, stored correctly 400 API.600.410.051
v3 shape 201 — every amount stored as 0.0000 201, stored correctly

So a basket built as Basket(amountTotalGross=…, basketItems=[BasketItem(amountPerUnitGross=…)]) — v1 total, v3 item — serializes to the v1 shape with amountGross: null, goes to /v1/baskets, and comes back as a basket worth 0.0000 with no error anywhere. Given the model happily accepts any mix of v1 and v3 attributes, and given that BaseModel.__init__ also swallows unknown kwargs, this is easy to hit and impossible to notice.

A validation in Basket.serialize() (or validateBeforeRequest) that refuses a basket mixing v1 and v3 item fields would close it, and unlike a returnUrl blocklist it does not encode any policy that Unzer could relax later — the two schemas are mutually exclusive by construction.

New: a basket can only be used once

API.330.200.152 "Resources: basket was used." on the second charge against the same basketId. Worth a row in the AGENTS.md table: a consumer that retries a failed authorize has to create a new basket, which is not obvious and is exactly what a checkout retry does.

The charge/basket claim (AGENTS.md:52) is true — and now testable

Four charges against four identical baskets whose amountTotalGross is 817.02:

amount 817.02  ACCEPTED  booked=817.02
amount 726.24  ACCEPTED  booked=726.24
amount 907.80  ACCEPTED  booked=907.80
amount   1.00  ACCEPTED  booked=1.00

So the charge really does not reconcile against the basket. The reason the row could not be tested was given as "it would create a payment on the account for every run" — but this used Prepayment, which is created server-side, so unlike the iDEAL measurement it can live in the sandbox suite. That would turn the one unmeasured row in the table into a measured one, and remove the tension with the two places that say iDEAL cannot be driven server-side.

…oes not fit

The `except ValueError` guarding `ErrorResponse.fromDict(r.json())` was wrong in
both directions.

Too narrow: `fromDict` indexes `data["timestamp"]`, `data["url"]` and
`data["errors"]` and builds `Error` from required positionals, so a 4xx whose
body *is* JSON but is not this API's envelope raised `KeyError`/`TypeError` --
neither of them a `ValueError`. Measured, all of these still escaped `_request`
bare: `{"message": "Forbidden"}` (the canonical API-gateway shape), `null`,
`[]`, `"Forbidden"`, and an error entry missing a field. That is the same
failure the commit set out to remove, one body shape over.

Too wide: `fromDict` parses the timestamp with a single hard-coded format, so a
well-formed API error with any other one was reported as "non-JSON body" --
while quoting JSON -- and lost its `errors` list on the way. The root of that
belongs in `fromDict`; here it at least no longer claims the body was not JSON,
and it is logged instead of swallowed.

Only `r.json()` is guarded now, and the two causes are reported separately.
`test_v3_discount_must_not_exceed_the_unit_price` sent an over-discounted item
*and* a negative `totalValueGross`, then asserted `API.600.200.131` "Amount has
to be positive" -- the same code `test_v3_rejects_negative_item_amounts`
asserts for a negative item. The negative total alone explains it, so the test
could not show that the item was what the API refused.

Measured with a second, larger item keeping the total positive: the API then
answers `API.600.410.064` and names the line, "Basket item i1
'amountDiscountPerUnitGross' does not equal to 'amountPerUnitGross'". So the
cap is real, but the test was asserting the wrong code for the wrong reason.

Also correct the module docstring's source for "a charge does not validate the
basket against the payment amount": that was attributed to a measurement with
iDEAL, which the same file states cannot be exercised server-side. Re-measured
with Prepayment, which can: against one basket worth 817.02 the amounts 817.02,
726.24, 907.80 and 1.00 are all accepted and booked at face value.
Four claims measured against the sandbox, three of them wrong:

- **The Pay later methods do not require v3.** A sandbox authorize with
  `PaylaterInstallment` and with Klarna succeeds on a v1 basket carrying
  `amountDiscount` just as it does on v3, booking the requested amount
  unchanged. The `Basket` docstring asserted the opposite.
- **Mixing the schemas fails silently in one direction.** A v3 item in a v1
  basket is accepted with a 201 and every item amount stored as `0.0000`; the
  reverse is refused loudly with `API.600.410.051`. A basket is also only
  readable through the schema it was created with (`API.600.410.024`).
- **`vat` is mandatory in v3**, not "optional" as `BasketItem` claimed --
  `API.600.410.052`, which this branch's own test already showed.
- **A basket is single use**: `API.330.200.152 "Resources: basket was used."`
  on a second charge, so a retry after a failed authorize needs a new one.

Also record the error code the per-item discount cap really produces
(`API.600.410.064`, and only when the basket total stays positive), and replace
the iDEAL attribution on the charge-does-not-check-the-basket row with the
Prepayment measurement, which is reproducible server-side.
@sveneberth

Copy link
Copy Markdown
Member Author

Applied on the branch

Scope kept to what this PR touches; everything else is filed.

client.py — the guard now separates the two causes

The except ValueError was wrong in both directions, both measured:

  • Too narrow. A 4xx whose body is JSON but is not this API's envelope still escaped bare, because fromDict raises KeyError/TypeError, neither of them a ValueError. All of these left _request unwrapped: {"message": "Forbidden"} (the canonical API-gateway body), null, [], "Forbidden", and an error entry missing a field.
  • Too wide. fromDict parses the timestamp with one hard-coded format, so a well-formed API error with any other one was reported as "non-JSON body" — while quoting JSON — and silently lost its errors.

Now only r.json() is guarded, and a body that decodes but does not fit the schema gets its own message plus a logger.exception. The root of the timestamp problem is in fromDict and is filed as #14, because it also empties the errors list that createOrUpdateCustomer (client.py:337) branches on — a path that is exercised routinely in production.

tests/test_client.py

assert "403" in str(error) and "403 Forbidden" in str(error) was satisfied by the fixture body alone, which contains <title>403 Forbidden</title> twice — a message that had dropped the status line entirely would still have passed. Replaced with a prefix assertion plus a body-only substring, and added parametrized coverage for the five foreign-JSON shapes and for the unparsable timestamp.

tests/sandbox/test_live_api.py — the discount cap test now isolates its claim

test_v3_discount_must_not_exceed_the_unit_price sent an over-discounted item and a negative totalValueGross, then asserted API.600.200.131 — the same code the negative-item test asserts, and explainable by the negative total alone.

Measured with a second, larger item keeping the total positive, the API names the line instead:

API.600.410.064
  Basket item i1 'amountDiscountPerUnitGross' does not equal to 'amountPerUnitGross'.

So the cap is real — the test was asserting the wrong code for the wrong reason. Now it asserts API.600.410.064, and the docstring records why the second item is there. Sandbox run: 13 passed.

The claims in AGENTS.md and the docstrings, re-measured

  • The Pay later methods do not require v3. A sandbox authorize with PaylaterInstallment and with Klarna succeeds on a v1 basket carrying amountDiscount exactly as on v3, booking the requested amount unchanged. The Basket docstring and the schema table both said otherwise.
  • Mixing the schemas fails silently one way round. A v3 item in a v1 basket → 201 with every item amount stored as 0.0000; the reverse → API.600.410.051. Filed as A v3 item in a v1 basket is accepted and silently stored as 0.0000 #16, documented here.
  • vat is mandatory in v3, not "optional" as BasketItem said — which this branch's own test already showed.
  • A basket is single use (API.330.200.152), so a retry after a failed authorize needs a new one.
  • The charge-does-not-check-the-basket row was attributed to a measurement with iDEAL, which this same repository twice states cannot be driven server-side. Re-measured with Prepayment, which can: against one basket worth 817.02, the amounts 817.02, 726.24, 907.80 and 1.00 were all accepted and booked at face value. That makes the row reproducible rather than unverifiable.

Full suite: 286 passed, 31 deselected. pycodestyle --diff clean.

Filed rather than changed

#14 (fromDict drops the error codes), #15 (the remaining _request branches — retry-exhausted statusCode == 0, the unguarded and doubled r.json() on 2xx, <= 201 sending 204 into the error branch, r not reset after a transport failure), #16 (schema mix), #17 (BaseModel swallows unknown kwargs — with examples/03_installment_plans.py:57 still carrying the type=/kind= bug this PR fixed in the tests), #18 (BasketItem.serialize does not round).

Also unchanged and worth a decision from you: the v1 amountDiscount row in the new table says "per line", while basketItem.py:47 says "multiplied by the quantity", i.e. per unit. Every v1 discount test uses quantity=1, where the two are indistinguishable, and v1 reconciles nothing — so the basket endpoint cannot settle it. Either measure it downstream or mark the row unverified.

`ErrorResponse.fromDict` was the strictest parser in the package, on the one path
that runs when something has already gone wrong: it indexed `timestamp`, `url` and
`errors`, parsed the timestamp with a single hard-coded format, and built `Error`
from three required positionals. Any of those failing cost the whole error.

That matters because the codes are the part a caller acts on --
`UnzerClient.createOrUpdateCustomer` falls back to `updateCustomer` on
`errors[0].code == "API.410.200.010"`, which is on the live checkout path of every
Unzer provider in viur-shop. Since the previous commit a parse failure no longer
crashes, so the loss had become silent: the guard saw an empty list and re-raised.

Now only `errors` is required -- it is what identifies the body as this API's error
envelope -- and everything else missing or unreadable costs that field alone. The
timestamp goes through `utils.parseDateTime`, which knows both formats the API uses,
and falls back to `None` with a warning rather than taking the codes down with it.
`Error` gained defaults, so an entry missing `merchantMessage` keeps its code; it
already tolerated *extra* keys, so refusing over a missing one was the wrong way
round.

A body that is not an envelope still raises, so the caller keeps its "the API
refused this" versus "something else answered" distinction.

Measured across `2026-08-21 10:15:32`, `21.08.2026 10:15:32`, `2026-08-21T10:15:32`,
`2026-08-21 10:15:32.123` and a missing timestamp: the codes arrive in all five and
the `createOrUpdateCustomer` fallback fires again.

Closes #14
The table claimed `amountDiscount` is per line while `basketItem.py:47` documents
it as per unit ("multiplied by the `quantity`"). Nothing here settles it: every v1
discount test uses `quantity=1`, where the two are indistinguishable, and v1
reconciles nothing, so the basket endpoint cannot be made to answer. Only the
rendered basket or the partner system can -- until then this repository's own rule
applies and it is labelled rather than asserted.
@sveneberth sveneberth changed the title Basket discounts: what the API actually accepts, and surviving a non-JSON 4xx Basket discounts: what the API actually accepts, and error responses that survive an unexpected body Sep 1, 2026
`Error.__init__` gained its defaults here and `fromDict`/`_parseTimestamp` were
rewritten, so they fall under "type hints everywhere" -- the surrounding older
models being unannotated is not a reason to add more of them.
The module-level `import datetime` had exactly one user left after this branch
replaced the `strptime` call: the type hint on `_parseTimestamp`. Matches the
existing `from datetime import datetime as dt` in `additional_transaction_data`.
Four statements did not survive the sandbox runs on this branch:

- "Amounts are rounded to four decimals on serialisation" holds for the basket, not
  for the items -- `BasketItem.serialize` rounds nothing, so an item goes out as
  `0.30000000000000004` next to a total rounded to `0.3` (#18). Worth stating
  precisely right here, because the v3 reconciliation this file documents is exact
  to the cent.
- The schema table read as if each method required a particular version. Measured,
  none of them does; the column records what was seen to work.
- Mixing the schemas within one basket was warned about in the `Basket` docstring but
  not here, and only one direction fails loudly: a v3 item in a v1 basket is accepted
  with a 201 and every item amount stored as `0.0000` (#16).
- `TestBasket` no longer covers "all of this": the v1 per-line/per-unit question is
  labelled unverified and no test can settle it.
Chased the per-line/per-unit question 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. No consumer evaluates the field: the v1 endpoint stores it and reconciles
nothing, a charge ignores the basket, Klarna lists the item by title and takes its
total from the authorize amount, and the Hosted Payment Page renders the line as
`3x T-Shirt € 300,00` -- the undiscounted gross -- with its total taken from the
request as well.

The distinction is therefore undecidable *and* inconsequential, which is a more
useful thing to record than the guess it replaces. It also means a discounted line
is shown to the customer at full price next to a lower total, with nothing naming
the difference.
@sveneberth
sveneberth merged commit fa1eea3 into develop Sep 2, 2026
10 checks passed
@sveneberth
sveneberth deleted the fix/basket-discounts-and-non-json-errors branch September 2, 2026 14:39
sveneberth added a commit to viur-framework/viur-shop that referenced this pull request Sep 2, 2026
…egative items (#202)

A basket-domain discount was sent to Unzer as its own line item with
negative amounts.
Unzer refuses that, so every Klarna or Paylater-Installment checkout
with a discount in
the cart failed.

## The problem

`build_discount_item()` emitted the discount as `kind="voucher"` with
`amountPerUnit`/`amountNet`/`amountGross` set to `-amount`. Both basket
schemas reject
negative item amounts:

```
POST /v1/baskets → 400
API.600.200.131  Amount -90.78 has to be positive          (per amount field)
API.600.410.018  Basket item has negative gross amount
```

The shape is the obvious one — one item per article, one for the
voucher, the grosses
adding up to the order total — which is why it looked right. It fails
deterministically, just rarely, because a basket discount and a BNPL
method seldom meet.

## Changes

- **`spread_discount()`** puts the discount where the API wants it: a
positive
`amountDiscount` on the items it reduces. Since the field hangs off an
item, a node
discount has to be split, and the split is done in whole cents — each
item gets the
floor of its exact share, the cents left over by rounding go one each to
the items
with the largest fractional part. The parts add up to the discount
exactly, which the
v3 endpoint requires. Weights are what is left of an item (gross minus
discounts
already spread on it by a nested node), so no item can be pushed below
zero; a
discount exceeding the whole basket raises instead of producing a
silently wrong
  basket.
- **`build_node_items()`** replaces the flat queue walk and mirrors the
`total_discount_price` computation of `CartNodeSkel` per node: the
discount applies to
the subtree, the node's own shipping is added afterwards and stays
undiscounted.
- The discount **amount** is no longer recomputed from the discount
skeleton but taken
as the difference between the items and the node's stored
`total_discount_price`. An
ordered cart is frozen, so that value is what the customer is charged
even if the
discount changed afterwards — and the basket keeps reconciling whatever
the discount
type does. A difference without a basket discount is logged rather than
skewing the
  basket silently.
- **`has_basket_discount()` reads the frozen discount**, for the same
reason. The bone
is `RelationalConsistency.SetNull` and core's `update_relations`
refreshes it, so
  deleting the discount entity — or re-scoping its condition to another
`application_domain` — after the order was frozen turned a reduction the
customer is
still charged for into an error log and a basket overstating the order
by the full
discount. Note the trap this needs to avoid: `freeze_cart` stores the
*dumped*
relation, where `application_domain` is a plain string, while the live
relation yields
  the enum member — and `ApplicationDomain` is a plain `enum.Enum`, so
`"basket" == ApplicationDomain.BASKET` is `False`. Reading
`frozen_values` without
normalising would have reported "no basket discount" for every frozen
cart, i.e.
  dropped the discount entirely.
- `get_basket_id()` reports the resulting `amountTotalDiscount`.

`build_discount_item()` is gone, `has_basket_discount()` is new.

## Verification

Measured against the Unzer sandbox, since neither the documentation nor
a `201` from
`POST /baskets` settles what a payment method does with a basket:

- **Real authorizes with the exact shape this PR builds** — v1,
`amountTotalGross`
already reduced, `amountTotalDiscount` reported, the discount as a
positive
  `amountDiscount` on the items:

  ```
Klarna, v1 + amountDiscount AUTHORIZED booked=817.02 pending=True
  Paylater Installment, v1 + amountDiscount   AUTHORIZED  success=True
  Paylater Installment, v3 equivalent         AUTHORIZED  success=True
  ```

`booked` is the authorize amount unchanged, so the discount is not
applied twice.
This also disproves the claim in the SDK's `Basket` docstring that the
Pay later
  methods require the v3 schema — corrected upstream in
  mausbrand/unzer-python-sdk#13.
- **A charge does not reconcile against its basket at all**: charges of
817.02, 726.24,
907.80 and 1.00 against four identical baskets worth 817.02 were all
accepted at face
value. So nothing on the Unzer side would have caught a wrong basket —
which is why
the authorizes above, not the basket creation, are the evidence that
matters.
- **The cent distribution** in `spread_discount` was checked by brute
force over 200k
randomized carts: the shares always sum to the amount, no item is pushed
below zero,
  the loop terminates.
- Eight real carts of a shop running this code, rebuilt with the patched
`spread_discount` and sent to the sandbox: accepted in both schemas,
item sums equal
to the order total in all eight. The previously refused basket now
passes, its 90.78
  split into 59.89 and 30.89.
- `pycodestyle` with the project config: no finding in the changed
lines.

The unit tests on the spreading arithmetic that this description
previously claimed are
**not in the repository** — it has no test directory, no runner in
`Pipfile [dev-packages]` and no CI test job. `spread_discount` is a pure
`@staticmethod`
over plain `BasketItem` objects and is the best possible candidate for
the first one;
filed as #205.

## Known gaps, deliberately not addressed here

- **The discount loses its name in the customer-visible basket** —
measured, not
assumed. The removed voucher item carried
`title=discount["dest"]["name"] or
"Discount"`; `amountDiscount` has no label and nothing replaces it. I
put an item of
`quantity=3`, gross 300.00 and `amountDiscount=10.00` through both
renderers:

- Unzer's Hosted Payment Page shows the line as `3x T-Shirt € 300,00` —
the
**undiscounted** gross — next to a total of `€ 290,00`, with nothing
explaining the
    20.00 difference.
- Klarna's checkout lists the item by title only, with no price at all,
and takes its
    total from the authorize amount.

So neither renderer evaluates `amountDiscount`; the customer sees
full-price lines
above a lower total. Whatever we decide, it should be a decision rather
than a side
  effect.
- **`BASKET_ITEM_VOUCHER` is left behind with no producer.** Nothing
emits a voucher
line any more, so the constant is the last remnant of the removed
builder. Dropping
it would be the tidier end state; kept out of this diff to hold it to
the fix.
- **`spread_discount()` raises `ValueError`** when the discount exceeds
the items, which
surfaces as an HTTP 500 on the customer's "order now" after the order
UID is assigned.
Kept deliberately: such a cart is a data error and should stop the
checkout rather
than ship a knowingly wrong basket. Reachable because
`DiscountSkel.absolute` has no
  upper bound and `Price.apply_discount` does not clamp at zero.
- Basket item titles reach the provider HTML-escaped. An earlier commit
on this branch
unescaped them at the call site; that was reverted, because it assumes
every project's
article bone escapes, which `ArticleAbstractSkel` does not require.
Filed as #204 with
a sketch for reconciling it in `Cart.copy_article_values`, which is the
one place that
  sees both bone definitions.

## Related

#203 (the basket builders read live prices and shipping against a frozen
cart total —
same class as the `has_basket_discount` bug fixed here), #204
(escaping), #205 (no
tests), #206 (pre-discount VAT and the unset `amountTotalVat`), #200
(`DiscountType.FREE_SHIPPING` raises in `Price.apply_discount()` — no
longer reachable
from this path, since the amount now comes from the stored total), #201
(cart discounts
are never revalidated).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ErrorResponse.fromDict drops the error codes for any body it cannot fully parse

1 participant