Support VAT return reporting frequencies in Denmark

[djukicmilica]

Summary

• parse SKAT VAT return periods with their reporting frequency
• support monthly, quarterly, and semi-annual period calculations
• retain a feature-flagged legacy quarterly fallback
• prevent overlapping VAT return periods and add focused test coverage

Created the secret:
DKElecVAT-ReportingFrequencyEnabled

Resource type: Azure Key Vault
Subscription ID: ecc6c8bc-614f-4bbe-8687-f80b99e8499c
Vault Name: NAVPRODNEUCustomerDataKV
Secret permissions: List, Get, Set

Fixes
AB#629987

[github-actions]

Could not find linked issues in the pull request description. Please make sure the pull request description contains a line that contains 'Fixes #' followed by the issue number being fixed. Use that pattern for every issue you want to link.

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Interfaces} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

CalcPeriodEndDate and CalcPeriodStartDate (ElecVATDeclGetPeriods.Codeunit.al lines 168-190) both branch on the same `Elec.

VAT Decl. Rep. Frequencyenum with the same three variants to pick aCalcDateformula. This is exactly the anti-pattern the knowledge file calls out: a case statement over an enum whose branches choose between variant computations, duplicated across more than one procedure. Adding a fourth frequency later means finding and editing bothcaseblocks (andGetReportingFrequency's case true of) instead of adding one implementation. Consider an interface (e.g. IElecVATDeclReportingPeriodwithCalcPeriodEndDate/CalcPeriodStartDatemembers) implemented per enum value viaImplementation = ... = Codeunit`, so a new frequency is a new enum value plus one codeunit.

Knowledge:

• microsoft/knowledge/interfaces/prefer-interface-over-case-branching.md

Line mapping was unavailable, so this was posted as an issue comment.

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why}

[github-actions]

Copilot PR Review

Iteration 6 · Outcome: completed

Knowledge source: https://github.com/microsoft/BCQuality@186d8a131465475c79244d994acb872cd5c0d4bf

Findings by domain

Findings split into Knowledge-backed (cite a BCQuality article) and Agent (the agent's own judgement, no matching BCQuality rule).

Domain	Findings	Knowledge-backed	Agent	Inline	Fallback
Error Handling	1	1	0	1	0
Style	1	1	0	1	0

Totals: 2 knowledge-backed · 0 agent findings.

Orchestrator pre-filter (2 file(s) excluded)

• layer-disabled (knowledge) : 2 file(s)

_{Findings produced by the AL review agent v1.7.3. Reply 👎 on any inline comment to flag false positives.}

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Interfaces} \quad \color{gray}{\texttt{\small Iteration\ 2}}$

CalcPeriodEndDate (line 168) and CalcPeriodStartDate (line 180) both branch on the same `Enum "Elec.

VAT Decl. Rep. Frequency"` value with the identical Monthly / Semi-Annual / else shape duplicated across the two procedures. Per this guidance, repeating a case-over-enum shape across more than one procedure is the detection signal for modeling the variant behavior with an enum-backed interface instead, so a new frequency only requires a new enum value and implementation codeunit rather than a synchronized edit to every case block.

Knowledge:

• microsoft/knowledge/interfaces/prefer-interface-over-case-branching.md

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why}

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Telemetry} \quad \color{gray}{\texttt{\small Iteration\ 3}}$

IsReportingFrequencyEnabled calls FeatureTelemetry.LogUsage unconditionally, on every code path including the 'Invalid' branch where Evaluate(Enabled, SecretValue.Trim()) failed to parse the Key Vault value.

LogUsage is meant to record a successful use of a feature; here it fires the same way whether the configuration read succeeded, was missing, or was malformed, which inflates usage telemetry with a failure case (a misconfigured Key Vault secret) instead of routing it through LogError or a distinguishable signal.

Knowledge:

• microsoft/knowledge/telemetry/feature-usage-only-after-success.md

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why}

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Error\ Handling} \quad \color{gray}{\texttt{\small Iteration\ 4}}$

In the new GetFrequencyAwareVATReturnPeriods procedure, Evaluate(DueDate, DueDateXmlNode.AsXmlElement().InnerText()) ignores Evaluate's Boolean return value.

If SKAT returns a due date text that does not parse as a Date, DueDate silently becomes 0D and the code proceeds to compute period Start/End dates and insert a VAT Return Period from that zero date instead of raising an error, unlike the sibling checks in the same method for the due-date and frequency XML nodes (which do raise DueDateNotFoundErr/FrequencyNotFoundErr) and unlike the equivalent Evaluate call in Elec. VAT Decl. Az. Key Vault's IsReportingFrequencyEnabled a few files over, which does check its Evaluate result. If this were flagged with high confidence it would rate as a major defect (silent bad-data insertion for a tax compliance record); recommend checking the Evaluate result and raising a descriptive error (mirroring DueDateNotFoundErr) when the due-date text fails to parse.

_{Agent judgement — not directly backed by a BCQuality knowledge article.}

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why}

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Testing} \quad \color{gray}{\texttt{\small Iteration\ 4}}$

The shared MessageHandler in ElecVATDeclTests.Codeunit.al (used by ResponsePeriodsKeepFrequencyAndDueDateAssociated, GettingSamePeriodsTwiceDoesNotInsertDuplicates, and DisabledReportingFrequencyCreatesQuarterlyPeriod) hardcodes its assertion inline (Assert.IsTrue(Message.Contains('periods were received from server'), Message)) instead of consuming an expectation the test enqueued.

