Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

have to_json and as_json return amount and currency json #138

Closed
wants to merge 2 commits into from

Conversation

elfassy
Copy link
Contributor

@elfassy elfassy commented Dec 14, 2017

Why

Fixes #76

What

  • as_json should return a hash of attributes (amount, currency) to be serialized, not a string
  • from_json is missing to assign attributes from a JSON object

@elfassy elfassy requested a review from sunblaze January 29, 2018 18:07
@elfassy elfassy closed this Jan 29, 2018
@elfassy elfassy reopened this Jan 29, 2018
Copy link
Contributor

@bdewater bdewater left a comment

Choose a reason for hiding this comment

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

LGTM, just move some lines around to conform to the ActiveSupport interface as described in the issue.

end

def as_json(*args)
to_s
to_json
Copy link
Contributor

Choose a reason for hiding this comment

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

This should have the actual implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was always wrong too, as_json needs to return a hash, not a string... fixed now

lib/money/money.rb Outdated Show resolved Hide resolved
Copy link

@sunblaze sunblaze left a comment

Choose a reason for hiding this comment

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

💯

@elfassy
Copy link
Contributor Author

elfassy commented Feb 8, 2018

DO NOT MERGE... until background jobs can handle the new format

@elfassy
Copy link
Contributor Author

elfassy commented Jun 4, 2018

this is ready to merge now since https://github.com/Shopify/shopify/pull/151350
@bdewater @sunblaze let me know if you see any other objections. Will run tests in core before merging

def as_json(*args)
to_s
def as_json(*_args)
{ value: to_s(:amount), currency: currency.to_s }
Copy link

Choose a reason for hiding this comment

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

Should we prefer currency.iso_code here instead to be a bit more clear and future proof if we want to_s to be more human readable?

Copy link
Contributor Author

@elfassy elfassy Jun 4, 2018

Choose a reason for hiding this comment

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

the main difference is with a NullCurrency, to_s will return an empty string, vs iso_code will return XXX. I'd rather keep it the way it is for now since the generated JSON might be sent to a third party which does not understand the valid "no currency" iso code.

@elfassy
Copy link
Contributor Author

elfassy commented Jun 4, 2018

@sunblaze
Copy link

sunblaze commented Jun 6, 2018

😮

@elfassy
Copy link
Contributor Author

elfassy commented Jun 21, 2021

this is the new default behaviour in v1.0, with an option to fallback to the legacy behaviour

@elfassy elfassy closed this Jun 21, 2021
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.

to_json, as_json and from_json should behave correctly
3 participants