[Master] - Slice 626305: [Excise Tax][VENDOR] Improving Excise Duty Calculation - #11002
[Master] - Slice 626305: [Excise Tax][VENDOR] Improving Excise Duty Calculation#11002v-rohangarg20 wants to merge 2 commits into
Conversation
github-actions
Bot
added
AL: Apps (W1)
v-rohangarg20
changed the title
|
The new action caption 'Generate Excise Tax Entries' reads as an imperative action caption, so it should use sentence case rather than title case. Use only the first word and proper nouns in uppercase for sentence-phrase action captions. Suggested fix (apply manually — could not be anchored as a one-click suggestion): Caption = 'Generate excise tax entries';Knowledge: Line mapping was unavailable, so this was posted as an issue comment. 👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.36.6 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
For each row returned by the Item Ledger Entry FindSet, the new event path re-resolves the effective rate from Excise Tax Rate. That adds repeated inner lookups against a persistent table on the hot path; cache the resolved rate per distinct source/category/date key or hoist the lookup out of the loop when the key is constant for the batch. Knowledge: Line mapping was unavailable, so this was posted as an issue comment. 👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.36.6 |
Good Sense Reviewer - Round 1Recommendation: Request ChangesWhat this PR doesThis adds calculation types for excise duty, introduces a new rate table with item-category matching, and carries the new rate data through journal lines and transaction logs. The rate lookup order is mostly consistent with the feature goal, but the item-ledger flow can calculate the taxable amount before the final item-category rate is applied, so an ad valorem or hybrid rate can still be missed in an important path. Problem-solution fitFit: Partial The requested behavior is clear and the new objects cover specific, ad valorem, hybrid, item-specific, category, and fallback rates. The implementation does not fully fit the item-ledger scenario because the taxable amount decision can use a stale calculation type before the final rate is resolved. SuggestionsS1 (🔴 High): Set taxable amount after final rate lookup S2 (🟠 Moderate): Fixed asset test misses fixed asset rate Risk assessment and necessityRisk: The main risk is in excise journal lines created from item ledger entries, where a wrong taxable amount can produce a wrong financial tax amount. The new table and enum are public app data, but the change keeps the old setup behind an obsolete path and adds migration, so the compatibility risk is mainly in the calculation flow and upgrade data copy. Necessity: The feature is useful because excise duty needs both per-unit and percentage-based calculation and a more specific rate hierarchy. The scope is appropriate for the requested feature, but the item-ledger calculation path must be fixed before it is safe to merge.
|
|
|
The new historical upgrade tags are registered through OnGetPerCompanyUpgradeTags, but this extension still has no install trigger that seeds those tags on first install. That subscriber only helps SetAllUpgradeTags when a new company is created; it does not mark an existing company complete when the app is first installed. As written, a company that installs this version fresh will still enter these old migration blocks on its next version upgrade. Seed both tags from an install codeunit so first-install companies skip historical upgrade work later. Knowledge:
Line mapping was unavailable, so this was posted as an issue comment. 👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.38.6 |
The item-ledger path evaluated RequiresTaxableAmount before the OnAfterUpdateExciseJournalLineFromItemLedgerEntry subscriber applied the rate for the ledger entry item category. When the item category and the ledger entry category resolved to different calculation types, an ad valorem or hybrid line could keep a zero taxable amount and calculate the wrong tax. The taxable amount is now set after the event so the final calculation type is used. Also fixed VerifyFAJournalLinesForAcquisitionCost to create the fixed asset rate with an explicit Excise Calculation Type, so the rate lookup resolves as the test expects.
Good Sense Reviewer - Round 2Recommendation: AcceptWhat this PR doesThe latest change applies the final excise rate before deciding whether the item-ledger journal line needs a taxable amount. It also updates the fixed asset test setup to use the new rate table with an explicit calculation type. These changes address the two prior review points without adding a new behavior change outside the same calculation flow. Status of previous suggestions
New observations (commits since round 1)None - changes only addressed prior suggestions. Risk assessment and necessityRisk: This remains a financially sensitive excise tax calculation path, but the round-2 commit reduces the identified risk by using the final resolved rate before calculating taxable amount. No new public API or BaseApp dependency was introduced by this commit. Necessity: The change is needed so percentage-based and hybrid excise rates calculate from the intended taxable amount in item-ledger and fixed asset scenarios. The scope of the round-2 change is targeted to the prior findings.
|
2 participants
Fixes AB#645043
Problem
An expense user linked to an employee without an Employee Posting Group was returned by the Expense Users API. The agent could sign that user in, but submission/posting later failed because the posting group is required.
Changes
Expense User.Employee No.validation now errors when the employee has no employee posting group, so such a link cannot be created.Employee Posting Grouplookup FlowField (field 25) onExpense Userand filtered on it inExpense Users APIOnOpenPage(FilterGroup 2, next to the existingEmployee No.filter). This also covers employees whose posting group is removed after the link was made.