Skip to content

feature/INT-1675 - BACS Direct Debit notifications + refactor - #371

Merged
david-ruiz-cko merged 4 commits into
masterfrom
feature/INT-1675
Sep 4, 2026
Merged

feature/INT-1675 - BACS Direct Debit notifications + refactor#371
david-ruiz-cko merged 4 commits into
masterfrom
feature/INT-1675

Conversation

@david-ruiz-cko

@david-ruiz-cko david-ruiz-cko commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Breaking changes (check at the end image)

This pull request introduces comprehensive support for Bacs and ACH Direct Debit instruments in the SDK, including new client classes, request models, and type definitions. It also updates core enums to recognize these new instrument and payment source types. The changes enable storing, configuring, and sending notifications for Bacs and ACH instruments, and ensure the SDK aligns with the latest API specifications.

Bacs Direct Debit Support:

  • Added BacsClient for sending Bacs Direct Debit pre-notifications, with corresponding request and notification type classes (BacsNotificationRequest, BacsNotificationType). Also exposed getBacsClient() in CheckoutApi. [1] [2] [3] [4] [5] [6] [7]
  • Introduced models for creating Bacs instruments: CreateBacsInstrumentRequest, CreateBacsInstrumentAccount, CreateBacsInstrumentData, CreateBacsAccountHolder, CreateBacsBillingAddress, and BacsPaymentType. [1] [2] [3] [4] [5] [6]

ACH Direct Debit Support:

  • Added models for creating ACH instruments: CreateAchInstrumentRequest, CreateAchInstrumentData, CreateAchAccountHolder, and AchAccountType. [1] [2] [3] [4]

SEPA Direct Debit Support:

  • Added CreateSepaAccountHolder for SEPA instrument account holder details.

Core Enum Updates:

  • Updated InstrumentType and PaymentSourceType to include bacs and ach as valid types, reflecting new instrument and payment source options. [1] [2] [3] [4]

These changes provide the necessary models and client interfaces to support Bacs and ACH Direct Debit instruments, improve maintainability, and ensure compatibility with the current API.

Breaking changes imageimageimage

This release is breaking and ships as 6.0.0 (already bumped in this branch via the merged
Release 6.0.0 (#370) commit: CheckoutUtils::PROJECT_VERSION and version.json).

The instruments models were reshaped so that each scheme (SEPA, Bacs, ACH) and each operation
(store, update) has its own type. Previously a single InstrumentData and the shared
Checkout\Common\AccountHolder were reused across operations they did not match, which exposed
fields the API rejects and hid fields it requires.

Removed classes

Removed Replacement
Checkout\Instruments\Create\InstrumentData Checkout\Instruments\Create\CreateSepaInstrumentData (store) / Checkout\Instruments\Update\UpdateSepaInstrumentData (update)

The removed class held account_number, country, currency, payment_type, mandate_id and
date_of_signature. All six exist on the replacements, plus type (the SEPA mandate type), which
the specification declares and the old class was missing.

Retyped properties

All three are on Checkout\Instruments\Create\CreateSepaInstrumentRequest:

Property Before After
$instrument_data Checkout\Instruments\Create\InstrumentData CreateSepaInstrumentData
$account_holder Checkout\Common\AccountHolder (15 properties) CreateSepaAccountHolder (5 properties)
$customer \Checkout\Instruments\Create\UpdateCustomerRequest CreateCustomerInstrumentRequest

The $customer entry is a documentation correction rather than a runtime change: the old @var
pointed at a class that does not exist in that namespace (the real one lives in
Checkout\Instruments\Update). Static analysis will now flag callers who pass the old type.

Migration

// Before
$instrumentData = new InstrumentData();
$instrumentData->account_number = "FR7630006000011234567890189";
$instrumentData->country        = Country::$FR;
$instrumentData->currency       = Currency::$EUR;
$instrumentData->payment_type   = PaymentType::$recurring;   // sent "Recurring" - rejected

$accountHolder = new AccountHolder();
$accountHolder->first_name      = "John";
$accountHolder->last_name       = "Wick";
$accountHolder->phone           = $phone;                    // not in the SEPA schema
$accountHolder->billing_address = $address;

$request = new CreateSepaInstrumentRequest();
$request->instrument_data = $instrumentData;
$request->account_holder  = $accountHolder;

// After
$instrumentData = new CreateSepaInstrumentData();
$instrumentData->type           = SepaMandateType::$core;    // new: was missing entirely
$instrumentData->account_number = "FR7630006000011234567890189";
$instrumentData->country        = Country::$FR;
$instrumentData->currency       = Currency::$EUR;
$instrumentData->payment_type   = SepaPaymentType::$recurring;  // sends "recurring"

$billingAddress = new CreateSepaBillingAddress();
$billingAddress->address_line1 = "Evergreen Terrace";
$billingAddress->address_line2 = "742";
$billingAddress->city          = "Paris";
$billingAddress->zip           = "75000";
$billingAddress->country       = Country::$FR;

$accountHolder = new CreateSepaAccountHolder();
$accountHolder->first_name      = "John";
$accountHolder->last_name       = "Wick";
$accountHolder->billing_address = $billingAddress;

$request = new CreateSepaInstrumentRequest();
$request->instrument_data = $instrumentData;
$request->account_holder  = $accountHolder;

@david-ruiz-cko
david-ruiz-cko requested a review from a team September 1, 2026 09:37
@agent-wall-e

agent-wall-e Bot commented Sep 1, 2026

Copy link
Copy Markdown

🔴 Risk Classification: MAJOR

Approval route: AI Review + Human Approval Required
Rollback controls: Change-freeze window + documented rollback plan

Classification reasons

  • file_removed:lib/Checkout/Instruments/Create/InstrumentData.php

Operational gates

  • ✅ jira_ticket (INT-1675)
  • ✅ independent_review

Files analysed: 46


wall-e 2026.06.19-02 · policy 376219bc71e6…

@agent-wall-e

agent-wall-e Bot commented Sep 1, 2026

Copy link
Copy Markdown
🔬 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.

Reason code Kind Clause Meaning
file_removedlib/Checkout/Instruments/Create/InstrumentData.php classifying §2.1 M1 A non-doc/test/lockfile was deleted — destructive change.

Kinds:

  • classifying — this rule contributed to the chosen tier.
  • informational — context only; did not by itself decide the tier.

See issue #3 for the proposal to formalise this map as Appendix A of the standards doc.

wall-e 2026.06.19-02 · debug

@agent-wall-e

agent-wall-e Bot commented Sep 1, 2026

Copy link
Copy Markdown

🟢 Advisory review: Looks good to me

This PR still needs a human approval — wall-e cannot auto-approve it. For what it's worth, I read the diff and found nothing I'd block on.

This PR adds Bacs/ACH Direct Debit instrument models, a BacsClient for pre-notifications, scheme-specific billing address and account holder classes, and removes the old shared InstrumentData class as a breaking change for v6.0.0. The implementation is internally consistent, well-documented, and matches its stated intent.

What I checked

  • BacsClient correctly builds the path apms/bacs/notifications and uses secretKey authorization, consistent with the stated NAS-only constraint.
  • BacsNotificationRequest covers all 10 properties documented by the API spec, and the serialization tests verify both full population and optional-field omission.
  • The removal of InstrumentData and the retyping of CreateSepaInstrumentRequest properties ($instrument_data, $account_holder, $customer) is correctly implemented and the breaking-change migration is documented in the PR description.
  • CreateBacsInstrumentRequest, CreateAchInstrumentRequest, and their Update counterparts all call parent::__construct with the correct InstrumentType constant (bacs/ach/sepa).
  • BacsPaymentType uses capitalized wire values (Recurring/Regular) while SepaPaymentType uses lowercase (recurring/regular), matching the noted API difference between the two schemes.
  • AchSourceAccountType (savings/checking/cash) and AchAccountType (savings/checking) are correctly kept as separate classes, and the test asserts that current and cash are absent from AchAccountType.
  • getBacsClient() is registered on CheckoutApi only (not CheckoutApmApi or the previous API), and tests assert both presence on CheckoutApi and absence from the other two.
  • The AccountHolderSepa type note about capitalization of the type field includes an honest 'pending confirmation from API owners' caveat, which a reviewer should verify is resolved before stable release.
  • The integration test is marked skipped and requires a Bacs-enabled merchant account, which is the correct handling for a sandboxed endpoint not universally available.

⚠️ The diff was too large to read in full, so this review covers only part of the change.


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

@agent-wall-e

agent-wall-e Bot commented Sep 1, 2026

Copy link
Copy Markdown

🔴 Risk Classification: MAJOR

Approval route: AI Review + Human Approval Required
Rollback controls: Change-freeze window + documented rollback plan

Classification reasons

  • file_removed:lib/Checkout/Instruments/Create/InstrumentData.php

Operational gates

  • ✅ jira_ticket (INT-1675)
  • ✅ independent_review

Files analysed: 46


wall-e 2026.06.19-02 · policy 376219bc71e6…

@agent-wall-e

agent-wall-e Bot commented Sep 1, 2026

Copy link
Copy Markdown
🔬 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.

Reason code Kind Clause Meaning
file_removedlib/Checkout/Instruments/Create/InstrumentData.php classifying §2.1 M1 A non-doc/test/lockfile was deleted — destructive change.

Kinds:

  • classifying — this rule contributed to the chosen tier.
  • informational — context only; did not by itself decide the tier.

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 david-ruiz-cko changed the title BACS Direct Debit notifications + refactor feature/INT-1675 - BACS Direct Debit notifications + refactor Sep 1, 2026
@agent-wall-e

agent-wall-e Bot commented Sep 3, 2026

Copy link
Copy Markdown

🔴 Risk Classification: MAJOR

Approval route: AI Review + Human Approval Required
Rollback controls: Change-freeze window + documented rollback plan

Classification reasons

  • file_removed:lib/Checkout/Instruments/Create/InstrumentData.php

Operational gates

  • ✅ jira_ticket (INT-1675)
  • ✅ independent_review

Files analysed: 51


wall-e 2026.06.19-02 · policy 376219bc71e6…

@agent-wall-e

agent-wall-e Bot commented Sep 3, 2026

Copy link
Copy Markdown
🔬 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.

Reason code Kind Clause Meaning
file_removedlib/Checkout/Instruments/Create/InstrumentData.php classifying §2.1 M1 A non-doc/test/lockfile was deleted — destructive change.

Kinds:

  • classifying — this rule contributed to the chosen tier.
  • informational — context only; did not by itself decide the tier.

See issue #3 for the proposal to formalise this map as Appendix A of the standards doc.

wall-e 2026.06.19-02 · debug

@sonarqubecloud

sonarqubecloud Bot commented Sep 3, 2026

Copy link
Copy Markdown

@david-ruiz-cko
david-ruiz-cko merged commit 765475f into master Sep 4, 2026
6 checks passed
@david-ruiz-cko
david-ruiz-cko deleted the feature/INT-1675 branch September 4, 2026 09:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants