Preserve the base price when publishing - #175
Preserve the base price when publishing#175Alexandre Zollinger Chohfi (azchohfi) wants to merge 11 commits into
Conversation
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
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
There was a problem hiding this comment.
🟡 Changes recommended
TryPreservePricing currently treats submission.Pricing == null as preservable (leading to a 400/exception path) and one new unit test asserts success for a “no pricing” product case that contradicts the documented API behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes a data-loss bug where msstore publish could unintentionally reset paid apps to Free by ensuring the outgoing submission payload always preserves a valid, round-trippable Pricing.PriceId, and by adding an explicit --priceId override for the non-round-trippable Base sentinel case.
Changes:
- Add
--priceId/-pidtopublishandinit, and thread it through project publishers into the packaged publish pipeline. - Replace the previous “only Free products” gating with
TryPreservePricing(...)logic that preserves round-trippable pricing and blocks unsafe updates. - Fix packaged
submission updateerror signaling/validation so failures returnnull(not boxed ints) and validation applies to the payload being sent.
File summaries
| File | Description |
|---|---|
| MSStore.CLI/ProjectConfigurators/PWAProjectConfigurator.cs | Thread priceId into the PWA publish call. |
| MSStore.CLI/ProjectConfigurators/MSIXProjectPublisher.cs | Thread priceId into MSIX publish. |
| MSStore.CLI/ProjectConfigurators/IProjectPublisher.cs | Extend publisher interface to accept priceId. |
| MSStore.CLI/ProjectConfigurators/FileProjectConfigurator.cs | Pass priceId into packaged publish from file-based publishers. |
| MSStore.CLI/ProjectConfigurators/ElectronProjectConfigurator.cs | Forward priceId through the Electron override. |
| MSStore.CLI/Helpers/IStorePackagedAPIExtensions.cs | Add TryPreservePricing and plumb priceId into publish pipeline. |
| MSStore.CLI/Commands/Submission/UpdateCommand.cs | Fix failure signaling and validate the outgoing payload’s pricing tier. |
| MSStore.CLI/Commands/PublishCommand.cs | Add and parse --priceId, then pass it into publishing. |
| MSStore.CLI/Commands/InitCommand.cs | Reuse PublishCommand.PriceIdOption and pass through to publish step. |
| MSStore.CLI.UnitTests/SubmissionCommandPackagedUnitTests.cs | Add coverage for packaged submission update pricing validation behavior. |
| MSStore.CLI.UnitTests/PublishCommandPricingUnitTests.cs | New tests asserting preserved pricing / no empty PriceId regression. |
| MSStore.CLI.UnitTests/BaseCommandLineTest.cs | Allow fake submissions to be created with configurable Pricing. |
| MSStore.API/Packaged/Models/PriceIds.cs | Centralize PriceId normalization and “round-trippable” rules. |
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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
There was a problem hiding this comment.
🟢 Approval recommended
The fix is well-scoped, includes strong payload-level regression tests, and only has a minor wording nit in one new error message.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
MSStore.CLI/Commands/Submission/UpdateCommand.cs:128
- The rejection message refers to the "provided product" having a base price, but this check is validating the payload being sent (updateSubmission.Pricing.PriceId). Wording it as a payload validation would be more accurate and less confusing when users are editing JSON locally.
- Files reviewed: 13/13 changed files
- Comments generated: 0 new
- Review effort level: Lite
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
There was a problem hiding this comment.
🟡 Changes recommended
The packaged submission update path introduces an unnecessary extra API call and ships inaccurate user-facing error text about how the API behaves for missing/invalid Pricing.PriceId.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 2
- Review effort level: Lite
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
There was a problem hiding this comment.
🟡 Changes recommended
UpdateCommand.PackagedUpdateCommandAsync can still call the API (and potentially leave a newly created draft behind) when the JSON omits Pricing entirely, so it should fail fast and clean up consistently.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 1
- Review effort level: Lite
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
There was a problem hiding this comment.
🔵 Needs a closer look
A newly added error log in TryPreservePricing is misleading in the missing-pricing case and should be adjusted for accurate diagnostics.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
MSStore.CLI/Helpers/IStorePackagedAPIExtensions.cs:836
- The error log here always says "The submission has PriceId '{PriceId}'", but in the
submission.Pricing == nullcasepriceIdis null and the real problem is the missing Pricing object (not an invalid PriceId). This makes logs misleading when diagnosing why publish stopped.
- Files reviewed: 13/13 changed files
- Comments generated: 0 new
- Review effort level: Lite
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
There was a problem hiding this comment.
🔵 Needs a closer look
A small but real validation bug remains in PriceIds.IsRoundTrippable where leading/trailing whitespace can bypass the Base sentinel check.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
MSStore.API/Packaged/Models/PriceIds.cs:55
IsRoundTrippabletreats values like"Base "/" Base"as round-trippable because it doesn’t trim before comparing toBase. Since leading/trailing whitespace is already treated as insignificant inTryNormalize(andIsNullOrWhiteSpaceis used), it’s safer/consistent to trim before theBasecheck so whitespace can’t bypass the guard.
- Files reviewed: 13/13 changed files
- Comments generated: 0 new
- Review effort level: Lite
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
There was a problem hiding this comment.
🟢 Approval recommended
The changes directly prevent the silent “paid → free” regression with explicit guardrails and strong unit tests validating the outgoing submission payload.
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 0 new
- Review effort level: Lite
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
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
There was a problem hiding this comment.
🟢 Approval recommended
The change set consistently prevents silent price resets by guarding outbound pricing, adds a safe override mechanism, and includes targeted unit tests that validate the outgoing payload behavior.
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
Its a good change and the best this tool can achieve. Hope you push your colleagues for a fix in the API that fixes the base price. When create a new app, the new pricing model is always used, resulting always in Having the argument --priceId is nice; gives an easy option to switch to the old model when already using the new one. |
|
So the only way to update a paid app would be to change its pricing to Tier<>. And that would use the old pricing model. Are there any cons of using the old pricing model? |
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
|
Dave Smits (@davesmits) isourabh - answering both of you here, since the two questions run into each other.
That's the part I hadn't appreciated, and it changes how I read this whole PR. If every new app starts out that way, then I'll pass that on. The root of it is that an update replaces the entire submission, so pricing rides along on every request, and there's no way to say "leave the price alone." I tried all of them against the live API:
That middle row is this whole bug. It succeeds, so nothing tells you anything went wrong. Either a no-op value that round-trips, or patch semantics so pricing can be left out of an update that was never about pricing, would make
isourabh - on the mechanics, yes, a real tier is the only thing the API will take back. On the cons, there's one I suspect but don't want to state as fact. A tier is a single base price that spreads into each market through the conversion table, whereas the per-market model exists so you can set them independently. So putting a tier on a product that's currently priced per market looks like it would flatten those custom prices into one. I couldn't verify it: that needs a product genuinely on the new model, and the only way to get one is the Partner Center UI, which seems to be a one-way door. Let me check with the pricing folks and come back to you rather than guess at it. What I have changed in the meantime is stopping the CLI from making that decision quietly. |
There was a problem hiding this comment.
🟢 Approval recommended
The changes address the silent price-reset failure mode with explicit guards and add targeted unit tests that verify the outgoing update payloads (the only reliable regression signal).
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
waiting for payment profile to be setup on an account to test the changes and complete this PR |

