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

Payment PropertyBag - Fix setAmount #17505

Merged
merged 1 commit into from
Jun 5, 2020

Conversation

mattwire
Copy link
Contributor

@mattwire mattwire commented Jun 5, 2020

Overview

This is quite a serious issue as it causes the amount to be set incorrectly on propertybag because the params are the wrong way around.

Before

Params switched so setAmount doesn't set amount correctly.

After

Amount set correctly on propertybag.

Technical Details

Comments

@eileenmcnaughton @artfulrobot Can you check please - we really need to get this in asap (before the 5.27 RC is cut as it causes the amount to be set incorrectly on propertybag.

@civibot
Copy link

civibot bot commented Jun 5, 2020

(Standard links)

@civibot civibot bot added the master label Jun 5, 2020
@mattwire mattwire force-pushed the propertybagfixsetamount branch from 30bc5e2 to 0c34d9b Compare June 5, 2020 09:25
@artfulrobot
Copy link
Contributor

Yes! Let's merge ASAP! I spotted this one when I was to busy/stressed to focus and couldn't remember what it was. Thanks

@seamuslee001 seamuslee001 merged commit 65748fa into civicrm:master Jun 5, 2020
@eileenmcnaughton
Copy link
Contributor

It's not a change in this PR - but what format are we actually returning here? Ideally a non-localised format but I have doubts...

@mattwire mattwire deleted the propertybagfixsetamount branch June 5, 2020 11:40
@mattwire
Copy link
Contributor Author

mattwire commented Jun 5, 2020

@eileenmcnaughton I think it's actually a localized numeric because it's passed through money_format. So we should change that. We should also consider whether we want to store this internally in propertybag as a string or without decimal point (ie. EUR30.12 = 3012) as is recommended when dealing with monetary values.

@eileenmcnaughton
Copy link
Contributor

@mattwire I think we should definitely switch to non-formatted

Re best format - I think we should add a php library & store as a money object per https://lab.civicrm.org/dev/translation/-/issues/48

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

Successfully merging this pull request may close these issues.

4 participants