feature/INT-1675 - BACS Direct Debit notifications + refactor - #659
feature/INT-1675 - BACS Direct Debit notifications + refactor#659david-ruiz-cko wants to merge 18 commits into
Conversation
🟡 Risk Classification: MINORApproval route: AI Review + Human Approval Classification reasons
Operational gates
Files analysed: 62 wall-e 2026.06.19-02 · policy |
🔬 Debug — why this classification?Each reason code emitted by the classifier, its source clause in the AI in SDLC Control Framework, and what it means.
Kinds:
See issue #3 for the proposal to formalise this map as Appendix A of the standards doc. wall-e 2026.06.19-02 · debug |
🔵 Advisory review: Sound, but needs your judgementThis PR needs a human approval. The code itself reads as correct; whether it should land depends on context I don't have. This PR adds Bacs Direct Debit support (new BacsClient, instrument types, serialization registrations) and bumps the version from 7.16.2 to 8.0.0, with several intentional breaking changes documented in the PR body. The code visible in the diff is technically sound, but the breaking changes and major version bump require human sign-off on blast radius and migration readiness. For you to decide
This is not an approval. wall-e cannot auto-approve this PR — it is an opinion to help whoever does. Advisory review · us.anthropic.claude-sonnet-4-6 · wall-e 2026.06.19-02 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
🟡 Risk Classification: MINORApproval route: AI Review + Human Approval Classification reasons
Operational gates
Files analysed: 79 wall-e 2026.06.19-02 · policy |
🔬 Debug — why this classification?Each reason code emitted by the classifier, its source clause in the AI in SDLC Control Framework, and what it means.
Kinds:
See issue #3 for the proposal to formalise this map as Appendix A of the standards doc. wall-e 2026.06.19-02 · debug |
🟡 Risk Classification: MINORApproval route: AI Review + Human Approval Classification reasons
Operational gates
Files analysed: 92 wall-e 2026.06.19-02 · policy |
🔬 Debug — why this classification?Each reason code emitted by the classifier, its source clause in the AI in SDLC Control Framework, and what it means.
Kinds:
See issue #3 for the proposal to formalise this map as Appendix A of the standards doc. wall-e 2026.06.19-02 · debug |
🟡 Risk Classification: MINORApproval route: AI Review + Human Approval Classification reasons
Operational gates
Files analysed: 100 wall-e 2026.06.19-02 · policy |
🔬 Debug — why this classification?Each reason code emitted by the classifier, its source clause in the AI in SDLC Control Framework, and what it means.
Kinds:
See issue #3 for the proposal to formalise this map as Appendix A of the standards doc. wall-e 2026.06.19-02 · debug |
🟡 Risk Classification: MINORApproval route: AI Review + Human Approval Classification reasons
Operational gates
Files analysed: 100 wall-e 2026.06.19-02 · policy |
🔬 Debug — why this classification?Each reason code emitted by the classifier, its source clause in the AI in SDLC Control Framework, and what it means.
Kinds:
See issue #3 for the proposal to formalise this map as Appendix A of the standards doc. wall-e 2026.06.19-02 · debug |
david-ruiz-cko
changed the title
david-ruiz-cko
changed the title
🟡 Risk Classification: MINORApproval route: AI Review + Human Approval Classification reasons
Operational gates
Files analysed: 100 wall-e 2026.06.19-02 · policy |
🔬 Debug — why this classification?Each reason code emitted by the classifier, its source clause in the AI in SDLC Control Framework, and what it means.
Kinds:
See issue #3 for the proposal to formalise this map as Appendix A of the standards doc. wall-e 2026.06.19-02 · debug |
…-out (#649) * feat: require environmentSubdomain or an explicit useLegacyDomain opt-out The merchant-specific subdomain is how merchants should reach the API, but it was optional and an unset value silently fell back to api.checkout.com, so a forgotten subdomain looked exactly like a deliberate opt-out and the SDK could not warn about either. Callers must now choose: set environmentSubdomain, or call the already-deprecated useLegacyDomain(). Both, or neither, throws. An invalid subdomain now throws instead of being quietly ignored, which is a second breaking change: callers passing a malformed value are currently served by the shared host and never find out. environmentSubdomain no longer needs environment() to be set first, since the EnvironmentSubdomain is now built when the configuration is assembled. The Previous (ABC) platform predates merchant-specific subdomains and stays exempt via requiresEnvironmentSubdomain(). Mirrors checkout-sdk-net#590. Refs INT-1688. * test: route every client the suite builds through TestDomainConfiguration Seven integration fixtures build their own clients outside SandboxTestFixture (OAuth, Issuing, Accounts, Accounts payout schedules, APM previews, card metadata), so the mandatory subdomain would have failed them at construction. They now share TestDomainConfiguration.configureDomain, which uses the shared hosts. Applying the merchant-specific subdomain instead looked better, since it is the path merchants are being moved to, but the sandbox OAuth clients are not provisioned for it: .NET CI failed 224 integration tests with invalid_client when the token request went to {subdomain}.access.sandbox.checkout.com. The reason is recorded on the class so nobody repeats the experiment. * test: route the remaining OAuth integration tests through TestDomainConfiguration CI runs the full suite, so three OAuthTestIT cases that build their own client were still failing at construction. Local runs excluded integration tests, which is why they were missed. * test: stop passing a subdomain to the Previous platform builder Flagged in review, and fair: a bulk edit added the subdomain to the Previous (ABC) builder, the one platform that is exempt from needing one. That made the test misleading and, worse, removed the only coverage of the exemption actually working. It builds without a subdomain again. * test: add a switch to run the suite against the merchant subdomain The suite could only run against the shared hosts, so the subdomain path this PR makes mandatory had no integration coverage. Reviewers flagged that on every SDK, and it is the right thing to flag. The domain helper now has two modes. Default is unchanged, the shared hosts, because the sandbox OAuth clients are not provisioned for the subdomain and the token request returns invalid_client. Set CHECKOUT_TEST_USE_SUBDOMAIN=true and the suite runs against CHECKOUT_MERCHANT_SUBDOMAIN instead, so once sandbox is provisioned like production it is a one-line change in the workflows, already wired and documented, rather than a rewrite of every fixture. The switch is deliberately separate from CHECKOUT_MERCHANT_SUBDOMAIN, which CI already exports: provisioning should drive the behaviour, not the presence of a secret. * revert: leave the version bump to the release Versions are bumped on master during the release, not in a feature branch, per the release workflow. This branch should carry only the change itself; the major bump is classified and applied when the release is cut. * revert: drop the test domain helpers and the workflow variable Two problems with the previous approach. It needed a new variable in 21 workflow files, which is not viable without access to create secrets. And it wrapped the builder chain in a configureDomain helper that is not part of the public API, so the tests stopped looking like the code a merchant would actually write. Every fixture now calls the real opt-out inline, in the chain, with a comment saying why: the sandbox OAuth clients are not provisioned for the merchant-specific subdomain, so the token request comes back invalid_client. When sandbox is provisioned, those calls become the subdomain setter. The unit tests covering all four combinations are untouched: they already used the public API directly. * chore: align the version file with master, the major bump happens at release * test: run the issuing and payout schedule suites with the default OAuth client The dedicated sandbox clients are not provisioned for the merchant subdomain; the default client now carries every scope the suites need. * Revert "test: run the issuing and payout schedule suites with the default OAuth client" This reverts commit 2e14a23. * fix: address the review findings on the subdomain PR - Sandbox test fixture: static-keys clients now use the subdomain from CHECKOUT_MERCHANT_SUBDOMAIN; the legacy-domain opt-out stays only on the OAuth client (sandbox OAuth clients lack subdomain provisioning), and the Previous platform applies neither since it is exempt - Hoist the subdomain validation regex to a private static final constant (Sonar S4248) - Reword the subdomain error messages: typically your client ID excluding the cli_ prefix - Treat environmentSubdomain(null) as unset and fail at build time with the standard required-subdomain error, matching the other SDKs; test added - README: note that Private Link merchants use their pl- prefixed subdomain, which the SDK also accepts * fix: reject an explicit authorization URI combined with the environment subdomain * fix: suppress the deliberate legacy-domain deprecation warnings in tests Also fixes a real miss: CardMetadataIT.createStaticKeyApi() used the legacy opt-out on a static-keys client, which never calls the token endpoint and so was never blocked by the sandbox OAuth provisioning gap - it now runs against the real merchant subdomain like the other static-keys fixtures. The other six sites are genuine OAuth clients whose sandbox clients lack subdomain provisioning; @SuppressWarnings("deprecation") marks that as deliberate instead of leaving 15 code-scanning findings open.
- The merchant-specific subdomain (environmentSubdomain) is now required. Set it, or call the deprecated useLegacyDomain() to keep using the shared checkout.com hosts. - An invalid subdomain now throws instead of being silently ignored. - Add optional amount to VoidRequest to support partial voids. - Add the ISV (SaaS seller) payout schedule fields balanceMinimum, carryForwardEnabled and paymentInstrumentId.
🟡 Risk Classification: MINORApproval route: AI Review + Human Approval Classification reasons
Operational gates
Files analysed: 100 wall-e 2026.06.19-02 · policy |
🔬 Debug — why this classification?Each reason code emitted by the classifier, its source clause in the AI in SDLC Control Framework, and what it means.
Kinds:
See issue #3 for the proposal to formalise this map as Appendix A of the standards doc. wall-e 2026.06.19-02 · debug |
…ut-sdk-java into feature/INT-1675
🟡 Risk Classification: MINORApproval route: AI Review + Human Approval Classification reasons
Operational gates
Files analysed: 100 wall-e 2026.06.19-02 · policy |
🔬 Debug — why this classification?Each reason code emitted by the classifier, its source clause in the AI in SDLC Control Framework, and what it means.
Kinds:
See issue #3 for the proposal to formalise this map as Appendix A of the standards doc. wall-e 2026.06.19-02 · debug |
🟡 Risk Classification: MINORApproval route: AI Review + Human Approval Classification reasons
Operational gates
Files analysed: 100 wall-e 2026.06.19-02 · policy |
🔬 Debug — why this classification?Each reason code emitted by the classifier, its source clause in the AI in SDLC Control Framework, and what it means.
Kinds:
See issue #3 for the proposal to formalise this map as Appendix A of the standards doc. wall-e 2026.06.19-02 · debug |
|
3 participants
Breaking changes (check at the end
)
This pull request introduces Bacs Direct Debit support to the SDK, enabling sending pre-notification (advance notice) events and full instrument lifecycle management for Bacs payment methods. The changes include new client interfaces and implementations, updates to serialization logic, and enhancements to the API surface. The most important changes are grouped below.
Bacs Direct Debit Client and API Enhancements:
BacsClientinterface and its implementationBacsClientImplto support sending Bacs Direct Debit pre-notification events, with both asynchronous and synchronous methods. This client is configured to use secret key authorization only. [1] [2]BacsClientinto the main API (AbstractCheckoutApmApiandCheckoutApmApi), making it accessible asbacsClient()alongside other APM clients. [1] [2] [3] [4]Serialization and Instrument Support:
GsonSerializerto handle new Bacs-related classes, including request and response types for Bacs sources and instruments, ensuring correct (de)serialization of Bacs payment flows. This includes registering new instrument response subtypes for Bacs and updating the runtime type adapter factories to support polymorphic deserialization. [1] [2] [3] [4]Documentation:
README.mddemonstrating how to use the new Bacs Direct Debit features, including sending pre-notifications, creating, updating, and retrieving Bacs instruments, and taking payments using stored Bacs instruments.Breaking changes
Property moved off a base class
GetInstrumentResponse.accountHolder removed from the base, now on each concrete variant. Forced by Gson: two fields mapping to the same JSON name throw at runtime, and the spec gives all five retrieve variants a different account-holder shape. Base-typed getAccountHolder() no longer compiles and needs a cast to the concrete variant.
Changed field types
instruments.create.InstrumentData.paymentType: payments.PaymentType → SepaPaymentType. The old enum serialized "Recurring"/"Regular"; StoreSepaInstrumentRequest pins lowercase recurring/regular, so SEPA store could not send a valid value. Callers passing PaymentType.RECURRING must switch enum — though no such call can currently succeed against the API.
GetSepaInstrumentResponse.instrumentData: InstrumentData → GetSepaInstrumentData, which adds the mandate type (Core/B2B) the old type omitted.
Wire value changed
UpdateInstrumentBankAccountRequest sent "type":"token", now sends "type":"bank_account". token is absent from the update discriminator mapping entirely, so PATCH /instruments/{id} could not be validated as a bank-account update. No test referenced the class before this PR.
Deserialization behaviour changed (throw → object)
type: "bacs" and type: "ach" on the create and get instrument responses previously threw JsonParseException, because the three RuntimeTypeAdapterFactory registrations have no default subtype. They now deserialize to the concrete type. Anyone catching that exception as control flow is affected. The factories remain strict for unmodelled types — deliberately, per decision A2.
Deprecated, not removed (non-breaking)
PaymentSourceType.SEPA — zero callers; use ID for the previous platform, SEPAV4 for the current one
CreateInstrumentResponse.customer — moves to the token and bank_account variants in the next major
Correction to something I told you earlier
previous.RequestSepaSource switching from PaymentSourceType.SEPA to PaymentSourceType.ID is wire-neutral, not a breaking change. Both constants already carried @SerializedName("id") on master, so the bytes are identical. I listed it as a wire change twice, including in the ledger — now fixed there too.