It never verifies the exact expected text per test, does not use LibraryVariableStorage Enqueue/Dequeue, and there is no AssertEmpty check that the message fired exactly once. Because an inline assertion failure inside a MessageHandler can be swallowed by the calling Message() call, a broken expectation in one of the three tests could leave the suite green while the underlying behavior is wrong.

Knowledge:

• microsoft/knowledge/testing/ui-handlers-in-tests.md

Line mapping was unavailable, so this was posted as an issue comment.

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why}

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Interfaces} \quad \color{gray}{\texttt{\small Iteration\ 5}}$

CalcPeriodEndDate and CalcPeriodStartDate both branch on the same "Elec.

VAT Decl. Rep. Frequency" enum with a case statement that maps each value to a CalcDate formula. This is exactly the duplicated-shape signal the referenced guidance flags: a new frequency value means editing both case blocks (and GetReportingFrequency's string matcher) in lockstep, with no compiler help if one is missed. An enum-with-implementation (interface exposing EndDateFormula/StartDateFormula, one implementation codeunit per frequency) would let a new variant plug in without touching either procedure.

Knowledge:

• microsoft/knowledge/interfaces/prefer-interface-over-case-branching.md

Line mapping was unavailable, so this was posted as an issue comment.

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why}

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Telemetry} \quad \color{gray}{\texttt{\small Iteration\ 5}}$

IsReportingFrequencyEnabled logs the same Session.LogMessage at Verbosity::Normal regardless of whether ConfigurationStatus is 'Missing' (expected, benign default), 'Valid', or 'Invalid'.

An 'Invalid' status means an operator-set Key Vault secret failed to parse as a Boolean - a misconfiguration that silently falls back to Enabled=true - and per the referenced guidance that explicit-failure branch should be logged at Verbosity::Warning (or higher) so it surfaces in severity-based alerting instead of blending in with normal successful reads.

Knowledge:

• microsoft/knowledge/telemetry/match-verbosity-to-signal-severity.md

Line mapping was unavailable, so this was posted as an issue comment.

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why}

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Error\ Handling} \quad \color{gray}{\texttt{\small Iteration\ 6}}$

In GetFrequencyAwareVATReturnPeriods, the Boolean result of Evaluate(DueDate, DueDateXmlNode.AsXmlElement().InnerText()) is discarded.

If the SKAT response contains a due-date node whose inner text does not parse as a Date, Evaluate silently leaves DueDate at its default (0D) instead of signalling failure, and the code proceeds to insert or compare a VAT return period against that invalid date.

Recommendation:

• check the Boolean result of Evaluate and raise a descriptive error (mirroring the existing DueDateNotFoundErr / FrequencyNotFoundErr pattern) when parsing fails, instead of silently continuing with an unparsed date.

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why}

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Style} \quad \color{gray}{\texttt{\small Iteration\ 6}}$

In the new local procedure GetFrequencyAwareVATReturnPeriods, the primitive DueDate: Date is declared between the Codeunit variable and the complex XmlNode/XmlNodeList/Enum variables, interleaving a primitive type ahead of composite types.

CodeCop AA0021 requires complex types (Record, Codeunit, XmlNode-family, Enum, ...) to be declared before primitives (Date, Integer, ...) in a var block.

Suggested fix (apply manually — could not be anchored as a one-click suggestion):

    var
        ElecVATDeclXml: Codeunit "Elec. VAT Decl. Xml";
        DueDateXmlNode: XmlNode;
        FrequencyXmlNode: XmlNode;
        PeriodXmlNode: XmlNode;
        PeriodXmlNodeList: XmlNodeList;
        ReportingFrequency: Enum "Elec. VAT Decl. Rep. Frequency";
        DueDate: Date;
        i: Integer;

Knowledge:

• microsoft/knowledge/style/variable-declaration-order-by-type.md

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why}

[github-actions]

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

In IsReportingFrequencyEnabled, the complex CustomDimensions: Dictionary of [Text, Text] variable is declared last, after the primitive SecretValue, Enabled, and ConfigurationStatus variables. CodeCop AA0021 requires complex (composite) types to precede primitive types in a var block.

Suggested fix (apply manually — could not be anchored as a one-click suggestion):

    var
        AzureKeyVault: Codeunit "Azure Key Vault";
        CustomDimensions: Dictionary of [Text, Text];
        SecretValue: Text;
        ConfigurationStatus: Text;
        Enabled: Boolean;

Knowledge:

• microsoft/knowledge/style/variable-declaration-order-by-type.md

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.12.4}

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Breaking\ Changes}$

The new enum "Elec. VAT Decl. Rep. Frequency" is introduced without an Access modifier, which defaults it to public and turns a reporting-frequency implementation detail into a published type that dependent extensions can bind to forever. Mark it Access = Internal (the test app can still see it via the added internalsVisibleTo entry) unless this enum is intentionally meant to be a supported external contract.

Suggested change

	enum 13605 "Elec. VAT Decl. Rep. Frequency"
	{
	Extensible = false;
	enum 13605 "Elec. VAT Decl. Rep. Frequency"
	{
	Access = Internal;
	Extensible = false;

Knowledge:

• microsoft/knowledge/breaking-changes/choose-access-modifiers-deliberately.md

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.12.4}

[github-actions]

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

The new VAT return period lookups filter on "Start Date" and "End Date", but table 737 "VAT Return Period" is keyed only by "No." (key(Key1; "No.")). Both the exact-match check (IsEmpty after SetRange on Start/End Date) and the overlap check (FindFirst after SetFilter on Start/End Date) therefore have no covering key to ride and fall back to scanning the table. Add a key that covers the date lookup pattern and select it with SetCurrentKey before these reads.

Knowledge:

• microsoft/knowledge/performance/setcurrentkey-aligns-key-with-filters.md

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.12.4}