feat(up): map Up category slugs to Sure categories on import#2487
feat(up): map Up category slugs to Sure categories on import#2487threatsurfer wants to merge 3 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (7)
📝 WalkthroughWalkthroughThe PR adds an Up category taxonomy, slug matching to Sure categories, and transaction import wiring that passes matched category ids into entry imports. Tests cover matcher resolution and import behavior. ChangesUp category matching flow
Sequence Diagram(s)sequenceDiagram
participant UpAccountProcessor as UpAccount::Transactions::Processor
participant EntryProcessor as UpEntry::Processor
participant CategoryMatcher as UpAccount::Transactions::CategoryMatcher
participant ImportAdapter as import_adapter
UpAccountProcessor->>CategoryMatcher: build from family categories
UpAccountProcessor->>EntryProcessor: initialize with category_matcher
EntryProcessor->>CategoryMatcher: match up_category_slug
CategoryMatcher-->>EntryProcessor: matched category or nil
EntryProcessor->>ImportAdapter: import_transaction(category_id)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possible related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a9ddced642
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| def category_details(up_category_slug) | ||
| return nil if up_category_slug.blank? | ||
|
|
||
| detailed_categories.find { |c| c[:key] == up_category_slug.to_s.downcase.to_sym } |
There was a problem hiding this comment.
For every Up transaction with a category slug, this comparison converts the incoming slug to a symbol while CATEGORIES_MAP stores all child keys as string slugs, so checks like "groceries" == :groceries never succeed. That makes category_details return nil, matched_category_id stay nil, and the new Up auto-categorization path never applies any category despite the imported Up payload containing one.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/models/up_account/transactions/category_matcher.rb (1)
43-45: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueNormalization strips
andas a substring, not just the conjunction.
gsub(/(and|&|\s+)/, "")removes every occurrence ofandinside a word (e.g.errand→err,grand→gr), which can collapse distinct names and produce a wrong match against a user-defined category. No collision exists among Sure's current defaults, so this is latent, but a word-boundary form is safer.♻️ Match the word, not the substring
- normalized_name = name.gsub(/(and|&|\s+)/, "").strip - normalized_alias = alias_str.gsub(/(and|&|\s+)/, "").strip + normalized_name = name.gsub(/(\band\b|&|\s+)/, "").strip + normalized_alias = alias_str.gsub(/(\band\b|&|\s+)/, "").strip🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/models/up_account/transactions/category_matcher.rb` around lines 43 - 45, The normalization in CategoryMatcher is removing the substring "and" anywhere in a word, which can create false matches. Update the matching logic in the name/alias normalization path to strip only the standalone conjunction (for example by using a word-boundary-based pattern) while still removing "&" and whitespace, and keep the comparison in the same method that computes normalized_name and normalized_alias.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@app/models/up_account/transactions/category_matcher.rb`:
- Around line 43-45: The normalization in CategoryMatcher is removing the
substring "and" anywhere in a word, which can create false matches. Update the
matching logic in the name/alias normalization path to strip only the standalone
conjunction (for example by using a word-boundary-based pattern) while still
removing "&" and whitespace, and keep the comparison in the same method that
computes normalized_name and normalized_alias.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a481b2d9-b11c-479f-8d91-a81385e3c922
📒 Files selected for processing (6)
app/models/up_account/transactions/category_matcher.rbapp/models/up_account/transactions/category_taxonomy.rbapp/models/up_account/transactions/processor.rbapp/models/up_entry/processor.rbtest/models/up_account/transactions/category_matcher_test.rbtest/models/up_entry/processor_test.rb
|
Re the Codex P1 on That said, the quoted-symbol syntax is easy to misread, and Up's category ids are strings anyway, so I have pushed 7d8c824 to compare them as strings ( |
|
One concern:
In Consider making the matcher return Generated by Claude Code |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/models/up_account/transactions/processor_test.rb (1)
65-66: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid asserting against the localized category name.
Line 66 hard-codes
"Groceries", so this test will fail if the bootstrapped label is renamed or translated even when the mapping still works. Assert against the expected category record/id instead of its display name. Based on learnings, avoid hard-coded category-name comparisons in Ruby/Rails code; prefer identity or predicate checks.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/models/up_account/transactions/processor_test.rb` around lines 65 - 66, The test in the transaction processor spec is asserting on the localized display name of the mapped category, which is brittle. Update the assertion in the `processor_test` around `entry.transaction.category` to check the expected category record/identity (or a predicate on the category association) instead of comparing `category&.name` to a hard-coded string. Keep the lookup by `external_id`, but replace the name-based expectation with an assertion tied to the category object or its stable identifier.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/models/up_account/transactions/processor_test.rb`:
- Around line 65-66: The test in the transaction processor spec is asserting on
the localized display name of the mapped category, which is brittle. Update the
assertion in the `processor_test` around `entry.transaction.category` to check
the expected category record/identity (or a predicate on the category
association) instead of comparing `category&.name` to a hard-coded string. Keep
the lookup by `external_id`, but replace the name-based expectation with an
assertion tied to the category object or its stable identifier.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3543612d-e34f-4b30-ac9f-b5e7984bb5e1
📒 Files selected for processing (3)
app/models/up_account/transactions/category_matcher.rbapp/models/up_account/transactions/processor.rbtest/models/up_account/transactions/processor_test.rb
🚧 Files skipped from review as they are similar to previous changes (1)
- app/models/up_account/transactions/category_matcher.rb
|
Good call, agreed. Pushed c5f9fca: FWIW I had lifted the bootstrap behaviour from Also folded in CodeRabbit's nitpick in the same commit: word-boundaried the |
UpEntry::Processor captured Up's category slug into extra but never applied it, so Up transactions imported uncategorised even though the user had already tagged them in the Up app. Add UpAccount::Transactions::CategoryTaxonomy + CategoryMatcher, mirroring PlaidAccount::Transactions::CategoryMatcher: map Up's child category slugs onto the family's existing/default Sure categories by alias, and wire the matcher through UpAccount::Transactions::Processor into UpEntry::Processor. The category is applied via the adapter's enrich_attribute, so a category the user has set or locked is preserved on re-sync. High-confidence mappings only. Up-specific categories with no honest Sure default (Booze, Pets, Apps & Games, Life Admin, Technology, ...) intentionally stay uncategorised for the user's own rules / AI, since a wrong auto-category is worse than none. Adds a matcher unit test and processor wiring tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Up category ids are string slugs; compare them against the taxonomy keys as strings so the lookup does not depend on the keys being symbols. No behaviour change (the "slug": hash syntax already produces symbol keys that matched the symbolized input, covered by the matcher unit test), but it removes a subtle footgun and reads clearer. Flagged by the Codex review on the PR. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…s match Per review feedback: do not bootstrap Sure's default categories during a sync. family_categories now returns the family's existing categories without creating defaults, so a family that has none (deliberately cleared, or pre-onboarding) gets uncategorised transactions rather than having the full default set silently created. Matching resumes once the user sets up categories through the normal UI flow. Also word-boundary the "and" stripping in the matcher normalization so it strips only the standalone conjunction, not "and" inside a word (e.g. errand). Adds a processor test for the non-destructive guarantee. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
threatsurfer
force-pushed
the
feat/up-apply-categories
branch
from
c5f9fca to
93fa1a6
Compare
3 participants
Summary
Up tags each spending transaction with a category (the one the user picks in the Up app), exposed as a child slug on
relationships.category. The merged provider captures that slug intoextrabut never applies it, so Up transactions import uncategorised. This PR maps Up's category slugs onto the family's existing/default Sure categories.Approach
Mirrors
PlaidAccount::Transactions::CategoryMatcher: a newUpAccount::Transactions::CategoryTaxonomy(Up child slugs to alias lists) plus aCategoryMatcherthat resolves those aliases against the family's categories. It bootstraps Sure's default categories if the family has none, and never creates a provider-specific taxonomy. The matcher is built once per account inUpAccount::Transactions::Processorand passed toUpEntry::Processor, which setscategory_idthrough the adapter.Coverage (real data)
Against my own ~7yr / ~26k dataset, about 70% of categorised Up transactions (~5,300 of ~7,600) map cleanly to a default Sure category: Groceries, Takeaway/Restaurants to Food & Drink, TV/Music to Subscriptions, Health to Healthcare, Fuel/Taxis to Transportation, Fitness to Sports & Fitness, and so on. Up-specific categories with no honest Sure default (Apps & Games, Life Admin, Technology, Booze, Pets) are deliberately left unmapped so they stay uncategorised for the user's own rules / AI. High confidence only: a wrong auto-category is worse than none.
User edits are preserved
category_idis applied through the adapter'senrich_attribute(ignore_locks defaults to false), and the protection check skips user_modified entries entirely, so a category a user has set or locked survives re-sync. Same concern raised on #2460.Stacking
This stacks on #2460 (both touch
UpEntry::Processor) and is branched off a newermain, so the two will conflict on merge. Happy to rebase whichever lands second.Tests
UpAccount::Transactions::CategoryMatcherunit test: high-confidence slugs resolve to the right default category; ambiguous / unknown / blank slugs return nil; nothing matches when the target category is absent.UpEntry::Processor: a matched category is applied (and the raw slug stays inextra); an unmatched category leaves the transaction uncategorised.Summary by CodeRabbit