fix(analytics): exclude internal categories by group flag, not by name - #25
Open
DerekLangley wants to merge 1 commit into
Open
fix(analytics): exclude internal categories by group flag, not by name#25DerekLangley wants to merge 1 commit into
DerekLangley wants to merge 1 commit into
Conversation
`MONEY_FLOW_EXCLUDE_GROUPS` matches the literal strings "Internal Master
Category" and "Credit Card Payments". That is locale-dependent, stops working
silently when a user renames a group, can be spoofed by a user-created group of
the same name, and misses the "Hidden Categories" group entirely.
The API already provides the signal: `internal` is required on both
`CategoryBase` and `CategoryGroup` in spec 1.86.0. The models did not carry the
field, so `extra="ignore"` was discarding it.
The flag does not inherit, and that is the trap. On a real budget the
"Credit Card Payments" group is `internal: true` while every category inside it
is `internal: false`. A category-level check therefore readmits all of them, and
a payment category's `activity` is the movement for spending already counted in
the real category — so credit-funded spending gets counted twice and
`pct_of_total` is computed against an inflated denominator. The predicate has to
be the category's *group's* flag.
`group.internal` comes from GET /plans/{id}/categories, not from
GET /plans/{id}/months/{month}, so analytics resolve a category -> group.internal
map from the delta-synced category list. That call is already cached; no new
endpoint is involved.
A category missing from that map — a historical month can name one that has since
been deleted — is treated as unknown rather than as "not internal", and falls
back to the previous group-name check. Those two situations are counted and
logged separately: a missing category is routine churn at INFO, while a resolved
group with no `internal` flag is a schema regression at WARNING.
`internal` is modeled as `bool | None` so an absent flag stays distinguishable
from a false one; `bool` with a False default cannot tell them apart, and absent
must degrade to the name check while false must not.
Co-Authored-By: Claude Opus 5 <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.
I noticed that the analytics code currently identifies YNAB's internal category groups by their English display names. That works for the usual names, but it breaks if those groups are localized or renamed, and it doesn't catch
Hidden Categories.Since the YNAB API exposes an
internalflag for category groups, this PR switches the exclusion logic to use that flag instead of relying on names.What was happening
MONEY_FLOW_EXCLUDE_GROUPSinsrc/server/analytics.pymatches the literal strings"Internal Master Category"and"Credit Card Payments". That is locale-dependent, silently stops working when a user renames a group, can be spoofed by a user-created group of the same name, and missesHidden Categoriesentirely.The API already provides the right signal:
internalisrequiredon bothCategoryBaseandCategoryGroupin spec 1.86.0. The models did not carry the field, andYNABBaseModelsetsextra="ignore", so Pydantic was discarding it.One important detail: the flag does not inherit
On a real budget, the
Credit Card Paymentsgroup isinternal: truewhile every category inside it isinternal: false.A category-level check therefore readmits all of them, and a payment category's
activityis the movement for spending already counted in the real category. That causes credit-funded spending to be double-counted andpct_of_totalto be computed against an inflated denominator.The predicate therefore needs to use the category's group's
internalflag.group.internalis available fromGET /plans/{id}/categories, but not fromGET /plans/{id}/months/{month}. Analytics now resolve acategory_id -> group.internalmap from the delta-synced category list — one already-cached call, with no new endpoint required.Behavior change
This deliberately changes output for some existing users in two cases:
Hidden Categoriesisinternal: true, but was missed by the old name check. Users with hidden categories carrying activity may therefore see differentget_money_flowandget_spending_by_categorytotals.Both are the intended result of the fix rather than incidental behavior changes.
Compatibility and fallback behavior
A category can be absent from the current category map — for example, a historical month can reference a category that has since been deleted. In that case, the code treats the internal status as unknown and falls back to the old name check rather than assuming the category is not internal.
Two fallback situations are counted and logged separately:
INFO.internalflag is treated as a potential schema regression and logged atWARNING.This keeps routine historical misses from obscuring an actual API/schema change.
internalis modeled asbool | Noneso an absent flag remains distinguishable from a realfalsevalue. An absent value must degrade to the name-based fallback; an explicitfalsemust not.Tests
Tests cover:
internalHidden CategoriesThe cache fixture in
tests/test_server.pyis changed to anAsyncMockso the addedget_categoriescall is awaitable in the existing tests.Test note: The full suite requires
YNAB_API_KEYto be set due to the repository's existing import-time configuration behavior (#4); with a token set, 118 tests pass.