Repository navigation
Bug/648925 VAT charged in Sales Invoice with Cash Basis (Unrealized VAT) is not paid on Sales Credit Note applied to that Posted Sales Invoice if we use a similar Cash Basis VAT Prod Posting Group in the Mexican version - regression. - #13015
Maria Jose Jofre (v-mjofre) wants to merge 6 commits into
Conversation
…alizado en factura
GitHub Actions
Inactive
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Good Sense Reviewer - Round 1Recommendation: Request ChangesWhat this PR doesThis change extends Cash Basis VAT application so a sales credit memo can realize invoice VAT when the documents use different but equivalent VAT posting groups. It maps groups in both directions, keeps the credit memo groups on the generated VAT entries, and adds coverage for full, partial, multi-group, ambiguous, and non-equivalent cases. Problem-solution fitFit: Strong The change targets the reported regression directly, and the new tests cover the main sales-document variants. One existing general-journal path is unintentionally changed by the new document-type lookup. SuggestionsS1 (🔴 High): Preserve general journal credit memo realization S2 (🟠 Moderate): Clear the new analyzer warning Risk assessment and necessityRisk: This changes Cash Basis VAT realization in core customer application posting. A wrong branch can leave unrealized VAT open or realize the wrong VAT group; the sales-document tests are broad, but the general-journal fallback is not covered and the required builds currently fail on a new analyzer warning. The existing public PostUnrealVATEntry interface is preserved. Necessity: Supporting equivalent VAT posting groups is needed to restore correct realization for the reported regression. The scope is focused, but it must preserve credit memos posted outside Sales documents before it is safe to merge.
|
Good Sense Reviewer - Round 2Recommendation: Request ChangesWhat this PR doesThe latest commit preserves the whole-settlement path for general journal credit memos that have no posted sales lines. It also adds full, partial, and unapply regression scenarios, moves repeated VAT allocation calculations outside the entry loop, and clears the prior parameter-name warning. The posting fallback now addresses the earlier general-journal regression, but the new test calls do not match either helper signature and stop the affected apps from compiling. Status of previous suggestions
New observations (commits since round 1)S3 (🔴 High): Fix the mismatched test helper calls Risk assessment and necessityRisk: This is core Cash Basis VAT application logic, where a wrong branch can leave VAT unrealized or post incorrect ledger amounts. The general-journal fallback is now present and the parameter warning is fixed, but the new regression tests currently prevent the MX, US, and CA app builds from completing, so the posting behavior cannot be validated in this revision. Necessity: Equivalent VAT posting groups must realize the same invoice VAT, and general journal credit memos must keep the existing whole-settlement behavior. The remaining correction is small but required before this financial posting change is safe to merge.
|
Good Sense Reviewer - Round 3Recommendation: AcceptWhat this PR doesThe latest commit fixes the mismatched test helper calls and adds full and partial multi-VAT-group coverage for general journal credit memos. The corrected calls now match the intended helper, and the added scenarios exercise the whole-settlement fallback across each invoice VAT group without changing the posting logic again. Status of previous suggestions
New observations (commits since round 2)None. Risk assessment and necessityRisk: This remains sensitive Cash Basis VAT application code, where a wrong allocation can leave VAT unrealized or post the wrong ledger amounts. The latest changes are limited to the missing namespace and regression tests, and the earlier posting fallback is now covered for full, partial, unapply, and multiple-group cases. The current MX check still reports analyzer warnings on earlier PR lines, so CI must be green before merge. Necessity: Equivalent VAT posting groups must realize the same invoice VAT, while general journal credit memos must keep the existing whole-settlement behavior. The code and test scope now match those requirements.
|
Good Sense Reviewer - Round 4Recommendation: AcceptWhat this PR doesThe latest commit corrects the general journal unapply test so it includes the reversal G/L entries, whose document type is blank, together with the original credit memo entries. It also verifies that each VAT account has exactly the original and reversal entry before checking that their amounts net to zero. The posting logic is unchanged, and the updated assertions match the unapply behavior. Status of previous suggestions
New observations (commits since round 3)None. Risk assessment and necessityRisk: Cash Basis VAT application is financially sensitive because a wrong allocation can leave VAT unrealized or post the wrong ledger amounts. The latest change only strengthens the unapply verification and does not alter production behavior. Required CI is still red: the default localized builds report two new AA0139 warnings from earlier test setup changes, and several clean jobs also report runner container failures, so CI must be green before merge. Necessity: Equivalent VAT posting groups must realize the same invoice VAT, while general journal credit memos must preserve the existing whole-settlement path. The latest test correction is needed to verify that unapplication restores VAT and reverses both G/L transfers correctly.
|
Good Sense Reviewer - Round 5Recommendation: AcceptWhat this PR doesThe latest commit updates two exchange-rate VAT tests to limit the user ID to the length stored by the My Notifications table. The field and the test-library parameter are both Status of previous suggestions
New observations (commits since round 4)None. Risk assessment and necessityRisk: Cash Basis VAT application remains financially sensitive because a wrong allocation can leave VAT unrealized or post incorrect ledger amounts. The latest change is limited to test setup, and its length limit matches both the stored user-ID field and the called procedure. The build matrix is still running, with no failing check at review time. Necessity: Equivalent VAT posting groups must realize the same invoice VAT while preserving the general-journal fallback. The latest correction is needed to remove the test analyzer warnings without changing the covered behavior.
|
|
Good Sense Reviewer - Round 6Recommendation: AcceptWhat this PR doesThe latest commit excludes blank or comment lines when collecting posted credit memo VAT groups and amounts. This keeps those lines from creating an empty VAT group during Cash Basis VAT allocation. It also removes overlapping tests while retaining coverage for full application through the unapply scenario, partial and multi-group general journal application, and equivalent, non-equivalent, and ambiguous VAT posting setups. Status of previous suggestions
New observations (commits since round 5)None. Risk assessment and necessityRisk: Cash Basis VAT application is financially sensitive because incorrect grouping can leave VAT unrealized or post incorrect ledger amounts. The new line-type filter is applied consistently to credit memo group discovery and allocation, and the remaining tests cover the important full, partial, multi-group, equivalent, non-equivalent, and unapply paths. The current build matrix is still running, so CI must complete successfully before merge. Necessity: Equivalent VAT posting groups must realize the same invoice VAT while general journal credit memos preserve the whole-settlement fallback. Ignoring blank posted lines is needed so descriptive lines do not participate in VAT grouping.
|
Pull request was closed
GitHub Actions
Inactive
Pull request was closed
GitHub Actions
Inactive
| SalesCrMemoLine.SetLoadFields(Type, "Amount Including VAT", "VAT Bus. Posting Group", "VAT Prod. Posting Group"); | ||
| if SalesCrMemoLine.FindSet() then | ||
| repeat | ||
| if CreditMemoVATPostingSetup.Get( |
There was a problem hiding this comment.
The unbounded Sales Cr.Memo Line loop performs CreditMemoVATPostingSetup.Get for every line. This creates an N+1 lookup pattern; cache each distinct VAT posting-group pair or restructure the lookup so the posting setups are loaded once per distinct key before processing the lines.
Knowledge:
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.51.6
| SalesInvoiceLine.SetLoadFields(Type, "VAT Bus. Posting Group", "VAT Prod. Posting Group"); | ||
| if SalesInvoiceLine.FindSet() then | ||
| repeat | ||
| if InvoiceVATPostingSetup.Get( |
There was a problem hiding this comment.
The unbounded Sales Invoice Line loop performs InvoiceVATPostingSetup.Get for every line. This creates an N+1 lookup pattern; cache each distinct VAT posting-group pair or restructure the lookup so the posting setups are loaded once per distinct key before processing the lines.
Knowledge:
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.51.6
Error Reported
When a posted sales credit memo was applied to a sales invoice under the Cash Basis VAT model, the invoice’s unrealized VAT was not realized if the credit memo used a different VAT product posting group—even when both VAT posting setups were functionally equivalent.
The existing logic matched VAT entries only by their exact VAT business and product posting groups, preventing the system from identifying the corresponding invoice VAT entry.
Solution
Updated the Cash Basis VAT realization logic to identify equivalent VAT posting setups between sales invoices and sales credit memos.
Two VAT posting setups are considered equivalent when they have matching VAT business posting groups, VAT calculation types, VAT percentages, Cash Basis unrealized VAT types, sales VAT accounts, and sales unrealized VAT accounts.
The solution:
Files Modified
GenJnlPostLine.Codeunit.al
Updated the unrealized VAT posting logic to map equivalent invoice and credit memo VAT groups and use the correct groups when creating VAT entries.
CustLedgerEntry.Table.al
Added logic to collect VAT posting setups from invoices and credit memos, identify equivalent Cash Basis VAT configurations, and calculate the applicable amounts by VAT group.
ERMCashBasis.codeunit.al
Added automated tests covering full and partial credit memos, multiple VAT groups, mixed exact/equivalent groups, and non-equivalent VAT configurations.
Result
Applying a sales credit memo with an equivalent but differently coded VAT posting group now correctly realizes the related invoice’s unrealized VAT.
The VAT amount is realized proportionally for partial credit memos and independently for each VAT group in documents with multiple rates. Non-equivalent configurations remain unchanged, and no unnecessary VAT realization G/L entries are generated.
Fixes
[[AB#654308]