Skip to content

Better multi-currency support - #131

Open
zbrox wants to merge 8 commits into
bscott:mainfrom
zbrox:fix/analytics-multicurrency
Open

Better multi-currency support#131
zbrox wants to merge 8 commits into
bscott:mainfrom
zbrox:fix/analytics-multicurrency

Conversation

@zbrox

@zbrox zbrox commented Jul 18, 2026

Copy link
Copy Markdown

Hey,

Since I am dealing with a bunch of different (from the default) currencies in my subscriptions I noticed that they don't mix very well. The absolute values are used to do aggregations, foreign currency values are displayed on the calendar with the default currency symbol, etc. Going over that I realized that some changes were also needed for the Fixer.io integration so it's used more evenly throughout. Also I wanted to make sure API calls are lowered to a minimum to be able to stay within the free tier of 100 calls per month as much as possible. No proper lock is implemented so technically there can be more calls made in certain cases I think, but it should be ok as is now.

This is a bit extensive but shouldn't require any data migrations. From my own local tests (with and without a fixer api key, with or without fetched and cached exchange rates) it looks pretty ok.

I tried to keep decisions on changes to a minimum. I understand it is still quite hefty for a PR. Hopefully I kept to the convention of the project and it's not too cumbersome for review.

Here's a not so short list of the changes included here:

  • Derive cross-currency rates from cached EUR-based rates, allowing one Fixer snapshot to serve every supported currency pair
  • Persist a 24-hour cooldown after each automatic Fixer refresh attempt, including failed attempts, reducing repeated calls across subsequent requests and application restarts
  • Fall back to stale cached rates when Fixer is unavailable, its quota is exhausted, or no API key is configured
  • Indicate when subscription values use stale cached rates and show the date of those rates
  • Convert dashboard and analytics totals into the preferred currency when all required rates are available
  • Show totals grouped by original currency when any required rate is missing, avoiding invalid mixed-currency sums
  • Hide category percentage bars when currencies cannot be safely combined (I was not really sure what is a good solution here)
  • Apply conversion-aware statistics consistently to the dashboard, analytics, stats API, backups, and MCP endpoint
  • Display calendar costs in the preferred currency when conversion is available, falling back to the original amount and symbol otherwise
  • Sort subscriptions using their converted displayed costs when the required rates are available. When rates are unavailable, group cost sorting by currency and sort numerically within each group
  • Remove the obsolete raw SQL category-statistics path replaced by conversion-aware aggregation

@bscott bscott left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the thorough work here. The overall direction is sound and the PR passes go test ./..., go vet ./..., and go build ./... on its own, but I found two concrete blockers before we add it to v0.6.5:

  1. A zero-cost subscription in a foreign currency produces invalid values. enrichWithCurrencyConversion calculates the conversion ratio by dividing the converted cost by the original cost. Zero is an allowed cost, so this becomes 0 divided by 0 and the converted annual, monthly, and share amounts become NaN. I reproduced this with a focused handler test. Please handle zero-cost subscriptions without dividing by the amount and add a regression test covering this case.

  2. This currently breaks when combined with PR #129. PR #131 changes NewCurrencyService to require the settings repository, while the new subscription_form_test.go from #129 uses the old one-argument call. The branches merge without a text conflict, but the combined build fails. This can be resolved by rebasing after #129 or by updating that test call when the PRs are staged together.

The second issue is cross-PR integration rather than a problem with this branch in isolation. The first is a user-visible correctness bug and needs to be fixed in this PR.

@zbrox
zbrox force-pushed the fix/analytics-multicurrency branch from d68ab73 to bbe025d Compare August 5, 2026 15:26
@zbrox

zbrox commented Aug 5, 2026

Copy link
Copy Markdown
Author
  1. A zero-cost subscription in a foreign currency produces invalid values. enrichWithCurrencyConversion calculates the conversion ratio by dividing the converted cost by the original cost. Zero is an allowed cost, so this becomes 0 divided by 0 and the converted annual, monthly, and share amounts become NaN. I reproduced this with a focused handler test. Please handle zero-cost subscriptions without dividing by the amount and add a regression test covering this case.

Oh, good catch. This should be fixed now. There's a test for that as well now.

@zbrox
zbrox force-pushed the fix/analytics-multicurrency branch from bbe025d to 9b438e7 Compare August 5, 2026 15:38
@zbrox

zbrox commented Aug 5, 2026

Copy link
Copy Markdown
Author

2. This currently breaks when combined with PR #129. PR #131 changes NewCurrencyService to require the settings repository, while the new subscription_form_test.go from #129 uses the old one-argument call. The branches merge without a text conflict, but the combined build fails. This can be resolved by rebasing after #129 or by updating that test call when the PRs are staged together.

I have rebased this branch on top of the one for #129. I also fixed the NewCurrencyService constructor and test, vet and build ran fine. Please merge #129 first. Once it is merged, I’ll sync my fork and rebase this branch onto the updated main. Until then, this branch demonstrates that #129 and #131 build and test successfully together.

@bscott bscott self-assigned this Aug 21, 2026
@bscott

bscott commented Aug 22, 2026

Copy link
Copy Markdown
Owner

Thanks for the quick turnaround on both blockers — the zero-cost fix with a regression test, and the rebase demonstrating #129 and #131 build together. Current plan: I'm mid-release on v0.6.5; once it ships I'll merge #129, and after you rebase onto the updated main I'll re-review this right away.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants