From 65b48f1789cdfc8c8eaa759d40cc09dc9585c6bc Mon Sep 17 00:00:00 2001 From: azchohfi Date: Wed, 2 Sep 2026 15:39:01 -0700 Subject: [PATCH 01/11] Preserve the base price when publishing (#112) The submission API replaces the whole submission on update, so publish has to send a complete and valid `pricing` object back. Verified against the live API, the base price behaves like this: * `Free`/`Tier96`/`Tier1012`/`Tier1424` - 200 OK, value preserved * `Base` - 400 "'Base' is not a valid PriceId for base price." * empty - 200 OK, and the product silently becomes free * `pricing` omitted - 400 "Pricing data was not provided in the request." 0b4b0bc sent an empty PriceId whenever `IsAdvancedPricingModel` was set, which is the third case: the API accepts it and resets the product to free, with no error to notice. The condition was wrong too - that flag only says which tier range a dashboard offers, and the API reports it inconsistently for the same product, so it fired for products that were never on the newer pricing model. 51c1bc4 stopped the data loss by refusing to publish anything whose PriceId is `Base`, reporting "App updates are supported only for Free products". That is not accurate: a product with a real tier round-trips unchanged and publishes fine. Only the `Base` sentinel cannot be sent back. Publish now leaves any round-trippable price alone, and `--priceId` states the base price explicitly for the products that come back as `Base`. When neither is possible it still stops rather than resetting the price, but now explains why and how to proceed. `submission update` was rejecting based on the price of the *current* submission, which blocked the one payload that can actually update such a product - one carrying a real tier. It now validates the payload being sent. Also fixes a latent bug in `PackagedUpdateCommandAsync`: it signalled failure by returning a boxed `int`, but the caller only checks for `null`, so those paths were reported as success and printed the code as the command's output. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 06440e13-c898-4e58-8764-d7abf304d9d8 --- MSStore.API/Packaged/Models/PriceIds.cs | 103 ++++++++ MSStore.CLI.UnitTests/BaseCommandLineTest.cs | 7 +- .../PublishCommandPricingUnitTests.cs | 225 ++++++++++++++++++ .../SubmissionCommandPackagedUnitTests.cs | 94 ++++++++ MSStore.CLI/Commands/InitCommand.cs | 4 +- MSStore.CLI/Commands/PublishCommand.cs | 26 +- .../Commands/Submission/UpdateCommand.cs | 29 ++- .../Helpers/IStorePackagedAPIExtensions.cs | 52 +++- .../ElectronProjectConfigurator.cs | 4 +- .../FileProjectConfigurator.cs | 4 +- .../ProjectConfigurators/IProjectPublisher.cs | 2 +- .../MSIXProjectPublisher.cs | 4 +- .../PWAProjectConfigurator.cs | 3 +- 13 files changed, 537 insertions(+), 20 deletions(-) create mode 100644 MSStore.API/Packaged/Models/PriceIds.cs create mode 100644 MSStore.CLI.UnitTests/PublishCommandPricingUnitTests.cs diff --git a/MSStore.API/Packaged/Models/PriceIds.cs b/MSStore.API/Packaged/Models/PriceIds.cs new file mode 100644 index 0000000..f6811e3 --- /dev/null +++ b/MSStore.API/Packaged/Models/PriceIds.cs @@ -0,0 +1,103 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +using System; +using System.Globalization; + +namespace MSStore.API.Packaged.Models +{ + /// + /// Well-known values for , and the rules for which + /// of them may be sent back to the Store submission API on an update. + /// + /// + /// These live outside on purpose. That type is serialized with + /// , so any member + /// added to it would show up in the request body. + /// + public static class PriceIds + { + /// + /// Sentinel meaning "the price tier is not set; use the base price for the app". + /// It is a legal value inside , but the + /// API also returns it as the base price of products managed by the newer + /// per-market pricing model - where it cannot be sent back. Updating a submission + /// with it fails with 'Base' is not a valid PriceId for base price. + /// + public const string Base = "Base"; + + /// The app is free. + public const string Free = "Free"; + + /// The app is not available in the given market. + public const string NotAvailable = "NotAvailable"; + + private const string TierPrefix = "Tier"; + + /// + /// Whether a price id read from a submission can be sent back unchanged on update. + /// + /// + /// Everything except (and a missing value) round-trips. Note that + /// an empty price id must never be sent: the API answers 200 OK and silently + /// resets the product to free, which is how paid apps lost their price. + /// + /// The price id to check. + /// true when is safe to send back. + public static bool IsRoundTrippable(string? priceId) => + !string.IsNullOrWhiteSpace(priceId) && + !string.Equals(priceId, Base, StringComparison.OrdinalIgnoreCase); + + /// + /// Validates a user supplied price id and converts it to the casing the API expects. + /// + /// + /// Tier numbers are deliberately not range checked. The documented ranges + /// (Tier2-Tier96 and Tier1012-Tier1424) describe what a + /// dashboard offers, not what the API accepts, and isAdvancedPricingModel is + /// not a reliable way to tell the two apart - the API reports it inconsistently for + /// the same product. Let the service reject an out of range tier. + /// + /// The price id to normalize. + /// The normalized price id, when valid. + /// true when is a value the API accepts. + public static bool TryNormalize(string? priceId, out string? normalized) + { + normalized = null; + + if (string.IsNullOrWhiteSpace(priceId)) + { + return false; + } + + var trimmed = priceId.Trim(); + + if (string.Equals(trimmed, Free, StringComparison.OrdinalIgnoreCase)) + { + normalized = Free; + return true; + } + + if (string.Equals(trimmed, NotAvailable, StringComparison.OrdinalIgnoreCase)) + { + normalized = NotAvailable; + return true; + } + + if (!trimmed.StartsWith(TierPrefix, StringComparison.OrdinalIgnoreCase)) + { + return false; + } + + var tier = trimmed[TierPrefix.Length..]; + + if (tier.Length == 0 || !int.TryParse(tier, NumberStyles.None, CultureInfo.InvariantCulture, out var tierNumber)) + { + return false; + } + + normalized = string.Concat(TierPrefix, tierNumber.ToString(CultureInfo.InvariantCulture)); + return true; + } + } +} diff --git a/MSStore.CLI.UnitTests/BaseCommandLineTest.cs b/MSStore.CLI.UnitTests/BaseCommandLineTest.cs index 8a13f5c..1563b48 100644 --- a/MSStore.CLI.UnitTests/BaseCommandLineTest.cs +++ b/MSStore.CLI.UnitTests/BaseCommandLineTest.cs @@ -410,13 +410,14 @@ internal void AddFakeAccount(AccountEnrollment? accountEnrollment) }); } - protected void AddDefaultFakeSubmission(string listingDescription = "BaseListingDescription") + protected void AddDefaultFakeSubmission(string listingDescription = "BaseListingDescription", Pricing? pricing = null) { var fakeSubmission = new DevCenterSubmission { Id = "123456789", ApplicationCategory = DevCenterApplicationCategory.NotSet, FileUploadUrl = "https://azureblob.com/fileupload", + Pricing = pricing, ApplicationPackages = [ new ApplicationPackage @@ -573,9 +574,9 @@ internal void InitDefaultFlightSubmissionStatusResponseQueue() }); } - protected void AddDefaultFakeSuccessfulSubmission() + protected void AddDefaultFakeSuccessfulSubmission(Pricing? pricing = null) { - AddDefaultFakeSubmission(); + AddDefaultFakeSubmission(pricing: pricing); InitDefaultSubmissionStatusResponseQueue(); FakeStorePackagedAPI diff --git a/MSStore.CLI.UnitTests/PublishCommandPricingUnitTests.cs b/MSStore.CLI.UnitTests/PublishCommandPricingUnitTests.cs new file mode 100644 index 0000000..d90c836 --- /dev/null +++ b/MSStore.CLI.UnitTests/PublishCommandPricingUnitTests.cs @@ -0,0 +1,225 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +using System.CommandLine; +using MSStore.API.Packaged.Models; +using MSStore.CLI.Commands; + +namespace MSStore.CLI.UnitTests +{ + /// + /// Guards the base price across a publish. + /// + /// + /// Verified against the live submission API. On update, the base price behaves like this: + /// + /// Free/Tier96/Tier1012/Tier1424 - 200 OK, value preserved. + /// Base - 400 'Base' is not a valid PriceId for base price. + /// empty - 200 OK, and the product silently becomes free. This is issue #112. + /// pricing omitted - 400 Pricing data was not provided in the request. + /// + /// + [TestClass] + public class PublishCommandPricingUnitTests : BaseCommandLineTest + { + public TestContext TestContext { get; set; } = null!; + + [TestInitialize] + public void Init() + { + FakeLogin(); + AddDefaultFakeAccount(); + AddFakeApps(); + } + + private async Task<((string Output, string Error) Result, DevCenterSubmission? Sent)> PublishMsixAsync( + Pricing? pricing, + int? expectedExitCode = 0, + params string[] extraArgs) + { + var path = CopyFilesRecursively("MSIXProject"); + var msixPath = Path.Combine(path, "test.msix"); + + AddDefaultFakeSuccessfulSubmission(pricing); + + DevCenterSubmission? sent = null; + FakeStorePackagedAPI + .Setup(x => x.UpdateSubmissionAsync( + It.IsAny(), + It.IsAny(), + It.IsAny(), + It.IsAny())) + .Callback((_, _, s, _) => sent = s) + .ReturnsAsync((string _, string _, DevCenterSubmission s, CancellationToken _) => s); + + string[] args = ["publish", msixPath, "--appId", FakeApps[0].Id!, "--verbose", .. extraArgs]; + + var result = await ParseAndInvokeAsync(args, expectedExitCode); + + return (result, sent); + } + + [TestMethod] + [DataRow("Tier1012")] + [DataRow("Tier1424")] + [DataRow("Tier96")] + [DataRow("Free")] + public async Task PublishShouldSendThePriceBackUnchanged(string priceId) + { + // The core regression for issue #112: publishing must never alter the base price. + var (result, sent) = await PublishMsixAsync(new Pricing { PriceId = priceId }); + + result.Error.Should().Contain("Submission commit success! Here is some data:"); + + sent.Should().NotBeNull(); + sent!.Pricing.Should().NotBeNull(); + sent.Pricing!.PriceId.Should().Be(priceId); + } + + [TestMethod] + public async Task PublishShouldNeverSendAnEmptyPrice() + { + // Commit 0b4b0bc set PriceId to null here. The API answers 200 OK and resets the + // product to free, so this can only be caught by inspecting the outgoing payload. + var (_, sent) = await PublishMsixAsync( + new Pricing { PriceId = "Tier1012", IsAdvancedPricingModel = true }); + + sent!.Pricing!.PriceId.Should().NotBeNullOrWhiteSpace(); + sent.Pricing.PriceId.Should().Be("Tier1012"); + } + + [TestMethod] + public async Task PublishShouldIgnoreIsAdvancedPricingModel() + { + // isAdvancedPricingModel only describes which tier range a dashboard offers, and the + // API reports it inconsistently for the same product. It must not drive any decision. + var (result, sent) = await PublishMsixAsync( + new Pricing { PriceId = "Tier1012", IsAdvancedPricingModel = true }); + + result.Error.Should().Contain("Submission commit success! Here is some data:"); + sent!.Pricing!.PriceId.Should().Be("Tier1012"); + } + + [TestMethod] + public async Task PublishShouldStopWhenThePriceCannotBePreserved() + { + var (result, sent) = await PublishMsixAsync(new Pricing { PriceId = "Base" }, -1); + + result.Error.Should().Contain("Could not preserve this product's price"); + result.Error.Should().Contain("--priceId"); + + // Nothing may be sent, otherwise the product would be reset to free. + sent.Should().BeNull(); + + FakeStorePackagedAPI + .Verify( + x => x.DeleteSubmissionAsync(It.IsAny(), It.IsAny(), It.IsAny()), + Times.Once); + } + + [TestMethod] + public async Task PublishShouldNotClaimOnlyFreeProductsAreSupported() + { + // The old message said "App updates are supported only for Free products", which is + // wrong: a product with a real tier publishes fine, as the tests above show. + var (result, _) = await PublishMsixAsync(new Pricing { PriceId = "Base" }, -1); + + result.Error.Should().NotContain("only for Free products"); + } + + [TestMethod] + public async Task PublishWithPriceIdShouldRecoverAProductWhoseBasePriceIsNotRoundTrippable() + { + var (result, sent) = await PublishMsixAsync( + new Pricing { PriceId = "Base" }, + 0, + "--priceId", + "Tier1012"); + + result.Error.Should().Contain("Submission commit success! Here is some data:"); + sent!.Pricing!.PriceId.Should().Be("Tier1012"); + } + + [TestMethod] + public async Task PublishWithPriceIdShouldOverrideAnExistingTier() + { + var (_, sent) = await PublishMsixAsync( + new Pricing { PriceId = "Tier1012" }, + 0, + "--priceId", + "Tier1424"); + + sent!.Pricing!.PriceId.Should().Be("Tier1424"); + } + + [TestMethod] + public async Task PublishShouldSucceedWhenTheProductHasNoPricingAtAll() + { + var (result, sent) = await PublishMsixAsync(null); + + result.Error.Should().Contain("Submission commit success! Here is some data:"); + sent!.Pricing.Should().BeNull(); + } + + [TestMethod] + public async Task PublishWithPriceIdShouldApplyEvenWhenTheProductHasNoPricingAtAll() + { + // The API rejects an update that omits pricing, so an explicit price has to be + // materialized rather than silently dropped. + var (result, sent) = await PublishMsixAsync(null, 0, "--priceId", "Tier1012"); + + result.Error.Should().Contain("Submission commit success! Here is some data:"); + sent!.Pricing.Should().NotBeNull(); + sent.Pricing!.PriceId.Should().Be("Tier1012"); + } + + private static ParseResult ParsePublish(params string[] args) => + new PublishCommand().Parse(args); + + [TestMethod] + public void PublishCommandPriceIdShouldDefaultToNullWhenOmitted() + { + ParsePublish("publish", ".") + .GetValue(PublishCommand.PriceIdOption) + .Should() + .BeNull(); + } + + [TestMethod] + [DataRow("Tier1012", "Tier1012")] + [DataRow("tier1012", "Tier1012")] + [DataRow("TIER1012", "Tier1012")] + [DataRow(" Tier1012 ", "Tier1012")] + [DataRow("Free", "Free")] + [DataRow("free", "Free")] + [DataRow("NotAvailable", "NotAvailable")] + [DataRow("Tier2", "Tier2")] + public void PublishCommandPriceIdShouldNormalizeAcceptedValues(string input, string expected) + { + var parseResult = ParsePublish("publish", ".", "--priceId", input); + + parseResult.Errors.Should().BeEmpty(); + parseResult.GetValue(PublishCommand.PriceIdOption).Should().Be(expected); + } + + [TestMethod] + [DataRow("Base")] + [DataRow("Tier")] + [DataRow("1012")] + [DataRow("TierAbc")] + [DataRow("Tier-1")] + [DataRow("not-a-tier")] + public void PublishCommandPriceIdShouldRejectValuesTheApiWouldNotAccept(string input) + { + // "Base" is rejected on purpose: it is exactly the value that cannot be sent back. + ParsePublish("publish", ".", "--priceId", input) + .Errors + .Should() + .ContainSingle() + .Which + .Message + .Should() + .Be("Invalid price id. The value must be 'Free', 'NotAvailable', or a tier such as 'Tier1012'."); + } + } +} diff --git a/MSStore.CLI.UnitTests/SubmissionCommandPackagedUnitTests.cs b/MSStore.CLI.UnitTests/SubmissionCommandPackagedUnitTests.cs index 7456a63..bf92d2d 100644 --- a/MSStore.CLI.UnitTests/SubmissionCommandPackagedUnitTests.cs +++ b/MSStore.CLI.UnitTests/SubmissionCommandPackagedUnitTests.cs @@ -144,6 +144,100 @@ public async Task PackagedSubmissionUpdateCommand() result.Output.Should().Contain("\"FileUploadUrl\": \"https://azureblob.com/fileupload\""); } + [TestMethod] + public async Task PackagedSubmissionUpdateCommandShouldAcceptAPayloadCarryingARealTier() + { + // A per-market priced product can only be updated by naming a real tier in the + // payload. Blocking on the *current* submission's price would reject this. + FakeApps[0].PendingApplicationSubmission = new ApplicationSubmissionInfo + { + Id = "123456789" + }; + + AddDefaultFakeSubmission(pricing: new Pricing { PriceId = "Base" }); + + DevCenterSubmission? sent = null; + FakeStorePackagedAPI + .Setup(x => x.UpdateSubmissionAsync( + It.IsAny(), + It.IsAny(), + It.IsAny(), + It.IsAny())) + .Callback((_, _, s, _) => sent = s) + .ReturnsAsync((string _, string _, DevCenterSubmission s, CancellationToken _) => s); + + var result = await ParseAndInvokeAsync( + [ + "submission", + "update", + FakeApps[0].Id!, + @" +{ +""Pricing"": { ""PriceId"": ""Tier1012"" }, +""ApplicationPackages"": + [ + { + ""FileName"":""C:\\temp\\installer.msix"" + } + ] +}" + ]); + + result.Error.Should().Contain("Updating submission product"); + sent!.Pricing!.PriceId.Should().Be("Tier1012"); + } + + [TestMethod] + public async Task PackagedSubmissionUpdateCommandShouldRejectAPayloadThatWouldResetThePrice() + { + // "Base" is what the API hands back for a per-market priced product, and sending it + // straight back is rejected with "'Base' is not a valid PriceId for base price." + var result = await ParseAndInvokeAsync( + [ + "submission", + "update", + FakeApps[0].Id!, + @"{ ""Pricing"": { ""PriceId"": ""Base"" } }" + ], -1); + + result.Error.Should().Contain("which the submission API will not accept"); + result.Error.Should().NotContain("only for Free products"); + + FakeStorePackagedAPI + .Verify( + x => x.UpdateSubmissionAsync( + It.IsAny(), + It.IsAny(), + It.IsAny(), + It.IsAny()), + Times.Never); + } + + [TestMethod] + public async Task PackagedSubmissionUpdateCommandShouldRejectAPayloadWithAnEmptyPrice() + { + // An empty price is the dangerous one: the API answers 200 OK and silently resets + // the product to free. + var result = await ParseAndInvokeAsync( + [ + "submission", + "update", + FakeApps[0].Id!, + @"{ ""Pricing"": { ""TrialPeriod"": ""NoFreeTrial"" } }" + ], -1); + + result.Error.Should().Contain("which the submission API will not accept"); + + FakeStorePackagedAPI + .Verify( + x => x.UpdateSubmissionAsync( + It.IsAny(), + It.IsAny(), + It.IsAny(), + It.IsAny()), + Times.Never); + } + [TestMethod] public async Task PackagedSubmissionUpdateCommandWithPayloadOption() { diff --git a/MSStore.CLI/Commands/InitCommand.cs b/MSStore.CLI/Commands/InitCommand.cs index 9801f6e..c8090f3 100644 --- a/MSStore.CLI/Commands/InitCommand.cs +++ b/MSStore.CLI/Commands/InitCommand.cs @@ -135,6 +135,7 @@ public InitCommand() Options.Add(ArchOption); Options.Add(VersionOption); Options.Add(PublishCommand.PackageRolloutPercentageOption); + Options.Add(PublishCommand.PriceIdOption); Options.Add(PublishCommand.UploadTimeoutOption); } @@ -175,6 +176,7 @@ public override async Task InvokeAsync(ParseResult parseResult, Cancellatio var flightId = parseResult.GetValue(PublishCommand.FlightIdOption); var version = parseResult.GetValue(VersionOption); var packageRolloutPercentage = parseResult.GetValue(PublishCommand.PackageRolloutPercentageOption); + var priceId = parseResult.GetValue(PublishCommand.PriceIdOption); var uploadTimeout = parseResult.GetValue(PublishCommand.UploadTimeoutOption); var output = parseResult.GetValue(OutputOption); var arch = parseResult.GetValue(ArchOption); @@ -363,7 +365,7 @@ public override async Task InvokeAsync(ParseResult parseResult, Cancellatio return await _telemetryClient.TrackCommandEventAsync(-5, props, ct); } - result = await projectPublisher.PublishAsync(pathOrUrl, app, flightId, outputDirectory, false, packageRolloutPercentage, uploadTimeout, storePackagedAPI, ct); + result = await projectPublisher.PublishAsync(pathOrUrl, app, flightId, outputDirectory, false, packageRolloutPercentage, priceId, uploadTimeout, storePackagedAPI, ct); } return await _telemetryClient.TrackCommandEventAsync(result, props, ct); diff --git a/MSStore.CLI/Commands/PublishCommand.cs b/MSStore.CLI/Commands/PublishCommand.cs index 00e26c7..1467fc1 100644 --- a/MSStore.CLI/Commands/PublishCommand.cs +++ b/MSStore.CLI/Commands/PublishCommand.cs @@ -12,6 +12,7 @@ using System.Threading.Tasks; using Microsoft.ApplicationInsights; using Microsoft.Extensions.Logging; +using MSStore.API.Packaged.Models; using MSStore.CLI.Helpers; using MSStore.CLI.ProjectConfigurators; using MSStore.CLI.Services; @@ -28,6 +29,7 @@ internal class PublishCommand : Command internal static readonly Option FlightIdOption; internal static readonly Option PackageRolloutPercentageOption; + internal static readonly Option PriceIdOption; internal static readonly Option UploadTimeoutOption; private static readonly Option InputDirectoryOption; private static readonly Option AppIdOption; @@ -67,6 +69,26 @@ static PublishCommand() } }; + PriceIdOption = new Option("--priceId", "-pid") + { + Description = "Specifies the base price tier to set on the submission, for example 'Tier1012', 'Free' or 'NotAvailable'. Only needed when the Store reports a base price the submission API will not accept back, which happens when the price is managed per market from Partner Center.", + CustomParser = result => + { + if (result.Tokens.Count == 0) + { + return null; + } + + if (!PriceIds.TryNormalize(result.Tokens.Single().Value, out var normalized)) + { + result.AddError("Invalid price id. The value must be 'Free', 'NotAvailable', or a tier such as 'Tier1012'."); + return null; + } + + return normalized; + } + }; + InputDirectoryOption = new Option("--inputDirectory", "-i") { Description = "The directory where the '.msix' or '.msixupload' file to be used for the publishing command. If not provided, the cli will try to find the best candidate based on the 'pathOrUrl' argument.", @@ -143,6 +165,7 @@ public PublishCommand() Options.Add(NoCommitOption); Options.Add(FlightIdOption); Options.Add(PackageRolloutPercentageOption); + Options.Add(PriceIdOption); Options.Add(UploadTimeoutOption); } @@ -165,6 +188,7 @@ public override async Task InvokeAsync(ParseResult parseResult, Cancellatio var appId = parseResult.GetValue(AppIdOption); var flightId = parseResult.GetValue(FlightIdOption); var packageRolloutPercentage = parseResult.GetValue(PackageRolloutPercentageOption); + var priceId = parseResult.GetValue(PriceIdOption); var inputDirectory = parseResult.GetValue(InputDirectoryOption); var noCommit = parseResult.GetRequiredValue(NoCommitOption); var uploadTimeout = parseResult.GetValue(UploadTimeoutOption); @@ -214,7 +238,7 @@ public override async Task InvokeAsync(ParseResult parseResult, Cancellatio } return await _telemetryClient.TrackCommandEventAsync( - await projectPublisher.PublishAsync(pathOrUrl, app, flightId, inputDirectory, noCommit, packageRolloutPercentage, uploadTimeout, storePackagedAPI, ct), props, ct); + await projectPublisher.PublishAsync(pathOrUrl, app, flightId, inputDirectory, noCommit, packageRolloutPercentage, priceId, uploadTimeout, storePackagedAPI, ct), props, ct); } } } diff --git a/MSStore.CLI/Commands/Submission/UpdateCommand.cs b/MSStore.CLI/Commands/Submission/UpdateCommand.cs index 7d3792d..0ffe294 100644 --- a/MSStore.CLI/Commands/Submission/UpdateCommand.cs +++ b/MSStore.CLI/Commands/Submission/UpdateCommand.cs @@ -12,6 +12,7 @@ using MSStore.API; using MSStore.API.Models; using MSStore.API.Packaged; +using MSStore.API.Packaged.Models; using MSStore.CLI.Helpers; using MSStore.CLI.Services; using Spectre.Console; @@ -48,6 +49,14 @@ public class Handler(ILogger logger, IStoreAPIFactory sto private readonly IAnsiConsole _ansiConsole = ansiConsole ?? throw new ArgumentNullException(nameof(ansiConsole)); private readonly TelemetryClient _telemetryClient = telemetryClient ?? throw new ArgumentNullException(nameof(telemetryClient)); + /// + /// Updates a packaged submission. + /// + /// + /// The updated submission, or null when the update did not happen. A failure + /// must never be signalled with an int: the caller only checks for null, + /// so a boxed code would be reported as success and printed as the command's output. + /// public static async Task PackagedUpdateCommandAsync(IAnsiConsole ansiConsole, IStoreAPIFactory storeAPIFactory, string product, string productId, ILogger logger, CancellationToken ct) { var updateSubmission = JsonSerializer.Deserialize(product, SourceGenerationContext.GetCustom().DevCenterSubmission); @@ -84,7 +93,7 @@ public class Handler(ILogger logger, IStoreAPIFactory sto if (storePackagedAPI == null || application == null || application?.Id == null) { - return 1; + return null; } string? submissionId = application.PendingApplicationSubmission?.Id; @@ -103,11 +112,21 @@ public class Handler(ILogger logger, IStoreAPIFactory sto } var currentSubmission = await storePackagedAPI.GetSubmissionAsync(application.Id, submissionId, ct); - if (currentSubmission?.Pricing != null && currentSubmission?.Pricing?.PriceId == "Base") + + // Only the payload actually being sent matters here. Blocking on the *current* + // submission would reject a perfectly good update whose JSON already carries a + // valid tier, which is the one way a per-market priced product can be updated. + var updatedPriceId = updateSubmission.Pricing?.PriceId; + if (updateSubmission.Pricing != null && !PriceIds.IsRoundTrippable(updatedPriceId)) { - await storePackagedAPI.DeleteSubmissionAsync(application.Id, submissionId: currentSubmission.Id!, ct); - ansiConsole.MarkupLine("[red bold]App updates are supported only for Free products.[/]"); - return -1; + if (currentSubmission?.Id != null && application.PendingApplicationSubmission?.Id == null) + { + await storePackagedAPI.DeleteSubmissionAsync(application.Id, currentSubmission.Id, ct); + } + + ansiConsole.MarkupLine($"[red bold]The provided product has a base price of '{(updatedPriceId ?? "").EscapeMarkup()}', which the submission API will not accept.[/]"); + ansiConsole.MarkupLine("Sending it would reset the product to [bold]Free[/]. Set 'Pricing.PriceId' to a real tier (for example 'Tier1012'), 'Free', or 'NotAvailable' and try again."); + return null; } return await ansiConsole.Status().StartAsync("Updating submission product", async ctx => diff --git a/MSStore.CLI/Helpers/IStorePackagedAPIExtensions.cs b/MSStore.CLI/Helpers/IStorePackagedAPIExtensions.cs index b2b41ad..b7e744a 100644 --- a/MSStore.CLI/Helpers/IStorePackagedAPIExtensions.cs +++ b/MSStore.CLI/Helpers/IStorePackagedAPIExtensions.cs @@ -352,6 +352,7 @@ public static async Task PublishAsync( IEnumerable input, bool noCommit, float? packageRolloutPercentage, + string? priceId, long uploadTimeout, IBrowserLauncher browserLauncher, IConsoleReader consoleReader, @@ -493,10 +494,10 @@ public static async Task PublishAsync( DevCenterSubmission? devCenterSubmission = submission as DevCenterSubmission; DevCenterFlightSubmission? devCenterFlightSubmission = submission as DevCenterFlightSubmission; - if (devCenterSubmission?.Pricing != null && devCenterSubmission?.Pricing?.PriceId == "Base") + if (devCenterSubmission != null + && !TryPreservePricing(ansiConsole, devCenterSubmission, priceId, logger)) { await storePackagedAPI.DeleteSubmissionAsync(app.Id, submission.Id, ct); - ansiConsole.MarkupLine("[red bold]App updates are supported only for Free products.[/]"); return -1; } @@ -785,6 +786,53 @@ private static async Task CreateImageAsync(string listingKey, Image image, return false; } + /// + /// Makes sure publishing never changes the product's price. + /// + /// + /// The submission API replaces the whole submission on update, so a complete and valid + /// pricing object has to be sent back. Only three things can go in + /// , and for a product on the newer per-market pricing model + /// the API returns , which is not one of them: + /// + /// - rejected with 'Base' is not a valid PriceId for base price. + /// an empty value - accepted with 200 OK, and the product silently becomes free. + /// omitting pricing - rejected with Pricing data was not provided in the request. + /// + /// Any other price id (Free, NotAvailable, Tier1012, ...) round-trips + /// unchanged, so paid products publish fine as long as we leave the value alone. + /// + /// false when the price cannot be preserved and publishing must stop. + internal static bool TryPreservePricing(IAnsiConsole ansiConsole, DevCenterSubmission submission, string? priceIdOverride, ILogger logger) + { + if (priceIdOverride != null) + { + logger.LogInformation("Overriding PriceId '{OriginalPriceId}' with '{PriceIdOverride}'.", submission.Pricing?.PriceId, priceIdOverride); + ansiConsole.MarkupLine($"Setting the base price to [green]{priceIdOverride.EscapeMarkup()}[/]."); + + // A product whose price is managed per market can come back without any pricing + // at all, and the API rejects an update that omits it. + submission.Pricing ??= new Pricing(); + submission.Pricing.PriceId = priceIdOverride; + return true; + } + + if (submission.Pricing == null || PriceIds.IsRoundTrippable(submission.Pricing.PriceId)) + { + return true; + } + + var priceId = submission.Pricing.PriceId; + logger.LogError("Cannot preserve the product's price. The API returned PriceId '{PriceId}', which it does not accept back on update.", priceId); + + ansiConsole.MarkupLine("[red bold]Could not preserve this product's price.[/]"); + ansiConsole.MarkupLine($"The Store returned a base price of [yellow]'{(priceId ?? "").EscapeMarkup()}'[/], which the submission API refuses on update. This happens when the price is managed per market from Partner Center."); + ansiConsole.MarkupLine("Publishing would reset the product to [bold]Free[/], so it has been stopped instead."); + ansiConsole.MarkupLine("Re-run with [green]--priceId[/] to state the base price explicitly (for example [green]--priceId Tier1012[/]), or publish this submission from Partner Center."); + + return false; + } + private static async Task FulfillApplicationAsync(IAnsiConsole ansiConsole, DevCenterApplication app, DevCenterSubmission submission, FirstSubmissionDataCallback firstSubmissionDataCallback, AllowTargetFutureDeviceFamily[] allowTargetFutureDeviceFamilies, IConsoleReader consoleReader, IEnvironmentInformationService environmentInformationService, ILogger logger, CancellationToken ct) { if (submission.ApplicationCategory == DevCenterApplicationCategory.NotSet) diff --git a/MSStore.CLI/ProjectConfigurators/ElectronProjectConfigurator.cs b/MSStore.CLI/ProjectConfigurators/ElectronProjectConfigurator.cs index 9ee24f1..1f2d5a3 100644 --- a/MSStore.CLI/ProjectConfigurators/ElectronProjectConfigurator.cs +++ b/MSStore.CLI/ProjectConfigurators/ElectronProjectConfigurator.cs @@ -406,7 +406,7 @@ private async Task EnsureElectronManifestAsync(FileInfo? fileInfo, CancellationT _electronManifest ??= await _electronManifestManager.LoadAsync(fileInfo, ct); } - public override async Task PublishAsync(string pathOrUrl, DevCenterApplication? app, string? flightId, DirectoryInfo? inputDirectory, bool noCommit, float? packageRolloutPercentage, long uploadTimeout, IStorePackagedAPI storePackagedAPI, CancellationToken ct) + public override async Task PublishAsync(string pathOrUrl, DevCenterApplication? app, string? flightId, DirectoryInfo? inputDirectory, bool noCommit, float? packageRolloutPercentage, string? priceId, long uploadTimeout, IStorePackagedAPI storePackagedAPI, CancellationToken ct) { if (_electronManifest == null) { @@ -414,7 +414,7 @@ public override async Task PublishAsync(string pathOrUrl, DevCenterApplicat await EnsureElectronManifestAsync(manifestFile, ct); } - return await base.PublishAsync(pathOrUrl, app, flightId, inputDirectory, noCommit, packageRolloutPercentage, uploadTimeout, storePackagedAPI, ct); + return await base.PublishAsync(pathOrUrl, app, flightId, inputDirectory, noCommit, packageRolloutPercentage, priceId, uploadTimeout, storePackagedAPI, ct); } } } \ No newline at end of file diff --git a/MSStore.CLI/ProjectConfigurators/FileProjectConfigurator.cs b/MSStore.CLI/ProjectConfigurators/FileProjectConfigurator.cs index d4b755a..eb1515e 100644 --- a/MSStore.CLI/ProjectConfigurators/FileProjectConfigurator.cs +++ b/MSStore.CLI/ProjectConfigurators/FileProjectConfigurator.cs @@ -138,7 +138,7 @@ protected static FileInfo FindFile(DirectoryInfo projectRootPath, string searchP return Task.FromResult<(string, List)>((description, images)); } - public virtual async Task PublishAsync(string pathOrUrl, DevCenterApplication? app, string? flightId, DirectoryInfo? inputDirectory, bool noCommit, float? packageRolloutPercentage, long uploadTimeout, IStorePackagedAPI storePackagedAPI, CancellationToken ct) + public virtual async Task PublishAsync(string pathOrUrl, DevCenterApplication? app, string? flightId, DirectoryInfo? inputDirectory, bool noCommit, float? packageRolloutPercentage, string? priceId, long uploadTimeout, IStorePackagedAPI storePackagedAPI, CancellationToken ct) { var (projectRootPath, projectFile) = GetInfo(pathOrUrl); @@ -181,7 +181,7 @@ public virtual async Task PublishAsync(string pathOrUrl, DevCenterApplicati Logger.LogInformation("Trying to publish these {FileCount} files: {FileNames}", packageFiles.Count(), string.Join(", ", packageFiles.Select(f => $"'{f.FullName}'"))); - return await storePackagedAPI.PublishAsync(ErrorAnsiConsole, app, flightId, GetFirstSubmissionDataAsync, AllowTargetFutureDeviceFamilies, output, packageFiles, noCommit, packageRolloutPercentage, uploadTimeout, _browserLauncher, _consoleReader, _zipFileManager, _fileDownloader, _azureBlobManager, _environmentInformationService, _logger, ct); + return await storePackagedAPI.PublishAsync(ErrorAnsiConsole, app, flightId, GetFirstSubmissionDataAsync, AllowTargetFutureDeviceFamilies, output, packageFiles, noCommit, packageRolloutPercentage, priceId, uploadTimeout, _browserLauncher, _consoleReader, _zipFileManager, _fileDownloader, _azureBlobManager, _environmentInformationService, _logger, ct); } protected virtual DirectoryInfo GetInputDirectory(DirectoryInfo projectRootPath) diff --git a/MSStore.CLI/ProjectConfigurators/IProjectPublisher.cs b/MSStore.CLI/ProjectConfigurators/IProjectPublisher.cs index af06f30..6bb88d5 100644 --- a/MSStore.CLI/ProjectConfigurators/IProjectPublisher.cs +++ b/MSStore.CLI/ProjectConfigurators/IProjectPublisher.cs @@ -17,6 +17,6 @@ internal interface IProjectPublisher SearchOption PackageFilesSearchOption { get; } AllowTargetFutureDeviceFamily[] AllowTargetFutureDeviceFamilies { get; } Task CanPublishAsync(string pathOrUrl, CancellationToken ct); - Task PublishAsync(string pathOrUrl, DevCenterApplication? app, string? flightId, DirectoryInfo? inputDirectory, bool noCommit, float? packageRolloutPercentage, long uploadTimeout, IStorePackagedAPI storePackagedAPI, CancellationToken ct); + Task PublishAsync(string pathOrUrl, DevCenterApplication? app, string? flightId, DirectoryInfo? inputDirectory, bool noCommit, float? packageRolloutPercentage, string? priceId, long uploadTimeout, IStorePackagedAPI storePackagedAPI, CancellationToken ct); } } \ No newline at end of file diff --git a/MSStore.CLI/ProjectConfigurators/MSIXProjectPublisher.cs b/MSStore.CLI/ProjectConfigurators/MSIXProjectPublisher.cs index f8d6850..43e9590 100644 --- a/MSStore.CLI/ProjectConfigurators/MSIXProjectPublisher.cs +++ b/MSStore.CLI/ProjectConfigurators/MSIXProjectPublisher.cs @@ -94,7 +94,7 @@ public Task CanPublishAsync(string pathOrUrl, CancellationToken ct) return Task.FromResult(_appXManifestManager.GetAppId(appxManifest)); } - public async Task PublishAsync(string pathOrUrl, DevCenterApplication? app, string? flightId, DirectoryInfo? inputDirectory, bool noCommit, float? packageRolloutPercentage, long uploadTimeout, IStorePackagedAPI storePackagedAPI, CancellationToken ct) + public async Task PublishAsync(string pathOrUrl, DevCenterApplication? app, string? flightId, DirectoryInfo? inputDirectory, bool noCommit, float? packageRolloutPercentage, string? priceId, long uploadTimeout, IStorePackagedAPI storePackagedAPI, CancellationToken ct) { var msix = new FileInfo(pathOrUrl); @@ -131,7 +131,7 @@ public async Task PublishAsync(string pathOrUrl, DevCenterApplication? app, _logger.LogInformation("Trying to publish these {FileCount} files: {FileNames}", packageFiles.Count, string.Join(", ", packageFiles.Select(f => $"'{f.FullName}'"))); - return await storePackagedAPI.PublishAsync(_ansiConsole, _app, flightId, GetFirstSubmissionDataAsync, AllowTargetFutureDeviceFamilies, output, packageFiles, noCommit, packageRolloutPercentage, uploadTimeout, _browserLauncher, _consoleReader, _zipFileManager, _fileDownloader, _azureBlobManager, _environmentInformationService, _logger, ct); + return await storePackagedAPI.PublishAsync(_ansiConsole, _app, flightId, GetFirstSubmissionDataAsync, AllowTargetFutureDeviceFamilies, output, packageFiles, noCommit, packageRolloutPercentage, priceId, uploadTimeout, _browserLauncher, _consoleReader, _zipFileManager, _fileDownloader, _azureBlobManager, _environmentInformationService, _logger, ct); } private Task<(string Description, List Images)> GetFirstSubmissionDataAsync(string listingLanguage, CancellationToken ct) diff --git a/MSStore.CLI/ProjectConfigurators/PWAProjectConfigurator.cs b/MSStore.CLI/ProjectConfigurators/PWAProjectConfigurator.cs index ffa0aab..8fe44ef 100644 --- a/MSStore.CLI/ProjectConfigurators/PWAProjectConfigurator.cs +++ b/MSStore.CLI/ProjectConfigurators/PWAProjectConfigurator.cs @@ -317,7 +317,7 @@ await _pwaAppInfoManager.SaveAsync( return Task.FromResult((0, (DirectoryInfo?)new DirectoryInfo(pathOrUrl))); } - public async Task PublishAsync(string pathOrUrl, DevCenterApplication? app, string? flightId, DirectoryInfo? inputDirectory, bool noCommit, float? packageRolloutPercentage, long uploadTimeout, IStorePackagedAPI storePackagedAPI, CancellationToken ct) + public async Task PublishAsync(string pathOrUrl, DevCenterApplication? app, string? flightId, DirectoryInfo? inputDirectory, bool noCommit, float? packageRolloutPercentage, string? priceId, long uploadTimeout, IStorePackagedAPI storePackagedAPI, CancellationToken ct) { Uri? uri = GetUri(pathOrUrl); @@ -391,6 +391,7 @@ public async Task PublishAsync(string pathOrUrl, DevCenterApplication? app, packageFiles, noCommit, packageRolloutPercentage, + priceId, uploadTimeout, _browserLauncher, _consoleReader, From b72ce89ebe2083bc7ae8fcd8032226c7cc6b815e Mon Sep 17 00:00:00 2001 From: azchohfi Date: Wed, 2 Sep 2026 16:00:55 -0700 Subject: [PATCH 02/11] Record that omitting PriceId also resets the price Follow-up to the #112 fix. The obvious way to avoid having to know a product's price is to leave the property out of the request instead of sending it empty. That does not work, and the API gives no hint of it - measured on a throwaway draft whose base price was Tier1012: * pricing present, `priceId` property removed - 200 OK, price becomes Free * `pricing: {}` - 200 OK, price becomes Free * `pricing: null` - 400 "Pricing data was not provided in the request." Update has no patch semantics: anything the request does not state explicitly is reset to its default, and the default is free. There is no way to say "leave the price alone", which is why the price has to be stated even by a publish that has no interest in it. The real price cannot be looked up either. It is absent from the application resource, the newer submission API returns 404 for packaged products ("No Product Found with Product Id present in API Request"), and no API exposes the price tier table. Documents all of that where the decision is made, and guards the serializer: adding JsonIgnore(WhenWritingNull) to Pricing.PriceId looks like a tidy-up but would silently reintroduce the bug. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 06440e13-c898-4e58-8764-d7abf304d9d8 --- MSStore.API/Packaged/Models/PriceIds.cs | 12 ++++++--- .../PublishCommandPricingUnitTests.cs | 26 +++++++++++++++++++ .../Helpers/IStorePackagedAPIExtensions.cs | 19 +++++++++----- 3 files changed, 48 insertions(+), 9 deletions(-) diff --git a/MSStore.API/Packaged/Models/PriceIds.cs b/MSStore.API/Packaged/Models/PriceIds.cs index f6811e3..acd0f1c 100644 --- a/MSStore.API/Packaged/Models/PriceIds.cs +++ b/MSStore.API/Packaged/Models/PriceIds.cs @@ -38,9 +38,15 @@ public static class PriceIds /// Whether a price id read from a submission can be sent back unchanged on update. /// /// - /// Everything except (and a missing value) round-trips. Note that - /// an empty price id must never be sent: the API answers 200 OK and silently - /// resets the product to free, which is how paid apps lost their price. + /// Everything except (and a missing value) round-trips. + /// + /// An empty price id must never be sent. Update is a full replace with no patch + /// semantics, so anything the request does not state explicitly is reset to its default, + /// and the default is free. Verified against the API: a null price id, a pricing + /// object with the property removed, and an empty pricing object all answer + /// 200 OK and silently turn the product free. Omitting the property is therefore + /// not a way to leave the price untouched - there is no such way. + /// /// /// The price id to check. /// true when is safe to send back. diff --git a/MSStore.CLI.UnitTests/PublishCommandPricingUnitTests.cs b/MSStore.CLI.UnitTests/PublishCommandPricingUnitTests.cs index d90c836..791e49a 100644 --- a/MSStore.CLI.UnitTests/PublishCommandPricingUnitTests.cs +++ b/MSStore.CLI.UnitTests/PublishCommandPricingUnitTests.cs @@ -2,6 +2,8 @@ // Licensed under the MIT License. using System.CommandLine; +using System.Text.Json; +using MSStore.API.Models; using MSStore.API.Packaged.Models; using MSStore.CLI.Commands; @@ -173,6 +175,30 @@ public async Task PublishWithPriceIdShouldApplyEvenWhenTheProductHasNoPricingAtA sent.Pricing!.PriceId.Should().Be("Tier1012"); } + [TestMethod] + public void PricingMustAlwaysSerializeThePriceIdProperty() + { + // Guards against "tidying" the payload by omitting a null PriceId. Update has no + // patch semantics: a pricing object with the property removed is accepted with + // 200 OK and turns the product free, exactly like sending it as null. Adding + // JsonIgnore(WhenWritingNull) to Pricing.PriceId would silently reintroduce #112. + var json = JsonSerializer.Serialize( + new DevCenterSubmission { Pricing = new Pricing { PriceId = null } }, + SourceGenerationContext.GetCustom().DevCenterSubmission); + + json.Should().Contain("\"PriceId\""); + } + + [TestMethod] + public void PricingShouldSerializeARealTierVerbatim() + { + var json = JsonSerializer.Serialize( + new DevCenterSubmission { Pricing = new Pricing { PriceId = "Tier1012" } }, + SourceGenerationContext.GetCustom().DevCenterSubmission); + + json.Should().Contain("\"PriceId\":\"Tier1012\""); + } + private static ParseResult ParsePublish(params string[] args) => new PublishCommand().Parse(args); diff --git a/MSStore.CLI/Helpers/IStorePackagedAPIExtensions.cs b/MSStore.CLI/Helpers/IStorePackagedAPIExtensions.cs index b7e744a..d47a7f4 100644 --- a/MSStore.CLI/Helpers/IStorePackagedAPIExtensions.cs +++ b/MSStore.CLI/Helpers/IStorePackagedAPIExtensions.cs @@ -790,17 +790,24 @@ private static async Task CreateImageAsync(string listingKey, Image image, /// Makes sure publishing never changes the product's price. /// /// - /// The submission API replaces the whole submission on update, so a complete and valid - /// pricing object has to be sent back. Only three things can go in - /// , and for a product on the newer per-market pricing model - /// the API returns , which is not one of them: + /// The submission API replaces the whole submission on update - there are no patch + /// semantics, so anything the request does not state explicitly is reset to its default. + /// A complete and valid pricing object therefore has to be sent back every time, + /// even when publishing has no interest in the price. For a product on the newer + /// per-market pricing model the API returns , and none of the + /// ways of expressing "leave the price alone" work: /// /// - rejected with 'Base' is not a valid PriceId for base price. - /// an empty value - accepted with 200 OK, and the product silently becomes free. - /// omitting pricing - rejected with Pricing data was not provided in the request. + /// an empty value, the property removed, or an empty pricing object - all accepted with 200 OK, and the product silently becomes free. + /// a null or omitted pricing - rejected with Pricing data was not provided in the request. /// + /// Nor can the real price be looked up: it is absent from the application resource, the + /// newer submission API does not know packaged products, and no API exposes the price + /// tier table. So for those products the price can only come from the caller. + /// /// Any other price id (Free, NotAvailable, Tier1012, ...) round-trips /// unchanged, so paid products publish fine as long as we leave the value alone. + /// /// /// false when the price cannot be preserved and publishing must stop. internal static bool TryPreservePricing(IAnsiConsole ansiConsole, DevCenterSubmission submission, string? priceIdOverride, ILogger logger) From 51576d4ceb20e703980e71ef8fe610700ff79a2d Mon Sep 17 00:00:00 2001 From: azchohfi Date: Wed, 2 Sep 2026 20:12:17 -0700 Subject: [PATCH 03/11] Stop publishing when the submission carries no pricing Addresses Copilot review feedback on #175. TryPreservePricing treated a missing pricing object as preservable and let it through, but the API rejects such an update with "Pricing data was not provided in the request.". Publish would have surfaced a raw 400 from deep inside UpdateSubmissionAsync instead of stopping with the same actionable guidance it already gives for a non round-trippable price id. Missing pricing is now handled by the existing stop path, with wording specific to that case, and --priceId still recovers it. The fixtures were modelling something the API never returns. Every real app submission comes back carrying a pricing object, so AddDefaultFakeSubmission now defaults to one instead of to null, which also means the existing publish tests exercise a realistic payload and assert the price survives. A withoutPricing switch covers the degenerate case on purpose. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 06440e13-c898-4e58-8764-d7abf304d9d8 --- MSStore.CLI.UnitTests/BaseCommandLineTest.cs | 11 ++++++---- .../PublishCommandPricingUnitTests.cs | 21 +++++++++++++------ .../Helpers/IStorePackagedAPIExtensions.cs | 13 ++++++++---- 3 files changed, 31 insertions(+), 14 deletions(-) diff --git a/MSStore.CLI.UnitTests/BaseCommandLineTest.cs b/MSStore.CLI.UnitTests/BaseCommandLineTest.cs index 1563b48..628261c 100644 --- a/MSStore.CLI.UnitTests/BaseCommandLineTest.cs +++ b/MSStore.CLI.UnitTests/BaseCommandLineTest.cs @@ -410,14 +410,17 @@ internal void AddFakeAccount(AccountEnrollment? accountEnrollment) }); } - protected void AddDefaultFakeSubmission(string listingDescription = "BaseListingDescription", Pricing? pricing = null) + protected void AddDefaultFakeSubmission(string listingDescription = "BaseListingDescription", Pricing? pricing = null, bool withoutPricing = false) { var fakeSubmission = new DevCenterSubmission { Id = "123456789", ApplicationCategory = DevCenterApplicationCategory.NotSet, FileUploadUrl = "https://azureblob.com/fileupload", - Pricing = pricing, + + // Every real app submission comes back carrying a pricing object, so that is what + // the fixtures model. 'withoutPricing' exists only to cover the degenerate case. + Pricing = withoutPricing ? null : pricing ?? new Pricing { PriceId = PriceIds.Free }, ApplicationPackages = [ new ApplicationPackage @@ -574,9 +577,9 @@ internal void InitDefaultFlightSubmissionStatusResponseQueue() }); } - protected void AddDefaultFakeSuccessfulSubmission(Pricing? pricing = null) + protected void AddDefaultFakeSuccessfulSubmission(Pricing? pricing = null, bool withoutPricing = false) { - AddDefaultFakeSubmission(pricing: pricing); + AddDefaultFakeSubmission(pricing: pricing, withoutPricing: withoutPricing); InitDefaultSubmissionStatusResponseQueue(); FakeStorePackagedAPI diff --git a/MSStore.CLI.UnitTests/PublishCommandPricingUnitTests.cs b/MSStore.CLI.UnitTests/PublishCommandPricingUnitTests.cs index 791e49a..6e49062 100644 --- a/MSStore.CLI.UnitTests/PublishCommandPricingUnitTests.cs +++ b/MSStore.CLI.UnitTests/PublishCommandPricingUnitTests.cs @@ -37,12 +37,13 @@ public void Init() private async Task<((string Output, string Error) Result, DevCenterSubmission? Sent)> PublishMsixAsync( Pricing? pricing, int? expectedExitCode = 0, + bool withoutPricing = false, params string[] extraArgs) { var path = CopyFilesRecursively("MSIXProject"); var msixPath = Path.Combine(path, "test.msix"); - AddDefaultFakeSuccessfulSubmission(pricing); + AddDefaultFakeSuccessfulSubmission(pricing, withoutPricing); DevCenterSubmission? sent = null; FakeStorePackagedAPI @@ -135,6 +136,7 @@ public async Task PublishWithPriceIdShouldRecoverAProductWhoseBasePriceIsNotRoun var (result, sent) = await PublishMsixAsync( new Pricing { PriceId = "Base" }, 0, + false, "--priceId", "Tier1012"); @@ -148,6 +150,7 @@ public async Task PublishWithPriceIdShouldOverrideAnExistingTier() var (_, sent) = await PublishMsixAsync( new Pricing { PriceId = "Tier1012" }, 0, + false, "--priceId", "Tier1424"); @@ -155,12 +158,18 @@ public async Task PublishWithPriceIdShouldOverrideAnExistingTier() } [TestMethod] - public async Task PublishShouldSucceedWhenTheProductHasNoPricingAtAll() + public async Task PublishShouldStopWhenTheProductHasNoPricingAtAll() { - var (result, sent) = await PublishMsixAsync(null); + // Missing pricing is just as unsendable as a bad price id: the API answers + // "Pricing data was not provided in the request.". Fail fast with guidance instead + // of letting UpdateSubmissionAsync surface a raw 400. + var (result, sent) = await PublishMsixAsync(null, -1, withoutPricing: true); - result.Error.Should().Contain("Submission commit success! Here is some data:"); - sent!.Pricing.Should().BeNull(); + result.Error.Should().Contain("Could not preserve this product's price"); + result.Error.Should().Contain("returned no pricing for this product"); + result.Error.Should().Contain("--priceId"); + + sent.Should().BeNull(); } [TestMethod] @@ -168,7 +177,7 @@ public async Task PublishWithPriceIdShouldApplyEvenWhenTheProductHasNoPricingAtA { // The API rejects an update that omits pricing, so an explicit price has to be // materialized rather than silently dropped. - var (result, sent) = await PublishMsixAsync(null, 0, "--priceId", "Tier1012"); + var (result, sent) = await PublishMsixAsync(null, 0, true, "--priceId", "Tier1012"); result.Error.Should().Contain("Submission commit success! Here is some data:"); sent!.Pricing.Should().NotBeNull(); diff --git a/MSStore.CLI/Helpers/IStorePackagedAPIExtensions.cs b/MSStore.CLI/Helpers/IStorePackagedAPIExtensions.cs index d47a7f4..39af682 100644 --- a/MSStore.CLI/Helpers/IStorePackagedAPIExtensions.cs +++ b/MSStore.CLI/Helpers/IStorePackagedAPIExtensions.cs @@ -824,16 +824,21 @@ internal static bool TryPreservePricing(IAnsiConsole ansiConsole, DevCenterSubmi return true; } - if (submission.Pricing == null || PriceIds.IsRoundTrippable(submission.Pricing.PriceId)) + if (submission.Pricing != null && PriceIds.IsRoundTrippable(submission.Pricing.PriceId)) { return true; } - var priceId = submission.Pricing.PriceId; - logger.LogError("Cannot preserve the product's price. The API returned PriceId '{PriceId}', which it does not accept back on update.", priceId); + // A missing pricing object is just as unsendable as a bad price id - the API answers + // "Pricing data was not provided in the request." - so stop here with usable guidance + // rather than letting the update fail with a raw 400 further down. + var priceId = submission.Pricing?.PriceId; + logger.LogError("Cannot preserve the product's price. The submission has PriceId '{PriceId}', which the API does not accept on update.", priceId); ansiConsole.MarkupLine("[red bold]Could not preserve this product's price.[/]"); - ansiConsole.MarkupLine($"The Store returned a base price of [yellow]'{(priceId ?? "").EscapeMarkup()}'[/], which the submission API refuses on update. This happens when the price is managed per market from Partner Center."); + ansiConsole.MarkupLine(submission.Pricing == null + ? "The Store returned no pricing for this product, and the submission API rejects an update that does not carry one." + : $"The Store returned a base price of [yellow]'{(priceId ?? "").EscapeMarkup()}'[/], which the submission API refuses on update. This happens when the price is managed per market from Partner Center."); ansiConsole.MarkupLine("Publishing would reset the product to [bold]Free[/], so it has been stopped instead."); ansiConsole.MarkupLine("Re-run with [green]--priceId[/] to state the base price explicitly (for example [green]--priceId Tier1012[/]), or publish this submission from Partner Center."); From 5eea4a5e37c99201fc1c818b5b7dd1db2759a941 Mon Sep 17 00:00:00 2001 From: azchohfi Date: Wed, 2 Sep 2026 20:26:40 -0700 Subject: [PATCH 04/11] Word the submission update rejection as payload validation Addresses the remaining Copilot review nit on #175. The check validates the pricing in the JSON the caller supplied, not the state of the product in the Store, so "The provided product has a base price of ..." was misleading while someone is editing that JSON locally. It now names the field being rejected, and distinguishes an unusable value from an absent one. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 06440e13-c898-4e58-8764-d7abf304d9d8 --- MSStore.CLI.UnitTests/SubmissionCommandPackagedUnitTests.cs | 2 ++ MSStore.CLI/Commands/Submission/UpdateCommand.cs | 4 +++- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/MSStore.CLI.UnitTests/SubmissionCommandPackagedUnitTests.cs b/MSStore.CLI.UnitTests/SubmissionCommandPackagedUnitTests.cs index bf92d2d..86ea203 100644 --- a/MSStore.CLI.UnitTests/SubmissionCommandPackagedUnitTests.cs +++ b/MSStore.CLI.UnitTests/SubmissionCommandPackagedUnitTests.cs @@ -200,6 +200,7 @@ public async Task PackagedSubmissionUpdateCommandShouldRejectAPayloadThatWouldRe @"{ ""Pricing"": { ""PriceId"": ""Base"" } }" ], -1); + result.Error.Should().Contain("sets 'Pricing.PriceId' to 'Base'"); result.Error.Should().Contain("which the submission API will not accept"); result.Error.Should().NotContain("only for Free products"); @@ -226,6 +227,7 @@ public async Task PackagedSubmissionUpdateCommandShouldRejectAPayloadWithAnEmpty @"{ ""Pricing"": { ""TrialPeriod"": ""NoFreeTrial"" } }" ], -1); + result.Error.Should().Contain("does not set 'Pricing.PriceId'"); result.Error.Should().Contain("which the submission API will not accept"); FakeStorePackagedAPI diff --git a/MSStore.CLI/Commands/Submission/UpdateCommand.cs b/MSStore.CLI/Commands/Submission/UpdateCommand.cs index 0ffe294..24d6917 100644 --- a/MSStore.CLI/Commands/Submission/UpdateCommand.cs +++ b/MSStore.CLI/Commands/Submission/UpdateCommand.cs @@ -124,7 +124,9 @@ public class Handler(ILogger logger, IStoreAPIFactory sto await storePackagedAPI.DeleteSubmissionAsync(application.Id, currentSubmission.Id, ct); } - ansiConsole.MarkupLine($"[red bold]The provided product has a base price of '{(updatedPriceId ?? "").EscapeMarkup()}', which the submission API will not accept.[/]"); + ansiConsole.MarkupLine(string.IsNullOrWhiteSpace(updatedPriceId) + ? "[red bold]The JSON you provided does not set 'Pricing.PriceId', which the submission API will not accept.[/]" + : $"[red bold]The JSON you provided sets 'Pricing.PriceId' to '{updatedPriceId.EscapeMarkup()}', which the submission API will not accept.[/]"); ansiConsole.MarkupLine("Sending it would reset the product to [bold]Free[/]. Set 'Pricing.PriceId' to a real tier (for example 'Tier1012'), 'Free', or 'NotAvailable' and try again."); return null; } From 0511a85bb1d672436a13f1e274356158c8a76e6a Mon Sep 17 00:00:00 2001 From: azchohfi Date: Wed, 2 Sep 2026 20:42:17 -0700 Subject: [PATCH 05/11] Drop a redundant fetch and describe the pricing failures accurately Addresses Copilot review feedback on #175. PackagedUpdateCommandAsync fetched the submission on every run but only used the result to delete a draft in the rejection branch. The submission id is already known at that point, so the fetch is gone and a flag records whether the draft was created here, which also makes it explicit that a draft the caller already had is never deleted. The user-facing text claimed the API "will not accept" a missing PriceId and that sending it "would reset the product to Free". Both were wrong, in opposite directions, and the messages conflated three distinct behaviours: * no pricing object - rejected, "Pricing data was not provided in the request." * PriceId "Base" - rejected, "'Base' is not a valid PriceId for base price." * empty PriceId - accepted, and the product silently becomes free Publish and submission update now name the case that actually applies. The same inaccuracy was present in TryPreservePricing, so it is corrected there too. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 06440e13-c898-4e58-8764-d7abf304d9d8 --- .../SubmissionCommandPackagedUnitTests.cs | 4 +-- .../Commands/Submission/UpdateCommand.cs | 23 ++++++++++------ .../Helpers/IStorePackagedAPIExtensions.cs | 27 +++++++++++++------ 3 files changed, 36 insertions(+), 18 deletions(-) diff --git a/MSStore.CLI.UnitTests/SubmissionCommandPackagedUnitTests.cs b/MSStore.CLI.UnitTests/SubmissionCommandPackagedUnitTests.cs index 86ea203..0d3267f 100644 --- a/MSStore.CLI.UnitTests/SubmissionCommandPackagedUnitTests.cs +++ b/MSStore.CLI.UnitTests/SubmissionCommandPackagedUnitTests.cs @@ -201,7 +201,7 @@ public async Task PackagedSubmissionUpdateCommandShouldRejectAPayloadThatWouldRe ], -1); result.Error.Should().Contain("sets 'Pricing.PriceId' to 'Base'"); - result.Error.Should().Contain("which the submission API will not accept"); + result.Error.Should().Contain("which the submission API rejects"); result.Error.Should().NotContain("only for Free products"); FakeStorePackagedAPI @@ -228,7 +228,7 @@ public async Task PackagedSubmissionUpdateCommandShouldRejectAPayloadWithAnEmpty ], -1); result.Error.Should().Contain("does not set 'Pricing.PriceId'"); - result.Error.Should().Contain("which the submission API will not accept"); + result.Error.Should().Contain("silently reset the product to Free"); FakeStorePackagedAPI .Verify( diff --git a/MSStore.CLI/Commands/Submission/UpdateCommand.cs b/MSStore.CLI/Commands/Submission/UpdateCommand.cs index 24d6917..a3c6b9c 100644 --- a/MSStore.CLI/Commands/Submission/UpdateCommand.cs +++ b/MSStore.CLI/Commands/Submission/UpdateCommand.cs @@ -97,6 +97,7 @@ public class Handler(ILogger logger, IStoreAPIFactory sto } string? submissionId = application.PendingApplicationSubmission?.Id; + var draftWasCreatedHere = submissionId == null; if (submissionId == null) { @@ -111,23 +112,29 @@ public class Handler(ILogger logger, IStoreAPIFactory sto } } - var currentSubmission = await storePackagedAPI.GetSubmissionAsync(application.Id, submissionId, ct); - // Only the payload actually being sent matters here. Blocking on the *current* // submission would reject a perfectly good update whose JSON already carries a // valid tier, which is the one way a per-market priced product can be updated. var updatedPriceId = updateSubmission.Pricing?.PriceId; if (updateSubmission.Pricing != null && !PriceIds.IsRoundTrippable(updatedPriceId)) { - if (currentSubmission?.Id != null && application.PendingApplicationSubmission?.Id == null) + // Clean up after ourselves, but never delete a draft the caller already had. + if (draftWasCreatedHere) + { + await storePackagedAPI.DeleteSubmissionAsync(application.Id, submissionId, ct); + } + + if (string.IsNullOrWhiteSpace(updatedPriceId)) + { + ansiConsole.MarkupLine("[red bold]The JSON you provided does not set 'Pricing.PriceId'.[/]"); + ansiConsole.MarkupLine("The submission API would accept that and silently reset the product to [bold]Free[/], so the update has been stopped instead."); + } + else { - await storePackagedAPI.DeleteSubmissionAsync(application.Id, currentSubmission.Id, ct); + ansiConsole.MarkupLine($"[red bold]The JSON you provided sets 'Pricing.PriceId' to '{updatedPriceId.EscapeMarkup()}', which the submission API rejects.[/]"); } - ansiConsole.MarkupLine(string.IsNullOrWhiteSpace(updatedPriceId) - ? "[red bold]The JSON you provided does not set 'Pricing.PriceId', which the submission API will not accept.[/]" - : $"[red bold]The JSON you provided sets 'Pricing.PriceId' to '{updatedPriceId.EscapeMarkup()}', which the submission API will not accept.[/]"); - ansiConsole.MarkupLine("Sending it would reset the product to [bold]Free[/]. Set 'Pricing.PriceId' to a real tier (for example 'Tier1012'), 'Free', or 'NotAvailable' and try again."); + ansiConsole.MarkupLine("Set 'Pricing.PriceId' to a real tier (for example 'Tier1012'), 'Free', or 'NotAvailable' and try again."); return null; } diff --git a/MSStore.CLI/Helpers/IStorePackagedAPIExtensions.cs b/MSStore.CLI/Helpers/IStorePackagedAPIExtensions.cs index 39af682..f87edd3 100644 --- a/MSStore.CLI/Helpers/IStorePackagedAPIExtensions.cs +++ b/MSStore.CLI/Helpers/IStorePackagedAPIExtensions.cs @@ -829,17 +829,28 @@ internal static bool TryPreservePricing(IAnsiConsole ansiConsole, DevCenterSubmi return true; } - // A missing pricing object is just as unsendable as a bad price id - the API answers - // "Pricing data was not provided in the request." - so stop here with usable guidance - // rather than letting the update fail with a raw 400 further down. + // Neither a missing pricing object nor an unusable price id can be sent, but they fail + // differently: the first two are rejected outright, an empty price id is accepted and + // silently turns the product free. Stop for all three, and say which one it is. var priceId = submission.Pricing?.PriceId; - logger.LogError("Cannot preserve the product's price. The submission has PriceId '{PriceId}', which the API does not accept on update.", priceId); + logger.LogError("Cannot preserve the product's price. The submission has PriceId '{PriceId}', which cannot be sent back.", priceId); ansiConsole.MarkupLine("[red bold]Could not preserve this product's price.[/]"); - ansiConsole.MarkupLine(submission.Pricing == null - ? "The Store returned no pricing for this product, and the submission API rejects an update that does not carry one." - : $"The Store returned a base price of [yellow]'{(priceId ?? "").EscapeMarkup()}'[/], which the submission API refuses on update. This happens when the price is managed per market from Partner Center."); - ansiConsole.MarkupLine("Publishing would reset the product to [bold]Free[/], so it has been stopped instead."); + + if (submission.Pricing == null) + { + ansiConsole.MarkupLine("The Store returned no pricing for this product, and the submission API rejects an update that does not carry one."); + } + else if (string.IsNullOrWhiteSpace(priceId)) + { + ansiConsole.MarkupLine("The Store returned no base price for this product. The submission API would accept that and silently reset the product to [bold]Free[/]."); + } + else + { + ansiConsole.MarkupLine($"The Store returned a base price of [yellow]'{priceId.EscapeMarkup()}'[/], which the submission API rejects on update. This happens when the price is managed per market from Partner Center."); + } + + ansiConsole.MarkupLine("Publishing has been stopped so the price is left as it is."); ansiConsole.MarkupLine("Re-run with [green]--priceId[/] to state the base price explicitly (for example [green]--priceId Tier1012[/]), or publish this submission from Partner Center."); return false; From 5a782be39aa24305443a817b8b5ed0202bc8cbfa Mon Sep 17 00:00:00 2001 From: azchohfi Date: Wed, 2 Sep 2026 21:03:21 -0700 Subject: [PATCH 06/11] Reject an update payload that carries no pricing at all Addresses Copilot review feedback on #175. The pricing guard only ran when the JSON contained a Pricing object, so a payload omitting it entirely slipped through, got sent, and was rejected with "Pricing data was not provided in the request." - after this command had already created a draft that nothing then cleaned up. An update replaces the whole submission, so pricing is required on every one of them and a payload without it can never succeed. It is now refused up front, alongside the other two unsendable cases, and the draft is deleted when this command was the one that created it. The existing update fixtures were sending payloads the API always rejects, so they now carry a price like a real caller would. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 06440e13-c898-4e58-8764-d7abf304d9d8 --- .../SubmissionCommandPackagedUnitTests.cs | 65 +++++++++++++++++++ .../Commands/Submission/UpdateCommand.cs | 9 ++- 2 files changed, 72 insertions(+), 2 deletions(-) diff --git a/MSStore.CLI.UnitTests/SubmissionCommandPackagedUnitTests.cs b/MSStore.CLI.UnitTests/SubmissionCommandPackagedUnitTests.cs index 0d3267f..b5e6e90 100644 --- a/MSStore.CLI.UnitTests/SubmissionCommandPackagedUnitTests.cs +++ b/MSStore.CLI.UnitTests/SubmissionCommandPackagedUnitTests.cs @@ -131,6 +131,7 @@ public async Task PackagedSubmissionUpdateCommand() FakeApps[0].Id!, @" { +""Pricing"": { ""PriceId"": ""Free"" }, ""ApplicationPackages"": [ { @@ -240,12 +241,69 @@ public async Task PackagedSubmissionUpdateCommandShouldRejectAPayloadWithAnEmpty Times.Never); } + [TestMethod] + public async Task PackagedSubmissionUpdateCommandShouldRejectAPayloadWithNoPricingObject() + { + // An update replaces the whole submission, so a payload without pricing is rejected + // outright ("Pricing data was not provided in the request."). Stopping here also + // avoids stranding the draft this command just created. + var result = await ParseAndInvokeAsync( + [ + "submission", + "update", + FakeApps[0].Id!, + @"{ ""ApplicationPackages"": [ { ""FileName"": ""test.msix"" } ] }" + ], -1); + + result.Error.Should().Contain("has no 'Pricing' object"); + + FakeStorePackagedAPI + .Verify( + x => x.UpdateSubmissionAsync( + It.IsAny(), + It.IsAny(), + It.IsAny(), + It.IsAny()), + Times.Never); + + // The draft was created by this command, so it must not be left behind. + FakeStorePackagedAPI + .Verify( + x => x.DeleteSubmissionAsync(It.IsAny(), It.IsAny(), It.IsAny()), + Times.Once); + } + + [TestMethod] + public async Task PackagedSubmissionUpdateCommandShouldNotDeleteADraftItDidNotCreate() + { + FakeApps[0].PendingApplicationSubmission = new ApplicationSubmissionInfo + { + Id = "123456789" + }; + + var result = await ParseAndInvokeAsync( + [ + "submission", + "update", + FakeApps[0].Id!, + @"{ ""Pricing"": { ""PriceId"": ""Base"" } }" + ], -1); + + result.Error.Should().Contain("which the submission API rejects"); + + FakeStorePackagedAPI + .Verify( + x => x.DeleteSubmissionAsync(It.IsAny(), It.IsAny(), It.IsAny()), + Times.Never); + } + [TestMethod] public async Task PackagedSubmissionUpdateCommandWithPayloadOption() { var payloadFilePath = CreateTemporaryPayloadFile( @" { +""Pricing"": { ""PriceId"": ""Free"" }, ""ApplicationPackages"": [ { @@ -273,6 +331,7 @@ public async Task PackagedSubmissionUpdateCommandWithFilePathArgument() var payloadFilePath = CreateTemporaryPayloadFile( @" { +""Pricing"": { ""PriceId"": ""Free"" }, ""ApplicationPackages"": [ { @@ -301,6 +360,7 @@ public async Task PackagedSubmissionUpdateCommandWithStandardInputArgument() .ReturnsAsync( @" { +""Pricing"": { ""PriceId"": ""Free"" }, ""ApplicationPackages"": [ { @@ -332,6 +392,7 @@ public async Task PackagedSubmissionUpdateCommandWithRedirectedStandardInput() .ReturnsAsync( @" { +""Pricing"": { ""PriceId"": ""Free"" }, ""ApplicationPackages"": [ { @@ -384,6 +445,7 @@ public async Task PackagedSubmissionUpdateCommandWithBothInlineJsonAndPayloadOpt var payloadFilePath = CreateTemporaryPayloadFile( @" { +""Pricing"": { ""PriceId"": ""Free"" }, ""ApplicationPackages"": [ { @@ -415,6 +477,7 @@ public async Task PackagedSubmissionUpdateCommandWithPayloadBiggerThanTheCommand var payloadFilePath = CreateTemporaryPayloadFile( @" { +""Pricing"": { ""PriceId"": ""Free"" }, ""Listings"": { ""en-us"": @@ -458,6 +521,7 @@ public async Task PackagedSubmissionUpdateMetadataCommand() FakeApps[0].Id!, @" { +""Pricing"": { ""PriceId"": ""Free"" }, ""Listings"": { ""en-us"": @@ -481,6 +545,7 @@ public async Task PackagedSubmissionUpdateMetadataCommandWithPayloadOption() var payloadFilePath = CreateTemporaryPayloadFile( @" { +""Pricing"": { ""PriceId"": ""Free"" }, ""Listings"": { ""en-us"": diff --git a/MSStore.CLI/Commands/Submission/UpdateCommand.cs b/MSStore.CLI/Commands/Submission/UpdateCommand.cs index a3c6b9c..f0e0de7 100644 --- a/MSStore.CLI/Commands/Submission/UpdateCommand.cs +++ b/MSStore.CLI/Commands/Submission/UpdateCommand.cs @@ -116,7 +116,7 @@ public class Handler(ILogger logger, IStoreAPIFactory sto // submission would reject a perfectly good update whose JSON already carries a // valid tier, which is the one way a per-market priced product can be updated. var updatedPriceId = updateSubmission.Pricing?.PriceId; - if (updateSubmission.Pricing != null && !PriceIds.IsRoundTrippable(updatedPriceId)) + if (updateSubmission.Pricing == null || !PriceIds.IsRoundTrippable(updatedPriceId)) { // Clean up after ourselves, but never delete a draft the caller already had. if (draftWasCreatedHere) @@ -124,7 +124,12 @@ public class Handler(ILogger logger, IStoreAPIFactory sto await storePackagedAPI.DeleteSubmissionAsync(application.Id, submissionId, ct); } - if (string.IsNullOrWhiteSpace(updatedPriceId)) + if (updateSubmission.Pricing == null) + { + ansiConsole.MarkupLine("[red bold]The JSON you provided has no 'Pricing' object.[/]"); + ansiConsole.MarkupLine("An update replaces the whole submission, and the submission API rejects one that does not carry pricing."); + } + else if (string.IsNullOrWhiteSpace(updatedPriceId)) { ansiConsole.MarkupLine("[red bold]The JSON you provided does not set 'Pricing.PriceId'.[/]"); ansiConsole.MarkupLine("The submission API would accept that and silently reset the product to [bold]Free[/], so the update has been stopped instead."); From c92d60f79fcf06473f6870957eeeaec3ccaa523f Mon Sep 17 00:00:00 2001 From: azchohfi Date: Wed, 2 Sep 2026 21:10:44 -0700 Subject: [PATCH 07/11] Log the actual pricing failure rather than a blanket PriceId message Addresses Copilot review feedback on #175. TryPreservePricing logged "The submission has PriceId '{PriceId}'" for all three failure modes, so the missing-pricing case reported a null PriceId as though an invalid one were the problem. Each branch now logs what actually happened, which matches what the console already told the user. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 06440e13-c898-4e58-8764-d7abf304d9d8 --- MSStore.CLI/Helpers/IStorePackagedAPIExtensions.cs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/MSStore.CLI/Helpers/IStorePackagedAPIExtensions.cs b/MSStore.CLI/Helpers/IStorePackagedAPIExtensions.cs index f87edd3..5abf949 100644 --- a/MSStore.CLI/Helpers/IStorePackagedAPIExtensions.cs +++ b/MSStore.CLI/Helpers/IStorePackagedAPIExtensions.cs @@ -833,20 +833,22 @@ internal static bool TryPreservePricing(IAnsiConsole ansiConsole, DevCenterSubmi // differently: the first two are rejected outright, an empty price id is accepted and // silently turns the product free. Stop for all three, and say which one it is. var priceId = submission.Pricing?.PriceId; - logger.LogError("Cannot preserve the product's price. The submission has PriceId '{PriceId}', which cannot be sent back.", priceId); ansiConsole.MarkupLine("[red bold]Could not preserve this product's price.[/]"); if (submission.Pricing == null) { + logger.LogError("Cannot preserve the product's price: the submission carries no pricing, which the API rejects on update."); ansiConsole.MarkupLine("The Store returned no pricing for this product, and the submission API rejects an update that does not carry one."); } else if (string.IsNullOrWhiteSpace(priceId)) { + logger.LogError("Cannot preserve the product's price: the submission has no PriceId, and sending that resets the product to free."); ansiConsole.MarkupLine("The Store returned no base price for this product. The submission API would accept that and silently reset the product to [bold]Free[/]."); } else { + logger.LogError("Cannot preserve the product's price: the submission has PriceId '{PriceId}', which the API rejects on update.", priceId); ansiConsole.MarkupLine($"The Store returned a base price of [yellow]'{priceId.EscapeMarkup()}'[/], which the submission API rejects on update. This happens when the price is managed per market from Partner Center."); } From 8ca87ecc674fdc28636a189ff2cf2d1f66996326 Mon Sep 17 00:00:00 2001 From: azchohfi Date: Wed, 2 Sep 2026 21:17:07 -0700 Subject: [PATCH 08/11] Do not let whitespace smuggle the Base sentinel past the guard Addresses Copilot review feedback on #175. IsRoundTrippable rejected an all-whitespace price id but compared to Base without trimming, so "Base " was reported as safe to send and would have come back as a 400. TryNormalize already treats surrounding whitespace as insignificant, so the check now does too. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 06440e13-c898-4e58-8764-d7abf304d9d8 --- MSStore.API/Packaged/Models/PriceIds.cs | 2 +- .../PublishCommandPricingUnitTests.cs | 27 +++++++++++++++++++ 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/MSStore.API/Packaged/Models/PriceIds.cs b/MSStore.API/Packaged/Models/PriceIds.cs index acd0f1c..2187ce5 100644 --- a/MSStore.API/Packaged/Models/PriceIds.cs +++ b/MSStore.API/Packaged/Models/PriceIds.cs @@ -52,7 +52,7 @@ public static class PriceIds /// true when is safe to send back. public static bool IsRoundTrippable(string? priceId) => !string.IsNullOrWhiteSpace(priceId) && - !string.Equals(priceId, Base, StringComparison.OrdinalIgnoreCase); + !string.Equals(priceId.Trim(), Base, StringComparison.OrdinalIgnoreCase); /// /// Validates a user supplied price id and converts it to the casing the API expects. diff --git a/MSStore.CLI.UnitTests/PublishCommandPricingUnitTests.cs b/MSStore.CLI.UnitTests/PublishCommandPricingUnitTests.cs index 6e49062..6c05d48 100644 --- a/MSStore.CLI.UnitTests/PublishCommandPricingUnitTests.cs +++ b/MSStore.CLI.UnitTests/PublishCommandPricingUnitTests.cs @@ -208,6 +208,33 @@ public void PricingShouldSerializeARealTierVerbatim() json.Should().Contain("\"PriceId\":\"Tier1012\""); } + [TestMethod] + [DataRow("Tier1012")] + [DataRow("Free")] + [DataRow("NotAvailable")] + [DataRow("Tier2")] + public void RoundTrippablePriceIdsAreSentBackUnchanged(string priceId) + { + PriceIds.IsRoundTrippable(priceId).Should().BeTrue(); + } + + [TestMethod] + [DataRow(null)] + [DataRow("")] + [DataRow(" ")] + [DataRow("Base")] + [DataRow("base")] + [DataRow("BASE")] + [DataRow("Base ")] + [DataRow(" Base")] + [DataRow(" base ")] + public void UnsendablePriceIdsAreNotRoundTrippable(string? priceId) + { + // Whitespace must not smuggle the Base sentinel past the guard: TryNormalize already + // treats surrounding whitespace as insignificant, and sending "Base " is still a 400. + PriceIds.IsRoundTrippable(priceId).Should().BeFalse(); + } + private static ParseResult ParsePublish(params string[] args) => new PublishCommand().Parse(args); From f776cf8b7c6ca868319290196cde1bc95896645f Mon Sep 17 00:00:00 2001 From: azchohfi Date: Wed, 2 Sep 2026 21:41:06 -0700 Subject: [PATCH 09/11] Stop asserting on console text that Spectre may wrap The new pricing assertions matched phrases verbatim, so they depended on the console width they happened to run at. They passed locally and failed on all three CI runners, where the guidance wrapped mid-sentence. Assertions on captured console output now go through Unwrapped, which collapses the wrapping back to single spaces. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 06440e13-c898-4e58-8764-d7abf304d9d8 --- MSStore.CLI.UnitTests/BaseCommandLineTest.cs | 10 ++++ .../PublishCommandPricingUnitTests.cs | 20 ++++---- .../SubmissionCommandPackagedUnitTests.cs | 50 +++++++++---------- 3 files changed, 45 insertions(+), 35 deletions(-) diff --git a/MSStore.CLI.UnitTests/BaseCommandLineTest.cs b/MSStore.CLI.UnitTests/BaseCommandLineTest.cs index 628261c..ca411ab 100644 --- a/MSStore.CLI.UnitTests/BaseCommandLineTest.cs +++ b/MSStore.CLI.UnitTests/BaseCommandLineTest.cs @@ -127,6 +127,16 @@ protected static void AssertBasedOnTestDataProjectSubPath(string[] testDataProje } } + /// + /// Collapses the line breaks Spectre.Console introduces when it wraps output to the + /// console width, so an assertion on message text does not depend on how wide the test + /// console happens to be. Local and CI runners wrap at different widths. + /// + /// The captured console output. + /// The same text with every run of whitespace collapsed to a single space. + protected static string Unwrapped(string text) => + System.Text.RegularExpressions.Regex.Replace(text, @"\s+", " "); + private readonly List _temporaryPayloadFiles = []; /// diff --git a/MSStore.CLI.UnitTests/PublishCommandPricingUnitTests.cs b/MSStore.CLI.UnitTests/PublishCommandPricingUnitTests.cs index 6c05d48..a54f177 100644 --- a/MSStore.CLI.UnitTests/PublishCommandPricingUnitTests.cs +++ b/MSStore.CLI.UnitTests/PublishCommandPricingUnitTests.cs @@ -72,7 +72,7 @@ public async Task PublishShouldSendThePriceBackUnchanged(string priceId) // The core regression for issue #112: publishing must never alter the base price. var (result, sent) = await PublishMsixAsync(new Pricing { PriceId = priceId }); - result.Error.Should().Contain("Submission commit success! Here is some data:"); + Unwrapped(result.Error).Should().Contain("Submission commit success! Here is some data:"); sent.Should().NotBeNull(); sent!.Pricing.Should().NotBeNull(); @@ -99,7 +99,7 @@ public async Task PublishShouldIgnoreIsAdvancedPricingModel() var (result, sent) = await PublishMsixAsync( new Pricing { PriceId = "Tier1012", IsAdvancedPricingModel = true }); - result.Error.Should().Contain("Submission commit success! Here is some data:"); + Unwrapped(result.Error).Should().Contain("Submission commit success! Here is some data:"); sent!.Pricing!.PriceId.Should().Be("Tier1012"); } @@ -108,8 +108,8 @@ public async Task PublishShouldStopWhenThePriceCannotBePreserved() { var (result, sent) = await PublishMsixAsync(new Pricing { PriceId = "Base" }, -1); - result.Error.Should().Contain("Could not preserve this product's price"); - result.Error.Should().Contain("--priceId"); + Unwrapped(result.Error).Should().Contain("Could not preserve this product's price"); + Unwrapped(result.Error).Should().Contain("--priceId"); // Nothing may be sent, otherwise the product would be reset to free. sent.Should().BeNull(); @@ -127,7 +127,7 @@ public async Task PublishShouldNotClaimOnlyFreeProductsAreSupported() // wrong: a product with a real tier publishes fine, as the tests above show. var (result, _) = await PublishMsixAsync(new Pricing { PriceId = "Base" }, -1); - result.Error.Should().NotContain("only for Free products"); + Unwrapped(result.Error).Should().NotContain("only for Free products"); } [TestMethod] @@ -140,7 +140,7 @@ public async Task PublishWithPriceIdShouldRecoverAProductWhoseBasePriceIsNotRoun "--priceId", "Tier1012"); - result.Error.Should().Contain("Submission commit success! Here is some data:"); + Unwrapped(result.Error).Should().Contain("Submission commit success! Here is some data:"); sent!.Pricing!.PriceId.Should().Be("Tier1012"); } @@ -165,9 +165,9 @@ public async Task PublishShouldStopWhenTheProductHasNoPricingAtAll() // of letting UpdateSubmissionAsync surface a raw 400. var (result, sent) = await PublishMsixAsync(null, -1, withoutPricing: true); - result.Error.Should().Contain("Could not preserve this product's price"); - result.Error.Should().Contain("returned no pricing for this product"); - result.Error.Should().Contain("--priceId"); + Unwrapped(result.Error).Should().Contain("Could not preserve this product's price"); + Unwrapped(result.Error).Should().Contain("returned no pricing for this product"); + Unwrapped(result.Error).Should().Contain("--priceId"); sent.Should().BeNull(); } @@ -179,7 +179,7 @@ public async Task PublishWithPriceIdShouldApplyEvenWhenTheProductHasNoPricingAtA // materialized rather than silently dropped. var (result, sent) = await PublishMsixAsync(null, 0, true, "--priceId", "Tier1012"); - result.Error.Should().Contain("Submission commit success! Here is some data:"); + Unwrapped(result.Error).Should().Contain("Submission commit success! Here is some data:"); sent!.Pricing.Should().NotBeNull(); sent.Pricing!.PriceId.Should().Be("Tier1012"); } diff --git a/MSStore.CLI.UnitTests/SubmissionCommandPackagedUnitTests.cs b/MSStore.CLI.UnitTests/SubmissionCommandPackagedUnitTests.cs index b5e6e90..4bb25a6 100644 --- a/MSStore.CLI.UnitTests/SubmissionCommandPackagedUnitTests.cs +++ b/MSStore.CLI.UnitTests/SubmissionCommandPackagedUnitTests.cs @@ -46,8 +46,8 @@ public async Task PackagedSubmissionStatusCommand() FakeApps[0].Id! ]); - result.Error.Should().Contain("Code1"); - result.Error.Should().Contain("Detail1"); + Unwrapped(result.Error).Should().Contain("Code1"); + Unwrapped(result.Error).Should().Contain("Detail1"); } [TestMethod] @@ -141,7 +141,7 @@ public async Task PackagedSubmissionUpdateCommand() }" ]); - result.Error.Should().Contain("Updating submission product"); + Unwrapped(result.Error).Should().Contain("Updating submission product"); result.Output.Should().Contain("\"FileUploadUrl\": \"https://azureblob.com/fileupload\""); } @@ -184,7 +184,7 @@ public async Task PackagedSubmissionUpdateCommandShouldAcceptAPayloadCarryingARe }" ]); - result.Error.Should().Contain("Updating submission product"); + Unwrapped(result.Error).Should().Contain("Updating submission product"); sent!.Pricing!.PriceId.Should().Be("Tier1012"); } @@ -201,9 +201,9 @@ public async Task PackagedSubmissionUpdateCommandShouldRejectAPayloadThatWouldRe @"{ ""Pricing"": { ""PriceId"": ""Base"" } }" ], -1); - result.Error.Should().Contain("sets 'Pricing.PriceId' to 'Base'"); - result.Error.Should().Contain("which the submission API rejects"); - result.Error.Should().NotContain("only for Free products"); + Unwrapped(result.Error).Should().Contain("sets 'Pricing.PriceId' to 'Base'"); + Unwrapped(result.Error).Should().Contain("which the submission API rejects"); + Unwrapped(result.Error).Should().NotContain("only for Free products"); FakeStorePackagedAPI .Verify( @@ -228,8 +228,8 @@ public async Task PackagedSubmissionUpdateCommandShouldRejectAPayloadWithAnEmpty @"{ ""Pricing"": { ""TrialPeriod"": ""NoFreeTrial"" } }" ], -1); - result.Error.Should().Contain("does not set 'Pricing.PriceId'"); - result.Error.Should().Contain("silently reset the product to Free"); + Unwrapped(result.Error).Should().Contain("does not set 'Pricing.PriceId'"); + Unwrapped(result.Error).Should().Contain("silently reset the product to Free"); FakeStorePackagedAPI .Verify( @@ -255,7 +255,7 @@ public async Task PackagedSubmissionUpdateCommandShouldRejectAPayloadWithNoPrici @"{ ""ApplicationPackages"": [ { ""FileName"": ""test.msix"" } ] }" ], -1); - result.Error.Should().Contain("has no 'Pricing' object"); + Unwrapped(result.Error).Should().Contain("has no 'Pricing' object"); FakeStorePackagedAPI .Verify( @@ -289,7 +289,7 @@ public async Task PackagedSubmissionUpdateCommandShouldNotDeleteADraftItDidNotCr @"{ ""Pricing"": { ""PriceId"": ""Base"" } }" ], -1); - result.Error.Should().Contain("which the submission API rejects"); + Unwrapped(result.Error).Should().Contain("which the submission API rejects"); FakeStorePackagedAPI .Verify( @@ -321,7 +321,7 @@ public async Task PackagedSubmissionUpdateCommandWithPayloadOption() payloadFilePath ]); - result.Error.Should().Contain("Updating submission product"); + Unwrapped(result.Error).Should().Contain("Updating submission product"); result.Output.Should().Contain("\"FileUploadUrl\": \"https://azureblob.com/fileupload\""); } @@ -348,7 +348,7 @@ public async Task PackagedSubmissionUpdateCommandWithFilePathArgument() payloadFilePath ]); - result.Error.Should().Contain("Updating submission product"); + Unwrapped(result.Error).Should().Contain("Updating submission product"); result.Output.Should().Contain("\"FileUploadUrl\": \"https://azureblob.com/fileupload\""); } @@ -377,7 +377,7 @@ public async Task PackagedSubmissionUpdateCommandWithStandardInputArgument() "-" ]); - result.Error.Should().Contain("Updating submission product"); + Unwrapped(result.Error).Should().Contain("Updating submission product"); result.Output.Should().Contain("\"FileUploadUrl\": \"https://azureblob.com/fileupload\""); } @@ -408,7 +408,7 @@ public async Task PackagedSubmissionUpdateCommandWithRedirectedStandardInput() FakeApps[0].Id! ]); - result.Error.Should().Contain("Updating submission product"); + Unwrapped(result.Error).Should().Contain("Updating submission product"); result.Output.Should().Contain("\"FileUploadUrl\": \"https://azureblob.com/fileupload\""); } @@ -422,7 +422,7 @@ public async Task PackagedSubmissionUpdateCommandWithNoPayload() FakeApps[0].Id! ], 1); - result.Error.Should().Contain("No 'product' was provided."); + Unwrapped(result.Error).Should().Contain("No 'product' was provided."); } [TestMethod] @@ -436,7 +436,7 @@ public async Task PackagedSubmissionUpdateCommandWithUnknownFilePath() "this-file-does-not-exist.json" ], 1); - result.Error.Should().Contain("is neither a JSON payload nor a path to an existing file"); + Unwrapped(result.Error).Should().Contain("is neither a JSON payload nor a path to an existing file"); } [TestMethod] @@ -464,7 +464,7 @@ public async Task PackagedSubmissionUpdateCommandWithBothInlineJsonAndPayloadOpt payloadFilePath ], 1); - result.Error.Should().Contain("Use only one of them."); + Unwrapped(result.Error).Should().Contain("Use only one of them."); } [TestMethod] @@ -507,7 +507,7 @@ public async Task PackagedSubmissionUpdateCommandWithPayloadBiggerThanTheCommand It.IsAny()), Times.Once); - result.Error.Should().Contain("Updating submission product"); + Unwrapped(result.Error).Should().Contain("Updating submission product"); result.Output.Should().Contain("\"FileUploadUrl\": \"https://azureblob.com/fileupload\""); } @@ -535,7 +535,7 @@ public async Task PackagedSubmissionUpdateMetadataCommand() }" ]); - result.Error.Should().Contain("Updating submission product"); + Unwrapped(result.Error).Should().Contain("Updating submission product"); result.Output.Should().Contain("\"FileUploadUrl\": \"https://azureblob.com/fileupload\""); } @@ -567,7 +567,7 @@ public async Task PackagedSubmissionUpdateMetadataCommandWithPayloadOption() payloadFilePath ]); - result.Error.Should().Contain("Updating submission product"); + Unwrapped(result.Error).Should().Contain("Updating submission product"); result.Output.Should().Contain("\"FileUploadUrl\": \"https://azureblob.com/fileupload\""); } @@ -593,7 +593,7 @@ public async Task PackagedSubmissionPublishCommand() FakeApps[0].Id! ]); - result.Error.Should().Contain("Submission Committed with status"); + Unwrapped(result.Error).Should().Contain("Submission Committed with status"); } [TestMethod] @@ -613,7 +613,7 @@ public async Task PackagedSubmissionPollCommand() FakeApps[0].Id! ]); - result.Error.Should().Contain("Submission commit success!"); + Unwrapped(result.Error).Should().Contain("Submission commit success!"); } [TestMethod] @@ -637,8 +637,8 @@ public async Task PackagedSubmissionDeleteCommand() FakeConsole.Verify(x => x.YesNoConfirmationAsync(It.IsAny(), It.IsAny()), Times.Once); - result.Error.Should().Contain($"Found Pending Submission with Id '{FakeApps[0].PendingApplicationSubmission!.Id}'"); - result.Error.Should().Contain("Existing submission deleted!"); + Unwrapped(result.Error).Should().Contain($"Found Pending Submission with Id '{FakeApps[0].PendingApplicationSubmission!.Id}'"); + Unwrapped(result.Error).Should().Contain("Existing submission deleted!"); } } } \ No newline at end of file From 93f842933c1cc0b84c9f29f88b80733063704c62 Mon Sep 17 00:00:00 2001 From: azchohfi Date: Wed, 2 Sep 2026 21:50:05 -0700 Subject: [PATCH 10/11] Strip ANSI styling before asserting on console output The previous attempt only collapsed wrapping, but the real problem was styling: Spectre emits colour escapes when the terminal supports them, so CI captured "reset the product to \e[1mFree\e[0m" and a verbatim assertion could not match. Local runs are not colourized, which is why this only ever failed on CI. PlainConsoleText now removes escape sequences as well as wrapping, and a test feeds it the exact output CI captured so the behaviour is pinned down without depending on the terminal the suite happens to run under. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 06440e13-c898-4e58-8764-d7abf304d9d8 --- MSStore.CLI.UnitTests/BaseCommandLineTest.cs | 17 ++++--- .../PublishCommandPricingUnitTests.cs | 37 ++++++++++---- .../SubmissionCommandPackagedUnitTests.cs | 50 +++++++++---------- 3 files changed, 63 insertions(+), 41 deletions(-) diff --git a/MSStore.CLI.UnitTests/BaseCommandLineTest.cs b/MSStore.CLI.UnitTests/BaseCommandLineTest.cs index ca411ab..8049269 100644 --- a/MSStore.CLI.UnitTests/BaseCommandLineTest.cs +++ b/MSStore.CLI.UnitTests/BaseCommandLineTest.cs @@ -128,14 +128,19 @@ protected static void AssertBasedOnTestDataProjectSubPath(string[] testDataProje } /// - /// Collapses the line breaks Spectre.Console introduces when it wraps output to the - /// console width, so an assertion on message text does not depend on how wide the test - /// console happens to be. Local and CI runners wrap at different widths. + /// Reduces captured console output to plain text: strips the ANSI escape sequences + /// Spectre.Console emits for styling, and collapses the line breaks it inserts when + /// wrapping to the console width. Without this, an assertion on message text depends on + /// both the width and the colour support of whatever terminal the test ran under, which + /// differs between local runs and CI. /// /// The captured console output. - /// The same text with every run of whitespace collapsed to a single space. - protected static string Unwrapped(string text) => - System.Text.RegularExpressions.Regex.Replace(text, @"\s+", " "); + /// The text without styling, with every run of whitespace collapsed to one space. + protected static string PlainConsoleText(string text) + { + var withoutAnsi = System.Text.RegularExpressions.Regex.Replace(text, @"\x1B\[[0-9;]*[a-zA-Z]", string.Empty); + return System.Text.RegularExpressions.Regex.Replace(withoutAnsi, @"\s+", " "); + } private readonly List _temporaryPayloadFiles = []; diff --git a/MSStore.CLI.UnitTests/PublishCommandPricingUnitTests.cs b/MSStore.CLI.UnitTests/PublishCommandPricingUnitTests.cs index a54f177..1ecd3b8 100644 --- a/MSStore.CLI.UnitTests/PublishCommandPricingUnitTests.cs +++ b/MSStore.CLI.UnitTests/PublishCommandPricingUnitTests.cs @@ -72,7 +72,7 @@ public async Task PublishShouldSendThePriceBackUnchanged(string priceId) // The core regression for issue #112: publishing must never alter the base price. var (result, sent) = await PublishMsixAsync(new Pricing { PriceId = priceId }); - Unwrapped(result.Error).Should().Contain("Submission commit success! Here is some data:"); + PlainConsoleText(result.Error).Should().Contain("Submission commit success! Here is some data:"); sent.Should().NotBeNull(); sent!.Pricing.Should().NotBeNull(); @@ -99,7 +99,7 @@ public async Task PublishShouldIgnoreIsAdvancedPricingModel() var (result, sent) = await PublishMsixAsync( new Pricing { PriceId = "Tier1012", IsAdvancedPricingModel = true }); - Unwrapped(result.Error).Should().Contain("Submission commit success! Here is some data:"); + PlainConsoleText(result.Error).Should().Contain("Submission commit success! Here is some data:"); sent!.Pricing!.PriceId.Should().Be("Tier1012"); } @@ -108,8 +108,8 @@ public async Task PublishShouldStopWhenThePriceCannotBePreserved() { var (result, sent) = await PublishMsixAsync(new Pricing { PriceId = "Base" }, -1); - Unwrapped(result.Error).Should().Contain("Could not preserve this product's price"); - Unwrapped(result.Error).Should().Contain("--priceId"); + PlainConsoleText(result.Error).Should().Contain("Could not preserve this product's price"); + PlainConsoleText(result.Error).Should().Contain("--priceId"); // Nothing may be sent, otherwise the product would be reset to free. sent.Should().BeNull(); @@ -127,7 +127,7 @@ public async Task PublishShouldNotClaimOnlyFreeProductsAreSupported() // wrong: a product with a real tier publishes fine, as the tests above show. var (result, _) = await PublishMsixAsync(new Pricing { PriceId = "Base" }, -1); - Unwrapped(result.Error).Should().NotContain("only for Free products"); + PlainConsoleText(result.Error).Should().NotContain("only for Free products"); } [TestMethod] @@ -140,7 +140,7 @@ public async Task PublishWithPriceIdShouldRecoverAProductWhoseBasePriceIsNotRoun "--priceId", "Tier1012"); - Unwrapped(result.Error).Should().Contain("Submission commit success! Here is some data:"); + PlainConsoleText(result.Error).Should().Contain("Submission commit success! Here is some data:"); sent!.Pricing!.PriceId.Should().Be("Tier1012"); } @@ -165,9 +165,9 @@ public async Task PublishShouldStopWhenTheProductHasNoPricingAtAll() // of letting UpdateSubmissionAsync surface a raw 400. var (result, sent) = await PublishMsixAsync(null, -1, withoutPricing: true); - Unwrapped(result.Error).Should().Contain("Could not preserve this product's price"); - Unwrapped(result.Error).Should().Contain("returned no pricing for this product"); - Unwrapped(result.Error).Should().Contain("--priceId"); + PlainConsoleText(result.Error).Should().Contain("Could not preserve this product's price"); + PlainConsoleText(result.Error).Should().Contain("returned no pricing for this product"); + PlainConsoleText(result.Error).Should().Contain("--priceId"); sent.Should().BeNull(); } @@ -179,7 +179,7 @@ public async Task PublishWithPriceIdShouldApplyEvenWhenTheProductHasNoPricingAtA // materialized rather than silently dropped. var (result, sent) = await PublishMsixAsync(null, 0, true, "--priceId", "Tier1012"); - Unwrapped(result.Error).Should().Contain("Submission commit success! Here is some data:"); + PlainConsoleText(result.Error).Should().Contain("Submission commit success! Here is some data:"); sent!.Pricing.Should().NotBeNull(); sent.Pricing!.PriceId.Should().Be("Tier1012"); } @@ -235,6 +235,23 @@ public void UnsendablePriceIdsAreNotRoundTrippable(string? priceId) PriceIds.IsRoundTrippable(priceId).Should().BeFalse(); } + [TestMethod] + public void PlainConsoleTextShouldStripStylingAndWrapping() + { + // Reproduces what CI actually captured. Spectre emitted bold escapes around "Free" + // and wrapped the sentence, so a verbatim assertion passed locally (no colour) and + // failed on every CI runner. Asserting through PlainConsoleText removes both. + var captured = + "The submission API would accept that and silently reset the product to \u001b[1mFree\u001b[0m,\n" + + "so the update has been\nstopped instead."; + + PlainConsoleText(captured) + .Should() + .Contain("silently reset the product to Free") + .And + .Contain("so the update has been stopped instead."); + } + private static ParseResult ParsePublish(params string[] args) => new PublishCommand().Parse(args); diff --git a/MSStore.CLI.UnitTests/SubmissionCommandPackagedUnitTests.cs b/MSStore.CLI.UnitTests/SubmissionCommandPackagedUnitTests.cs index 4bb25a6..09bee59 100644 --- a/MSStore.CLI.UnitTests/SubmissionCommandPackagedUnitTests.cs +++ b/MSStore.CLI.UnitTests/SubmissionCommandPackagedUnitTests.cs @@ -46,8 +46,8 @@ public async Task PackagedSubmissionStatusCommand() FakeApps[0].Id! ]); - Unwrapped(result.Error).Should().Contain("Code1"); - Unwrapped(result.Error).Should().Contain("Detail1"); + PlainConsoleText(result.Error).Should().Contain("Code1"); + PlainConsoleText(result.Error).Should().Contain("Detail1"); } [TestMethod] @@ -141,7 +141,7 @@ public async Task PackagedSubmissionUpdateCommand() }" ]); - Unwrapped(result.Error).Should().Contain("Updating submission product"); + PlainConsoleText(result.Error).Should().Contain("Updating submission product"); result.Output.Should().Contain("\"FileUploadUrl\": \"https://azureblob.com/fileupload\""); } @@ -184,7 +184,7 @@ public async Task PackagedSubmissionUpdateCommandShouldAcceptAPayloadCarryingARe }" ]); - Unwrapped(result.Error).Should().Contain("Updating submission product"); + PlainConsoleText(result.Error).Should().Contain("Updating submission product"); sent!.Pricing!.PriceId.Should().Be("Tier1012"); } @@ -201,9 +201,9 @@ public async Task PackagedSubmissionUpdateCommandShouldRejectAPayloadThatWouldRe @"{ ""Pricing"": { ""PriceId"": ""Base"" } }" ], -1); - Unwrapped(result.Error).Should().Contain("sets 'Pricing.PriceId' to 'Base'"); - Unwrapped(result.Error).Should().Contain("which the submission API rejects"); - Unwrapped(result.Error).Should().NotContain("only for Free products"); + PlainConsoleText(result.Error).Should().Contain("sets 'Pricing.PriceId' to 'Base'"); + PlainConsoleText(result.Error).Should().Contain("which the submission API rejects"); + PlainConsoleText(result.Error).Should().NotContain("only for Free products"); FakeStorePackagedAPI .Verify( @@ -228,8 +228,8 @@ public async Task PackagedSubmissionUpdateCommandShouldRejectAPayloadWithAnEmpty @"{ ""Pricing"": { ""TrialPeriod"": ""NoFreeTrial"" } }" ], -1); - Unwrapped(result.Error).Should().Contain("does not set 'Pricing.PriceId'"); - Unwrapped(result.Error).Should().Contain("silently reset the product to Free"); + PlainConsoleText(result.Error).Should().Contain("does not set 'Pricing.PriceId'"); + PlainConsoleText(result.Error).Should().Contain("silently reset the product to Free"); FakeStorePackagedAPI .Verify( @@ -255,7 +255,7 @@ public async Task PackagedSubmissionUpdateCommandShouldRejectAPayloadWithNoPrici @"{ ""ApplicationPackages"": [ { ""FileName"": ""test.msix"" } ] }" ], -1); - Unwrapped(result.Error).Should().Contain("has no 'Pricing' object"); + PlainConsoleText(result.Error).Should().Contain("has no 'Pricing' object"); FakeStorePackagedAPI .Verify( @@ -289,7 +289,7 @@ public async Task PackagedSubmissionUpdateCommandShouldNotDeleteADraftItDidNotCr @"{ ""Pricing"": { ""PriceId"": ""Base"" } }" ], -1); - Unwrapped(result.Error).Should().Contain("which the submission API rejects"); + PlainConsoleText(result.Error).Should().Contain("which the submission API rejects"); FakeStorePackagedAPI .Verify( @@ -321,7 +321,7 @@ public async Task PackagedSubmissionUpdateCommandWithPayloadOption() payloadFilePath ]); - Unwrapped(result.Error).Should().Contain("Updating submission product"); + PlainConsoleText(result.Error).Should().Contain("Updating submission product"); result.Output.Should().Contain("\"FileUploadUrl\": \"https://azureblob.com/fileupload\""); } @@ -348,7 +348,7 @@ public async Task PackagedSubmissionUpdateCommandWithFilePathArgument() payloadFilePath ]); - Unwrapped(result.Error).Should().Contain("Updating submission product"); + PlainConsoleText(result.Error).Should().Contain("Updating submission product"); result.Output.Should().Contain("\"FileUploadUrl\": \"https://azureblob.com/fileupload\""); } @@ -377,7 +377,7 @@ public async Task PackagedSubmissionUpdateCommandWithStandardInputArgument() "-" ]); - Unwrapped(result.Error).Should().Contain("Updating submission product"); + PlainConsoleText(result.Error).Should().Contain("Updating submission product"); result.Output.Should().Contain("\"FileUploadUrl\": \"https://azureblob.com/fileupload\""); } @@ -408,7 +408,7 @@ public async Task PackagedSubmissionUpdateCommandWithRedirectedStandardInput() FakeApps[0].Id! ]); - Unwrapped(result.Error).Should().Contain("Updating submission product"); + PlainConsoleText(result.Error).Should().Contain("Updating submission product"); result.Output.Should().Contain("\"FileUploadUrl\": \"https://azureblob.com/fileupload\""); } @@ -422,7 +422,7 @@ public async Task PackagedSubmissionUpdateCommandWithNoPayload() FakeApps[0].Id! ], 1); - Unwrapped(result.Error).Should().Contain("No 'product' was provided."); + PlainConsoleText(result.Error).Should().Contain("No 'product' was provided."); } [TestMethod] @@ -436,7 +436,7 @@ public async Task PackagedSubmissionUpdateCommandWithUnknownFilePath() "this-file-does-not-exist.json" ], 1); - Unwrapped(result.Error).Should().Contain("is neither a JSON payload nor a path to an existing file"); + PlainConsoleText(result.Error).Should().Contain("is neither a JSON payload nor a path to an existing file"); } [TestMethod] @@ -464,7 +464,7 @@ public async Task PackagedSubmissionUpdateCommandWithBothInlineJsonAndPayloadOpt payloadFilePath ], 1); - Unwrapped(result.Error).Should().Contain("Use only one of them."); + PlainConsoleText(result.Error).Should().Contain("Use only one of them."); } [TestMethod] @@ -507,7 +507,7 @@ public async Task PackagedSubmissionUpdateCommandWithPayloadBiggerThanTheCommand It.IsAny()), Times.Once); - Unwrapped(result.Error).Should().Contain("Updating submission product"); + PlainConsoleText(result.Error).Should().Contain("Updating submission product"); result.Output.Should().Contain("\"FileUploadUrl\": \"https://azureblob.com/fileupload\""); } @@ -535,7 +535,7 @@ public async Task PackagedSubmissionUpdateMetadataCommand() }" ]); - Unwrapped(result.Error).Should().Contain("Updating submission product"); + PlainConsoleText(result.Error).Should().Contain("Updating submission product"); result.Output.Should().Contain("\"FileUploadUrl\": \"https://azureblob.com/fileupload\""); } @@ -567,7 +567,7 @@ public async Task PackagedSubmissionUpdateMetadataCommandWithPayloadOption() payloadFilePath ]); - Unwrapped(result.Error).Should().Contain("Updating submission product"); + PlainConsoleText(result.Error).Should().Contain("Updating submission product"); result.Output.Should().Contain("\"FileUploadUrl\": \"https://azureblob.com/fileupload\""); } @@ -593,7 +593,7 @@ public async Task PackagedSubmissionPublishCommand() FakeApps[0].Id! ]); - Unwrapped(result.Error).Should().Contain("Submission Committed with status"); + PlainConsoleText(result.Error).Should().Contain("Submission Committed with status"); } [TestMethod] @@ -613,7 +613,7 @@ public async Task PackagedSubmissionPollCommand() FakeApps[0].Id! ]); - Unwrapped(result.Error).Should().Contain("Submission commit success!"); + PlainConsoleText(result.Error).Should().Contain("Submission commit success!"); } [TestMethod] @@ -637,8 +637,8 @@ public async Task PackagedSubmissionDeleteCommand() FakeConsole.Verify(x => x.YesNoConfirmationAsync(It.IsAny(), It.IsAny()), Times.Once); - Unwrapped(result.Error).Should().Contain($"Found Pending Submission with Id '{FakeApps[0].PendingApplicationSubmission!.Id}'"); - Unwrapped(result.Error).Should().Contain("Existing submission deleted!"); + PlainConsoleText(result.Error).Should().Contain($"Found Pending Submission with Id '{FakeApps[0].PendingApplicationSubmission!.Id}'"); + PlainConsoleText(result.Error).Should().Contain("Existing submission deleted!"); } } } \ No newline at end of file From 2bd7e921412360c83715f0252000a232b9250791 Mon Sep 17 00:00:00 2001 From: azchohfi Date: Tue, 8 Sep 2026 13:55:50 -0700 Subject: [PATCH 11/11] Warn that --priceId sets one base price for the whole product Raised in review discussion on #175. The option described itself as stating the base price explicitly, which reads as inert, but it writes a single base price for the product and does not preserve per-market prices. Since a product that reports "Base" is exactly one whose price is managed per market, that is precisely the case where the distinction matters. The option description and the stop message now say so, and point at --noCommit and at Partner Center as the safer route. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 06440e13-c898-4e58-8764-d7abf304d9d8 --- MSStore.CLI/Commands/PublishCommand.cs | 2 +- MSStore.CLI/Helpers/IStorePackagedAPIExtensions.cs | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/MSStore.CLI/Commands/PublishCommand.cs b/MSStore.CLI/Commands/PublishCommand.cs index 1467fc1..cec9f7c 100644 --- a/MSStore.CLI/Commands/PublishCommand.cs +++ b/MSStore.CLI/Commands/PublishCommand.cs @@ -71,7 +71,7 @@ static PublishCommand() PriceIdOption = new Option("--priceId", "-pid") { - Description = "Specifies the base price tier to set on the submission, for example 'Tier1012', 'Free' or 'NotAvailable'. Only needed when the Store reports a base price the submission API will not accept back, which happens when the price is managed per market from Partner Center.", + Description = "Specifies the base price tier to set on the submission, for example 'Tier1012', 'Free' or 'NotAvailable'. Only needed when the Store reports a base price the submission API will not accept back, which happens when the price is managed per market from Partner Center. This sets a single base price for the product rather than preserving per-market prices, so consider '--noCommit' to review the submission before it goes live.", CustomParser = result => { if (result.Tokens.Count == 0) diff --git a/MSStore.CLI/Helpers/IStorePackagedAPIExtensions.cs b/MSStore.CLI/Helpers/IStorePackagedAPIExtensions.cs index 5abf949..b3b8c31 100644 --- a/MSStore.CLI/Helpers/IStorePackagedAPIExtensions.cs +++ b/MSStore.CLI/Helpers/IStorePackagedAPIExtensions.cs @@ -854,6 +854,7 @@ internal static bool TryPreservePricing(IAnsiConsole ansiConsole, DevCenterSubmi ansiConsole.MarkupLine("Publishing has been stopped so the price is left as it is."); ansiConsole.MarkupLine("Re-run with [green]--priceId[/] to state the base price explicitly (for example [green]--priceId Tier1012[/]), or publish this submission from Partner Center."); + ansiConsole.MarkupLine("[yellow]--priceId[/] sets a single base price for the product rather than preserving per-market prices, so publishing from Partner Center is the safer option when this product is priced per market."); return false; }