Integrate money-rails gem #268
Merged
Conversation
This file contains 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
|
Hi @Shpigford Let me know if implementation looks good to proceed further with tests for the code |
|
Thanks @sarvaiyanidhi! Can you expound on what that (Also going to convert this to a draft PR since it's still in progres) |
As mentioned in money-rails gem doc, if you are using another database column name, or you prefer another name for the money attribute, then you can provide an as argument with a string value to the monetize macro. So, as we have a model with an attribute named balance and we have to monetize it with a specific currency, we can use the as option to define the name of the virtual attribute. In this case, the as: :balance_cents option specifies that the virtual attribute representing the monetized balance should be named balance_cents. This means you can access the Money object through balance_cents in our Ruby code:
so, virtual attribute balance_cents representing the monetized version of their original attribute balance |
|
@sarvaiyanidhi Gotcha! Thanks for the explanation! Overall, looking good. |
|
Hi @Shpigford I have integrated basic tests for monetize feature. I was also rethinking on implementation based on your earlier comment on balance and balance_cents attribute which may be confusing in future for others too. There are two options which can be considered to simplify and clear this
Right now we have balance field in database which is Integer datatype and balance_cents virtual attribute is Money type. You can check this article for more reference https://blog.appsignal.com/2023/11/29/keep-your-ruby-code-maintainable-with-money-rails.html You can play around current implementation in this branch by adding balance like 19.99 from frontend which gets stored in database in balance field as integer 1999 and balance_cents as Let me know if you want me to implement changes in database field in new branch for your reference or want to look into it later as we proceed with features. |
|
@zachgoll what are your thoughts on having both the |
I think updating the existing field from |
|
Ultimately we ideally store a single instance of the amount and its currency (really for any instance of money...balance, transaction, etc). Then we can easily convert to other currencies as required for a given user's account. |
|
Right, The suggestion is about renaming the DB table field to match the proper naming convention and avoid future confusion. |
zachgoll
requested changes
Feb 5, 2024
|
This may have a migration problem, I just removed data account data and successfully run but if it is in production it may worth to take a look. @zachgoll |
|
@kiliczsh thanks, will take a look! |
|
@zachgoll has reviewed the problem and promptly opened a PR to address the migration issue. While this action resolves the issue directly, I'm uncertain if it aligns with the project standard procedure. If you prefer to have first a formal issue to outline the problem before proceeding with the solution via a PR, please feel free to ping me and I'll accommodate accordingly |
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.
Claim #265
Closes #265
Have used existing column balance of accounts field but value is stored in cents by using as option in model.
Default currency is set in money initializer file to USD