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

dev/financial#184 Fix currency name for Ghana and Belarus #21751

Merged
merged 1 commit into from
Dec 1, 2021

Conversation

mlutfy
Copy link
Member

@mlutfy mlutfy commented Oct 6, 2021

Overview

https://lab.civicrm.org/dev/financial/-/issues/184

To reproduce:

  • Go to dmaster
  • Admin > Localisation > Language etc
  • Set the default currency to "GHC"

Result:

Brick\Money\Exception\UnknownCurrencyException: Unknown currency code: GHC in Brick\Money\Exception\UnknownCurrencyException::unknownCurrency() (line 19 of /srv/buildkit/build/dmaster/web/sites/all/modules/civicrm/vendor/brick/money/src/Exception/UnknownCurrencyException.php).

The ISO code for Ghana is "GHS", which is what Brick/Money uses.

https://en.wikipedia.org/wiki/Ghanaian_cedi

Technical Details

The problem surfaced when upgrading to the Brick/Money library. The old values were not technically wrong, in the year when CiviCRM was first created.

  • Ghana adopted the Second cedi (GHC) in 1967 until 2007, and then it was replaced by the Third cedi (GHS) after "rampant inflation" (according to Wikipedia).
  • Belarus has been using BYN since 2016, replacing the BYR.

The proper thing to do would have been to add a new currency and phase out the old one, but in this specific case, I think it's safe to say that most CiviCRM installations, if they are using those currencies, probably have already converted their accounting to the newer currencies.

I wrote a generic-ish function, knowing it might not be used very often, but I figure that my clients are pretty good at using currencies that not many people are using, so if we find any, I will re-use that function. In the situations I encountered, they did not have any financial transactions in the CRM.

@civibot
Copy link

civibot bot commented Oct 6, 2021

(Standard links)

@civibot civibot bot added the master label Oct 6, 2021
@mlutfy
Copy link
Member Author

mlutfy commented Oct 10, 2021

rebased

@totten
Copy link
Member

totten commented Oct 11, 2021

I did some r-run on the upgrader (eg v5.42 => master=patch) and confirmed that the civicrm_currency table got the update. ✔️

But one thing seems off to me -- updateCurrencyName updates 4 tables, converting existing records (eg ContributionPages) to use the new names (eg BYR=>BYN). However, when I grep xml/schema for foreign-keys to civicrm_currency, I find this list:

xml/schema/Contribute/ContributionPage.xml
xml/schema/Contribute/ContributionRecur.xml
xml/schema/Contribute/ContributionSoft.xml
xml/schema/Contribute/Contribution.xml
xml/schema/Contribute/Product.xml
xml/schema/Event/Event.xml
xml/schema/Event/Participant.xml
xml/schema/Financial/FinancialItem.xml
xml/schema/Financial/FinancialTrxn.xml
xml/schema/Grant/Grant.xml
xml/schema/PCP/PCP.xml
xml/schema/Pledge/PledgePayment.xml
xml/schema/Pledge/Pledge.xml

The list is long, but I think all of those tables store the currency-symbol. So doesn't it need to rename the symbol on all of those tables?

@totten
Copy link
Member

totten commented Oct 11, 2021

Or are these actually different currencies with different valuations? (From the description, it sounds like Ghana actually issued a new currency, and Belarus simply changed the symbol.)

I guess the difference is this:

  • If it's a change symbol (which sounds like the Belarus case), then I guess you'd want to search/replace all the records to provide consistent migration?
  • If it's a distinct currency (where the basis of its value changed dramatically), then I guess you'd want to keep records of both (new+old) currencies - and merely deprecate new usages of the old one?

@totten
Copy link
Member

totten commented Oct 11, 2021

Alternatively, if it's something quite subjective - then maybe it's fair to take a best-guess at what transition will be needed most often, and add an upgrade-message to alert anyone who might have a funny case.

@mlutfy
Copy link
Member Author

mlutfy commented Oct 11, 2021

@totten Thanks for that grep & r-run. I added those tables to the update function.

And agree on the subjectivity - considering those currencies have been in circulation since 2007 (Ghana) and 2016 (Belarus), I think it's safe to assume that CiviCRM installations have already converted, and that those who were using the old currencies were doing so assuming it was the new currency. Otherwise I would have suggested to introduce a new currency and phase out the old one.

It's also a slight concern to keep in mind with the Brick/Money library: they do not keep currencies that have been removed from circulation.

@mlutfy
Copy link
Member Author

mlutfy commented Oct 11, 2021

(I squashed the commits because I did a mistake in an amend and the c-i was not able to apply the patch)

@yashodha
Copy link
Contributor

@mlutfy

The column is inconsistently named fee_currency for civicrm_participant. So this fails
UPDATE civicrm_participant SET currency = 'GHS' WHERE currency = 'GHC' [nativecode=1054 ** Unknown column 'currency' in 'where clause']

@mlutfy
Copy link
Member Author

mlutfy commented Oct 19, 2021

@yashodha Thanks! Should be fixed.

@eileenmcnaughton
Copy link
Contributor

@mlutfy it looks like you have addressed feedback - but in the meantime 5.43 was cut so you need to move the upgrade to 5.44

@colemanw
Copy link
Member

@mlutfy actually now it's 5.45. If the feedback is all addressed then we can go ahead and merge this once it's rebased onto the latest version.

@yashodha
Copy link
Contributor

@mlutfy looks good, can be merged after upgrade version fixed to 5.45.

@mlutfy
Copy link
Member Author

mlutfy commented Dec 1, 2021

Rebased on master, and moved the upgrade to FiveFortyFive.php

@eileenmcnaughton
Copy link
Contributor

test fail is unrelated - failing elsewhere=, date based

@eileenmcnaughton eileenmcnaughton merged commit 5f126d4 into civicrm:master Dec 1, 2021
@eileenmcnaughton eileenmcnaughton deleted the fixCurrencies branch December 1, 2021 19:37
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.

5 participants