Skip to content

[NL][Telebank] Payment History export crashes when combining entries - #10511

Merged
dcenic merged 5 commits into
mainfrom
bugs/647493NLTelebankComineCrash
Aug 24, 2026
Merged

[NL][Telebank] Payment History export crashes when combining entries#10511
dcenic merged 5 commits into
mainfrom
bugs/647493NLTelebankComineCrash

Conversation

@dcenic

@dcenic dcenic commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

[NL][Telebank] Payment History export crashes when combining entries because of string length overflow

What & why

Introducing a standard string-length cap when saving the combined remittance text to the 140-character SEPA CT "ustrd" text element.

Linked work

Fixes AB#647493

How I validated this

  • I read the full diff and it contains only changes I intended.
  • I built the affected app(s) locally with no new analyzer warnings.
  • I ran the change in Business Central and confirmed it behaves as expected.
  • I added or updated tests for the new behavior, or explained below why none are needed.

What I tested and the outcome (required — be specific: scenarios, commands, screenshots for UI changes)

Added a new automated test for the string-length cap.

Risk & compatibility

Low risk
Compatibility: Makes the combined export more flexible instead of crashing.

@dcenic
dcenic requested a review from a team August 21, 2026 13:32
@dcenic
dcenic requested a review from a team as a code owner August 21, 2026 13:32
@dcenic
dcenic enabled auto-merge August 21, 2026 13:32
@github-actions github-actions Bot added the Team: Finance GitHub request for Finance area label Aug 21, 2026
@github-actions github-actions Bot added this to the Version 29.0 milestone Aug 21, 2026
Comment thread src/Layers/NL/Tests/Local/UTTABTelebank.Codeunit.al
@alexei-dobriansky

Copy link
Copy Markdown
Contributor

Agentic PR Review - Round 1

Recommendation: Accept with Suggestions

What this PR does

This fixes the second CopyStr call in codeunit 1222 "SEPA CT-Prepare Source". When the combined applied document numbers are longer than the Description field, the leftover text is placed in Message to Recipient. The old code passed DescriptionLen + MaxStrLen(TempGenJnlLine."Message to Recipient") as the CopyStr length. In AL the third CopyStr argument is a length, not an end position, so this asked for up to about 240 characters and could return more than 140. Assigning that result to the Text[140] Message to Recipient field overflowed and crashed the Payment History export. The new code passes MaxStrLen(TempGenJnlLine."Message to Recipient") as the length, so at most 140 characters are copied starting after the Description part. This is the correct root-cause fix and matches the standard CopyStr pattern. Dropping text past 140 characters is expected, because the SEPA CT "ustrd" element is limited to 140 characters.

Suggestions

S1 - Assert against a fixed expected value in the test
The [THEN] check builds the expected Message to Recipient value with the same CopyStr expression that the production code uses. If the start offset were wrong, both sides would change together and the test would still pass. Compare against a fixed expected substring (or a literal) so the test can catch an offset mistake.

Risk assessment and necessity

Risk: Small and local. Only the NL Telebank SEPA CT export path in codeunit 1222 is touched, plus one new unit test in codeunit 144055 "UT TAB Telebank". There is no change to a public API, event signature, or table field, and no upgrade impact. The only behavior change is that over-long remittance text is now truncated to 140 characters instead of crashing.

Necessity: Required. The old code crashes the Payment History export whenever the combined entries are longer than the Description field, which blocks a real customer workflow. The scope is right for a bug fix: a one-line correctness change plus a regression test that reproduces the crash.


[AI-PR-REVIEW] version=1 promptVersion=2 system=github pr=10511 round=1 by=alexei-dobriansky at=2026-08-21T19:07:18Z lastSha=df78a409724e831cd17f70a04650a286df7c8fc0 reviewKey=0deedf374cd407d64fe998ed466085b6997f5f25df3d8cf834bd807fecd5a125 suggestions=S1@05ca5672

Comment thread src/Layers/NL/BaseApp/Bank/DirectDebit/SEPACTPrepareSource.Codeunit.al Outdated
Comment thread src/Layers/NL/Tests/Local/UTTABTelebank.Codeunit.al
@alexei-dobriansky

Copy link
Copy Markdown
Contributor

Agentic PR Review - Round 2

Recommendation: Accept with Suggestions

What this PR does

This fixes the SEPA CT export crash in codeunit 1222 where a CopyStr length argument could exceed the Text[140] "Message to Recipient" field. The core one-character fix (using MaxStrLen(...) as the length) is correct and unchanged this round. The commits since round 1 are additive: a new RemittanceTruncated flag is set when the combined applied-document list is longer than Description plus Message to Recipient can hold, and after the loop the code raises a user notification with a "Don't show again" action and a My Notifications default registration. The truncation condition is scoped inside the existing AppliedDocNoList <> '' block, so it only fires when text is actually dropped, and the notification path is guarded by GuiAllowed(). No public API, event, or table field is changed.

Status of previous suggestions
ID Title Status Author response
S1 Assert against a fixed expected value in the test Not addressed The test file is unchanged since round 1; the [THEN] block still rebuilds the expected value with the same CopyStr expression the production code uses, so an offset error would change both sides together. No author reply.
New observations (commits since round 1)

S2 - New truncation notification path is untested
The round-2 notification logic (RemittanceTruncated and the notify call) has no test or notification handler. The existing test triggers truncation but only checks the field split, so a regression that fails to raise, or wrongly raises, the notification would not be caught. This does not block because it is a user message, not a posting path.

S3 - Silent truncation in non-interactive export runs
The notification is skipped when not GuiAllowed(), so batch or job-queue exports shorten the remittance text with no log or telemetry. The 140-character SEPA limit makes truncation expected, but background runs get no record that data was dropped. Consider a small telemetry or log entry for the headless path.

Risk assessment and necessity

Risk: Low and local. Changes stay inside codeunit 1222 (NL Telebank SEPA CT path) plus one test codeunit. The round-2 additions are additive notification UX with correct GuiAllowed() and enable guards and a standard My Notifications registration. No public API, event signature, or table field is altered, and there is no upgrade impact.

Necessity: The underlying fix is required, because the old length argument overflowed the Text[140] field and crashed a real customer export. The round-2 notification is a reasonable, low-risk usability signal that tells interactive users their remittance list was shortened. The remaining items are quality suggestions, not blockers.


[AI-PR-REVIEW] version=1 promptVersion=2 system=github pr=10511 round=2 by=alexei-dobriansky at=2026-08-24T13:27:19Z lastSha=9a85f388d71c0db01886b8ddd584c207abba08b1 reviewKey=1f73b1496c83ba89f7b419a8c4fdea3f1fcd200a5fa56cf05f7caa1351b482fb suggestions=S1@05ca5672:notaddressed,S2@55bf532f:new,S3@29eff27d:new parentRound=1

@dcenic
dcenic added this pull request to the merge queue Aug 24, 2026
Merged via the queue into main with commit 2080915 Aug 24, 2026
171 of 172 checks passed
@dcenic
dcenic deleted the bugs/647493NLTelebankComineCrash branch August 24, 2026 19:39
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