Skip to content

[Master] - Slice 626305: [Excise Tax][VENDOR] Improving Excise Duty Calculation - #11002

Open
v-rohangarg20 wants to merge 1 commit into
mainfrom
features/Slice-626305-Excise-Tax-Improving-Excise-Duty-Calculation
Open

[Master] - Slice 626305: [Excise Tax][VENDOR] Improving Excise Duty Calculation#11002
v-rohangarg20 wants to merge 1 commit into
mainfrom
features/Slice-626305-Excise-Tax-Improving-Excise-Duty-Calculation

Conversation

@v-rohangarg20

@v-rohangarg20 v-rohangarg20 commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Summary

This change improves the excise duty calculation flow by introducing explicit calculation-type handling and a dedicated excise tax rate table. It resolves the effective rate using the most specific applicable match, supports both specific-per-unit and ad valorem calculation types, and keeps the rate hierarchy consistent for item-level, category-level, and general fallback scenarios.

What this does

  • Adds an Excise Calculation Type enum to distinguish specific-per-unit vs. ad valorem calculation modes.
  • Introduces the Excise Tax Rate table to store rate records with source type, source number, item category, and effective date metadata.
  • Updates the excise tax lookup to select the most specific valid rate before falling back to broader matches.
  • Validates incompatible rate fields for the selected calculation type to prevent invalid combinations.
  • Adds dedicated tests covering the calculation-type behavior and rate resolution flow.

Work item

Notes

  • Related to the Excise Tax vendor update.

@v-rohangarg20
v-rohangarg20 requested a review from a team September 3, 2026 10:49
@v-rohangarg20
v-rohangarg20 requested a review from a team as a code owner September 3, 2026 10:49
@github-actions github-actions Bot added AL: Apps (W1) Add-on apps for W1 Team: Finance GitHub request for Finance area labels Sep 3, 2026
@v-rohangarg20 v-rohangarg20 changed the title [Excise Tax][VENDOR] Improving Excise Duty Calculation [Master] - Slice 626305: [Excise Tax][VENDOR] Improving Excise Duty Calculation Sep 3, 2026
@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

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

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

Comment thread src/Apps/W1/ExciseTaxes/test/src/LibraryExciseTax.Codeunit.al
Comment thread src/Apps/W1/ExciseTaxes/app/src/table/ExciseTaxRate.Table.al
@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

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

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

Comment thread src/Apps/W1/ExciseTaxes/app/src/page/ExciseTaxRates.Page.al
Comment thread src/Apps/W1/ExciseTaxes/test/src/LibraryExciseTax.Codeunit.al
@github-actions github-actions Bot added this to the Version 30.0 milestone Sep 3, 2026
@alexei-dobriansky

Copy link
Copy Markdown
Contributor

Good Sense Reviewer - Round 1

Recommendation: Request Changes

What this PR does

This 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 fit

Fit: 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.

Suggestions

S1 (🔴 High): Set taxable amount after final rate lookup
The item-ledger path checks RequiresTaxableAmount before OnAfterUpdateExciseJournalLineFromItemLedgerEntry applies the rate for the ledger entry item category. If the current item category and the ledger entry category resolve to different calculation types, an ad valorem or hybrid line can keep a zero taxable amount and calculate the wrong tax. Move the taxable amount update after the final rate application, or make the final rate application also set the taxable amount when it is needed.

S2 (🟠 Moderate): Fixed asset test misses fixed asset rate
The fixed asset test only creates an item-category rate, but the lookup clears the category for fixed assets. Add the fixed-asset fallback rate that the test expects, otherwise the Assert.IsTrue call can fail before it verifies the intended behavior.

Risk assessment and necessity

Risk: 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.


[AI-PR-REVIEW] version=1 promptVersion=4 system=github pr=11002 round=1 by=alexei-dobriansky at=2026-09-03T13:18:00.8361054Z lastSha=56e2e0ad2e9f5de467462fe47cf538fe73791189 reviewKey=1f69aed26e0e291246838592a4c4ef0fc73aa32d398df18267175977dab8d9d7 suggestions=S1@f01b49f6,S2@a09f32b5

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 Team: Finance GitHub request for Finance area

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants