CRM-20923 refactor tpl to be based on metadata not hard-coding #10704

eileenmcnaughton · 2017-07-20T01:06:42Z

CRM-20923: Refactor tpl for admin mail so it respects metadata

omarabuhussein · 2017-07-20T10:02:28Z

CRM/Admin/Form/Setting.php

@@ -78,10 +78,11 @@ public function setDefaultValues() {
   * Build the form object.
   */
  public function buildQuickForm() {
-    $session = CRM_Core_Session::singleton();
-    $session->pushUserContext(CRM_Utils_System::url('civicrm/admin', 'reset=1'));
+


no need for this space

omarabuhussein · 2017-07-20T10:12:16Z

CRM/Admin/Form/Setting.php

    $args = func_get_args();
-    $check = reset($args);
+    // does this do anything?
+    reset($args);


// does this do anything?

no it doesn't , $args function is not used at all so lines from 83 to 85 can be safely removed .

omarabuhussein · 2017-07-20T10:18:26Z

CRM/Admin/Form/Setting.php

-    foreach ($this->_settings as $setting => $group) {
-      $settingMetaData = civicrm_api('setting', 'getfields', array('version' => 3, 'name' => $setting));
-      $props = $settingMetaData['values'][$setting];
+    // Getting all once seems better than multiple calls.


this comment doesn't make sense for someone reading the code without looking at the commit history ... the code comments should describe the current version of the code not how it was before and how it get changed... this comment should be removed or replaced with something related .. but I prefer removing it.

omarabuhussein · 2017-07-20T13:01:45Z

CRM/Admin/Form/Setting.php

-      $settingMetaData = civicrm_api('setting', 'getfields', array('version' => 3, 'name' => $setting));
-      $props = $settingMetaData['values'][$setting];
+    // Getting all once seems better than multiple calls.
+    $allSettingMetaData = civicrm_api3('setting', 'getfields', array());


probably better for this to move to seperate method :

```php $settingMetaData = $this->getSettingsMetaData(); ```php /** * Docblock goes here */ private function getSettingsMetaData() { $allSettingsMetaData = civicrm_api3('setting', 'getfields', array()); $settingMetaData = array_intersect_key($allSettingMetaData['values'], $this->_settings); // re-ordering the settings to follow $this->_settings order. $settingMetaData = array_merge($this->_settings, $settingMetaData); return $settingMetaData ; }

omarabuhussein · 2017-07-20T13:02:37Z

CRM/Admin/Form/Setting.php

+    $settingMetaData = array_intersect_key($allSettingMetaData['values'], $this->_settings);
+    // This array_merge re-orders to the key order of $this->_settings.
+    $settingMetaData = array_merge($this->_settings, $settingMetaData);
+


better to remove the space since the variables above used in the loop so it make more sense for them to be right before the loop , whole thing would be :

$descriptions = array(); $settingMetaData = $this->getSettingsMetaData(); foreach ($settingMetaData as $setting => $props) { //etc

omarabuhussein · 2017-07-20T13:05:54Z

CRM/Admin/Form/Setting.php

+    // This array_merge re-orders to the key order of $this->_settings.
+    $settingMetaData = array_merge($this->_settings, $settingMetaData);
+
+    foreach ($settingMetaData as $setting => $props) {


actually would be nicer (I think( if you moved the whole loop to another separate private method such buildingSettingFields($settingMetaData) or something like that .. it would make the could much easier to understand

omarabuhussein · 2017-07-20T13:07:12Z

CRM/Admin/Form/Setting.php

    $this->assign('setting_descriptions', $descriptions);
+    $this->assign('settings', $settingMetaData);


isn't settingFields would be more descriptive name instead of just settings ?

omarabuhussein · 2017-07-20T13:11:43Z

@eileenmcnaughton generally these modifications look fine to me , though left view notes about the code itself ... also can you provide more details in the PR description + ticket to make sure that anyone read it understand what it about without the need to look into the code .

Change-Id: Iee2a7aa9fa93c5d20b95f3dbc45ec2f3cf7a216b

eileenmcnaughton · 2017-07-20T23:30:32Z

I've made the changes you suggested with the exception of the second function extraction. My hesitation there is I really want to get the whole $descriptions array out of that loop & those adhoc rules & feel like extracting makes it easier to ignore those issues next pass through

eileenmcnaughton · 2017-07-21T02:48:02Z

@colemanw -are you able to merge this?

monishdeb · 2017-07-21T08:53:32Z

Hmm encountered some style issue.. working on it

eileenmcnaughton · 2017-07-21T08:54:44Z

you mean the green tick doesn't mean it passed?

monishdeb · 2017-07-21T09:17:30Z

@eileenmcnaughton there was extra { on SettingForm.tpl and other minor changes. I have updated the PR and works fine as expected.

monishdeb · 2017-07-21T11:02:31Z

Tested, working fine

eileenmcnaughton · 2017-07-21T21:56:32Z

Thanks @monishdeb - clearly a mistake I made in the second round when I decided to move the code into a sharable tpl ! I thought I did check it but obviously not very well!

civicrm-builder added the master label Jul 20, 2017

eileenmcnaughton mentioned this pull request Jul 20, 2017

CRM-20905: Make (max emails kill switch) a configuration instead of hardcoded value #10693

Closed

eileenmcnaughton force-pushed the admin_form branch from 32bb8e4 to 8079d54 Compare July 20, 2017 04:39

omarabuhussein suggested changes Jul 20, 2017

View reviewed changes

CRM-20923 refactor tpl to be based on metadata not hard-coding

089360b

Change-Id: Iee2a7aa9fa93c5d20b95f3dbc45ec2f3cf7a216b

eileenmcnaughton force-pushed the admin_form branch from 8079d54 to 089360b Compare July 20, 2017 23:24

style fix

ded0c21

monishdeb added the merge on pass label Jul 21, 2017

monishdeb merged commit fabcf30 into civicrm:master Jul 21, 2017

monishdeb deleted the admin_form branch July 21, 2017 11:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CRM-20923 refactor tpl to be based on metadata not hard-coding #10704

CRM-20923 refactor tpl to be based on metadata not hard-coding #10704

eileenmcnaughton commented Jul 20, 2017 •

edited by civicrm-builder

Loading

omarabuhussein Jul 20, 2017

omarabuhussein Jul 20, 2017

omarabuhussein Jul 20, 2017

omarabuhussein Jul 20, 2017

omarabuhussein Jul 20, 2017

omarabuhussein Jul 20, 2017

omarabuhussein Jul 20, 2017

omarabuhussein commented Jul 20, 2017 •

edited

Loading

eileenmcnaughton commented Jul 20, 2017

eileenmcnaughton commented Jul 21, 2017

monishdeb commented Jul 21, 2017

eileenmcnaughton commented Jul 21, 2017

monishdeb commented Jul 21, 2017 •

edited

Loading

monishdeb commented Jul 21, 2017

eileenmcnaughton commented Jul 21, 2017

		$this->assign('setting_descriptions', $descriptions);
		$this->assign('settings', $settingMetaData);

CRM-20923 refactor tpl to be based on metadata not hard-coding #10704

CRM-20923 refactor tpl to be based on metadata not hard-coding #10704

Conversation

eileenmcnaughton commented Jul 20, 2017 • edited by civicrm-builder Loading

omarabuhussein Jul 20, 2017

Choose a reason for hiding this comment

omarabuhussein Jul 20, 2017

Choose a reason for hiding this comment

omarabuhussein Jul 20, 2017

Choose a reason for hiding this comment

omarabuhussein Jul 20, 2017

Choose a reason for hiding this comment

omarabuhussein Jul 20, 2017

Choose a reason for hiding this comment

omarabuhussein Jul 20, 2017

Choose a reason for hiding this comment

omarabuhussein Jul 20, 2017

Choose a reason for hiding this comment

omarabuhussein commented Jul 20, 2017 • edited Loading

eileenmcnaughton commented Jul 20, 2017

eileenmcnaughton commented Jul 21, 2017

monishdeb commented Jul 21, 2017

eileenmcnaughton commented Jul 21, 2017

monishdeb commented Jul 21, 2017 • edited Loading

monishdeb commented Jul 21, 2017

eileenmcnaughton commented Jul 21, 2017

eileenmcnaughton commented Jul 20, 2017 •

edited by civicrm-builder

Loading

omarabuhussein commented Jul 20, 2017 •

edited

Loading

monishdeb commented Jul 21, 2017 •

edited

Loading