Skip to content

[Bug][SubscriptionBilling] Contract lines of type G/L Account never post to the selected G/L account (backport to releases/28.x) - #10091

Closed
Miljan Milosavljević (miljance) wants to merge 2 commits into
microsoft:releases/28.xfrom
miljance:SBGLAccountContractLinePostingFor28.x
Closed

[Bug][SubscriptionBilling] Contract lines of type G/L Account never post to the selected G/L account (backport to releases/28.x)#10091
Miljan Milosavljević (miljance) wants to merge 2 commits into
microsoft:releases/28.xfrom
miljance:SBGLAccountContractLinePostingFor28.x

Conversation

@miljance

@miljance Miljan Milosavljević (miljance) commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

What & why

  • This is a backport of PR for 29: to 28.x [Bug][SubscriptionBilling] Contract lines of type G/L Account never post to the selected G/L account #9905
  • post G/L Account contract lines to their own account. Subscription Billing contract lines of type "G/L Account" always posted to the Customer/Vendor Subscription Contract Account from the General Posting Setup, ignoring the account actually selected on the contract line.
  • Without deferrals, CustomerDeferralsMngmt/VendorDeferralsMngmt now skip the General Posting Setup account lookup for "G/L Account" lines so the invoice posts directly to the line's account.
  • With deferrals, a new "G/L Account No." field (27) on Cust./Vend. Sub. Contract Deferral carries the account from the sales/purchase line, and is surfaced on the Customer/Vendor Contract Deferrals pages.
  • ContractDeferralsRelease now posts to that account when releasing the deferral, falling back to the General Posting Setup account when it is blank, and only requires the General Posting Setup account to be filled when no contract-line account is available.
  • Added TDD tests to CustomerDeferralsTest and VendorDeferralsTest covering both the immediate-posting and deferral-release paths, with a shared fixture in ContractTestLibrary.

Linked work

Fixes #9885

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

  • This backport is a clean cherry-pick of the commit from [Bug][SubscriptionBilling] Contract lines of type G/L Account never post to the selected G/L account #9905 onto releases/28.x — the diff is identical to the original PR's diff, with no conflicts and no context drift.
  • The change itself (including the new tests in CustomerDeferralsTest and VendorDeferralsTest) was built, run, and validated on main as part of the original PR: posted contract invoices for G/L Account contract lines with and without deferrals and confirmed the line's G/L account is used for posting and for deferral release.
  • The backport itself was not built or test-run against a 28.x environment.

Risk & compatibility

  • Intentional behavior change: contract lines of type "G/L Account" without
    deferrals now post to the G/L account selected on the line instead of the
    Cust./Vend. Subscription Contract Account from General Posting Setup. Anyone
    who relied on the old (incorrect) routing will see revenue/cost on different
    accounts going forward.
  • Schema: purely additive — new field 27 "G/L Account No." on the
    Cust./Vend. Sub. Contract Deferral tables. No breaking changes, no upgrade
    code needed: the field is blank on pre-existing deferral rows, and release
    falls back to the General Posting Setup account, so existing deferrals
    release exactly as before.
  • Relaxed validation: Contract Deferrals Release no longer requires the
    General Posting Setup contract account when the deferral carries its own
    account — setups with a blank GPS account now succeed where they previously
    errored (intended).
  • Credit memos: reversal copies the deferral record, so the new field is
    propagated automatically.
  • Test library: ContractTestLibrary got a new overload; the existing
    CreateServiceObjectForGLAccountWithServiceCommitments signature is unchanged.
  • No permissions, telemetry, or feature-flag impact. No follow-up work planned.
  • The cherry-pick onto releases/28.x applied cleanly with no conflict resolution.

🤖 Generated with Claude Code

Subscription Billing contract lines of type "G/L Account" always
posted to the Customer/Vendor Subscription Contract Account from the
General Posting Setup, ignoring the account actually selected on the
contract line.

- Without deferrals, CustomerDeferralsMngmt/VendorDeferralsMngmt now
  skip the General Posting Setup account lookup for "G/L Account"
  lines so the invoice posts directly to the line's account.
- With deferrals, a new "G/L Account No." field (27) on
  Cust./Vend. Sub. Contract Deferral carries the account from the
  sales/purchase line, and is surfaced on the Customer/Vendor
  Contract Deferrals pages.
- ContractDeferralsRelease now posts to that account when releasing
  the deferral, falling back to the General Posting Setup account
  when it is blank, and only requires the General Posting Setup
  account to be filled when no contract-line account is available.
- Added TDD tests to CustomerDeferralsTest and VendorDeferralsTest
  covering both the immediate-posting and deferral-release paths,
  with a shared fixture in ContractTestLibrary.

Fixes microsoft#9885

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@github-actions github-actions Bot added AL: Apps (W1) Add-on apps for W1 From Fork Pull request is coming from a fork Team: Finance GitHub request for Finance area needs-approval Workflow runs require maintainer approval to start labels Aug 10, 2026
@alexei-dobriansky

Copy link
Copy Markdown
Contributor

Agentic PR Review - Round 1

Recommendation: Accept with Suggestions

What this PR does

This is a backport of PR #9905 to releases/28.x. It fixes a bug where Subscription Contract lines of type "G/L Account" always posted to the GPS (General Posting Setup) account instead of the G/L account selected on the contract line.

