Add-Expense-VAT-settings-to-Contoso-demo-tool - #10990
Add-Expense-VAT-settings-to-Contoso-demo-tool#10990Alexander Yakunin (Alexander-Ya) wants to merge 10 commits into
Conversation
GitHub Actions
Inactive
github-actions
Bot
added
AL: Apps (W1)
Alexander Yakunin (Alexander-Ya)
added
ExpenseManagement
and removed
Team: Finance
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 doesThe change wires Expense VAT rate creation into the Contoso demo module and shortens the room service subcategory code so it matches the demo subcategory code. This should let demo data create VAT product posting groups, VAT posting setup, and default VAT settings for Expense Agent categories. The room service code alignment looks correct. The demo module call is not enough, though, because the VAT rate code depends on the Expense Agent setup already having a default VAT business posting group. The existing setup helper fills that default and records that VAT rates were applied, but the new direct codeunit call bypasses both parts. Problem-solution fitFit: Partial The change adds VAT rates to demo master data, but it does not apply the full setup state that the VAT flow needs. A demo company can still be left without the default VAT business posting group used by agent-created VAT specifications. SuggestionsS1 (🔴 High): Use the setup helper for VAT defaults Risk assessment and necessityRisk: This affects Contoso Expense Agent demo setup and VAT posting setup. If the default VAT business group is missing or the rates are created against a blank group, demo expenses can fail VAT specification insertion or use incomplete VAT setup. Necessity: The feature is needed so demo data includes the VAT settings used by Expense Agent. The scope is right, but it needs to use the setup path that creates a complete configuration.
|
…Add-Expense-VAT-settings-to-Contoso-demo-tool
| procedure InsertDefaultRates() | ||
| var | ||
| CompanyInfo: Record "Company Information"; | ||
| GLAccount: Record "G/L Account"; |
There was a problem hiding this comment.
InsertDefaultRates() no longer guarantees the VAT purchase account exists: the old fallback EXPENSE VAT account creation was removed, and the procedure now resolves rate-specific accounts by name before writing VAT posting setup rows. Any existing caller of this published procedure that relied on the old side effect can now fail unless it is updated to precreate those accounts. Preserve the old behavior for InsertDefaultRates() or introduce a new entry point for the rate-specific-account flow and keep the old procedure backward-compatible.
Agent judgement — not directly backed by a BCQuality knowledge article.
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.37.6
| end; | ||
| end; | ||
|
|
||
| local procedure GetPurchaseVATAccount(VATPercent: Decimal): Code[20] |
There was a problem hiding this comment.
The new purchase-VAT-account lookup resolves G/L Account by names like Expense VAT 20 %, but the main app no longer creates any matching account on its own. CreateCountryVATRatesDefaults and Create Expense Categories can run in regular installations without the demo-data app, so this lookup returns no record and VAT Posting Setup is seeded with no valid purchase VAT account. Keep the VAT-account provisioning self-contained in the main app, or fail with an explicit guard instead of depending on demo-data accounts to exist.
Agent judgement — not directly backed by a BCQuality knowledge article.
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.37.6
| var | ||
| GLAccount: Record "G/L Account"; | ||
| begin | ||
| GLAccount.SetRange(Name, GetExpenseVATAccountName(VATPercent)); |
There was a problem hiding this comment.
GetPurchaseVATAccount filters G/L Account by Name and Account Type and then returns FindFirst(). Because Name is not a unique key, multiple posting accounts can satisfy this filter and the code will silently bind VAT posting setup to whichever record happens to come first on the current key. Use a unique identifier for the account, or explicitly detect and reject zero-or-multiple matches before returning an account number.
Agent judgement — not directly backed by a BCQuality knowledge article.
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.37.6
Good Sense Reviewer - Round 2Recommendation: Request ChangesWhat this PR doesThe latest changes set the default VAT business posting group before running Expense VAT rate creation, add demo G/L accounts for Expense VAT rates, and align hotel subcategory codes used by the VAT setup. The earlier ordering issue is addressed for the demo module. However, the shared VAT rate code now looks up G/L accounts that are created by the demo data path, while the same codeunit is also used by app setup/default-settings flows. Those non-demo flows can still fail before VAT rates are created. Status of previous suggestions
New observations (commits since round 1)S2 (🔴 High): Default setup can fail without demo VAT accounts Risk assessment and necessityRisk: This affects shared Expense Agent VAT default setup, not only Contoso demo data. If the required G/L account is missing, the default VAT rate creation can fail before any VAT setup is created. Necessity: The change is needed so demo data has Expense VAT settings, but the app-level VAT setup must remain independent from demo-only G/L account creation.
|
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
Good Sense Reviewer - Round 3Recommendation: Request ChangesWhat this PR doesThe latest commit adds an IsVATCountry() helper and uses it to skip Expense VAT demo setup for Sales Tax companies. That direction is useful, but it does not close the open setup failure. The shared Create Expense VAT Rates code still resolves purchase VAT accounts by finding demo-created G/L accounts. That codeunit is also run from app setup/default-settings flows, where those demo accounts may not exist. The new gate also introduces a second failure path because it reads the Contoso setup record with Get() before some callers reach their existing safe missing-record exit. Status of previous suggestions
New observations (commits since round 2)S3 (🔴 High): VAT country check can stop setup early Risk assessment and necessityRisk: This still affects Expense Agent VAT default setup and Contoso demo VAT setup. If the required G/L account or setup record is missing, VAT rate creation can stop before the settings are created. Necessity: The change is needed so demo data has the Expense VAT settings used by Expense Agent. Without it, the workaround is manual VAT setup after demo data creation, but the implementation must keep shared setup paths independent from demo-only records.
|
| var | ||
| GLAccount: Record "G/L Account"; | ||
| begin | ||
| GLAccount.SetRange(Name, GetExpenseVATAccountName(VATPercent)); |
There was a problem hiding this comment.
GetPurchaseVATAccount resolves the purchase VAT account by filtering "G/L Account" on Name and then taking the first posting account. Name is editable and not unique, so this can bind VAT Posting Setup to the wrong ledger account as soon as a seeded account is renamed or another posting account happens to share the same caption. Resolve these VAT accounts through a stable identifier such as the account No. (or a persisted mapping) instead of SetRange(Name, ...).
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.38.6
| AddSubcategorySeed(TempSubcategory, XROOMSERVICETxt, XHOTELSTxt, XHotelRoomServiceDescTxt, XSubRoomServicePostingTxt, true, false); | ||
| AddSubcategorySeed(TempSubcategory, XPARKINGTxt, XHOTELSTxt, XHotelParkingDescTxt, XSubHotelParkingPostingTxt, true, false); | ||
| AddSubcategorySeed(TempSubcategory, XOTHERTxt, XHOTELSTxt, XHotelOtherDescTxt, XSubOtherPostingTxt, true, false); | ||
| AddSubcategorySeed(TempSubcategory, XHOTELPARKTxt, XHOTELSTxt, XHotelParkingDescTxt, XSubHotelParkingPostingTxt, true, false); |
There was a problem hiding this comment.
This PR renames Locked=true subcategory codes that are seeded as part of the primary key of "Expense Subcategory" under HOTELS (PARKING -> HOTEL-PARK, OTHER -> HOTELOTHER in CreateExpenseCategories.Codeunit.al; and demo-data ROOM-SER -> ROOM-SERVICE in CreateExpenseSubCategories.Codeunit.al), and CreateExpenseVATRates.Codeunit.al's per-country seed lists were switched to reference the new codes, but no Subtype = Upgrade codeunit renames the old rows for companies that already ran setup with the previous codes. Create Expense Categories also skips reseeding subcategories once a category exists, so an environment seeded before this change would keep the stale keys while later code (VAT-rate lookups by CategoryCode/SubcategoryCode) expects the new ones. The Expense Agent app is currently versioned/labelled '(Preview)', which limits how many real installs could already hold the old codes, so the practical urgency is lower than a GA product would warrant — but the gap should be closed with an upgrade codeunit (or a documented statement that no pre-existing installs use the old codes) before this ships to broader preview/GA audiences.
Knowledge:
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.38.6
Good Sense Reviewer - Round 4Recommendation: Request ChangesWhat this PR doesThe latest commit changes the demo Expense VAT G/L accounts so they no longer use a fixed G/L account subcategory. That makes the demo-account creation path less dependent on country-specific account-category setup, but it does not close the open setup failures. The shared VAT rate setup still finds purchase VAT accounts by looking for demo-created G/L accounts. That shared setup can run from default-settings flows where those demo accounts are not guaranteed to exist. The VAT-country guard also still reads the Contoso setup record without handling a missing record. Status of previous suggestions
New observations (commits since round 3)None. The only new hunk changes the Expense VAT G/L account subcategory, and I did not find a new issue in that change. Risk assessment and necessityRisk: This still affects Expense Agent VAT default setup and Contoso demo VAT setup. If the needed G/L account or setup record is missing, VAT rate creation can stop before the settings are created. The demo-data G/L account publisher was checked; the remaining risk is in the shared setup path, not in the publisher timing. Necessity: The change is needed so demo data has the Expense VAT settings used by Expense Agent. Without it, the workaround is manual VAT setup after demo data creation, but the implementation must keep shared setup paths independent from demo-only records.
|
…Add-Expense-VAT-settings-to-Contoso-demo-tool
| end; | ||
| end; | ||
|
|
||
| local procedure GetPurchaseVATAccount(VATPercent: Decimal): Code[20] |
There was a problem hiding this comment.
IMPACT NOTE: this would be major/blocker in a knowledge-backed rule, but is capped to minor as an agent finding. CreateExpenseVATRates.Codeunit.al (app/src/Setup, a production code path invoked from ExpenseAgentSetup.CreateCountryVATRatesDefaults, independent of any demo data) previously always guaranteed a 'Purchase VAT Account' by creating/using a virtual 'EXPENSE VAT' G/L account inline. The PR removes that unconditional creation and replaces it with GetPurchaseVATAccount(), which looks up a G/L Account by Name = 'Expense VAT %' via GLAccount.SetRange/FindFirst and only assigns 'Purchase VAT Account' when a match is found (PurchaseVATAccountNo <> ''). Those named accounts are only ever created by CreateExpenseGLAccount.Codeunit.al under the 'demo data' folder (via AddExpenseVATAccountsForLocalization/InsertExpenseVATAccounts), which only runs as part of the Contoso Demo Data module pipeline. For a real (non-demo) tenant enabling Expense Agent, CreateCountryVATRatesDefaults creates new VAT Posting Setup rows via InsertRate with 'Purchase VAT Account' left blank, because no matching G/L account will exist. This is a functional regression versus current behavior for production customers who do not load Contoso demo data. Recommend keeping a fallback that creates or assigns a default Purchase VAT Account when GetPurchaseVATAccount finds nothing, so production VAT Posting Setup rows are never left without a Purchase VAT Account.
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.38.6
| AccountNo := 5650; | ||
| foreach VATRate in VATRates do begin | ||
| if AccountNo > 5659 then | ||
| Error(ExpenseVATAccountRangeExceededErr, ContosoCoffeeDemoDataSetup."Country/Region Code"); |
There was a problem hiding this comment.
This range-overflow guard in demo-data setup is an internal invariant, but it raises a plain client-visible Error. Use ErrorInfo with ErrorType::Internal so the detailed message is kept for telemetry while the user sees a generic failure.
Knowledge:
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.38.6
Good Sense Reviewer - Round 5Recommendation: Request ChangesWhat this PR doesThe latest commit changes the missing purchase VAT account path so a missing rate-specific G/L account no longer stops VAT rate setup immediately. It now skips the Purchase VAT Account assignment when no matching account exists. That prevents one runtime error, but it still does not make the shared setup path complete. The code can create VAT Posting Setup rows without a purchase VAT account in non-demo setup flows, and the VAT-country guard still reads the demo setup record before callers can safely exit when that record is missing. Status of previous suggestions
New observations (commits since round 4)None. The latest hunk only changes how a missing purchase VAT account is handled, and that is covered by the still-open S2 finding. Risk assessment and necessityRisk: This still affects Expense Agent VAT default setup and Contoso demo VAT setup. If the shared setup creates VAT Posting Setup rows without a purchase VAT account, later expense posting that uses those VAT groups can fail or post incomplete VAT setup. If the demo setup record is missing, Necessity: The change is needed so demo data has the Expense VAT settings used by Expense Agent. Without it, the workaround is manual VAT setup after demo data creation, but the implementation must keep shared setup paths independent from demo-only records and must skip safely when demo setup is absent.
|
3 participants
What & why
Linked work
Fixes AB#648889
How I validated this
What I tested and the outcome (required — be specific: scenarios, commands, screenshots for UI changes)
Risk & compatibility