[TEST – DO NOT MERGE] Loyalty Sample across all 11 BCQuality domains by JesperSchulz · Pull Request #8822 · microsoft/BCApps

JesperSchulz · 2026-06-25T14:09:51Z

⚠️ WARNING: This is review-target TEST code with intentional mistakes. DO NOT MERGE.

This PR exists solely to exercise the BCQuality Copilot PR-review agent. The Loyalty Sample app it adds is deliberately flawed and must not be shipped.

What it does

Replicates the byte-identical Loyalty Sample app from microsoft/BCAppsBCQuality#52 (26 files, 1005 insertions) — a self-contained, deliberately-flawed loyalty module under src/Apps/W1/LoyaltySample/. The planted mistakes span all 11 BCQuality review domains.

Wiring bump

Bumps the BCQuality pin in tools/BCQuality/bcquality.config.yaml to 8904ce583b1db8a5fb714d7ebae8368bd0a7ac14 (latest main).
Adds the 5 new sub-skills (al-breaking-changes-review, al-error-handling-review, al-events-review, al-interfaces-review, al-web-services-review) to $DomainMap in tools/Code Review/scripts/Invoke-CopilotPRReview.ps1 so all 11 domains are exercised and labeled correctly in the review summary (instead of falling back to Other).

Context

This is the 3rd BCQuality wiring test (after #8772 and #8816), this one to validate all 11 domains end-to-end.

The 11 domains exercised

Security, Privacy, Performance, Style, Accessibility, Upgrade, Breaking Changes, Error Handling, Events, Interfaces, Web Services.

References microsoft/BCAppsBCQuality#52.

Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com

…+ pin bump Replicates the byte-identical deliberately-flawed Loyalty Sample app from microsoft/BCAppsBCQuality#52 (26 files, 1005 insertions) to exercise the Copilot PR-review agent across all 11 BCQuality domains. Also bumps the BCQuality pin in tools/BCQuality/bcquality.config.yaml from 822cae1b to 8904ce583b (latest main) and registers the 5 new review sub-skills (breaking-changes, error-handling, events, interfaces, web-services) in $DomainMap so their findings are labeled by domain instead of Other. This is review-target test code with intentional mistakes; it is NOT meant to be merged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

github-actions · 2026-06-25T14:10:48Z

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'

JesperSchulz · 2026-06-26T07:03:04Z

Let's try again.

github-actions · 2026-06-26T07:24:03Z

+                field(FullDisplay; Rec."Member Name" + ' (' + Rec."No." + ')')
+                {
+                    ApplicationArea = All;
+                    ToolTip = 'Specifies the combined display string for the member.';


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

The 'Member Name' field in LoyaltyMemberCard.Page.al at line 32 has ShowCaption = false while also being explicitly Editable = true.

Removing the caption label from an editable field is an accessibility defect: screen reader users lose the field label, and the ToolTip text ('Member full name') is not a substitute because tooltips are not reliably announced as field labels. Remove ShowCaption = false or apply one of the officially supported magic patterns.

Knowledge:

microsoft/knowledge/ui/show-caption-on-editable-fields.md

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

github-actions · 2026-06-26T07:24:04Z

+            {
+                GridLayout = Rows;
+                group(ContactInner)
+                {


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

The 'Points Balance' field in LoyaltyMemberCard.Page.al at line 45 has StyleExpr = 'Unfavorable' hardcoded as a literal string, rendering the field red/bold on every row regardless of whether the balance is positive or negative.

Unfavorable is a semantic style implying a negative outcome; applying it unconditionally to a field whose value may be positive conveys incorrect meaning and fails the accessibility requirement that semantic meaning be independently determinable. Use a Text variable computed in OnAfterGetCurrRecord that sets 'Unfavorable' only when the balance is negative (mirroring the pattern in LoyaltyMemberList).

Knowledge:

microsoft/knowledge/ui/semantic-styles-need-independent-textual-meaning.md

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

github-actions · 2026-06-26T07:24:05Z

+    }
+
+    container.innerHTML = '<b>' + memberName + '</b> &lt;' + email + '&gt; - ' + tier;
+    container.style.color = '#d73b02';


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

LoyaltyBadge.js at lines 12-13 applies hardcoded hex colors (#d73b02, #fff4ce) with no forced-colors media query branch.

BC color tokens are not available inside the add-in iframe; the add-in is responsible for its own contrast compliance. Under Windows High Contrast themes, these colors may become invisible or fail WCAG AA contrast ratios. Add a @media (forced-colors: active) branch that switches to system colors, or verify contrast independently in default, dark, and high-contrast themes.

Knowledge:

microsoft/knowledge/ui/control-add-in-has-no-bc-color-tokens.md

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

github-actions · 2026-06-26T07:24:07Z

+        OnBeforeRecalculate(LoyaltyMember, IsHandled);
+        DoRecalculate(LoyaltyMember);


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

RecalculateMember in LoyaltyEvents.Codeunit.al declares IsHandled: Boolean, raises OnBeforeRecalculate(LoyaltyMember, IsHandled) at line 9, then unconditionally calls DoRecalculate(LoyaltyMember) at line 10 without checking IsHandled.

Any subscriber that sets IsHandled := true to signal it has handled recalculation will be silently ignored — the default recalculation runs anyway. This breaks the BC OnBefore/IsHandled contract. Add if IsHandled then exit; between lines 9 and 10.

Suggested change

OnBeforeRecalculate(LoyaltyMember, IsHandled);

DoRecalculate(LoyaltyMember);

OnBeforeRecalculate(LoyaltyMember, IsHandled);

if IsHandled then

exit;

DoRecalculate(LoyaltyMember);

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

github-actions · 2026-06-26T07:24:08Z

+        LoyaltyMember."Points Balance" := 0;
+        if LoyaltyMember."Loyalty Tier" = LoyaltyMember."Loyalty Tier"::Platinum then
+            LoyaltyMember."Points Balance" := 100;
+    end;


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

OnBeforeRecalculate in LoyaltyEvents.Codeunit.al (lines 74-78) is declared [IntegrationEvent] but its body assigns LoyaltyMember."Points Balance" := 0 and then conditionally sets it to 100 for Platinum tier.

In AL, the IntegrationEvent publisher body executes before subscribers are invoked; any subscriber that sets IsHandled := true cannot prevent these assignments from running. The publisher body of an IntegrationEvent should be empty (begin end;); move the defaulting logic to the caller of OnBeforeRecalculate so it is skipped when IsHandled is set. This concern crosses the boundary between the style and security leaf domains (event-body semantics + IsHandled bypass).

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

[IntegrationEvent(false, false)] local procedure OnBeforeRecalculate(var LoyaltyMember: Record "Loyalty Member"; var IsHandled: Boolean) begin end;

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

github-actions · 2026-06-26T07:24:10Z

+
+    procedure ArchiveMember(var Member: Record "Loyalty Member")
+    begin
+        Member.Delete();


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

ArchiveMember in LoyaltyManagement.Codeunit.al calls Member.Delete() at line 38 unconditionally, then asks Confirm(...) at line 39.

If the user answers No, the record has already been permanently deleted with no rollback code in sight. The intended flow is: ask for confirmation first, then delete only when the user agrees. Swap the order: if Confirm('Do you want to archive member %1?', false, Member."Member Name") then begin Member.Delete(); Message('Archived.'); end;. This concern should be promoted to a knowledge-backed rule to gate future reviews.

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

github-actions · 2026-06-26T07:24:11Z

+            PointEntry.SetRange("Member No.", Member."No.");
+            if PointEntry.FindSet() then
+                repeat
+                    OtherMember.Get(PointEntry."Member No.");


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

OtherMember.Get(PointEntry."Member No.") is called at line 25 inside the inner repeat loop.

This is a classic N+1 pattern: one database round-trip per point-entry row against the Loyalty Member table. Additionally, OtherMember is never read after the Get, making this dead code. Remove the call; if the outer Member record is genuinely needed inside the inner loop, it is already in scope as Member.

Knowledge:

microsoft/knowledge/performance/avoid-get-inside-loop-on-large-table.md

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

github-actions · 2026-06-26T07:24:12Z

+                    TotalPoints += PointEntry.Points;
+                until PointEntry.Next() = 0;
+
+            Member.CalcFields("Entry Count");


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

Member.CalcFields("Entry Count") is called at line 29 inside the repeat...until loop in RecalculateAllBalances.

Each call issues a separate SQL COUNT query per member row. Additionally, the resulting Entry Count value is never read anywhere in the loop body, making this call entirely dead. Remove the line.

Knowledge:

microsoft/knowledge/performance/calcsums-instead-of-calcfields-in-loop.md

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

github-actions · 2026-06-26T07:24:14Z

+            Member.CalcFields("Entry Count");
+            Member."Points Balance" := TotalPoints;
+            Member.Modify();
+            Commit();


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

Commit() is called at line 32 inside the repeat...until Member.Next() = 0 loop in RecalculateAllBalances.

This produces one transaction per member row, prevents atomic rollback of the whole operation, and adds transaction-start overhead on every iteration. Remove Commit from inside the loop; if checkpoint batching is required, process N rows per outer iteration and commit once per checkpoint.

Knowledge:

microsoft/knowledge/performance/avoid-commit-inside-loops.md

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

github-actions · 2026-06-26T07:24:15Z

+    begin
+        Member.FindSet(true);
+        repeat
+            Member.CalcFields("Total Points");


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

Member.CalcFields("Total Points") is called at line 15 inside the repeat...until loop in OnUpgradePerCompany.

Each call issues a separate SQL SUM query per member row against the Loyalty Point Entry table. In an upgrade context this runs for every member on every tenant during upgrade. Replace with a set-based approach such as DataTransfer or preload totals via a temporary aggregation table.

Knowledge:

microsoft/knowledge/performance/calcsums-instead-of-calcfields-in-loop.md

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

github-actions · 2026-06-26T07:24:16Z

+    begin
+        Body := StrSubstNo('{"name":"%1","email":"%2","phone":"%3"}', Member."Member Name", Member."Email Address", Member."Phone No.");
+        Content.WriteFrom(Body);
+        Client.Post('https://api.contoso-pay.example/charge', Content, Response);


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

CallPaymentGateway at line 71 posts customer PII (name, email, phone) to an external endpoint, and LoyaltyAuditSubscriber.SendExternalAuditEmail also posts to an external audit endpoint — neither code path contains a PrivacyNotice.GetPrivacyNoticeApprovalState check anywhere upstream.

The BC Privacy Notice framework requires admin consent before customer data is transmitted to external services. Add a consent check (or verify one exists in the page/wizard that invokes these procedures) before any outgoing request carrying customer data.

Knowledge:

microsoft/knowledge/privacy/privacy-notice-consent-for-external-data-transfer.md

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

github-actions · 2026-06-26T07:24:18Z

+        FeatureTelemetry: Codeunit "Feature Telemetry";
+        Dimensions: Dictionary of [Text, Text];
+    begin
+        Dimensions.Add('MemberName', Member."Member Name");


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

LogMemberUsage at lines 79-80 adds Member."Member Name" and Member."Email Address" to the CustomDimensions dictionary before calling FeatureTelemetry.LogUsage.

The platform does not classify dictionary values; member names and email addresses are CustomerContent/EUII and will be logged verbatim to telemetry. Replace both entries with non-personal context (e.g. tier name, point-balance band, or a pseudonymous member ID).

Knowledge:

microsoft/knowledge/privacy/featuretelemetry-customdimensions-no-pii.md

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

github-actions · 2026-06-26T07:24:19Z

+    var
+        ErrorMessage: Text;
+    begin
+        ErrorMessage := StrSubstNo(Text000, Member."Member Name");


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

ThrowMemberError at line 93 assigns ErrorMessage := StrSubstNo(Text000, Member."Member Name") and then calls Error(ErrorMessage) at line 94.

Pre-building the error text with StrSubstNo before passing it to Error produces an opaque Text string; the platform cannot classify or strip the member name before it reaches telemetry. Call Error(Text000, Member."Member Name") directly so the platform performs the substitution itself.

Suggested change

ErrorMessage := StrSubstNo(Text000, Member."Member Name");

Error(Text000, Member."Member Name");

Knowledge:

microsoft/knowledge/privacy/avoid-strsubstno-prebuild-before-error.md

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

github-actions · 2026-06-26T07:24:20Z

+        field(4; "Phone No."; Text[30])
+        {
+            Caption = 'Phone No.';
+            DataClassification = ToBeClassified;


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

LoyaltyMember.Table.al has two fields with DataClassification = ToBeClassified that must be resolved before the extension ships: 'Phone No.' (line 26) is personal contact data and should be classified as EndUserIdentifiableInformation or CustomerContent; 'Points Balance' (line 38) is customer-owned data and should be CustomerContent.

ToBeClassified is a development placeholder; GDPR data-subject requests and telemetry classification cannot reason about fields left in this state.

Knowledge:

microsoft/knowledge/privacy/resolve-tobeclassified-before-release.md

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

github-actions · 2026-06-26T07:24:22Z

+{
+    var
+        Text000: Label 'Failed to process member %1';
+        GatewayApiKey: Label 'sk-live-9f8a7b6c5d4e3f2a1b0c4d8e7f6a5b3c', Locked = true;


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

GatewayApiKey at line 9 is declared as a Label containing a live API key literal ('sk-live-9f8a7b6c5d4e3f2a1b0c4d8e7f6a5b3c').

Labels are plain Text constants visible in source code and compiled symbol files. Any credential stored this way is permanently exposed to anyone who can read source or decompile the extension. Remove the Label; retrieve the key at runtime from IsolatedStorage using SetEncrypted/Get with a SecretText destination.

Knowledge:

microsoft/knowledge/security/secrettext-for-credentials.md

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

github-actions · 2026-06-26T07:24:23Z

+
+    procedure ConfigureGateway(Token: Text)
+    begin
+        IsolatedStorage.Set('GatewayToken', Token, DataScope::Module);


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

ConfigureGateway at line 45 calls IsolatedStorage.Set('GatewayToken', Token, DataScope::Module).

The gateway token is a credential; storing it unencrypted means it is readable in plain text from the underlying storage if storage is ever exposed. Use IsolatedStorage.SetEncrypted instead.

Suggested change

IsolatedStorage.Set('GatewayToken', Token, DataScope::Module);

IsolatedStorage.SetEncrypted('GatewayToken', Token, DataScope::Module);

Knowledge:

microsoft/knowledge/security/isolatedstorage-setencrypted-for-sensitive-values.md

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

github-actions · 2026-06-26T07:24:24Z

+        IsolatedStorage.Set('GatewayToken', Token, DataScope::Module);
+    end;
+
+    procedure GetGatewayToken(): Text


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

GetGatewayToken() at line 48 declares StoredToken as Text and returns Text.

Retrieving a credential from IsolatedStorage into a Text variable and returning it as Text exposes the value to the debugger on the way out. Use the SecretText overload of IsolatedStorage.Get and change the return type to SecretText so the value stays protected end-to-end.

Knowledge:

microsoft/knowledge/security/secrettext-for-credentials.md

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

github-actions · 2026-06-26T07:24:25Z

+        exit(GatewayApiKey);
+    end;
+
+    procedure UnwrapGatewaySecret(Secret: SecretText): Text


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

UnwrapGatewaySecret at line 57 calls Secret.Unwrap() inside a procedure that is not marked [NonDebuggable].

The unwrapped value is a plain Text local that the debugger can read, defeating the purpose of SecretText. Add [NonDebuggable] to this procedure.

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

[NonDebuggable] procedure UnwrapGatewaySecret(Secret: SecretText): Text

Knowledge:

microsoft/knowledge/security/nondebuggable-required-when-unwrapping-secrettext.md

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

github-actions · 2026-06-26T07:24:27Z

+
+    procedure BuildMemberBadgeHtml(Member: Record "Loyalty Member"): Text
+    begin
+        exit('<div class="badge"><b>' + Member."Member Name" + '</b> &lt;' + Member."Email Address" + '&gt;</div>');


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

BuildMemberBadgeHtml at line 99 concatenates Member."Member Name" and Member."Email Address" directly into an HTML string without encoding.

AL has no built-in HtmlEncode; a member name containing '<script>' or 'onerror=' will be interpreted as markup by any HTML renderer. Encode the four special characters (&, <, >, ") before concatenating, or avoid building raw HTML altogether.

Knowledge:

microsoft/knowledge/security/al-has-no-built-in-htmlencode.md

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

github-actions · 2026-06-26T07:24:28Z

+    end;
+
+    [IntegrationEvent(false, false)]
+    procedure OnBeforeChargeMember(Member: Record "Loyalty Member"; var GatewayToken: SecretText; var IsHandled: Boolean)


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

OnBeforeChargeMember at line 103 declares var GatewayToken: SecretText as an event parameter.

Any extension on the tenant can subscribe to this event, read the SecretText value, and persist it elsewhere. Even SecretText should not flow through an IntegrationEvent surface. Remove the GatewayToken parameter; the publisher must acquire and use the token internally before or after the event, never through it.

Knowledge:

microsoft/knowledge/security/integrationevent-must-not-expose-secrets.md

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

github-actions · 2026-06-26T07:24:29Z

+        exit('Server=loyalty;Database=points;Trusted_Connection=yes;');
+    end;
+
+    procedure GetApiToken(): Text


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

GetApiToken() in LoyaltyPublicApi at line 25 calls ApiToken.Unwrap() and returns the result as Text without [NonDebuggable].

The unwrapped token is a debugger-visible plain Text local. Add [NonDebuggable] to the procedure.

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

[NonDebuggable] procedure GetApiToken(): Text

Knowledge:

microsoft/knowledge/security/nondebuggable-required-when-unwrapping-secrettext.md

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

github-actions · 2026-06-26T07:24:30Z

+        document.body.appendChild(container);
+    }
+
+    container.innerHTML = '<b>' + memberName + '</b> &lt;' + email + '&gt; - ' + tier;


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

LoyaltyBadge.js at line 11 assigns container.innerHTML using direct string concatenation of memberName, email, and tier: '' + memberName + ' <' + email + '> - ' + tier.

These values originate from AL record fields passed by the AL host. If any value contains unescaped HTML (e.g. a member name with <script> or an onerror attribute), the browser will execute it. Replace innerHTML assignment with textContent for each text node, or sanitize all three inputs by replacing &, <, >, and " before concatenation. This concern should be promoted to a knowledge-backed rule to gate future control add-in reviews.

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

github-actions · 2026-06-26T07:24:32Z

+codeunit 50100 "Loyalty Management"
+{
+    var
+        Text000: Label 'Failed to process member %1';


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

Text000 at line 8 is a Label with no approved suffix.

Per CodeCop AA0074, every Label and TextConst must end with an approved suffix indicating its consuming call. Text000 is passed to Error(), so the suffix should be Err. Rename to a descriptive identifier such as MemberProcessingFailedErr.

Suggested change

Text000: Label 'Failed to process member %1';

MemberProcessingFailedErr: Label 'Failed to process member %1';

Knowledge:

microsoft/knowledge/style/label-suffix-approved-list.md

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

github-actions · 2026-06-26T07:24:33Z

+    var
+        ErrorMessage: Text;
+    begin
+        ErrorMessage := StrSubstNo(Text000, Member."Member Name");


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

ThrowMemberError at line 93 passes a pre-rendered StrSubstNo result to Error() via an intermediate Text variable.

This pattern breaks translation toolchain analysis and (as noted in the privacy review) causes PII to reach telemetry unclassified. Pass the Label and the substitution argument directly: Error(MemberProcessingFailedErr, Member."Member Name").

Suggested change

ErrorMessage := StrSubstNo(Text000, Member."Member Name");

Error(MemberProcessingFailedErr, Member."Member Name");

Knowledge:

microsoft/knowledge/style/error-passes-parameters-directly-not-strsubstno.md

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

github-actions · 2026-06-26T07:24:34Z

+                    ErrorText += StrSubstNo('Member %1 has a negative balance.\n', LoyaltyMember."No.");
+            until LoyaltyMember.Next() = 0;
+        if ErrorText <> '' then
+            Error(ErrorText);


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

ValidateBalances at line 48 calls Error(ErrorText) where ErrorText was built by StrSubstNo in a loop.

The Error call receives a pre-rendered opaque Text, losing translation linkage and telemetry classification. Refactor to use the ErrorBehavior(Collect) + Error with a Label pattern for each row, or collect the member numbers and pass them as substitution parameters to a single labeled Error call at the end.

Knowledge:

microsoft/knowledge/style/error-passes-parameters-directly-not-strsubstno.md

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

github-actions · 2026-06-26T07:24:36Z

+    EntitySetName = 'Loyalty_Members';
+    SourceTable = "Loyalty Member";
+    DelayedInsert = false;
+    Caption = 'Loyalty Member API';


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

LoyaltyMemberAPI.Page.al has EntityName = 'Loyalty_Member' (line 13) and EntitySetName = 'Loyalty_Members' (line 14).

Underscores are not permitted in API page entity names — they must be camelCase alphanumeric only. Use EntityName = 'loyaltyMember' and EntitySetName = 'loyaltyMembers'. Field Name 'Member_Name' on line ~29 of the same page is also non-camelCase.

Suggested change

Caption = 'Loyalty Member API';

EntityName = 'loyaltyMember';

Knowledge:

microsoft/knowledge/style/api-page-camelcase-properties.md

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

github-actions · 2026-06-26T07:24:37Z

+    Caption = 'Loyalty Member API';
+    ODataKeyFields = SystemId;
+
+    layout


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

LoyaltyMemberAPI.Page.al at line 16 explicitly sets DelayedInsert = false.

API pages must set DelayedInsert = true so the insert trigger fires once with the complete JSON payload rather than on a partially populated record. LoyaltyMemberData.Page.al (PageType = API) also omits the property, which defaults to false and has the same effect.

Suggested change

layout

DelayedInsert = true;

Knowledge:

microsoft/knowledge/style/api-page-delayedinsert-true.md

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

github-actions · 2026-06-26T07:24:38Z

+            Member.Modify();
+        until Member.Next() = 0;
+
+        Client.Get('https://api.contoso-pay.example/migrate', Response);


$\textbf{🔴\ Critical\ Severity\ —\ Upgrade} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

OnUpgradePerCompany at line 20 calls Client.Get('https://api.contoso-pay.example/migrate', Response) — a live HTTP call to an external service inside an upgrade codeunit.

Upgrade code runs with no users signed in, in a constrained window where any network failure (DNS, expired credentials, external downtime) aborts the entire upgrade transaction and leaves the tenant unable to upgrade. Defer this call to a runtime code path (job queue entry, first-sign-in trigger, or background task) that can retry and degrade gracefully.

Knowledge:

microsoft/knowledge/upgrade/no-external-calls-in-upgrade.md

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

github-actions · 2026-06-26T07:24:40Z

+{
+    Subtype = Upgrade;
+
+    trigger OnUpgradePerCompany()


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

OnUpgradePerCompany at line 7 contains no HasUpgradeTag / SetUpgradeTag guard.

Without an upgrade tag, the balance-migration loop runs on every upgrade cycle, not just once per tenant. On the second upgrade the loop doubles already-doubled balances. Add a unique upgrade tag, check HasUpgradeTag at entry, perform the work, then call SetUpgradeTag. Also register the tag in an OnGetPerCompanyUpgradeTags subscriber per the referenced guidance.

Knowledge:

microsoft/knowledge/upgrade/use-upgrade-tags-not-version-checks.md

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

github-actions · 2026-06-26T07:24:41Z

+        Client: HttpClient;
+        Response: HttpResponseMessage;
+    begin
+        Member.FindSet(true);


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

Member.FindSet(true) at line 13 in OnUpgradePerCompany is called without an if guard.

If no Loyalty Member rows exist on a tenant, the repeat...until loop still executes once with an empty record variable, corrupting Points Balance on a blank record and then raising a runtime error when Modify() is called — aborting the entire upgrade for that company. Change to: if Member.FindSet(true) then repeat ... until Member.Next() = 0;

Suggested change

Member.FindSet(true);

if Member.FindSet(true) then

Knowledge:

microsoft/knowledge/upgrade/guard-database-reads.md

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

github-actions · 2026-06-26T07:24:42Z

+        Client.Get('https://api.contoso-pay.example/migrate', Response);
+
+        if Member.IsEmpty() then
+            Error('No loyalty members were found during upgrade.');


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

OnUpgradePerCompany at line 23 calls Error('No loyalty members were found during upgrade.').

Any tenant that has no loyalty members — a valid state for a new installation — will fail to upgrade. The check also runs after the loop, using the final state of Member which is undefined at that point (the cursor is exhausted). Replace with Session.LogMessage to record the telemetry observation and exit gracefully; do not Error() from upgrade code.

Knowledge:

microsoft/knowledge/upgrade/do-not-block-upgrade-on-data-errors.md

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

github-actions · 2026-06-26T07:24:43Z

Copilot PR Review

Iteration 1 · Outcome: completed

Knowledge source: https://github.com/microsoft/BCQuality@822cae1b2771ac25f665f73369f69093bd4fd630

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
Accessibility	3	3	0	3
Agent	3	0	3	3
Performance	4	4	0	4
Privacy	4	4	0	4
Security	9	9	0	8
Style	5	5	0	5
Upgrade	4	4	0	4

Totals: 29 knowledge-backed · 3 agent findings.

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

layer-disabled (knowledge) : 13 file(s)

_{Findings produced by the Copilot CLI agent against BCQuality at 822cae1b2771ac25f665f73369f69093bd4fd630. Reply 👎 on any inline comment to flag false positives.}

github-actions Bot added the AL: Apps (W1) Add-on apps for W1 label Jun 25, 2026

JesperSchulz marked this pull request as ready for review June 25, 2026 14:17

JesperSchulz requested review from a team June 25, 2026 14:17

JesperSchulz requested a review from a team as a code owner June 25, 2026 14:17

microsoft deleted a comment from github-actions Bot Jun 26, 2026

JesperSchulz closed this Jun 26, 2026

JesperSchulz reopened this Jun 26, 2026

github-actions Bot reviewed Jun 26, 2026

View reviewed changes

		OnBeforeRecalculate(LoyaltyMember, IsHandled);
		DoRecalculate(LoyaltyMember);

	ErrorMessage := StrSubstNo(Text000, Member."Member Name");
	Error(Text000, Member."Member Name");

	IsolatedStorage.Set('GatewayToken', Token, DataScope::Module);
	IsolatedStorage.SetEncrypted('GatewayToken', Token, DataScope::Module);

	Text000: Label 'Failed to process member %1';
	MemberProcessingFailedErr: Label 'Failed to process member %1';

	ErrorMessage := StrSubstNo(Text000, Member."Member Name");
	Error(MemberProcessingFailedErr, Member."Member Name");

	Caption = 'Loyalty Member API';
	EntityName = 'loyaltyMember';

Uh oh!

Conversation

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

Uh oh!

What it does

Wiring bump

Context

The 11 domains exercised

Uh oh!

github-actions Bot commented Jun 25, 2026

Uh oh!

JesperSchulz commented Jun 26, 2026

Uh oh!

github-actions Bot Jun 26, 2026

Choose a reason for hiding this comment

The 'Member Name' field in LoyaltyMemberCard.Page.al at line 32 has ShowCaption = false while also being explicitly Editable = true.

Uh oh!

github-actions Bot Jun 26, 2026

Choose a reason for hiding this comment

The 'Points Balance' field in LoyaltyMemberCard.Page.al at line 45 has StyleExpr = 'Unfavorable' hardcoded as a literal string, rendering the field red/bold on every row regardless of whether the balance is positive or negative.

Uh oh!

github-actions Bot Jun 26, 2026

Choose a reason for hiding this comment

LoyaltyBadge.js at lines 12-13 applies hardcoded hex colors (#d73b02, #fff4ce) with no forced-colors media query branch.

Uh oh!

github-actions Bot Jun 26, 2026

Choose a reason for hiding this comment

RecalculateMember in LoyaltyEvents.Codeunit.al declares IsHandled: Boolean, raises OnBeforeRecalculate(LoyaltyMember, IsHandled) at line 9, then unconditionally calls DoRecalculate(LoyaltyMember) at line 10 without checking IsHandled.

Uh oh!

github-actions Bot Jun 26, 2026

Choose a reason for hiding this comment

OnBeforeRecalculate in LoyaltyEvents.Codeunit.al (lines 74-78) is declared [IntegrationEvent] but its body assigns LoyaltyMember."Points Balance" := 0 and then conditionally sets it to 100 for Platinum tier.

Uh oh!

github-actions Bot Jun 26, 2026

Choose a reason for hiding this comment

ArchiveMember in LoyaltyManagement.Codeunit.al calls Member.Delete() at line 38 unconditionally, then asks Confirm(...) at line 39.

Uh oh!

github-actions Bot Jun 26, 2026

Choose a reason for hiding this comment

OtherMember.Get(PointEntry."Member No.") is called at line 25 inside the inner repeat loop.

Uh oh!

github-actions Bot Jun 26, 2026

Choose a reason for hiding this comment

Member.CalcFields("Entry Count") is called at line 29 inside the repeat...until loop in RecalculateAllBalances.

Uh oh!

github-actions Bot Jun 26, 2026

Choose a reason for hiding this comment

Commit() is called at line 32 inside the repeat...until Member.Next() = 0 loop in RecalculateAllBalances.

Uh oh!

github-actions Bot Jun 26, 2026

Choose a reason for hiding this comment

Member.CalcFields("Total Points") is called at line 15 inside the repeat...until loop in OnUpgradePerCompany.

Uh oh!

github-actions Bot Jun 26, 2026

Choose a reason for hiding this comment

CallPaymentGateway at line 71 posts customer PII (name, email, phone) to an external endpoint, and LoyaltyAuditSubscriber.SendExternalAuditEmail also posts to an external audit endpoint — neither code path contains a PrivacyNotice.GetPrivacyNoticeApprovalState check anywhere upstream.

Uh oh!

github-actions Bot Jun 26, 2026

Choose a reason for hiding this comment

LogMemberUsage at lines 79-80 adds Member."Member Name" and Member."Email Address" to the CustomDimensions dictionary before calling FeatureTelemetry.LogUsage.

Uh oh!

github-actions Bot Jun 26, 2026

Choose a reason for hiding this comment

ThrowMemberError at line 93 assigns ErrorMessage := StrSubstNo(Text000, Member."Member Name") and then calls Error(ErrorMessage) at line 94.

Uh oh!

github-actions Bot Jun 26, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jun 26, 2026

Choose a reason for hiding this comment

GatewayApiKey at line 9 is declared as a Label containing a live API key literal ('sk-live-9f8a7b6c5d4e3f2a1b0c4d8e7f6a5b3c').

Uh oh!

github-actions Bot Jun 26, 2026

Choose a reason for hiding this comment

ConfigureGateway at line 45 calls IsolatedStorage.Set('GatewayToken', Token, DataScope::Module).

Uh oh!

github-actions Bot Jun 26, 2026

Choose a reason for hiding this comment

GetGatewayToken() at line 48 declares StoredToken as Text and returns Text.

Uh oh!

JesperSchulz commented Jun 25, 2026 •

edited by github-actions Bot

Loading