Skip to content

[29.X]-[CH] SEPA XML Files should append the Address 1 + '' + Address 2 fields under <PstlAdr><StrtNm> node if we use "Address 2" as Building Number for QR-Bills - #13301

Open
Fixes4BC (neeleshsinghal) wants to merge 2 commits into
releases/29.xfrom
bugs/Bug-656863-29X-SEPA-XML-Files-should-append-the-Address-1-Address-2-fields
Open

Fixes4BC (neeleshsinghal) wants to merge 2 commits into
releases/29.xfrom
bugs/Bug-656863-29X-SEPA-XML-Files-should-append-the-Address-1-Address-2-fields

Conversation

@neeleshsinghal

@neeleshsinghal Fixes4BC (neeleshsinghal) commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor

Fixes AB#656863

@neeleshsinghal
Fixes4BC (neeleshsinghal) requested a review from a team October 27, 2026 09:07
@neeleshsinghal
Fixes4BC (neeleshsinghal) requested a review from a team as a code owner October 27, 2026 09:07
@github-actions github-actions Bot added the Team: Finance GitHub request for Finance area label Oct 27, 2026
@github-actions github-actions Bot added this to the Version 29.1 milestone Oct 27, 2026
@alexei-dobriansky

Copy link
Copy Markdown
Contributor

Good Sense Reviewer - Round 1

Recommendation: Accept with Suggestions

What this PR does

This change appends Address 2 to the recipient street for Swiss payment exports when it contains the building number. The helper also handles either address field being blank and limits the combined value to 70 characters.

The reported vendor case is fixed, and both Swiss SEPA credit-transfer formats have direct buffer tests. However, the shared recipient methods also change other CH payment formats, and the current payment-export suites show that broader effect.

Problem-solution fit

Fit: Partial

The change produces the requested street and building number for Swiss SEPA exports. Its shared placement also changes non-SEPA receiver addresses, so that extra behavior needs to be confirmed or limited.

Suggestions

S1 (🟠 Moderate): Resolve the non-SEPA export impact
The shared recipient setup now appends Address 2 in AMC Banking exports too, and the affected payment-export tests fail. Limit the change to Swiss SEPA, or update those expectations after confirming that the other format should also change.

S2 (🟠 Moderate): Cover the 70-character address boundary
Add a test where Address plus Address 2 exceeds 70 characters and assert the exact result. The current tests use short values, so they do not protect the required truncation behavior.

Risk assessment and necessity

Risk: The shared table methods feed several CH payment formats and four recipient account types. The concatenation is simple, but it currently changes AMC Banking output and leaves those payment-export suites failing; the 70-character boundary is also not covered.

Necessity: The change is needed because Swiss QR-Bill data stores the building number in Address 2, and omitting it creates an incomplete creditor street in the SEPA file. The production change should stay limited to formats where the combined street is intended.


[AI-PR-REVIEW] version=1 promptVersion=4 system=github pr=13301 round=1 by=alexei-dobriansky at=2026-10-27T14:11:17Z lastSha=d09f27fe35adc2ccf1e3b8b775f6934b7bab8f16 reviewKey=ce95ada3925c25b9bd0f736802285d02db79a92ebb892dd4fd2a33ac8266151b suggestions=S1@e4a1756e,S2@79db27cc

@alexei-dobriansky

Copy link
Copy Markdown
Contributor

Good Sense Reviewer - Round 2

Recommendation: Accept with Suggestions

What this PR does

Appends Address 2 to the creditor street for Swiss SEPA credit transfers, with a space between nonempty fields and a 70-character limit. The only own-new change in round 2 adds a SwissExport guard so other export formats keep the original address-only behavior.

The new SwissExport guard restores Address-only output for other export formats while preserving concatenation in both Swiss credit-transfer formats. The export buffer sets the flag before choosing the recipient, and both XMLports consume the resulting Recipient Address. Existing tests cover short combined values and blank Address 2; S2's 70-character boundary case is still missing.

Problem-solution fit

Fit: Strong

The reported building-number omission is corrected for both Swiss credit-transfer formats, and the follow-up change confines concatenation to the intended export path. The remaining 70-character boundary coverage request is non-blocking.

Status of previous suggestions
ID Title Status Author response / evidence
S1 Resolve the non-SEPA export impact Addressed The new SwissExport guard prevents Address 2 from being appended in AMC Banking exports.
S2 Cover the 70-character address boundary Not addressed No boundary test was added. Add a case where Address plus Address 2 exceeds 70 characters and assert the exact truncated value.
New observations (commits since round 1)

None - no new issues found in the changes since the previous round.

Risk assessment and necessity

Risk: The helper is shared by several recipient types, but concatenation is now limited to explicitly enabled Swiss exports. Amounts, posting logic, stored schema, and public signatures are unchanged. The remaining non-blocking gap is regression coverage of the truncation boundary.

Necessity: Swiss payment exports need the building number when it is stored separately in Address 2. Without the concatenation, the creditor street is incomplete; the guard is needed to avoid changing other formats while fixing that scenario.


[AI-PR-REVIEW] version=1 promptVersion=4 system=github pr=13301 round=2 by=alexei-dobriansky at=2026-10-28T20:19:12.1453570Z lastSha=85ba1654736e9e31776b89d6b4b90fcaa96b1783 reviewKey=2770e27619147e679332abaa0a5ca6b6a044c5ac978a7558ecdcb339d0f6a9fd suggestions=S1@e4a1756e:addressed,S2@79db27cc:notaddressed parentRound=1

This branch was previously deployed

1 inactive deployment
triage — 85ba1654 Deployed Oct 31, 2026 by neeleshsinghal via Classify team ownership #9646
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Team: Finance GitHub request for Finance area

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants