feature/INT-1675 - BACS Direct Debit notifications + refactor - #659
feature/INT-1675 - BACS Direct Debit notifications + refactor#659david-ruiz-cko wants to merge 6 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: Concerns worth a lookThis PR needs a human approval. Before you give it, these are the things I'd want resolved. This PR adds Bacs Direct Debit notification support and a significant instrument/serialization refactor. The code is generally well-structured, but there are concrete issues in the GsonSerializer change and a potential null-safety regression in CreateCustomerInstrumentRequest. Concerns
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 |
|
| assertEquals("customer@example.com", result.getCustomer().getEmail()); | ||
| assertEquals("John Smith", result.getCustomer().getName()); | ||
| assertEquals("cus_udst2tfldj6upmye2reztkmm4i", result.getCustomer().getId()); | ||
| assertTrue(result.getCustomer().isDefaultInstrument()); |
| assertEquals("customer@example.com", result.getCustomer().getEmail()); | ||
| assertEquals("John Smith", result.getCustomer().getName()); | ||
| assertEquals("cus_udst2tfldj6upmye2reztkmm4i", result.getCustomer().getId()); | ||
| assertTrue(result.getCustomer().isDefaultInstrument()); |
| final String json = serializer.toJson(request); | ||
| final CreateInstrumentSepaRequest result = | ||
| serializer.fromJson(json, CreateInstrumentSepaRequest.class); |
| @@ -140,7 +197,7 @@ void shouldRoundTripSerialize() { | |||
2 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.