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

PCHR-3546: Make All Currencies Available #2698

Conversation

backstageel
Copy link
Contributor

Overview

Made all currencies available for new installations

Before

Only few currencies was available on new installations
screenshot from 2018-06-20 18 18 00

After

All Currencies are available on new CiviHR installations by default
screenshot from 2018-06-20 18 12 42

Technical Details

Couldn't find any CiviCRM API to retrieve currencies, so made direct query.

@backstageel backstageel force-pushed the PCHR-3546-Make-All-Currencies-Available branch from 58f5af7 to c46351c Compare June 20, 2018 20:04
@backstageel backstageel requested a review from tunbola June 21, 2018 07:10
@backstageel backstageel force-pushed the PCHR-3546-Make-All-Currencies-Available branch from c46351c to 0272322 Compare June 21, 2018 07:27
'option_group_id' => 'currencies_enabled',
'label' => $dao->name . ' (' . $dao->symbol . ')',
'value' => $dao->name,
'name' => $dao->name,
Copy link
Contributor

Choose a reason for hiding this comment

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

Took a quick look in my local db, seems the enabled currencies have the same value for both name and label.
e.g EUR (€).

'label' => $dao->name . ' (' . $dao->symbol . ')',
'value' => $dao->name,
'name' => $dao->name,
'is_active' => 1,
Copy link
Contributor

@tunbola tunbola Jun 21, 2018

Choose a reason for hiding this comment

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

I don't think we need to set is_active, is_reserved, is_optgroup and is_default, The values set for them are the same they have as the default database values.

while ($dao->fetch()) {
$currencyExists = civicrm_api3('OptionValue', 'get', [
'value' => $dao->name,
]);
Copy link
Contributor

Choose a reason for hiding this comment

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

You need to add the option_group_id to the parameter to fetch the option value. In Civi it is possible for an option value with same names to exist and they will belong to different option groups. So for example we can have GBP belonging as an option value to another option group.

*/
private function makeAllCurrenciesAvailable() {
$dao = CRM_Core_DAO::executeQuery('SELECT * from civicrm_currency');
while ($dao->fetch()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Also since the currency records might be up to 200(on my local installation), wondering if we could optimize this by using a single API call to get all enable currencies names.

$result = civicrm_api3('OptionValue', 'get', array(
  'sequential' => 1,
  'return' => array("name"),
  'option_group_id' => "currencies_enabled",
));

Then in this loop, we can check if

if(!in_array($dao->name, $enabledCurrency){
// create the currency
}

@@ -194,20 +194,18 @@ private function createDefaultRelationshipTypes() {
*/
private function makeAllCurrenciesAvailable() {
$dao = CRM_Core_DAO::executeQuery('SELECT * from civicrm_currency');
$result = civicrm_api3('OptionValue', 'get', array(
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use short array syntax and single quote for strings.

'return' => array("name"),
'option_group_id' => "currencies_enabled",
));
$enabledCurrency = array_column($result['values'],'name');
Copy link
Contributor

Choose a reason for hiding this comment

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

I think enabledCurrencies might be a better name since there will most likely be more than one.

* Making All Currencies Available for new installations
*/
private function makeAllCurrenciesAvailable() {
$dao = CRM_Core_DAO::executeQuery('SELECT * from civicrm_currency');
Copy link
Contributor

Choose a reason for hiding this comment

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

Please move this line to just before the while loop.
You can also add a space before that so the code is not cluttered together

@backstageel backstageel force-pushed the PCHR-3546-Make-All-Currencies-Available branch from 1c68dc6 to 772a8ea Compare June 21, 2018 11:54
@backstageel backstageel force-pushed the PCHR-3546-Make-All-Currencies-Available branch from 772a8ea to cb193f0 Compare June 21, 2018 11:57
@backstageel backstageel force-pushed the PCHR-3536-Display-Preferences branch from 0e4fc54 to 748cc72 Compare June 21, 2018 14:23
@backstageel backstageel force-pushed the PCHR-3546-Make-All-Currencies-Available branch from cb193f0 to 34bd51f Compare June 21, 2018 14:30
@backstageel backstageel force-pushed the PCHR-3536-Display-Preferences branch from 748cc72 to 45176dd Compare June 21, 2018 14:54
@backstageel backstageel force-pushed the PCHR-3546-Make-All-Currencies-Available branch from 34bd51f to 0c8ded2 Compare June 21, 2018 14:56
@backstageel backstageel merged commit ead88fa into PCHR-3536-Display-Preferences Jun 21, 2018
@backstageel backstageel deleted the PCHR-3546-Make-All-Currencies-Available branch June 21, 2018 15:23
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.

2 participants