Skip to content

Test BCQuality code review local skill - #960

Open
aholstrup1 wants to merge 4 commits into
mainfrom
test-bcquality-code-review-local-skill
Open

Test BCQuality code review local skill#960
aholstrup1 wants to merge 4 commits into
mainfrom
test-bcquality-code-review-local-skill

Conversation

@aholstrup1

Copy link
Copy Markdown
Owner

Test PR for validating whether built-in Copilot code review uses BCQuality marketplace/plugin skills and repo-local review skills. Includes a sentinel phrase in both paths.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Rate sequencing and validation defects can produce stale data or incorrect zero-tax entries.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds calculation-type and item-category excise rates while configuring BCQuality review guidance.

Changes:

  • Adds specific, ad-valorem, and hybrid rate calculation.
  • Migrates legacy rates and updates pages, permissions, and tests.
  • Configures repository-local and plugin-based AL review guidance.
File summaries
File Description
LibraryExciseTax.Codeunit.al Updates test helpers for the new rate table.
ExciseTaxCalculationTests.Codeunit.al Updates rate assertions.
ExciseCalculationTypeTests.Codeunit.al Tests new rate types and precedence.
ExciseTaxesTransLogExt.TableExt.al Stores new calculation details.
ExciseJournalLineExt.TableExt.al Calculates specific and percentage tax.
ExciseTaxType.Table.al Uses the replacement rate table.
ExciseTaxRate.Table.al Defines rate storage and resolution.
ExciseTaxItemFARate.Table.al Obsoletes the legacy table.
ExciseTaxesRead.permissionset.al Adds read permission.
ExciseTaxesObjects.permissionset.al Adds new table and page objects.
ExciseTaxesEdit.permissionset.al Adds edit permission.
ExciseTaxTransLogExt.PageExt.al Displays calculation details.
ExciseJournalLineExt.PageExt.al Displays new journal fields.
ExciseTaxTypes.Page.al Opens the new rates page.
ExciseTaxTypeCard.Page.al Opens the new rates page.
ExciseTaxRates.Page.al Provides rate configuration UI.
ExciseTaxItemFARates.Page.al Obsoletes the legacy page.
ExciseCalculationType.Enum.al Defines calculation types.
ExciseTaxUpgrade.Codeunit.al Migrates legacy rates.
ExciseTaxTransSubscriber.Codeunit.al Applies and validates rates.
ExciseTaxCalculation.Codeunit.al Derives taxable amounts.
.github/skills/code-review/SKILL.md Adds local review guidance.
.github/copilot/settings.json Pins the BCQuality plugin source.
.github/copilot-instructions.md Updates BCQuality skill instructions.
Review details

Suppressed comments (2)

src/Apps/W1/ExciseTaxes/app/src/table/ExciseTaxRate.Table.al:34

  • Changing Source Type leaves Source No. intact, so an item number can become a fixed-asset number (or vice versa); if both tables contain the same code, the rate is silently retargeted to the wrong entity. Clear the dependent source number whenever the type changes, matching the journal-line behavior in SustExciseJnlLine.Table.al:213-219.
            trigger OnValidate()
            begin
                if "Source Type" <> "Source Type"::Item then
                    "Item Category Code" := '';

src/Apps/W1/ExciseTaxes/app/src/codeunit/ExciseTaxTransSubscriber.Codeunit.al:123

  • The posting check validates both components for Hybrid and the percentage component for Ad valorem, but the default Specific per Unit branch performs no validation. A missing rate therefore leaves the default type with zero duty/quantity and can be posted as a zero-tax entry. Validate the specific component for this branch as well.
        case SustainabilityExciseJnlLine."Excise Calculation Type" of
            "Excise Calculation Type"::"Ad valorem":
                TestAdValoremFields(SustainabilityExciseJnlLine);
            "Excise Calculation Type"::Hybrid:
                begin
  • Files reviewed: 24/24 changed files
  • Comments generated: 3
  • Review effort level: Balanced

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +288 to +292
ExciseJnlLine.Validate("Item Category Code", ItemLedgerEntry."Item Category Code");
ExciseJnlLine.Validate("Source Qty.", Abs(ItemLedgerEntry.Quantity));
if RequiresTaxableAmount(ExciseJnlLine) then
ExciseJnlLine.Validate("Excise Taxable Amount", GetTaxableAmountFromItemLedgerEntry(ItemLedgerEntry));
OnAfterUpdateExciseJournalLineFromItemLedgerEntry(ExciseJnlLine, ItemLedgerEntry);
ExciseJournalLine.Validate("Excise Unit of Measure Code", FixedAsset."Excise Unit of Measure Code");
ExciseJournalLine.Validate("Quantity for Excise Tax", FixedAsset."Quantity for Excise Tax");
ExciseJournalLine.Validate("Excise Duty", GetExciseDutyForSource(ExciseJournalLine."Excise Tax Type", ExciseJournalLine."Source Type", ExciseJournalLine."Source No.", ExciseJournalLine."Posting Date"));
ApplyExciseRate(ExciseJournalLine, '');
using Microsoft.Inventory.Item;
using Microsoft.Sustainability.ExciseTax;

table 7416 "Excise Tax Rate"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants