Superseded: Bug 626576 AppSource workaround

[martinsrui-msft]

Superseded

Investigation showed that this must be fixed in the Platform extension-operation model rather than through BCApps error handling.

The BCApps workaround and its test have been removed, leaving this branch with no net application changes. The root fix is now tracked by:

https://microsoft.ghe.com/bic/BC-Platform/pull/46282

AB#626576

[github-actions]

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

In both InstallAppsourceExtensionWithRefreshSession overloads, the new ShowAppInstallationError(InstallErrorText) call happens right after ExtensionPendingSetup.DeleteAll(), while AL's implicit write transaction for that delete is still open. Message() blocks on user input, so the dialog can hold row locks on the pending-setup table until the user dismisses it. Move the UI prompt outside the write scope (e.g. return the error text and show it after the transaction-affecting cleanup has completed) to avoid extending lock hold time.

Knowledge:

• microsoft/knowledge/performance/avoid-user-prompts-inside-transactions.md
• microsoft/knowledge/performance/understand-implicit-transaction-boundary.md

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}

[github-actions]

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

CreatePendingExtensionSetup, ClearPendingExtensionSetup, IsPendingExtensionSetupEmpty, UninstallExtensionIfInstalled, and InstallMarketplaceExtension are newly added public procedures on the "Extension Mgt. Test Library" codeunit, but none has XML documentation comments. As part of a library codeunit's public surface, add procedure-specific

documentation (and / tags where applicable) so dependent test apps get IntelliSense guidance.

Knowledge:

• microsoft/knowledge/style/xmldoc-for-public-library-procedures.md

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}

[github-actions]

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

InstallMarketplaceExtensionPageHandler always clicks Continue then Install without asserting anything about the shown "Marketplace Extn Deployment" page (e.g. which fields/state it presents). This does not prove the expected deployment dialog appeared, or appeared only once; a different modal flow could still satisfy the handler silently.

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 · AL review agent v1.36.6}

[github-actions]

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

Traced end-to-end: ExtInstallFailureSetup.OnInstallAppPerCompany raises Error(InstallFailureErr) = "The test extension installation failed with a detailed error." This is caught by TryInstallApp inside ExtensionMarketplace.InstallApp, which re-raises Error(AppInstallationErrorErr, InstallErrorText) = "The app could not be installed. Error message: The test extension installation failed with a detailed error." That wrapped text is what the outer InstallAppsourceExtension TryFunction actually captures via GetLastErrorText(), and it is that wrapped text which ShowAppInstallationError() displays via Message(). However, the tests (FailedFirstPartyInstallShowsOriginalError and its Marketplace-ID variant) enqueue only the raw DetailedInstallFailureMsg ("The test extension installation failed with a detailed error.") as the expected message, with no wrapping prefix. The displayed message and the test's expected message do not match as written — either the production code should propagate/display the original unwrapped error text (matching the test's stated intent 'shows original error'), or the test's expected string should include the "The app could not be installed. Error message: %1" wrapper.

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}

[martinsrui-msft]

Addressed the latest automated review in d5c53ba6ee by reducing the PR to the active Guid path and one focused test:

• Performance / Message() after DeleteAll() - No code change. AL defers Message() until execution completes, so it does not hold the write transaction across user dismissal like Confirm or StrMenu would. Adding Commit() would introduce an unnecessary transaction boundary.
• XML documentation - Obsolete after this commit: all newly added public test-library helpers were removed.
• Modal-page handler verification - Obsolete after this commit: the fixture integration flow and modal handler were removed.
• Wrapped expected-message mismatch - Obsolete after this commit: the inner try/rethrow and wrapped error label were removed. The remaining test verifies the error captured directly by the existing outer TryFunction.

The final net diff now contains only ExtensionMarketplace.Codeunit.al and ExtensionMarketplaceTest.Codeunit.al.

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Error\ Handling}$

The new install-failure reporting was added only to the AppId overload. Callers that go through InstallAppsourceExtensionWithRefreshSession(MarketplaceApplicationID: Text; ...) still silently delete the pending-setup record and return without showing the captured installation error, so the fix is incomplete and the two public overloads now handle the same failure differently. Mirror the ClearLastError() / GetLastErrorText() / ShowAppInstallationError(...) flow in the text overload as well.

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

    procedure InstallAppsourceExtensionWithRefreshSession(MarketplaceApplicationID: Text; TelemetryURL: Text);
    var
        ExtensionPendingSetup: Record "Extension Pending Setup";
        ExtensionInstallationImpl: Codeunit "Extension Installation Impl";
        MySessionSettings: SessionSettings;
        AppId: Guid;
        InstallErrorText: Text;
    begin
        ExtensionInstallationImpl.CheckPermissions();

        ClearLastError();
        // The page-level TryFunction can return false after a successful install, so verify the installed state.
        if not InstallAppsourceExtension(MarketplaceApplicationID, TelemetryURL) then begin
            InstallErrorText := GetLastErrorText();
            AppId := MapMarketplaceIdToAppId(MarketplaceApplicationID);
            if ExtensionInstallationImpl.IsInstalledByAppId(AppId) then begin
                SaveExtensionPendingSetup(AppId);
                MySessionSettings.Init();
                MySessionSettings.RequestSessionUpdate(false);
            end else begin
                ExtensionPendingSetup.SetRange("User Id", UserSecurityId());
                ExtensionPendingSetup.DeleteAll();
                ShowAppInstallationError(InstallErrorText);
            end;
        end;
    end;

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}

[Thaddeus Loke (thloke)]

This is the wrong fix, it's at best just a workaround. Our extension model has all extension installations recorded under the Extension Management page. The error should be recorded there, just the same as the error we get today when trying to install an extension that is not supported in the current locale for example:

[github-actions]

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

In ShowAppInstallationError (ExtensionMarketplace.Codeunit.al), when InstallErrorText (captured from parameterless GetLastErrorText()) is non-empty it is passed directly to Message(InstallErrorText) with no generic wrapping label. Parameterless GetLastErrorText() can carry customer content (record keys, field values, file names) or unexpected internal exception text from any failure inside the InstallAppsourceExtension TryFunction call graph, and here it is surfaced to the end user verbatim with no context/label — only the empty-string case falls back to a generic label (AppInstallationFailedMsg). Consider always showing a generic label and passing the error text as a %1 substitution (e.g. a label like 'The app could not be installed: %1') so the message always has a stable, reviewable static portion, consistent with the pattern in microsoft/knowledge/privacy/getlasterrortext-customer-content-in-errors.md even though that article's literal anti-pattern targets Error()/StrSubstNo telemetry loss rather than Message() dialogs.

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

    local procedure ShowAppInstallationError(InstallErrorText: Text)
    begin
        if InstallErrorText = '' then
            Message(AppInstallationFailedMsg)
        else
            Message(AppInstallationFailedWithDetailMsg, InstallErrorText);
    end;

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