For lines without deferrals, the fix adds an early exit in CustomerDeferralsMngmt and VendorDeferralsMngmt inside the else branch of CreateContractDeferrals(). This means the GPS override is skipped and standard BC posting uses the line's own G/L account directly — which is the correct behavior.

For lines with deferrals, a new field 27 G/L Account No. on both deferral tables (CustSubContractDeferral, VendSubContractDeferral) captures the account from the sales/purchase line at deferral creation time. ContractDeferralsRelease uses a new GetContractAccount helper that prefers the deferral's own account over the GPS account when releasing. When the field is blank (pre-existing deferrals, non-G/L Account lines), the GPS account is used as before.

The two paths are handled correctly and symmetrically for Customer and Vendor. CheckGenPostingSetup skips the TestField validation on the GPS contract account when the deferral already carries its own account, which removes a false validation error for setups with a blank GPS account.

Suggestions

S1 - Credit memo path has no explicit test
The PR description states that credit memo reversal copies the deferral record, so the new G/L Account No. field propagates automatically. This is likely correct, but no test verifies that a credit memo for a G/L Account contract line reverses to the correct account (both with and without deferrals). Since this is a financial posting path and the backport has not been built or run on a 28.x environment, a test for the reversal path would reduce risk.

Risk assessment and necessity

Risk: This is an intentional behavior change: G/L Account contract lines without deferrals now post to the account on the line instead of the GPS Customer/Vendor Subscription Contract Account. Customers who relied on the old (incorrect) routing will see amounts on different accounts going forward. The schema change (field 27) is purely additive with a safe GPS fallback, so upgrade is safe. The backport has not been built or test-run against a 28.x environment, which is a small extra risk for a financial area.

Necessity: Issue #9885 clearly describes the bug and the expected behavior. The fix is targeted — it addresses both the non-deferral and deferral paths, adds tests for both, and does not touch unrelated code. The scope is right for a bug fix.


[AI-PR-REVIEW] version=1 promptVersion=1 system=github pr=10091 round=1 by=alexei-dobriansky at=2026-08-11T10:30:56Z lastSha=7d1c77df5b286979ba3ff5ae58a2e5a4195d176b reviewKey=72270da18e4d36722b879a281ac7e44735a45c1c7ad0b99c899a7fa0a3170ea6 suggestions=S1@a2ee382b

@github-actions github-actions Bot added needs-approval Workflow runs require maintainer approval to start and removed needs-approval Workflow runs require maintainer approval to start labels Aug 19, 2026
…ests

On the IT localization the CI pipeline failed for the two new vendor
tests with "The record in table Payment Lines already exists".

The IT Base Application inserts a transient Payment Lines record
(Type "General Journal", blank journal key) in Purch.-Post.CheckAndUpdate
for every posted invoice and consumes/deletes it in
GenJnlPostLine.PostVend.InitPaymentTerms, which filters on the journal
template name of the posted line. The pre-existing test
DeferralsReleaseSucceedsWhenGLAccountHasDefaultDeferralTemplateAndJournalTemplMandatory
enables "Journal Templ. Name Mandatory", so its posting leaves that
record orphaned (the consuming filter no longer matches the blank
template). With codeunit-level test isolation the orphan survives into
subsequent tests, and any later purchase-invoice posting in the codeunit
fails on the duplicate insert - which hit the two new G/L Account tests,
the only posting tests declared after it (its NotMandatory sibling is
already on the DisabledTests list for the same reason).

Move the two tests to their alphabetical position earlier in the
codeunit so they post before the orphan is created, matching how every
other posting test in the suite avoids the trap. No test logic changed.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
(cherry picked from commit 91fa021)
@alexei-dobriansky

Copy link
Copy Markdown
Contributor

Good Sense Reviewer - Round 2

Recommendation: Accept with Suggestions

What this PR does

The latest update only moves the new vendor G/L Account deferral tests earlier in the test codeunit so they run before the journal-template cleanup test. The test bodies are unchanged, and the move is safe.

The backport still matches the intended Subscription Billing fix: G/L Account contract lines keep the selected account for immediate posting, and deferral release uses the account carried on the deferral record with a safe fallback for old records.

Status of previous suggestions
ID Title Status Author response
S1 Credit memo path has no explicit test Not addressed No reply. The current diff still covers invoice posting and deferral release, but it does not add an explicit credit memo reversal test for the new G/L Account field.
New observations (commits since round 1)

None - the new commit only reorders tests.

Risk assessment and necessity

Risk: This is a financial posting change, so the main risk is posting to the wrong account. The implementation stays narrow and the added field is backward-compatible because blank values keep the old setup-account behavior. The remaining risk is limited to the untested credit memo reversal path.

Necessity: The bug is important because the selected G/L account on the contract line should affect the final posting. The backport scope is appropriate and mirrors the main fix, with the same non-blocking test gap noted above.


[AI-PR-REVIEW] version=1 promptVersion=4 system=github pr=10091 round=2 by=alexei-dobriansky at=2026-09-03T13:19:19Z lastSha=2e98b19f5f0cd4bbc325b7768ea9b5c6a9f33952 reviewKey=0f7fe04307502358424a6d53d6e8298059d3a7506158475266174aa4b205c636 suggestions=S1@a2ee382b:notaddressed parentRound=1

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

Labels

AL: Apps (W1) Add-on apps for W1 From Fork Pull request is coming from a fork needs-approval Workflow runs require maintainer approval to start Team: Finance GitHub request for Finance area

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants