Skip to content

29.x: Allow bank rec. duplicated transactions surfaced in UI - #11056

Merged
Joshua (mynjj) merged 4 commits into
releases/29.xfrom
bugs/649134-29.x
Sep 9, 2026
Merged

29.x: Allow bank rec. duplicated transactions surfaced in UI#11056
Joshua (mynjj) merged 4 commits into
releases/29.xfrom
bugs/649134-29.x

Conversation

@mynjj

@mynjj Joshua (mynjj) commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

We already had a functionality that unblocks several scenarios when importing bank statements. However, there was no UI surface for it.

This change resurfaces this functionality, to give this option to users that need it. In particular, this was discovered when a DE customer was trying to import SEPA CAMT files, that have Transaction ID coming from EndToEndId, which I validated in https://www.iso20022.org/iso-20022-message-definitions?scope%5B0%5D=messages&search=camt MDR part 3 that the uniqueness in enforced by the initiator of the transaction, making imports with same id under the same file plausible.

Used the opportunity to change the captions to something a bit more precise.

Fixes AB#649134

Joshua Martínez Pineda added 2 commits September 4, 2026 14:20
@mynjj
Joshua (mynjj) requested a review from a team September 4, 2026 12:21
@mynjj
Joshua (mynjj) requested a review from a team as a code owner September 4, 2026 12:21
@github-actions github-actions Bot added the Team: Finance GitHub request for Finance area label Sep 4, 2026
@github-actions github-actions Bot added this to the Version 29.1 milestone Sep 4, 2026
@alexei-dobriansky

Copy link
Copy Markdown
Contributor

Good Sense Reviewer - Round 1

Recommendation: Accept with Suggestions

What this PR does

This change exposes an existing bank reconciliation setting that lets statement imports keep transactions with duplicate transaction IDs instead of skipping them. It adds the setting to reconciliation pages, adds a confirmation action on the payment reconciliation journal, and updates the caption and tooltip text.

The backend import behavior was already present and covered by existing import tests. The new action initializes the current reconciliation, confirms the change, then validates and modifies the same header field that the import code reads, so the chosen value is used by the existing import path.

Problem-solution fit

Fit: Strong

The reported problem is that valid bank files can contain repeated transaction IDs, but the available setting was not reachable enough from the UI. The change keeps default behavior unchanged and lets users opt in before running the existing import process.

Suggestions

S1 (🟠 Moderate): Add coverage for the toggle action
Add a small page test for the new action. It should confirm that the Yes path changes Bank Acc. Reconciliation.Allow Duplicated Transactions and the No path leaves it unchanged, so this UI surface cannot regress while the import tests still pass.

S2 (🟡 Minor): Make the disable prompt clearer
Change the disable confirmation to say duplicate transactions are currently imported, not considered. This makes the toggle state clear before a user turns the option off.

Risk assessment and necessity

Risk: The change is in bank reconciliation import UI and updates a header setting that affects which statement lines are imported. The default remains false, so existing imports continue to skip duplicates unless a user opts in. No public API or event surface changes are involved, and the relevant page variants were updated consistently.

Necessity: The change is justified because duplicate transaction IDs can be valid in real bank files, and the behavior already existed without a practical UI entry point. The scope is small and limited to surfacing that setting.


[AI-PR-REVIEW] version=1 promptVersion=4 system=github pr=11056 round=1 by=alexei-dobriansky at=2026-09-04T13:59:32Z lastSha=ecce51e29492c2161ccdafb703fea91ccc256a1f reviewKey=8a12196b9cfc8f757981169607108badedc97243d1c60f2b4922392b6847f3a7 suggestions=S1@9afbb40e,S2@7bd8b157

Comment thread src/Layers/CH/BaseApp/Bank/Reconciliation/PaymentReconciliationJournal.Page.al Outdated
dcenic
dcenic previously approved these changes Sep 7, 2026
@alexei-dobriansky

Copy link
Copy Markdown
Contributor

Good Sense Reviewer - Round 2

Recommendation: Accept with Suggestions

What this PR does

The current change exposes an existing bank reconciliation setting that lets statement imports keep transactions with duplicate transaction IDs. Since round 1, the only code-bearing update renames the payment reconciliation journal action to Allow Duplicated Transactions, which better describes that the action changes a setting instead of importing lines.

