Improve Contoso App by WaelAbuSeada · Pull Request #9553 · microsoft/BCApps

WaelAbuSeada · 2026-07-16T21:35:37Z

Improve Contoso App.

Addresses code-review findings in the Contoso Coffee demo dataset:

Field-level DataClassification on setup-table fields (per-field AS0016).
Locked = true on Tok-suffixed code Labels.
OnOpenPage -> InitRecord() on EService Demo Data Setup.
Correct Contoso Demo - Read permission-set caption.
Reset IsHandled before OnBeforeGetResidualCurrencyAccounts.
Fix truncated tooltip on Jobs setup "Item 4 No.".

Address code-review findings in the Contoso Coffee demo dataset: - Add field-level DataClassification to setup-table fields that only had a table-level declaration (AS0016 requires per-field classification). - Add Locked = true to Tok-suffixed code/identifier Labels so they are not sent for translation. - Add OnOpenPage -> InitRecord() to the EService Demo Data Setup card. - Fix the "Contoso Demo - Read" permission set caption (was "- Edit"). - Reset IsHandled before raising OnBeforeGetResidualCurrencyAccounts. - Fix a truncated tooltip on JobsModuleSetup "Item 4 No.". Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e02a80f6-45db-4ce0-bc65-e85810ddc728

github-actions · 2026-07-16T21:39:24Z

Could not find a linked ADO work item. Please link one by using the pattern 'AB#' followed by the relevant work item number. You may use the 'Fixes' keyword to automatically resolve the work item when the pull request is merged. E.g. 'Fixes AB#1234'

github-actions · 2026-07-16T21:49:17Z

$\textbf{🟠\ High\ Severity\ —\ Data\ Modeling} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

EService Demo Data Setup keeps an Integer "Primary Key" field instead of the singleton convention's blank-valued Code[10] key, and this PR now adds an OnOpenPage trigger (Rec.InitRecord()) that relies on the row being an unambiguous singleton.

With an Integer key, nothing besides InsertAllowed=false/DeleteAllowed=false on this one card page stops another code path from inserting a second row with a different key value, at which point Get() in InitRecord (and any other consumer) can silently pick up the wrong record.

Knowledge:

microsoft/knowledge/data-modeling/setup-table-is-a-singleton.md

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

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

github-actions · 2026-07-16T21:49:18Z

Copilot PR Review

Iteration 1 · 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
Data Modeling	1	1	0	0	1

Totals: 1 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.}

WaelAbuSeada · 2026-07-17T02:12:19Z

        }
        field(2; "Starting Year"; Integer)
        {
+            DataClassification = CustomerContent;


Privacy — field-level DataClassification (major)

Finding: this table declares DataClassification only at table level, leaving its Normal fields implicitly ToBeClassified. Per AS0016 the table-level property does not cascade — every Normal field must declare its own classification.

Fix: added DataClassification = CustomerContent; to each previously-unclassified field (these hold per-company configuration values).

📖 table-level-data-classification-cascades

WaelAbuSeada · 2026-07-17T02:12:19Z

    {
        field(1; Module; Enum "Contoso Demo Data Module")
        {
+            DataClassification = SystemMetadata;


Privacy — field-level DataClassification (major)

Finding: this table declares DataClassification only at table level, leaving its Normal fields implicitly ToBeClassified. Per AS0016 the table-level property does not cascade — every Normal field must declare its own classification.

Fix: added DataClassification = SystemMetadata; to each previously-unclassified field (these hold internal module-management metadata, not customer data).

📖 table-level-data-classification-cascades

WaelAbuSeada · 2026-07-17T02:12:19Z

    {
        field(1; Name; Enum "Contoso Demo Data Module")
        {
+            DataClassification = SystemMetadata;


Privacy — field-level DataClassification (major)

Finding: this table declares DataClassification only at table level, leaving its Normal fields implicitly ToBeClassified. Per AS0016 the table-level property does not cascade — every Normal field must declare its own classification.

Fix: added DataClassification = SystemMetadata; to each previously-unclassified field (these hold internal module-management metadata, not customer data).

📖 table-level-data-classification-cascades

WaelAbuSeada · 2026-07-17T02:12:19Z

        }
        field(2; "VAT Prod. Post Grp. Standard"; Code[20])
        {
+            DataClassification = CustomerContent;


Privacy — field-level DataClassification (major)

Finding: this table declares DataClassification only at table level, leaving its Normal fields implicitly ToBeClassified. Per AS0016 the table-level property does not cascade — every Normal field must declare its own classification.

Fix: added DataClassification = CustomerContent; to each previously-unclassified field (these hold per-company configuration values).

📖 table-level-data-classification-cascades

WaelAbuSeada · 2026-07-17T02:12:19Z

        }
        field(2; "Default Depreciation Book"; Code[10])
        {
+            DataClassification = CustomerContent;


Privacy — field-level DataClassification (major)

Finding: this table declares DataClassification only at table level, leaving its Normal fields implicitly ToBeClassified. Per AS0016 the table-level property does not cascade — every Normal field must declare its own classification.

Fix: added DataClassification = CustomerContent; to each previously-unclassified field (these hold per-company configuration values).

📖 table-level-data-classification-cascades

WaelAbuSeada · 2026-07-17T02:12:19Z

-        CheckingTok: Label 'CHECKING', MaxLength = 20;
-        OperatingTok: Label 'OPERATING', MaxLength = 20;
-        SavingsTok: Label 'SAVINGS', MaxLength = 20;
+        CashTok: Label 'CASH', MaxLength = 20, Locked = true;


Style — Locked labels (minor)

Finding: Tok-suffixed Labels hold code/identifier values. Without Locked = true the translation pipeline extracts them, and a localized tenant that receives a translated code (e.g. CASH → ARGENT) fails on demo-data insertion because BC table keys are locale-invariant.

Fix: added Locked = true to these token Labels.

📖 label-locked-for-non-translatable

WaelAbuSeada · 2026-07-17T02:12:19Z


    var
-        AnalyticsReferenceTok: Label 'ANALYTICS', MaxLength = 35;
+        AnalyticsReferenceTok: Label 'ANALYTICS', MaxLength = 35, Locked = true;


Style — Locked labels (minor)

Finding: Tok-suffixed Labels hold code/identifier values. Without Locked = true the translation pipeline extracts them, and a localized tenant that receives a translated code (e.g. CASH → ARGENT) fails on demo-data insertion because BC table keys are locale-invariant.

Fix: added Locked = true to these token Labels.

📖 label-locked-for-non-translatable

WaelAbuSeada · 2026-07-17T02:12:19Z

        CreateGLAccount: Codeunit "Create G/L Account";
        IsHandled: Boolean;
    begin
+        IsHandled := false;


Events — reset IsHandled before raising (minor)

Finding: IsHandled was passed to OnBeforeGetResidualCurrencyAccounts without an explicit IsHandled := false; beforehand. A freshly declared Boolean defaults to false, but the guidance requires an explicit reset immediately before every OnBefore raise so intent is documented and the routine stays correct if a second raise is later added.

Fix: added IsHandled := false; before the event call.

📖 initialize-ishandled-to-false-before-publishing

WaelAbuSeada · 2026-07-17T02:12:19Z

    Access = Public;
    Assignable = true;
-    Caption = 'Contoso Demo - Edit';
+    Caption = 'Contoso Demo - Read';


AppSource — permission set caption (minor)

Finding: the assignable Contoso Demo - Read set had Caption = 'Contoso Demo - Edit'. The caption is what administrators see in the role-assignment UI and AppSource listings, so a Read role labelled Edit is misleading.

Fix: corrected the caption to Contoso Demo - Read. (Agent finding — no knowledge article.)

WaelAbuSeada · 2026-07-17T02:12:19Z

                field("Item 4 No."; Rec."Item Supply No.")
                {
-                    ToolTip = 'Specifies s item number to use for the scenarios.';
+                    ToolTip = 'Specifies the supply item number to use for the scenarios.';


Accessibility — tooltip text (minor)

Finding: the Item 4 No. tooltip read 'Specifies s item number…' (a truncated word); the field binds to Rec.""Item Supply No."".

Fix: corrected to 'Specifies the supply item number to use for the scenarios.' (Agent finding — no knowledge article.)

WaelAbuSeada requested review from a team July 16, 2026 21:35

WaelAbuSeada temporarily deployed to triage July 16, 2026 21:35 — with GitHub Actions Inactive

github-actions Bot added AL: Apps (W1) Add-on apps for W1 Integration GitHub request for Integration area labels Jul 16, 2026

WaelAbuSeada commented Jul 17, 2026

View reviewed changes

Uh oh!

Conversation

WaelAbuSeada commented Jul 16, 2026 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Jul 16, 2026

Uh oh!

github-actions Bot commented Jul 16, 2026

EService Demo Data Setup keeps an Integer "Primary Key" field instead of the singleton convention's blank-valued Code[10] key, and this PR now adds an OnOpenPage trigger (Rec.InitRecord()) that relies on the row being an unambiguous singleton.

Uh oh!

github-actions Bot commented Jul 16, 2026

Copilot PR Review

Findings by domain

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

WaelAbuSeada commented Jul 16, 2026 •

edited by github-actions Bot

Loading