CRM-16395 Installation with localized default settings (more generic implementation) #8538

samuelsov · 2016-06-08T20:26:48Z

New patch for CRM-16395 that replace #7322
Done the new way as proposed by @totten with SystemInstallEvent event.
Next step is to create the l10n/LANG/settings.default.json for each language in l10n

To test, copy the file provided settings.default.json.txt and copy it as l10n/YOUR_LOCALE/settings.default.json
and change the values as needed before starting the installation. In the installation process, choose the locale corresponding to the file.

CRM-16395: Installation with localized default settings

JoeMurray · 2016-06-20T16:51:27Z

@omarabuhussein would you be interested in reviewing this PR?

JoeMurray · 2016-06-20T22:15:51Z

@jlillyreed would you able to look at this PR as well?

omarabuhussein · 2016-06-20T22:24:24Z

@JoeMurray yes sure ... I will make my best to review it by tomorrow

JoeMurray · 2016-06-21T02:34:14Z

@omarabuhussein Wonderful! Thanks!!

omarabuhussein · 2016-06-25T16:02:04Z

Civi/Core/LocalizationInitializer.php

+        // disable every language that is not in the given list
+        $params['is_active'] = 0;
+      }
+      civicrm_api3('OptionValue', 'create', $params);


The logic in this method is heavy on the database , you're calling the database through this API method maybe more than 100 times , I would disable all languages first with a single API call (or a database call => using executeQuery) , and then loop through the languages from the configuration file and enable them one by one . (I believe users will not define more than two to three languages in the configuration file ) .

By the way ,, If we have the freedom to use PDO or mysqli direclty .. this logic could be replaced with a single SQL query :

UPDATE civicrm_option_value cov LEFT JOIN civicrm_option_group cog ON cov.option_group_id = cog.id SET cov.is_active = CASE WHEN cov.name IN (Langagues_list) THEN 1 ELSE 0 END WHERE cog.name = 'languages' ;

For me, the performance is not really a concern in this context because it's done only once when CiviCRM is installed. I prefer avoid direct queries when i can. I will see if we can inverse the logic by disabling all first as you suggest.

I don't know what others may think ... but I believe that we should care about installation speed because every time someone add some heavy stuff like this we will end up with installation take very long time... also put into your account that some people may install civicrm on shared servers with limited plans so this would affect them ... ( but maybe I am wrong anyway :) )

Ideally, the API would support batch operations. Perhaps a short-term compromise would be to add a BAO helper function?

class CRM_Core_BAO_OptionGroup { /** * Set the given values to active, and set all other values to inactive. * * @param string $optionGroupName * e.g "languages" * @param array<string> $activeValues * e.g. array("en_CA","fr_CA") */ public static function setActiveValues($optionGroupName, $activeValues) { CRM_Core_DAO::executeQuery( /* ...Omar's suggested query, with escaping for $optionGroupName and $activeValues... */ ); } }

Ok, i have done as suggested by totten.

omarabuhussein · 2016-06-25T16:49:42Z

Hey @samuelsov ... Sorry for being late in reviewing this I was a busy in the previous days ... I left some notes , I think the first one is the most important where the other two are minor ...

samuelsov · 2016-06-27T13:36:46Z

@omarabuhussein thanks for the review ! i will comment each of your note.

totten · 2016-06-27T22:39:38Z

Civi/Core/LocalizationInitializer.php

+        if (isset($settings['defaultCurrency'])) {
+          self::updateCurrencies(array($settings['defaultCurrency']), $settings['defaultCurrency']);
+        }
+


It seems like we're mapping 'settings.default.json into the Settings, but with exceptions. Taking a new look at this, I'm wondering about those exceptions.

If setting languageLimit during installation is a bad idea, then wouldn't we just omit it from settings.default.json? (There's probably 100 settings which are irrelevant/bad-ideas for a localization package... but we don't need to blacklist all of them.) Any special reason to blacklist this one?