The updated caption does not change the data path. The action still initializes the reconciliation header, asks for confirmation, validates Allow Duplicated Transactions, and modifies the header that the existing import logic reads.

Status of previous suggestions
ID Title Status Author response
S1 Add coverage for the toggle action Not addressed None. No test or page-handler coverage was added for the new action.
S2 Make the disable prompt clearer Not addressed None. The disable prompt still says duplicate transactions are currently considered, not imported.
New observations (commits since round 1)

None - the new commit only changes the action caption and does not add a new issue.

Risk assessment and necessity

Risk: The touched code is in bank reconciliation import UI. A wrong value on Bank Acc. Reconciliation.Allow Duplicated Transactions could change which statement lines are imported, but the default remains false and the latest update is only a caption change. No public API, event surface, or posting flow is changed.

Necessity: The change is still justified because valid bank files can contain repeated transaction IDs, and users need a supported UI path to choose the existing behavior. The remaining issues are non-blocking: stronger UI coverage would protect the toggle, and the disable prompt should be clearer.


[AI-PR-REVIEW] version=1 promptVersion=4 system=github pr=11056 round=2 by=alexei-dobriansky at=2026-09-07T13:42:28Z lastSha=74141190feceb7959b59d0af14c0d5dbffd68e76 reviewKey=e3ceae96067ebd1e87e0519c69040da2d1b04f3408e8783428895a7efca042e9 suggestions=S1@9afbb40e:notaddressed,S2@7bd8b157:notaddressed parentRound=1

@alexei-dobriansky

Copy link
Copy Markdown
Contributor

Good Sense Reviewer - Round 3

Recommendation: Accept with Suggestions

What this PR does

The current change still exposes the bank reconciliation setting that lets statement imports keep transactions with duplicate transaction IDs. Since round 2, it also exposes the existing setting for importing posted-but-not-closed transactions by adding hidden reconciliation page fields and a payment reconciliation journal action.

The new action follows the same safe pattern as the duplicate-transaction action: it initializes the reconciliation header, asks for confirmation, validates Import Posted Transactions, and modifies the same header that the import code reads. The change is narrow and keeps existing defaults, but the new UI toggle has no direct page-test coverage.

Status of previous suggestions
ID Title Status Author response
S1 Add coverage for the toggle action Not addressed None. No test or page-handler coverage was added for the Allow Duplicated Transactions action.
S2 Make the disable prompt clearer Not addressed None. The disable prompt still says duplicate transactions are currently considered, not imported.
New observations (commits since round 2)

S3 (🟠 Moderate): Add coverage for import posted toggle
Add a page test for the new Import Posted Transactions action. It should cover the confirmation Yes path and the cancel path, so the option cannot be changed by mistake in a bank import flow.

Risk assessment and necessity

Risk: The touched code is in bank reconciliation import UI. These settings affect which statement lines can be imported, including duplicate transaction IDs and posted-but-not-closed bank ledger entries, so a wrong toggle value can affect reconciliation data. The defaults stay unchanged, and no public API, event surface, or posting routine is changed.

Necessity: The change is justified because valid bank files can contain repeated transaction IDs, and users need a supported UI path to choose the existing behavior. Surfacing the related posted-transaction setting is also useful because the import code already supports it. The remaining issues are non-blocking and are limited to clearer wording and stronger UI regression coverage.


[AI-PR-REVIEW] version=1 promptVersion=4 system=github pr=11056 round=3 by=alexei-dobriansky at=2026-09-08T19:20:52Z lastSha=0d0a96ef05f833c201c78f8f96df1b8d2a329f83 reviewKey=96951739b4b2713cf18a1bd1d1c5e748d528193e7abd0993f2b73caf47f6d35b suggestions=S1@9afbb40e:notaddressed,S2@7bd8b157:notaddressed,S3@6321e4f9:new parentRound=2

@mynjj
Joshua (mynjj) merged commit d7bab3d into releases/29.x Sep 9, 2026
374 of 384 checks passed
@mynjj
Joshua (mynjj) deleted the bugs/649134-29.x branch September 9, 2026 14:08
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.

7 participants