Skip to content

Add PaymentLink resource - #164

Open
nicolasalmeida-stark wants to merge 1 commit into
masterfrom
feature/payment-link-python
Open

nicolasalmeida-stark wants to merge 1 commit into
masterfrom
feature/payment-link-python

Conversation

@nicolasalmeida-stark

Copy link
Copy Markdown
Contributor

No description provided.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.

Tip: disable this comment in your organization's Code Review settings.

@nicolasalmeida-stark
nicolasalmeida-stark force-pushed the feature/payment-link-python branch from 408d615 to 9caaff5 Compare July 13, 2026 17:03

@edu-stark edu-stark left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review against the Python reference and the backend services (read-only pass over every open PR, 2026-09-17). Findings below; happy to help with the rebase once the content points are addressed.

#164 — Add PaymentLink resource

  • Author: Nicolas | 1 commit (9caaff5) | 20 files, +1025 / -0
  • Fork point: 9ef6834 (2026-06-29) | 8 behind origin/master (the PR facts say 3; recount is 8)
  • Merge state: CONFLICTING — CHANGELOG.md only
  • Verdict: hold (code is merge-ready; the API it calls does not exist)

1. Backend is still not live — confirmed by re-check

The earlier "backend not live" verdict still holds. Three independent checks:

  • /Users/eduardo.santos/git/starkbank/api-v2-ms-payment-link/routes/public.py:5 — the whole public
    route table is routes = [Route("/", WelcomeHandler)]. routes/private.py exposes only
    /private/welcome. models/__init__.py is empty, gateways/ and middlewares/ are empty. The
    repo has exactly one commit: 1c5caa4 Initial commit, Massu, 2026-04-20 — an untouched ms-
    bootstrap skeleton.
  • /Users/eduardo.santos/git/starkbank/api-v1-ms-gateway/utils/pathAuth.py:12
    mapsBySource[Source.starkbank] has no "payment-link" key (the nearest neighbours are
    "payment-request" / "payment-preview"Microservice.approval, lines 73-74). Without that key
    a public /payment-link request never reaches any service.
  • A case-insensitive payment.?link grep over all 2661 api-v2-ms-* repos (20375 .py files)
    returns nothing outside the skeleton.

What does exist is the plumbing around the hole, which is why this looks half-shipped:
lib-server/server/utils/microservices.py:493 declares paymentLink = Project("payment-link", ...)
with dev/sbx/prd URLs, and lib-server/server/utils/permission.py:123-126 declares
PaymentLinkPermission.viewPaymentLinks / createPaymentLinks. Registered, permissioned, unbuilt.

Consequence: every method in this PR (create, get, query, page, update, and the three
sub-resource readers) would ship as a public SDK surface that 404s at the gateway.

2. The code itself is clean and repo-consistent

Verified rather than eyeballed (PR tree extracted to scratchpad, imported against
starkinfra/core-python with requests stubbed):

  • Endpoint derivation through starkcore.utils.api.endpoint is right for all four resource names:
    PaymentLink → payment-link, PaymentLinkLog → payment-link/log,
    PaymentLinkAttempt → payment-link/attempt, PaymentLinkAttemptLog → payment-link/attempt/log
    (the double .replace() chain happens to compose correctly for the 3-level name).
  • api_json(PaymentLink(...)) round-trips to allowedInstallments / allowedMethods / usageMode,
    expiration as 36000, nested items as camelCase — correct wire shape.
  • from_api_json on a partial link payload inside a Log does not blow up despite the required
    positional args (from_api_json back-fills missing params with None).
  • compileall -q starkbank tests passes and import starkbank passes → the PR-gate workflow
    (.github/workflows/python-app.yml: compileall + import + flake8 E9,F63,F7,F82) would be green.
  • Shapes match the house pattern: Log.__init__(self, id, created, type, errors, link) mirrors
    starkbank/invoice/log/__log.py:21; Attempt.__init__ all-positional mirrors
    starkbank/event/attempt/__attempt.py:19; empty item/__init__.py and
    allowedinstallment/__init__.py match invoice/rule/__init__.py and
    merchantsession/allowedinstallment/__init__.py.
  • Tree position (starkbank/paymentlink/{log,attempt/log,item,allowedinstallment}), README section
    order and index entry, and the CHANGELOG ### Added bullet all follow the repo's existing shape.

3. Non-blocking findings (fix before the eventual merge)

  • starkbank/paymentlink/__paymentlink.py:83 — the comment cites routes.md:214; no routes.md
    exists anywhere in the repo. A dangling internal-doc pointer in a public SDK; drop the citation and
    keep the WHY sentence.
  • starkbank/paymentlink/__paymentlink.py:81-85_parse_expiration("") raises
    RuntimeError: invalid datetime string (reproduced locally). The sibling field at line 55-56 guards
    the same API quirk with check_datetime(timestamp or None); expiration has no such guard. Cannot
    be settled either way until the service defines its response shape.
  • starkbank/paymentlink/log/__log.py:16 and starkbank/paymentlink/attempt/log/__log.py:16 document
    errors [list of strings]. PaymentLink is absent from the backend errors-shape table, so this is
    unverifiable. Suspicion: Attempt.payment is documented as "merchant-session/6543210987654321",
    and everything Merchant* carries error objects, so strings is likely wrong here. Note that master
    already says "list of strings" for merchantsession / merchantpurchase logs, so this PR is at
    least consistent with the repo's existing (also wrong) wording — a separate cleanup.
  • tests/sdk/test_payment_link*.py are live sandbox tests (assert len(links) == 10, real create).
    They can only fail until the service exists. They land in the scheduled sandbox-suite.yml, which
    its own header says has failed on every run since May 2026, so they add no new red — but they also
    prove nothing.
  • starkbank/paymentlink/allowedinstallment/__allowedinstallment.py duplicates
    starkbank/merchantsession/allowedinstallment/__allowedinstallment.py with the constructor args in
    the opposite order (count, total_amount vs total_amount, count). The repo already duplicates
    rule sub-resources per parent, so this is acceptable; worth aligning the arg order anyway.

4. Rebase

Needed: yes, 8 behind. Trivial. merge-tree reports exactly one conflict, CHANGELOG.md:15
under ## [Unreleased]: master added the VerifiedAccount / VerifiedTransfer + Fixed bullets, this
branch adds ### Added / - PaymentLink resource. Resolution is to keep both. README.md and
starkbank/__init__.py both auto-merge (master's 8 commits touched VerifiedAccount, docstring
grammar, business rules and CI — no overlap with paymentlink/).

Action

Do not merge and do not close. Park it behind the service: the SDK is the last step, not the first.
Ask Nicolas to (a) rebase onto master and resolve the one CHANGELOG hunk, (b) drop the
routes.md:214 pointer, and then hold until api-v2-ms-payment-link has real routes and models and
pathAuth.py maps "payment-link"Microservice.paymentLink. At that point re-verify the three
undetermined items against the live service: the errors shape, the expiration response type, and
whether timestamp really comes back as "".

Two-line summary

The code is genuinely merge-ready — endpoints, wire shape, tree position and docstrings all check out,
and PR CI would pass — but /payment-link routes nowhere: the service is a one-commit bootstrap
skeleton with a welcome handler, and the gateway has no path entry for it.
Hold it; it needs an 8-commit rebase with one trivial CHANGELOG conflict, and should only merge once
the backend is live, when the errors / expiration / timestamp shapes can finally be verified.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants