Better multi-currency support - #131
Conversation
bscott
left a comment
There was a problem hiding this comment.
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:
-
A zero-cost subscription in a foreign currency produces invalid values.
enrichWithCurrencyConversioncalculates 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 becomeNaN. 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. -
This currently breaks when combined with PR #129. PR #131 changes
NewCurrencyServiceto require the settings repository, while the newsubscription_form_test.gofrom #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.
d68ab73 to
bbe025d
Compare
Oh, good catch. This should be fixed now. There's a test for that as well now. |
bbe025d to
9b438e7
Compare
I have rebased this branch on top of the one for #129. I also fixed the |
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: