Category rename/delete, payment-source rename, subscription currency picker - #149
Merged
Merged
Conversation
CategoriesController exposed GET and POST only, so a user-created category was permanent. A typo sat in the picker and the dashboard breakdown forever, and the only escape was to reassign every subscription and live with the empty row. Delete carries the design question. category_id is ON DELETE SET NULL, so removing a category uncategorises the subscriptions that used it rather than deleting them - losing a grouping beats losing the record. That is now a deliberate, stated outcome: the endpoint counts the affected rows before the delete (afterwards there is nothing left to count by, the column having been nulled) and returns the number, the client warns before, and reports after. Both operations scope on c.UserId == userId, not the "visible to me" predicate GetForUserAsync uses. Seeing a system default is not owning it, and renaming one would change it for every account on the instance - so those answer 404 like any other category the caller does not own. Tests cover it from both directions, along with another user's category. Payment sources gain rename for the reason the issue called out: DELETE existed but no PUT, and delete-and-recreate is not equivalent. The subscriptions foreign key is SET NULL, so recreating detaches every subscription that billed to it. Renaming updates in place and keeps the type, since the type is chosen at creation and silently resetting it would change the row's icon and tint. On the client both lists get swipe actions rather than inline buttons - the rows are mostly a name, and two buttons each would crowd out the thing being named. The category actions are hidden for system defaults, though the check that matters is still the server's. IUserPrompt grows AlertAsync and PromptAsync alongside ConfirmAsync. PromptAsync returns null on cancel, which is deliberately not the same as an empty string: cancelling a rename must not be read as clearing the name. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…e-letter code Settings picks the home currency from a dropdown; the subscription form still asked for the raw ISO code in a MaxLength=3 Entry. Same value, same validation, two ways of entering it - and the typed one was the worse half, since a typo passed client-side and only failed as a 400 after Save. Reuses SupportedCurrencies and the two-way selection sync from Settings, including the guard that stops the two properties recursing, and the rule that a stored code the runtime does not recognise is inserted into the list and selected rather than silently replaced. One difference from Settings, and it is deliberate: this field is what the subscription is billed in, not a preference, so a new subscription starts on the form's own default rather than following the user's home currency. Most subscriptions are billed locally, and inheriting the home currency would be right only by coincidence. Fixes a bug the tests found on the way: Currency's property initializer assigns the backing field directly, so OnCurrencyChanged never ran for the initial value and the add form would have opened with an empty picker. Set in the constructor now, for the same reason RecalculateNextBillingDate is called there. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Branched from current
main(27ff0ea). The two straightforward gaps from the review, one commit each.Closes #143
Closes #147
#143 — categories were create-only
CategoriesControllerhadGETandPOSTand nothing else, so a user-created category was permanent. A typo — "Entertainmnet" — sat in the picker and the dashboard breakdown forever, and the only escape was reassigning every subscription and living with the empty row.Delete is where the design question lives.
category_idisON DELETE SET NULL, so removing a category uncategorises its subscriptions rather than deleting them — losing a grouping beats losing the record. That's now a stated outcome rather than a silent one:Both operations scope on
c.UserId == userId, not the "visible to me" predicateGetForUserAsyncuses. Seeing a system default is not owning it, and renaming one would change it for every account on the instance — so those answer 404, same as another user's category. Tests cover both directions.Payment sources gain rename for the reason the issue called out:
DELETEexisted but noPUT, and delete-and-recreate isn't equivalent — the subscriptions FK isSET NULL, so recreating detaches everything that billed to it. Rename updates in place and keeps the type, since the type is chosen at creation and resetting it would change the row's icon and tint.Client-side both lists get swipe actions rather than inline buttons — the rows are mostly a name, and two buttons each would crowd out the thing being named. Category actions are hidden for system defaults, though the check that matters is still the server's.
IUserPromptgrowsAlertAsyncandPromptAsync.PromptAsyncreturns null on cancel, deliberately not an empty string: cancelling a rename must not read as clearing the name. Three tests pin that.#147 — subscription currency
Now a picker, reusing
SupportedCurrenciesand the two-way sync from Settings, including the unrecognised-code rule.One deliberate difference: a new subscription starts on the form's own default rather than the user's home currency. This field is what the subscription is billed in, not a preference — most are billed locally, and inheriting the home currency would be right only by coincidence.
A bug the tests caught:
Currency's property initializer assigns the backing field directly, soOnCurrencyChangednever ran for the initial value and the add form would have opened with an empty picker. Now set in the constructor, for the same reasonRecalculateNextBillingDateis.Testing
SubVora.Api.TestsSubVora.Mobile.TestsSubVora.Infrastructure.TestsSubVora.Application.Tests538 total, 0 failures, against real Postgres via Testcontainers.
Worth calling out in the new coverage: the uncategorise count is asserted end-to-end (create category → two subscriptions → delete → both survive with null category); system defaults and other users' rows are rejected on both rename and delete; and renaming a payment source keeps its subscriptions attached with the new label.
Not visually verified
Same standing caveat — XAML compiled through the Windows TFM, view-model tests pass, but the swipe actions haven't been used on a device.
Remaining
Only the two product calls: #144 (offline outbox) and #145 (brand logos). Both want a decision before implementation, not just a green light.