Skip to content

[main] features 640066 Add integration events for events for CZ Adv. Payment and CZ Cash Desk - #10079

Open
v-janpopr wants to merge 6 commits into
mainfrom
features/640066-main-EventsforAdvPaymentandCashDesk
Open

[main] features 640066 Add integration events for events for CZ Adv. Payment and CZ Cash Desk#10079
v-janpopr wants to merge 6 commits into
mainfrom
features/640066-main-EventsforAdvPaymentandCashDesk

Conversation

@v-janpopr

@v-janpopr v-janpopr commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

What & why

Added the OnBeforeValidateAdvanceLetterNoCZZ and OnBeforeLookupAdvanceLetterNoCZZ integration events. These events allow subscribers to override the default validation and lookup behavior of the Advance Letter No. CZZ field on cash document lines. Added the SkipAmountsTestFields parameter to the OnBeforeCheckMandatoryFields event.

To enable partners to customize selected parts of advance payment processing and better adapt the functionality to specific business requirements. Allows subscribers to skip mandatory validation of the cash document amount fields when needed.

Linked work

Fixes AB#640066

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

Tests Performed
Verified that validation of the Advance Letter No. CZZ field can be handled by an event subscriber before standard cash document line validation is executed.
Verified that the subscriber event is raised and the entered advance letter number is retained.
Verified that lookup of the Advance Letter No. CZZ field can be handled by an event subscriber before standard checks are executed.
Verified the lookup behavior on an otherwise invalid cash document line using the Cash Document Subform CZP page.
Verifies the default (unchanged) behavior: when a Cash Document line has a zero amount and no subscriber is bound, releasing the document still fails with the standard TestField error on Amount Including VAT.
ReleaseCashDocumentSkipsAmountsWithSubscriber
Verifies the new extensibility point: when a subscriber (Cash Doc. Release Handler CZP) sets SkipAmountsTestFields := true in the new event, the amount check is bypassed during release. Since the line still has an invalid (zero) amount, release then fails later for a different reason ("no lines to release"), confirming the amount check itself was actually skipped rather than the whole release logic being short-circuited.
Verified successful compilation of the test application.

Risk & compatibility

  • Non-breaking change: The default behavior is identical
  • No data migration, upgrade, or permission changes required.

@github-actions github-actions Bot added the Finance GitHub request for Finance area label Aug 10, 2026
@github-actions github-actions Bot added this to the Version 29.0 milestone Aug 10, 2026
@v-janpopr
v-janpopr marked this pull request as ready for review August 10, 2026 13:42
@v-janpopr
v-janpopr requested a review from a team August 10, 2026 13:42
@alexei-dobriansky

Copy link
Copy Markdown
Contributor

Agentic PR Review - Round 1

Recommendation: Request Changes

What this PR does

This PR adds three integration events to the CZ localization apps: OnBeforeValidateAdvanceLetterNoCZZ and OnBeforeLookupAdvanceLetterNoCZZ in CashDocumentLineCZZ.TableExt.al, and extends the existing OnBeforeCheckMandatoryFields event in CashDocumentReleaseCZP.Codeunit.al with a new SkipAmountsTestFields parameter. The goal is to let partner extensions bypass default validation and mandatory field checks on cash document lines.

The two new events in the table extension are correctly structured: they use the standard IsHandled pattern, pass Rec/xRec where the trigger provides them, and are placed at the correct point in the trigger logic (before the main work). The SkipAmountsTestFields flag in CheckMandatoryFields is also correctly guarded and defaults to false, so the default execution path is unchanged. However, the existing OnBeforeCheckMandatoryFields event already has subscribers in partner extensions; adding a new parameter to it breaks those subscribers at compile time.

Suggestions

S1 - Adding a parameter to an existing event breaks subscribers
OnBeforeCheckMandatoryFields in CashDocumentReleaseCZP.Codeunit.al already exists and may have subscribers in partner extensions. Adding var SkipAmountsTestFields: Boolean to its signature changes the event's public contract: existing subscribers with the old two-parameter signature will fail to compile. Instead of changing the existing event, add a new event (e.g. OnBeforeCheckMandatoryFieldsExt or OnBeforeCheckMandatoryFieldsSkipAmounts) that carries the new parameter, and raise it alongside the original event so existing subscribers keep working.

S2 - No tests for new IsHandled bypass paths
The diff does not include test code for the new IsHandled paths in OnBeforeValidateAdvanceLetterNoCZZ and OnBeforeLookupAdvanceLetterNoCZZ. The PR checkbox claims tests were added, but no test files appear in the diff. The bypass path (setting IsHandled = true in a subscriber and calling exit) is a real execution branch on financial data; a codeunit test that subscribes to each event and verifies the early exit would close this gap.

Risk assessment and necessity

Risk: The signature change to OnBeforeCheckMandatoryFields is a compile-time breaking change for any partner extension that already subscribes to this event. The event is in the CZ Cash Desk localization app, which is distributed to production customers. Any such extension would stop compiling after an update that includes this PR. The two new events carry no similar risk because they are net-new. The SkipAmountsTestFields bypass skips validation of Amount Including VAT and Amount Including VAT (LCY) on a cash document header; if a subscriber sets this flag incorrectly, a zero-amount or negative-amount cash document could be released and posted.

Necessity: The extensibility goal (letting partners skip amount validation and override advance letter lookup/validate logic) is reasonable for a CZ localization app serving diverse partner scenarios. The new events for OnBeforeValidateAdvanceLetterNoCZZ and OnBeforeLookupAdvanceLetterNoCZZ are straightforward additions. The SkipAmountsTestFields capability is also justified. The only change needed is to deliver this via a new event rather than modifying the existing event signature.


[AI-PR-REVIEW] version=1 promptVersion=1 system=github pr=10079 round=1 by=alexei-dobriansky at=2026-08-11T10:29:10Z lastSha=8337702abee0eadd2d1936a9d6d73696bf2e220a reviewKey=9c9e5dfdf91d456235d2e1bdacaba62727a747837f653e3708ff15590f0dd563 suggestions=S1@08560f19,S2@0c7c3516

@alexei-dobriansky

Copy link
Copy Markdown
Contributor

Agentic PR Review - Round 2

Recommendation: Accept with Suggestions

What this PR does

This PR adds CZ Cash Desk and advance payment integration points. Since round 1, the amount FlowField calculation was moved inside the SkipAmountsTestFields branch, so a subscriber that sets the new flag now skips both calculation and TestField checks for the amount fields.

The default path is still the same: SkipAmountsTestFields starts as false, the event is raised, VATRounding() runs, and the amount FlowFields are calculated and checked before the remaining mandatory fields. The new OnBeforeValidateAdvanceLetterNoCZZ and OnBeforeLookupAdvanceLetterNoCZZ events are placed before the standard validation and lookup logic, so the IsHandled pattern can fully replace those paths when a subscriber chooses to do that.

Status of previous suggestions
ID Title Status Author response
S1 Adding a parameter to an existing event breaks subscribers Re-evaluated No author response. With prompt version 2, adding a parameter to an integration event is treated as additive, so I am not carrying this forward.
S2 No tests for new IsHandled bypass paths Not addressed No author response and no test files were added. This remains a non-blocking suggestion for this event request.
New observations (commits since round 1)

None - the new commit only moves CalcFields into the existing SkipAmountsTestFields branch, and I did not find a new issue in that hunk.

Risk assessment and necessity

Risk: Default behavior remains unchanged when no subscriber sets the new flags. The risk is limited to extensions that opt in: a subscriber can skip standard advance letter validation or amount mandatory checks, so that subscriber must do its own validation before release or posting. I did not need BaseApp publisher verification because this PR adds publishers in BCApps and does not subscribe to BaseApp events.

Necessity: The PR description gives a clear extensibility need for CZ advance payment and cash desk customizations. The scope is narrow and fits an event request. The remaining test gap is worth considering, but it does not block this low-risk extensibility change.


[AI-PR-REVIEW] version=1 promptVersion=2 system=github pr=10079 round=2 by=alexei-dobriansky at=2026-08-12T15:15:07Z lastSha=f35fa6a80f773c6fbc7b39af74ccfcf5f8ad5809 reviewKey=e4d16651fe3815788976600fdb308efd0a56a9f07cfa516a45e4c0a1c4d367dc suggestions=S1@08560f19:disputed,S2@0c7c3516:notaddressed parentRound=1

@github-actions

This comment was marked as duplicate.

@github-actions

Copy link
Copy Markdown
Contributor

$\textbf{🟡\ Medium\ Severity\ —\ Testing}$

LookupAdvanceLetterNoCanBeHandled only verifies the IsHandled=true branch via CashDocLineTestHandlerCZZ. There is no companion test exercising "Advance Letter No. CZZ".Lookup() without the subscriber bound, to confirm the original (unhandled) lookup behavior/validation still works. Since the new early exit sits before the standard document/account checks, a regression in the unhandled path would go undetected. Add an unhandled-path lookup test alongside the handled-path test.

Line mapping was unavailable, so this was posted as an issue comment.

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.32.4

@github-actions

Copy link
Copy Markdown
Contributor

$\textbf{🟡\ Medium\ Severity\ —\ Testing}$

The PR adds OnBeforeCheckMandatoryFieldsSkipAmounts and a branch that skips both amount TestField checks during cash-document release, but none of the added tests exercise SkipAmountsTestFields = true or verify the default-false path still enforces the checks. Add targeted tests that (a) release a cash document with a subscriber setting the flag, and (b) confirm the amount checks still fail when the flag is not set.

Line mapping was unavailable, so this was posted as an issue comment.

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.32.4

@alexei-dobriansky

Copy link
Copy Markdown
Contributor

Agentic PR Review - Round 3

Recommendation: Accept with Suggestions

Status of previous suggestions
ID Title Status Author response
S1 Adding a parameter to an existing event breaks subscribers Addressed The existing OnBeforeCheckMandatoryFields signature is restored, and the amount skip flag moved to a new event.
S2 No tests for new IsHandled bypass paths Addressed New tests bind a manual subscriber and verify the validate and lookup events can handle the standard path.
Current suggestions

S3 - Add tests for amount-skip event
The new tests cover the validate and lookup handled paths, but they do not cover OnBeforeCheckMandatoryFieldsSkipAmounts. Add a release test with a subscriber that sets SkipAmountsTestFields := true, and keep a default-path test that proves amount checks still run. This protects the cash document release behavior around amount validation.

What this PR does

This PR adds CZ Cash Desk and advance payment integration events. Since round 2, the existing OnBeforeCheckMandatoryFields event was restored to its old signature, a new OnBeforeCheckMandatoryFieldsSkipAmounts event was added for the amount-skip scenario, xRec was removed from OnBeforeValidateAdvanceLetterNoCZZ, and tests were added for the validate and lookup handled paths.

The public event surface is now additive: the existing event signature is no longer changed, and the new events are placed before the default validation or lookup logic they are meant to replace. The default behavior remains unchanged unless a subscriber sets a skip or handled flag.

Risk assessment and necessity

Risk: The main risk is opt-in extensibility in financial release and advance payment flows. A subscriber can skip standard amount checks or advance letter validation, so the subscriber must enforce its own business rules. I did not need BaseApp publisher verification because this PR adds publishers in BCApps and does not subscribe to BaseApp events.

Necessity: The PR description gives a clear extensibility need for CZ advance payment and cash desk customizations. The scope is narrow and fits an event request. The remaining test gap is worth fixing, but it does not block this additive event work.


[AI-PR-REVIEW] version=1 promptVersion=2 system=github pr=10079 round=3 by=alexei-dobriansky at=2026-08-13T12:54:45Z lastSha=d6267819734a37efb01988b771dd096dc0b5e1b5 reviewKey=03bc0a6cffc6b462f01e91ef4d4d7974c1c8c2047084ed1178e6f82369ef0733 suggestions=S1@08560f19:addressed,S2@0c7c3516:addressed,S3@314f6c9b:new parentRound=2

end;

[Test]
[HandlerFunctions('YesConfirmHandler')]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$\textbf{🟠\ High\ Severity\ —\ Testing}$

The two new release tests use [HandlerFunctions('YesConfirmHandler')], but that shared handler always replies true and these scenarios never enqueue or verify the expected confirm text, nor assert that all expected UI interactions were consumed. An unexpected or duplicated confirm can therefore still leave the tests green, so the scenarios do not prove the right dialog fired.

Knowledge:

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.32.4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Finance GitHub request for Finance area

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants