refactor: move gateways onto base/live/stub service clients

.claude/skills/add-payment-gateway/SKILL.md

@@ -0,0 +1,142 @@
+---
+name: add-payment-gateway
+description: Add a new payment provider (e.g. PayPal, Paymob, Checkout.com, Tap, HyperPay, Moyasar, Adyen) to bwh_payments as a Gateway Settings Single plus a base/live/stub service client in bwh_payments/services/NAME/, wired into the gateway contract, webhooks, refunds, tests and the storefront admin. Use this whenever someone wants to support, integrate, add, or "plug in" a new payment gateway, PSP, BNPL or hosted checkout in bwh_payments or commera, even if they only name the provider.
+---
+
+# Add a payment provider to bwh_payments
+
+A provider is three things: a **Single DocType** that holds credentials and implements
+`PaymentGatewayBase`, a **service client** (`services/<name>/{base,live,stub}.py`) that talks to the
+provider, and a **Payment Gateway Profile** record that makes it selectable. Read `AGENTS.md`
+("The gateway contract", "Service clients", "Money and currency rules", "Security conventions") before
+starting. Those rules are not repeated here. Copy structure from the closest existing gateway:
+
+| Provider looks like | Copy from |
+|---|---|
+| raw JSON HTTP API, HMAC-signed webhooks | `razorpay` |
+| official Python SDK | `stripe` |
+| no signed webhook, status only by polling | `telr` |
+| authorise-then-capture (BNPL), token-checked webhooks | `tabby` |
+
+Branch in every repo you will touch, and make sure the bench's Redis is running before you run tests
+(see the migrate skill, step 0).
+
+## 1. Pin down the provider's facts first
+
+Get these from the provider's API docs (ask the user for the link if you don't have one) and write them
+in your first message back. Each one changes code, and guessing any of them costs real money:
+
+- **Checkout:** which endpoint creates a hosted page, which id identifies it for later status reads, and
+  whether that id is the one webhooks carry. Tabby's checkout id is not; its payment id is.
+- **Amount format:** integer minor units (Stripe, Razorpay), a major-unit decimal string at the
+  currency's precision (Tabby, Telr), or something else. Find out how 3-decimal (KWD, BHD) and 0-decimal
+  (JPY) currencies are handled.
+- **Supported currencies and markets.**
+- **Status vocabulary,** and which statuses mean money is actually taken.
+- **Capture model:** automatic, manual, or per-account. If it can't be known from the API, add a
+  `treat_authorised_as_paid` style switch that defaults to the safe side, as Razorpay and Telr do.
+- **Webhook verification:** an HMAC over the raw body (with which secret), a static token, IP allowlists,
+  or nothing. Also find which header carries an event id; derive a stable one if there is none, as Tabby
+  does.
+- **Refunds:** what you refund against (session, payment, transaction), whether partial refunds are
+  allowed, what an accepted refund looks like, and whether the refund id comes back.
+- **Errors:** the error body shape, and whether failures come back as HTTP 200 (Telr does).
+
+## 2. Create the settings Single
+
+Do not `mkdir` doctype folders. With `developer_mode` on, create the DocType through the desk or
+`bench --site <site> console` (module `BWH Payments`, `issingle: 1`, System Manager permissions), then
+`bench --site <site> migrate`. Mirror the Razorpay field layout:
+
+- `enabled`, `test_mode`, plus any capture or notification switches (Check)
+- credentials: public ids as Data, secrets as **Password** (required where the API can't work without them)
+- `webhook_secret` (Password) and, if the provider publishes IPs, `webhook_ips` (Small Text)
+- `currency` (Link), `success_url`, `failure_url`, `cancelled_url` (Data)
+
+Migrate regenerates the `# begin: auto-generated types` block in the controller. Don't write it by hand.
+
+## 3. Build the service client
+
+Follow AGENTS.md "Service clients" and the migrate skill steps 3–5 for `base.py`, `live.py` and `stub.py`.
+For a new provider:
+
+- `base.py` methods map one to one to the provider calls you listed in step 1. Model only the fields the
+  controller reads. Every field on the refund model is optional, `id` and `amount` included (see the
+  migrate skill, step 3).
+- `stub.py` must behave like the provider's real state machine for the paths you support: created, then
+  paid or authorised, then captured, then refunded. Include the refusal paths (`next_refund_status`, a
+  rejection control) and raise for unknown ids the way live does.
+
+## 4. Write the controller
+
+`class <Name>GatewaySettings(Document, PaymentGatewayBase)` with `get_gateway_name()` and `get_client()`
+(stub iff `frappe.in_test`). Then implement the contract:
+
+- **`create_session(amount, currency, reference, customer)` → `CheckoutSession`.**
+  - Validate the currency with `validate_transaction_currency`, plus the provider's own allowlist if it
+    has one.
+  - Convert amounts only through `bwh_payments/currency.py`.
+  - Build return URLs with `urlsplit` and `parse_qsl`, append `reference_id=<request name>`, and run them
+    through `get_localised_url`.
+  - Drop customer fields the provider rejects when blank, rather than letting checkout fail.
+  - `session_id` must be the id webhooks and status reads use.
+- **`get_payment_status(session_id)` → a Gateway Payment Request status.**
+  - Normalise once (`strip().casefold()`), then use a status map in which anything unrecognised is
+    `Pending`.
+  - A partial or authorised-but-uncaptured payment is never `Paid` unless the store's switch says so. If
+    the provider needs an explicit capture, do it here, for the authorised amount only (see Tabby).
+- **`handle_webhook(payload, headers)` → `WebhookEvent | None`.**
+  - Verify first: HMAC over the raw bytes with `hmac.compare_digest`, guard non-ASCII signatures with
+    `isascii()`, and `frappe.throw` on a missing secret, header or event id.
+  - Return `None` for events you don't act on.
+  - Throw on a "paid" event you can't read, so the provider retries and it lands in the Error Log.
+  - Header names must match werkzeug's title-case (`X-Foo-Signature`), because the handler receives a
+    plain dict.
+- **`refund_payment(session_id, amount, currency)` → `RefundResult`** with `amount` in major units. The
+  request reads only `refund_id`, and it reads it after the money has moved, so return what you have
+  rather than raising on a thin provider response. Use the provider's echoed amount when it sends one,
+  and the requested `amount` when it does not.
+  Refuse when there is nothing captured to refund, and throw on any status that isn't an accepted one.
+- **`cancel_session(session_id)` → bool,** if the provider can cancel or expire a session. Return
+  `False` instead of raising when the provider refuses.
+
+Mark deliberate limits with `# ponytail:` comments that name the ceiling and the upgrade path (a missing
+void endpoint, no timeout on `make_request`, and so on).
+
+## 5. Tests (stub-driven, in the doctype folder)
+
+`test_<name>_gateway_settings.py` with `StubXClient.reset()` in `setUp`. Cover at least:
+
+- amounts for a 2-, a 3- and a 0-decimal currency, as sent to the provider
+- an unsupported currency refused before any call
+- the session id being the one webhooks use, and the return URLs carrying `reference_id`
+- the status map: each known status, an unknown one staying `Pending`, and authorised versus captured
+- capture, if any: triggered once, for the authorised amount
+- webhooks: valid, forged, missing header, wrong secret (for example the API key in place of the webhook
+  secret), an ignored event type, and an unreadable paid event
+- refunds: accepted, refused, nothing captured, unknown session
+- `release_if_unpaid` on an open session and on a paid one
+- one spine test through `bwh_payments.bwh_payments.webhook.handle` (see `tests/test_razorpay_webhook.py`)
+
+If the class writes Integration Requests or Profiles, clean up in `tearDownClass` (see
+`remove_razorpay_gateway`), because Frappe 17 rolls back only at class end and `create_request_log`
+commits.
+
+## 6. Make it selectable
+
+- **Payment Gateway Profile:** a record named after the provider, with `gateway_settings` set to the new
+  Single. Its validate rejects a Single that doesn't implement the contract.
+- **ERPNext refunds** need a Mode of Payment named exactly like the profile.
+- **Commera:** add an entry to `PAYMENT_GATEWAYS` in `commera/api/admin/payments.py` (slug, label, blurb,
+  `settings_doctype`, docs URL). Enabling it there creates the Mode of Payment, and the webhook URL shown
+  to the merchant is `/api/method/bwh_payments.bwh_payments.webhook.handle?gateway=<profile>`.
+- For an end-to-end guarantee in commera, copy `commera/tests/test_razorpay_checkout.py` and swap the
+  driver helpers for the new stub.
+
+## 7. Document and verify
+
+- Add a "Gateway-specific behavior" bullet to AGENTS.md: transport, webhook scheme, and anything
+  surprising.
+- Run the full bwh_payments and consumer suites, `pre-commit run --files ...`, and a leak check (no
+  stray profiles or enabled settings on the site).
+- One commit in bwh_payments (`feat: add <Name> gateway`), and a separate one for the commera wiring.

.claude/skills/migrate-payment-gateway/SKILL.md

@@ -0,0 +1,173 @@
+---
+name: migrate-payment-gateway
+description: Move an existing bwh_payments gateway (or any Frappe integration controller that calls make_post_request / make_get_request / an SDK directly) onto the base/live/stub service-client layout in bwh_payments/services/GATEWAY/, with typed pydantic models and tests that drive a stub instead of patching transports. Use this whenever someone wants to refactor, restructure, "clean up", make testable, or "move to the services pattern" a Gateway Settings controller, delete a tests/fake_*.py, stop patching HTTP in tests, or add stubs for a payment, shipping or other third-party integration, even if they don't say "migrate".
+---
+
+# Migrate a gateway to base/live/stub service clients
+
+The target layout and its rules are in `AGENTS.md` under "Service clients (base / live / stub)". Read that
+section first; it is the source of truth, and this skill is the procedure for getting there safely. The
+four gateways already migrated (`bwh_payments/services/{razorpay,stripe,telr,tabby}/`) are worked
+examples. Razorpay (raw HTTP) and Stripe (SDK) are the cleanest to copy from.
+
+The whole point of the migration is that **behaviour does not change**. So the procedure is built around
+proving that: pin current behaviour, move code in thin slices, and re-run the same tests after each slice.
+
+## 0. Set up
+
+- Branch in **every** repo you will touch (bwh_payments and each consumer app such as commera) before the
+  first edit.
+- Integration tests need the bench's Redis. If they crash with `Error 61 connecting to 127.0.0.1:<port>`,
+  start it from the bench root: `redis-server config/redis_cache.conf --daemonize yes`, and the same for
+  `config/redis_queue.conf`. Or use `bench start`.
+- Run the baseline and write the counts down:
+  `bench --site <site> run-tests --app bwh_payments`, then the same for each consumer app. You will
+  compare against these numbers at the end.
+
+## 1. Pin behaviour before touching anything
+
+Look at what the gateway's existing tests and the consumer apps' tests actually exercise. If a consumer
+(commera, buzz) has no end-to-end test through this gateway, write one first, against the old code:
+checkout, then paid via webhook, then paid via the return page, then abandoned-cart release, then refund.
+`commera/tests/test_razorpay_checkout.py` is the template.
+
+- Put every reference to the transport fake in a small block of "gateway driver" helpers (`shopper_pays`,
+  `created_links`, `refunds_sent`, `link_status`, `checkout_url`). The migration may change those
+  helper bodies and nothing else. That is the parity proof.
+- Prove the test can fail. Temporarily break one decision in the controller (for example map `paid` to
+  `Pending`), run the test, see it go red, then restore the file with `git checkout -- <file>`.
+
+## 2. Sort every line of the controller into transport or decision
+
+Read the controller top to bottom and classify:
+
+| Transport: moves to `live.py` | Decision: stays in the controller |
+|---|---|
+| base URL, auth headers, Basic/Bearer tokens | status maps (unknown → `Pending`, never `Paid`) |
+| `make_post_request` / `make_get_request` / SDK calls | currency → minor units, decimal formatting |
+| reading error bodies, non-JSON guards, `frappe.throw` on refusal | success/return URL building |
+| `create_request_log` and summarising for logs | capture and refund rules, "authorised counts as paid" switches |
+| request encoding (e.g. Telr's refund XML) and response parsing | webhook signature/token verification |
+
+Webhook verification always stays in the controller. It is local crypto, and if it lived in a client, the
+stub that replaces the client under tests would accept forged events.
+
+## 3. Write `services/<gateway>/base.py`
+
+- One abstract method per gateway call the controller makes. Name it after the gateway's own operation
+  (`create_payment_link`, `check_order`, `capture_payment`). Pass request bodies in the gateway's own
+  shape (`payload: dict`) when the controller builds them, so recordings stay comparable to the docs.
+- Models mirror the gateway's JSON, keeping only fields the controller reads. Subclass
+  `bwh_payments.services.GatewayModel`, which ignores unknown fields and coerces bare numbers into `str`
+  fields, so an id the gateway sometimes sends as `1234567` still parses. Use nested models where the JSON
+  nests, and `| None = None` wherever the gateway may omit a field or send null. Use
+  `str | float | None` for amounts that arrive both as `100` and as `"100.00"`.
+- A model the controller reads after money has moved (a refund echo) has **no** required fields, not
+  even `id` or `amount`. If it fails to parse, the refund is never recorded and stays refundable, so a
+  retry refunds twice. The controller falls back to the requested amount when the echo has none.
+- The docstring on the ABC says what a refusal raises (`frappe.ValidationError`, or the SDK's own error
+  type when the controller already catches that, as with Stripe).
+
+## 4. Write `live.py`
+
+Move the transport code out of the controller. Live clients are deliberately untested, so keep them to
+transport only: call, log, translate errors, parse. If you catch yourself writing an `if` about payment
+state here, it belongs in the controller.
+
+- Credentials go in the constructor. A secret that is optional on the settings (Telr's `remote_auth_key`)
+  goes as an argument to the one method that needs it. `get_password` raises when the field is empty,
+  and `get_client()` runs on every call.
+- Turn every refusal into `frappe.ValidationError`, including a body that is not JSON: check
+  `isinstance(response, dict)` before reading it. Annotate the helper that throws
+  (`throw_request_error`) as `-> NoReturn`, so `response` after the `except` is never read unbound.
+- Log against the settings Single (`reference_doctype = reference_docname = "<Name> Gateway Settings"`).
+  Log endpoints and ids only, never payloads: they carry keys and shopper contact details.
+- SDK objects: Stripe SDK 15 objects are attribute-based, not dicts, so parse with
+  `Model.model_validate(obj, from_attributes=True)`. Call the SDK's current namespace (`StripeClient.v1`,
+  from 12.5); the deprecated top-level services warn on every call.
+- Smoke-test the parsing offline: feed each model a payload shaped like the provider's documented
+  response, including null blocks and numeric ids. Live clients have no tests, so this is the only check
+  their parsing gets.
+
+## 5. Write `stub.py`
+
+An in-memory version of the gateway that behaves like it, not a mock that returns whatever a test wants.
+
+- State in `ClassVar`s, a `reset()` classmethod, ids from `frappe.generate_hash` (the `order_ref` column is
+  unique, and rows can outlive a test).
+- Name the controls after real-world events: `pay()`, `authorise()`, `set_status()`, `add_<thing>()` for
+  objects that exist at the gateway without this app creating them, and `next_refund_status` for refusals.
+- Recordings (`created_*`, `refunds`, `captures`) keep the gateway's request shape.
+- Unknown ids and refused operations raise the same exception type the live client raises, with the same
+  message prefix. Controllers catch these (for example `cancel_session` returning `False`).
+- Return copies (`model_copy(deep=True)`) so the controller cannot mutate the stub's store.
+- Webhook signing and event builders for tests live in `stub.py` too, replacing the ones in
+  `tests/fake_*.py`.
+
+## 6. Rewire the controller, in a tracer slice first
+
+```python
+def get_client(self) -> BaseXClient:
+	if frappe.in_test:
+		return StubXClient()
+
+	return LiveXClient(self.key_id, self.get_password("key_secret"))
+```
+
+Rewire `create_session` alone first and run the gateway's tests. Failures that only come from tests still
+reading the old fake are expected, and they show the stub path works end to end. Then rewire the rest:
+
+- `create_session` returns `CheckoutSession`, `refund_payment` returns `RefundResult`, and
+  `handle_webhook` returns `WebhookEvent | None` (`None` replaces the old `{}` for "ignore this delivery").
+- Helpers that took response dicts now take models. Replace `payment.get("status")` with
+  `payment.status`, and `.get("payments") or []` with `payment_link.payments or []`.
+- Keep `frappe.in_test` as the only switch. No env var, no "no keys configured" fallback: on a real
+  site that would mark orders Paid with no money taken.
+
+## 7. Move the tests onto the stub
+
+- In `setUp`: `StubXClient.reset()`. Delete the `patch.object(..., "make_post_request", ...)` blocks.
+- Replace fake reads with stub controls and recordings, one for one. Keep every assertion's meaning. If a
+  test patched a controller method that no longer exists (Telr's `get_order`), drive the same scenario
+  through stub controls instead.
+- Result dict access becomes attribute access (`session["session_id"]` becomes `session.session_id`), and
+  `assertEqual(result, {})` becomes `assertIsNone(result)`.
+- A gateway with no tests gets them now. The stub makes them cheap (see the Tabby tests).
+
+## 8. Fix consumers in the same change
+
+```bash
+grep -rn "fake_<gateway>\|Fake<Gateway>\|<gateway>_gateway_settings\.\(stripe\|make_\)" --include='*.py' apps/
+```
+
+Swap consumer tests one for one, with no assertion changes, and commit them separately in the consumer
+repo. Say in the message which bwh_payments commit they need, because merge order matters.
+
+Also grep every app on the bench for controller methods you removed (`get_order`, `get_headers`, `post`,
+`get_resource`) before deleting them.
+
+## 9. Delete the fake and verify
+
+- `git rm bwh_payments/tests/fake_<gateway>.py`.
+- Run both full suites. Counts must match the baseline plus any tests you added, with nothing skipped that
+  wasn't skipped before.
+- Run the consumer parity test. Its diff should touch only imports and the driver helpers.
+- Mutation-check one decision again, so the tests still bite with the stub in place.
+- `pre-commit run --files <changed files>`, twice if ruff-format rewrites something.
+- Leak check: no Payment Gateway Profile, enabled settings or Integration Requests left behind on the site.
+
+## Test-harness gotchas (Frappe 17)
+
+- `IntegrationTestCase` rolls back only at **class** end, and `create_request_log` calls
+  `frappe.db.commit()`. A test class that logs (webhooks, refunds, live calls) either cleans up in
+  `tearDownClass` (`remove_*_gateway` helpers) or patches `frappe.db.commit` for the class and rolls back
+  itself.
+- Importing another `TestCase` class by name into a test module makes unittest run its tests a second
+  time. Borrow its helpers inside your class body (`helper = OtherSuite.helper`, then `del OtherSuite`).
+- `frappe.clear_cache()` with no arguments also empties the per-worker `site_cache`, including
+  `get_available_payment_modes`.
+
+## Commit shape
+
+One commit per gateway in bwh_payments (`refactor: move <Gateway> onto base/live/stub service clients`),
+one matching commit in each consumer, and AGENTS.md updated when a rule changes.