gavin/maybe
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 28 Wiki Activity

Multi-currency support #425

Merged
Shpigford merged 13 commits from feature/currency into main 2024-02-11 06:18:56 +08:00
Conversation 11 Commits 13 Files Changed 41 +634 -176
Shpigford commented 2024-02-11 01:00:41 +08:00 (Migrated from github.com)
Copy Link

There's so much here that it may genuinely be difficult to actually fully review this (not to mention, it requires a $12/mo Open Exchange Rates account to pull in exchange rate data).

But, putting it here for sanity checks on the code.

There's so much here that it may genuinely be difficult to actually fully review this (not to mention, it requires a $12/mo Open Exchange Rates account to pull in exchange rate data). But, putting it here for sanity checks on the code.
zachgoll (Migrated from github.com) approved these changes 2024-02-11 04:44:31 +08:00
zachgoll (Migrated from github.com) left a comment
Copy Link

The direction of this makes sense to me (GoodJob looks interesting, nice that we can avoid the extra Redis dependency!). Eventually it might be nice to abstract away the "original" and "converted" concept away from the views and have a account.balance instance method to use, but nothing that can't be improved later!

The direction of this makes sense to me (GoodJob looks interesting, nice that we can avoid the extra Redis dependency!). Eventually it might be nice to abstract away the "original" and "converted" concept away from the views and have a `account.balance` instance method to use, but nothing that can't be improved later!
zachgoll (Migrated from github.com) reviewed 2024-02-11 04:47:55 +08:00
db/migrate/20240209174912_redo_money_storage.rb
@@ -0,0 +7,4 @@
remove_column :accounts, :balance_cents
remove_column :accounts, :balance_currency
remove_column :accounts, :currency
zachgoll (Migrated from github.com) commented 2024-02-11 04:47:54 +08:00
Copy Link

Guessing we're cool with dropping existing balances at this stage?

Guessing we're cool with dropping existing balances at this stage?
zachgoll (Migrated from github.com) reviewed 2024-02-11 04:50:45 +08:00
config/solid_queue.yml
@@ -0,0 +15,4 @@
# <<: *default
#
# production:
# <<: *default
zachgoll (Migrated from github.com) commented 2024-02-11 04:50:45 +08:00
Copy Link

Delete?

Delete?
Shpigford (Migrated from github.com) reviewed 2024-02-11 04:56:40 +08:00
db/migrate/20240209174912_redo_money_storage.rb
@@ -0,0 +7,4 @@
remove_column :accounts, :balance_cents
remove_column :accounts, :balance_currency
remove_column :accounts, :currency
Shpigford (Migrated from github.com) commented 2024-02-11 04:56:40 +08:00
Copy Link

Yeah. Especially since those balance_ columns were associated with that Money gem. Not worth the hassle.

Yeah. Especially since those `balance_` columns were associated with that Money gem. Not worth the hassle.
Shpigford (Migrated from github.com) reviewed 2024-02-11 04:57:46 +08:00
config/solid_queue.yml
@@ -0,0 +15,4 @@
# <<: *default
#
# production:
# <<: *default
Shpigford (Migrated from github.com) commented 2024-02-11 04:57:46 +08:00
Copy Link

It's all just commented out for now. We'll likely customize some of it as we add more background jobs.

It's all just commented out for now. We'll likely customize some of it as we add more background jobs.
Shpigford commented 2024-02-11 04:58:53 +08:00 (Migrated from github.com)
Copy Link

The direction of this makes sense to me (GoodJob looks interesting, nice that we can avoid the extra Redis dependency!). Eventually it might be nice to abstract away the "original" and "converted" concept away from the views and have a account.balance instance method to use, but nothing that can't be improved later!

I agree that I don't loovvvvve having both, but yeah...can abstract once we get more data coming in for things like transactions. That way we don't prematurely abstract things.

