Skip to content

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

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

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

Conversation

@david-ruiz-cko

Copy link
Copy Markdown
Contributor

This pull request adds full support for Bacs Direct Debit to the SDK, including API client, documentation, endpoint registration, and integration tests. It also expands and clarifies documentation for bank instrument creation, retrieval, updating, and payment flows, especially for Bacs, SEPA, and ACH. Finally, it enhances the method for retrieving bank account field requirements with new filtering options.

Bacs Direct Debit Support:

  • Added a new Bacs class (src/api/apm-specific/bacs.js) to support sending Bacs Direct Debit pre-notifications via the /apms/bacs/notifications endpoint. This includes detailed request/response documentation and error handling.
  • Registered the Bacs endpoint in the SDK factory (src/endpoints-factory.js), main index (src/index.js), and updated the README to document access via cko.bacs. [1] [2] [3]
  • Added integration tests for Bacs pre-notification, including positive and negative cases (test/apm-specific/bacs-it.js).

Bank Instrument Documentation Improvements:

  • Expanded and clarified JSDoc comments for instrument creation, retrieval, and updating in src/api/instruments/instruments.js, with detailed field requirements and response shapes for Bacs, SEPA, and ACH variants. [1] [2] [3]

Bank Account Field Formatting Enhancements:

  • Enhanced getBankAccountFieldFormatting to accept optional filters (accountHolderType and paymentNetwork), supporting both camelCase and hyphenated parameter names.

Payments API Documentation:

  • Updated src/api/payments/payments.js documentation to clarify how to use Bacs and SEPA sources for payments, including required fields and API quirks.

@david-ruiz-cko
david-ruiz-cko requested a review from a team September 2, 2026 10:47
@agent-wall-e

agent-wall-e Bot commented Sep 2, 2026

Copy link
Copy Markdown

🟡 Risk Classification: MINOR

Approval route: AI Review + Human Approval
Rollback controls: Staged rollout + rollback

Classification reasons

  • exceeds_bounded_scope:1757>200

Operational gates

  • ✅ jira_ticket (INT-1675)
  • ✅ independent_review

Files analysed: 15


wall-e 2026.06.19-02 · policy 376219bc71e6…

@agent-wall-e

agent-wall-e Bot commented Sep 2, 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
exceeds_bounded_scope1757>200 classifying §2.1 M8 More than 200 non-test, non-doc, non-lockfile lines changed.

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 2, 2026

Copy link
Copy Markdown

🔵 Advisory review: Sound, but needs your judgement

This PR needs a human approval. The code itself reads as correct; whether it should land depends on context I don't have.

Adds a new Bacs class for Direct Debit pre-notifications, wires it into the SDK factory and type declarations, expands instrument and payments JSDoc, and adds unit tests — all technically sound, but the integration tests are entirely skipped and the implementation cannot be verified against a live Bacs-enabled sandbox.

For you to decide

  • The integration test suite (test/apm-specific/bacs-it.js) is fully wrapped in describe.skip with an explicit comment that the endpoint has never been exercised against the live API; a reviewer must decide whether merging untested-against-live-API code is acceptable.
  • The src/api/instruments/instruments.js diff is truncated — the actual implementation of getBankAccountFieldFormatting with the new query parameter is not visible, so correctness of camelCase→hyphenated key mapping (tested in instruments-bank-account-fields-unit.js) cannot be confirmed from the diff alone.
  • The payments.js JSDoc addition notes a suspected spec defect (account_holder.type capitalisation for SEPA source) and recommends a workaround pending confirmation from API owners — a reviewer should decide if shipping guidance based on an unconfirmed spec interpretation is appropriate.
  • Unit tests in test/apm-specific/bacs-unit.js and test/instruments/instruments-bacs-unit.js are present and cover happy path, field pass-through, auth header, 401, and 422 cases — coverage looks adequate for what can be tested with nock.
  • The Bacs.sendNotification implementation correctly uses post, passes this.config.sk as the auth token, and re-throws via determineError — consistent with sibling APM classes like Sepa.
  • TypeScript declaration in types/dist/api/apm-specific/bacs.d.ts uses Object for both the body and return type, which is consistent with the rest of the SDK's typing conventions.

⚠️ 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 2, 2026

Copy link
Copy Markdown

🟡 Risk Classification: MINOR

Approval route: AI Review + Human Approval
Rollback controls: Staged rollout + rollback

Classification reasons

  • exceeds_bounded_scope:249>200

Operational gates

  • ✅ jira_ticket (INT-1675)
  • ✅ independent_review

Files analysed: 15


wall-e 2026.06.19-02 · policy 376219bc71e6…

@agent-wall-e

agent-wall-e Bot commented Sep 2, 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
exceeds_bounded_scope249>200 classifying §2.1 M8 More than 200 non-test, non-doc, non-lockfile lines changed.

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 2, 2026

Copy link
Copy Markdown

@david-ruiz-cko
david-ruiz-cko merged commit 6574ed1 into master Sep 4, 2026
3 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