Skip to content

docs: Document every class and fix the docstrings that were wrong - #12

Merged
sveneberth merged 1 commit into
developfrom
docs/class-docstrings
Aug 21, 2026
Merged

docs: Document every class and fix the docstrings that were wrong#12
sveneberth merged 1 commit into
developfrom
docs/class-docstrings

Conversation

@sveneberth

@sveneberth sveneberth commented Aug 21, 2026

Copy link
Copy Markdown
Member

Stacked on #11 — merge that first, GitHub then retargets this to develop.

All 60 classes have a docstring now, where 31 did. The ones worth reading are the models that
carry behaviour rather than just fields: what a payment is as opposed to a transaction, that a
pending response is not a failure but a redirect the customer still has to follow, that
processing holds different fields per payment method and can be incomplete while a verification
is outstanding.

The payment type classes explain why they are empty — created in the browser, so there is
nothing for the server to send — and Card says outright that taking card data would put the
integration under PCI-DSS obligations. That question came up twice while working on this SDK, and
the answer belongs where the empty class is.

Three docstrings were factually wrong: Customer had firstname and lastname swapped in their
descriptions, and Address called two positional parameters optional.

Property getters now carry the documentation rather than only their setters, because the getter's
docstring is the one Sphinx renders. And the two public charge() methods had none at all, which
for a method that captures money is the wrong place to be terse.

Module docstrings for the six modules where they orient rather than restate the filename.

serialize/fromDict overrides stay undocumented on purpose: their contract is on BaseModel,
and an override that does nothing special has nothing to add. That leaves 60/60 classes and 69 of
126 public functions documented, the gap being those 55 overrides.

One non-doc change came with it, named in the commit: the client check in get_configuration was
duplicated in get_configurations, and now sits only where the client is actually used.

@sveneberth sveneberth added the documentation Improvements or additions to documentation label Aug 21, 2026
@sveneberth
sveneberth force-pushed the docs/class-docstrings branch from d1d7f5c to 8bcca94 Compare August 21, 2026 23:24
Base automatically changed from chore/ruff to develop August 21, 2026 23:31
@sveneberth
docs: Document every class, and fix the docstrings that were wrong
79416b5 
All 60 classes have a docstring now, where 31 did. The ones worth reading are
the models that carry behaviour rather than just fields: what a payment is as
opposed to a transaction, that a pending response is not a failure, that
`processing` holds different fields per payment method and may be incomplete
while a verification is outstanding.

The payment type classes explain why they are empty — created in the browser,
so there is nothing for the server to send — and `Card` says outright that
taking card data would put the integration under PCI-DSS obligations.

Three docstrings were wrong: `Customer` had firstname and lastname swapped in
their descriptions, and `Address` called two positional parameters optional.

Property getters now carry the documentation rather than only their setters,
because that is the one Sphinx renders. And the two public `charge()` methods
were undocumented, which for a method that captures money is the wrong place to
be terse.

Module docstrings for the six modules where they orient rather than repeat the
filename. `serialize`/`fromDict` overrides stay undocumented on purpose: their
contract is on `BaseModel` and an override that does nothing special has nothing
to add.

Along the way: the client check in `get_configuration` was duplicated in
`get_configurations`, which is where the client is actually used.
@sveneberth
sveneberth force-pushed the docs/class-docstrings branch from 8bcca94 to 79416b5 Compare August 21, 2026 23:32
@sveneberth
sveneberth merged commit c5c4085 into develop Aug 21, 2026
10 checks passed
@sveneberth
sveneberth deleted the docs/class-docstrings branch August 21, 2026 23:33
@sveneberth sveneberth mentioned this pull request Sep 1, 2026
Merged
sveneberth added a commit that referenced this pull request Sep 2, 2026
@sveneberth
Basket discounts: what the API actually accepts, and error responses …
fa1eea3 
…that survive an unexpected body (#13)

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

```py
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 #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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

documentation Improvements or additions to documentation

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@sveneberth