For languageOptions, this seems to be preventing you from configuring other languages. (For example, in the screen Administer => Localization => Languages, wouldn't it hide a number of options?) Wouldn't this make it harder to setup multilingual? (Example: An NGO in France installs with the French language-pack, but it has branch-offices in Moracco, so it wants to enable multilingual with Arabic. But Arabic was disabled by the French language-pack.)

Is updateCurrencies() something unique to installation process... or would it apply anytime you try to change defaultCurrency? If should apply anytime you update defaultCurrency, then perhaps it should be triggered this way:

diff --git a/settings/Localization.setting.php b/settings/Localization.setting.php index e375d92..3b7eac6 100644 --- a/settings/Localization.setting.php +++ b/settings/Localization.setting.php @@ -142,6 +142,9 @@ return array( 'defaultCurrency' => array( 'group_name' => 'Localization Preferences', 'group' => 'localization', 'name' => 'defaultCurrency', 'type' => 'String', 'quick_form_type' => 'Select', 'html_type' => 'Select', 'html_attributes' => array( 'class' => 'crm-select2', ), 'default' => 'USD', 'add' => '4.3', 'title' => 'Default Currency', 'is_domain' => 1, 'is_contact' => 0, 'description' => 'Default currency assigned to contributions and other monetary transactions.', 'help_text' => NULL, 'pseudoconstant' => array( 'callback' => 'CRM_Admin_Form_Setting_Localization::getCurrencySymbols', ), + 'on_change' => array( + 'CRM_Foo_Bar::onChangeDefaultCurrency', + ), ),

@totten I'm looking at your comments again and :
2. You are right... disabling preferred languages prevents user from selecting them in the localization admin page. I think we should always have the full list of localizable languages in admin/settings/localization. Also, it might be a good idea to ensure the available languages are always in preferred language (using the on_change event you mentioned).
3. Very nice. I have done as you have proposed and it fixes an inconsistency between the UI and the api at the same time - i.e. ensure the default currency is always in the enabled currencies.

Oh, no, you can enable language that are not in the list of available preferred languages. But the label is lost in the admin localization setting page - so you get es_ES instead of Spanish and it gets to the bottom of the list. I have fixed this bug.

Except that there is no settings for currency_enabled. I have to manually initialize to the same as default language. The logic applies only in the installation process. So i guess i need the code after all. To avoid duplication of code, I have moved it to CRM_Admin_Form_Setting_Localization. We might want to have an simple api call for updating currency_enabled but it's not in the scope of this issue.

JoeMurray · 2016-07-06T20:31:52Z

Heh guys, what is the status of this PR? Could we get it ready for review in a week as part of 4.7.10? Currently it's not on the list of i18n / l10n issues https://issues.civicrm.org/jira/secure/RapidBoard.jspa?rapidView=28&view=planning

Also, would you be able to help out in that group (or another one) this coming month? If so, please signup in https://docs.google.com/document/d/1UAFOy0gXar_ouzWgOpCdbMA2r6_sIyO_KPiZey9jVDE/edit (we haven't figured out how to do that in JIRA yet).

samuelsov · 2016-07-15T13:33:03Z

@JoeMurray : Yes i should be able to work on this next week.

@totten : regarding your comments:

in fact, it might be a good idea to add support for multilingual but if we do, we have to do more than just changing the setting i believe. Most of the settings are harmless and they only change value in the database but multilingual can break the database if not done correctly. That's why i just make an exception for it until i feel confident enough to support it.
For languageOptions, the goal is to disable choices for the preferred language (http://dmaster.demo.civicrm.org/civicrm/admin/options?gid=79&reset=1). It won't impact the choices for locale or multilingual language. I will double check but i'm pretty sure those two are unrelated.
Sure, i have taken some code from the admin setting page but yes, it should definitively be better like that. And i think this method can be applied to multilingual languageLimit if it's not the case. So maybe it could solve the 1.

colemanw · 2016-09-30T23:03:57Z

@samuelsov can you give an update on this please?

samuelsov · 2016-10-04T20:21:45Z

@colemanw I need to rework the patch and i haven't got the time to do so yet, unfortunately. I might be able to do so next week.

…stall event context

…nactive languages display in i18n settings form

samuelsov · 2016-10-13T21:01:03Z

I still need to work on @omarabuhussein performance concern...

samuelsov · 2016-10-14T01:54:38Z

All right. I think the PR is ready for another review / testing.

seamuslee001 · 2016-10-14T02:58:07Z

CRM/Admin/Form/Setting/Localization.php

+    // get labels for all the currencies
+    $options = array();
+
+    $currencySymbols = \CRM_Admin_Form_Setting_Localization::getCurrencySymbols();


not sure the \ is needed here

yes, an unfortunate copy/paste

seamuslee001 · 2016-10-14T02:58:13Z

CRM/Admin/Form/Setting/Localization.php

+    }
+
+    $dontCare = NULL;
+    \CRM_Core_OptionGroup::createAssoc('currencies_enabled', $options, $dontCare);


samuelsov · 2016-10-14T15:11:05Z

@seamuslee001 it's fixed and i have removed some unnecessary duplicated code.

eileenmcnaughton · 2016-10-14T21:15:24Z

@omarabuhussein @seamuslee001 looks like a lot of work has gone into this commit - including on the review side. Do you feel it's ready to merge?

mlutfy · 2017-06-16T15:42:38Z

new PR in #10517

civicrm-builder added the master label Jun 8, 2016

samuelsov mentioned this pull request Jun 8, 2016

(WIP) CRM-16395 - Installation with localized default settings #7322

Closed

mlutfy changed the title ~~CRM 16395 generic~~ CRM-16395 Installation with localized default settings (more generic implementation) Jun 8, 2016

omarabuhussein reviewed Jun 25, 2016
View reviewed changes

totten reviewed Jun 27, 2016
View reviewed changes

JoeMurray mentioned this pull request Jul 7, 2016

Invitation: CiviCRM 4.7.10 release August 2016 civicrm/release-management#4

Closed

samuelsov added 7 commits October 13, 2016 10:06

CRM-16395 - WIP with generic installer

930ef8a

CRM-16395 doesn't seems to be possible to have the seedLanguage in In…

17cbdd5

…stall event context

CRM-16395 it now works + extra languages and currencies initialization

2ed3bdd

CRM-16395 remove settings.json - was there for dev purpose only

b9e491e

CRM-16395 revert change from CRM/Core/Config.php

b69560f

Checkstyle and documentation

926e58e

CRM-16395 Adding generic correction for enabled currencies + fixing i…

5affa15

…nactive languages display in i18n settings form

samuelsov force-pushed the CRM-16395-generic branch from 5c0d57a to 5affa15 Compare October 13, 2016 20:02

CRM-16395 civilint

4a851ea

CRM-16395 better performance for languages initialization

5ac92b2

seamuslee001 reviewed Oct 14, 2016

View reviewed changes

samuelsov added 2 commits October 14, 2016 09:48

CRM-16395 faulty copy/paste syntax

fb42a73

CRM-16395 remove duplicate code

2d6dbdb

mlutfy mentioned this pull request Jun 16, 2017

CRM-16395: Installation with localized default settings (3) #10517

Closed

mlutfy closed this Jun 16, 2017

mlutfy mentioned this pull request Jun 16, 2017

CRM-16395: Installation with localized default settings (4) #10518

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CRM-16395 Installation with localized default settings (more generic implementation) #8538

CRM-16395 Installation with localized default settings (more generic implementation) #8538

samuelsov commented Jun 8, 2016 •

edited by civicrm-builder

Loading

JoeMurray commented Jun 20, 2016

JoeMurray commented Jun 20, 2016

omarabuhussein commented Jun 20, 2016

JoeMurray commented Jun 21, 2016

omarabuhussein Jun 25, 2016 •

edited

Loading

samuelsov Jun 27, 2016

omarabuhussein Jun 27, 2016

totten Jun 27, 2016

samuelsov Oct 14, 2016

omarabuhussein commented Jun 25, 2016 •

edited

Loading

samuelsov commented Jun 27, 2016

totten Jun 27, 2016

samuelsov Oct 13, 2016

samuelsov Oct 13, 2016

JoeMurray commented Jul 6, 2016

samuelsov commented Jul 15, 2016

colemanw commented Sep 30, 2016

samuelsov commented Oct 4, 2016

samuelsov commented Oct 13, 2016

samuelsov commented Oct 14, 2016

seamuslee001 Oct 14, 2016

samuelsov Oct 14, 2016

seamuslee001 Oct 14, 2016

samuelsov commented Oct 14, 2016

eileenmcnaughton commented Oct 14, 2016

mlutfy commented Jun 16, 2017

CRM-16395 Installation with localized default settings (more generic implementation) #8538

CRM-16395 Installation with localized default settings (more generic implementation) #8538

Conversation

samuelsov commented Jun 8, 2016 • edited by civicrm-builder Loading

JoeMurray commented Jun 20, 2016

JoeMurray commented Jun 20, 2016

omarabuhussein commented Jun 20, 2016

JoeMurray commented Jun 21, 2016

omarabuhussein Jun 25, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omarabuhussein commented Jun 25, 2016 • edited Loading

samuelsov commented Jun 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoeMurray commented Jul 6, 2016

samuelsov commented Jul 15, 2016

colemanw commented Sep 30, 2016

samuelsov commented Oct 4, 2016

samuelsov commented Oct 13, 2016

samuelsov commented Oct 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samuelsov commented Oct 14, 2016

eileenmcnaughton commented Oct 14, 2016

mlutfy commented Jun 16, 2017

samuelsov commented Jun 8, 2016 •

edited by civicrm-builder

Loading

omarabuhussein Jun 25, 2016 •

edited

Loading

omarabuhussein commented Jun 25, 2016 •

edited

Loading