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

CRM-21664 Modified--getting currencies from civicrm_currency table instead bein… #11533

Merged
merged 1 commit into from
Jan 16, 2018

Conversation

sunnycs121
Copy link
Contributor

@sunnycs121 sunnycs121 commented Jan 16, 2018

Modified file CRM/Core/PseudoConstant.php -- getting currencies from civicrm_currency table instead being hard-coded. CRM-21664

Before

Currencies were hardcoded and were missing some currencies like Serbian Dinar, code: RSD and when any contribution with that currency was tried to be created it resulted in error:
"error_message": "Currency not a valid code: "

After

All currencies in modification are being pulled from database table civicrm_currency instead of hardcoded.

Comments

Test by creating a contribution through API call mentioned in Jira issue.

@civicrm-builder
Copy link

Can one of the admins verify this patch?

@seamuslee001
Copy link
Contributor

Jenkins ok to test

@seamuslee001
Copy link
Contributor

This looks sane to me. I haven't tested this yet but seems sensible ping @monishdeb @eileenmcnaughton

@eileenmcnaughton eileenmcnaughton changed the title Modified--getting currencies from civicrm_currency table instead bein… CRM-21664 Modified--getting currencies from civicrm_currency table instead bein… Jan 16, 2018
@eileenmcnaughton
Copy link
Contributor

OK I agree with this - there are at least 4 different currencies used in the test suite so I think we are covered on it

@seamuslee001
Copy link
Contributor

@eileenmcnaughton @monishdeb Tests have passed now and I agree with Eileen that we probably have enough unit tests already to cover this

@eileenmcnaughton eileenmcnaughton merged commit 1d93a2d into civicrm:master Jan 16, 2018
@eileenmcnaughton
Copy link
Contributor

Merged - I think this was your first PR merged?

@sunnycs121
Copy link
Contributor Author

@eileenmcnaughton Yes this was my first one.

@eileenmcnaughton
Copy link
Contributor

Well done then - we need a pretty badge or something :-)

@mlutfy mlutfy added this to the 4.7.31 milestone Feb 9, 2018
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.

5 participants