> The direction of this makes sense to me (GoodJob looks interesting, nice that we can avoid the extra Redis dependency!). Eventually it might be nice to abstract away the "original" and "converted" concept away from the views and have a `account.balance` instance method to use, but nothing that can't be improved later! I agree that I don't loovvvvve having both, but yeah...can abstract once we get more data coming in for things like transactions. That way we don't prematurely abstract things.
👍 1
Shpigford commented 2024-02-11 06:18:51 +08:00 (Migrated from github.com)
Copy Link

YOLO. Let's do this. 🫣

YOLO. Let's do this. 🫣
🚀 1
sergey-tyan (Migrated from github.com) reviewed 2024-02-11 09:43:30 +08:00
Gemfile
@@ -29,3 +32,3 @@
gem "jbuilder"
gem "tzinfo-data", platforms: %i[ windows jruby ]
gem "money-rails", "~> 1.12"
gem "faraday"
sergey-tyan (Migrated from github.com) commented 2024-02-11 09:43:30 +08:00
Copy Link

could you please explain the reasoning behind not using this gem?

could you please explain the reasoning behind not using this gem?
Shpigford (Migrated from github.com) reviewed 2024-02-11 09:47:28 +08:00
Gemfile
@@ -29,3 +32,3 @@
gem "jbuilder"
gem "tzinfo-data", platforms: %i[ windows jruby ]
gem "money-rails", "~> 1.12"
gem "faraday"
Shpigford (Migrated from github.com) commented 2024-02-11 09:47:28 +08:00
Copy Link

No major benefit to using it.

No major benefit to using it.
jhliberty (Migrated from github.com) reviewed 2024-02-11 13:03:55 +08:00
Gemfile
@@ -29,3 +32,3 @@
gem "jbuilder"
gem "tzinfo-data", platforms: %i[ windows jruby ]
gem "money-rails", "~> 1.12"
gem "faraday"
jhliberty (Migrated from github.com) commented 2024-02-11 13:03:55 +08:00
Copy Link

4d5d35b277/db/migrate/20240206031739_replace_money_field.rb (L3)

This migration fails without it!

https://github.com/maybe-finance/maybe/blob/4d5d35b277ce89002f02b3a63d3380abdcc384e7/db/migrate/20240206031739_replace_money_field.rb#L3 This migration fails without it!
adrienpoly (Migrated from github.com) reviewed 2024-02-12 01:27:15 +08:00
Gemfile
@@ -29,3 +32,3 @@
gem "jbuilder"
gem "tzinfo-data", platforms: %i[ windows jruby ]
gem "money-rails", "~> 1.12"
gem "faraday"
adrienpoly (Migrated from github.com) commented 2024-02-12 01:27:15 +08:00
Copy Link

I had this issue, It should be fixed by #438

I had this issue, It should be fixed by #438
Sign in to join this conversation.
Reviewers
No Reviewers
zachgoll
sergey-tyan
Shpigford
jhliberty
adrienpoly
Labels
Clear labels
📆 Planned
Not ready to be worked on yet, but planned for future
 💻 Self Hosted only
Issues pertaining to self-hosted versions of Maybe
 ⚙️ Config
ℹ️ Needs info
Further information is requested
 📱 Mobile
Bugs exclusively happening on mobile devices
 1️⃣ High Priority
High priority issues will be worked on next
 🚨 Urgent
Issues that are urgent for app functionality
 3️⃣ Low priority
Contributions accepted, but Maybe team will not be working on this in the near term
 2️⃣ Medium Priority
Community contributions accepted, Maybe team only works on if there are no high priority items open
 ⚡ Performance
Performance issues to address
 🎨 UI / UX
Issues related to UI / UX interactions and design.
 💎 Bounty
💰 Rewarded
📊 Data issue
Issues where the app functionally works, but the data is missing or inaccurate.
 🙋 Bounty claim
$1.5K
Community
This is a great issue for community members to work on
 dependencies
Pull requests that update a dependency file
 Design
Hosted only
Hosted app only
 Required self hoster data migration
Self hosters must run data migrations to resolve this issue
 ruby
Pull requests that update Ruby code
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
gavin
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: gavin/maybe#425
Delete Branch "feature/currency"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?