PCHR-3544: Hide Fields for Contact Summary

[ajesamson]

Overview

Some contact summary fields are visible by default. This PR changes the default for Suffix, Instant messenger and Social Accounts to un-check so they are hidden from users by default

Before

After

Technical Details

Before un-checking the fields, it was ensured that no record previously existed relating to them.

private function up1022_getUnusedContactEditOptionNames() {
    $names = [];
    
    foreach (['Contact', 'IM', 'Website'] as $name) {
      $params = ['return' => ['id']];
      if ($name === 'Contact') {
        $params['suffix_id'] = ['IS NOT NULL' => 1];
      }
      
      $result = civicrm_api3($name, 'get', $params);
      if ($result['count'] == 0) {
        array_push($names, $name === 'Contact' ? 'Suffix' : $name);
      }
    }
    
    return $names;
}

They are also compared against previously activated fields. This implies that they are ignored if not previously checked.

public function upgrade_1022() {
    $valuesRemoved = $this->up1022_getContactEditOptionValues();
    $optionValues = $this->up1022_getActiveContactEditOptionValues();
    
    $activeOptionValues = array_diff($optionValues, $valuesRemoved);
    civicrm_api3('Setting', 'create', [
      'contact_edit_options' => $activeOptionValues,
    ]);
    
    return TRUE;
  }

[mickadoo]

Looks good overall, some minor changes

[mickadoo]

I know it's quite confusing with OptionValue.value but maybe we could make the naming a bit clearer here?

In the end we want to do a diff of "values to remove" and "active values" and only remove those that we want to that are not active, right? $valuesRemoved sounds like they have already been removed, and $optionValues is a bit vague about which option values they are.

What do you think about renaming $valuesRemoved to $valuesToRemove and up1022_getContactEditOptionValues to up1022_getContactEditOptionValuesToRemove?

And then renaming $optionValues to $activeOptionValues. We could then keep the variable $valuesToRemove and reassign it with the result of the diff, because really these are the "values we will remove"

[ajesamson]

Noted thanks.

[mickadoo]

It's just a personal preference, but I like removing sequential and using array_shift on the result instead, what do you think?

[ajesamson]

Worth exploring. My main idea behind sequential is using it whenever i do not care about iterating over entity id. By not using sequential, results will be indexed their ids and not array index.

[mickadoo]

Missing @return

[ajesamson]

Fixed thanks

[mickadoo]

Is sequential required here?

[ajesamson]

By using sequential, i did not have to worry myself with the key in my foreach

foreach ($optionValues['values'] as $optionValue) {

Like you said, i think its just preference.

[mickadoo]

We can use === here

[mickadoo]

=== should be fine here too

[mickadoo]

Just a general question, if this upgrader is run twice will the result be the same? That's what we aim for with upgraders. I think this one looks fine, but just asking

[ajesamson]

As a benefit of doubt, i tested with running the upgrade twice by changing the schema_version on civicrm_extension for HR Core backward and running drush cvapi extension.upgrade. The results remains the same. This was made possible by $activeOptionValues = array_diff($optionValues, $valuesRemoved); which will always return same result after upgrade.

[mickadoo]

Great, thanks a lot for checking

[mickadoo]

Looks good ⚡

[mickadoo]

Great, thanks a lot for checking

[mickadoo]

oh, that's a good approach too