Why
msstore publishwas resetting paid apps to USD 0.00 (#112).The submission API replaces the whole submission on update, so publish has to send a complete, valid
pricingobject back even when it has no interest in the price. There are no patch semantics: anything the request does not state explicitly is reset to its default, and that default is free.Every behaviour below was measured against the live DevCenter API using a throwaway submission draft that was never committed:
priceId: "Free"/"Tier96"/"Tier1012"/"Tier1424"priceId: "Base"'Base' is not a valid PriceId for base price.priceId: nullpriceIdproperty removedpricing: {}pricing: nullor omittedPricing data was not provided in the request.0b4b0bcsent an emptyPriceIdwheneverIsAdvancedPricingModelwas set, which is the silent case: the API returns 200 and wipes the price, with no error to notice.That condition was wrong on its own terms too.
isAdvancedPricingModelis documented as read-only and only says which tier range a dashboard offers. Live data shows it is unreliable: on the test account it istruefor free apps andfalsefor others, it flippedtruetofalseafter a PUT that echoed it back unchanged, and both documented tier ranges were accepted on the same product. It must not gate any logic.51c1bc4then stopped the data loss by refusing to publish anything whosePriceIdisBase, reporting "App updates are supported only for Free products". That message is not accurate. A product with a real tier round-trips unchanged and publishes fine; only theBasesentinel cannot be sent back. The practical effect was that paid products could not publish at all.What this does
Publish now leaves any round-trippable price alone, so ordinary paid products publish normally again. It never sends an empty
PriceId.For products where the API hands back
Base, the real price cannot be recovered from anywhere: it is absent from the application resource, the newer submission API returns 404 for packaged products, and no API exposes the price tier table. So the price can only come from the caller, and a new--priceId/-pidoption onpublishandinitstates it explicitly. When neither is possible, publishing still stops rather than resetting the price, but now explains why and how to proceed.Tier numbers are deliberately not range checked. Both documented ranges were accepted on the same product and
isAdvancedPricingModelcannot distinguish them, so the service decides.submission updatewas 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.Worth a closer look
PackagedUpdateCommandAsyncsignalled failure by returning a boxedint, but the caller only checks fornull, so those paths were reported as success and printed the code as the command's output. The pre-existing "only Free products" path had this defect as well.--priceIdonly ever surfaces for products the API hands back asBase, which are currently blocked outright. Free and normally priced paid products never see it. Happy to drop the option and simply leave those users to Partner Center if that is preferred; the rest of the fix stands either way.JsonIgnore(WhenWritingNull)toPricing.PriceIdlooks like a tidy-up but would silently reintroduce this bug, since omitting the property wipes the price exactly like sending it as null. There is a test for that.Validation
225 tests, 0 build warnings. The two
PublishCommandFor{WinUI,Maui}AppsShouldCallMSBuildIfWindowsfailures onnet10.0are pre-existing and unrelated, confirmed by stashing this change and re-running on a clean tree; the Windows target framework is 225/0.Also exercised end to end with the built CLI against the live API on a real app, using a throwaway draft that was deleted afterwards:
PriceId: "Base"- blocked, exit -1, no writePricingbut noPriceId- blocked, exit -1, no writeTier1012- accepted, and an independent server side read confirmedpriceId = Tier1012One gap worth stating plainly: I could not reproduce a product that the API actually returns
Basefor, because that requires the per market pricing flow in the Partner Center web UI and appears to be effectively one way. The API side of that case is proven (400 onBase) and the payload side is proven end to end, but the publish path readingBaseback from a real product is covered only by unit tests.Fixes: #112