dev/core#785 Differentiate smart group from regular group using icon in select2 field

[monishdeb]

Overview

Currently, there is no way to tell which group is smart or regular group from UI. It would be ideal to use icon against such smart group options to differentiate them from regular ones. This patch appends an icon against smart group option of the select2 widget

https://lab.civicrm.org/dev/core/-/issues/785

Before

After

Comments

ping @colemanw @lcdservices

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[mattwire]

@monishdeb +1 on concept. I see there are some change relating to how select2 is added to the form in this PR. I think it might be easier to extract and merge those changes first (separately) to the new icon "feature".

[monishdeb]

Thanks for your feedback. Will move out the select2 element type changes to addField(...) in a separate PR.

[eileenmcnaughton]

test fail relates

[monishdeb]

@mattwire I have submitted a separate PR #14416 that only contain the new addField('select2', ....) feature. Can you please check now?

[eileenmcnaughton]

test this please

[eileenmcnaughton]

test fail relates

@colemanw are you +1 on concept / general code approach?

[monishdeb]

Jenkins test this please

[eileenmcnaughton]

@colemanw there is still a related test fail here but can you confirm that you have no concerns on this pr

[colemanw]

Yes I approve this concept, actually the lightbulb was my bright idea, just got lost in some other thread when we were discussing this proposal.

@civicrm-builder retest this please

[eileenmcnaughton]

oh the test is failing

[lcdservices]

That looks to me like an unrelated issue with the test, not a problem with this PR.

[lcdservices]

Ignore that last comment. It's an issue with the test included with this PR and should be fixed.

[eileenmcnaughton]

lol - I thought that when I first saw your comment - so you were snapped

[monishdeb]

Jenkins test this please.

[monishdeb]

@eileenmcnaughton @colemanw the test build passed. Can we merge this PR?

[lcdservices]

@monishdeb this looks good. Edit form/groups issue is fixed.
@colemanw can we get this merged? I think it's good to go.

[seamuslee001]

@monishdeb is there a reason why we are changing the array key here? also shouldn't this use the short array syntax?

[monishdeb]

@seamuslee001 the answer lies in here where we are now relying on select2 widget, which expects the array keys to be sequential and not hardcoded group ID. And better coverage for tests

[mlutfy]

Could you update for the short array syntax?

[colemanw]

@monishdeb can you check the review comment from Seamus above?

Also I would +1 the suggestion from BGM that we move the asterisk to after the group name as

1. it doesn't mess up alignment
2. that's normally where asterisks go

[francescbassas]

-1 to move the asterisk after the group name. For long group names the asterisk would either be lost at the end of the line or it would simply disappear. Maybe adding a CSS solution, padding-left and aligning all elements and putting an "asterisk" only on smart groups might be the best solution.

[lcdservices]

I don't think there's a perfect solution for the asterisk location. I'd lean toward the beginning, as it ensures the best visibility.

[ray-wright]

I would agree that a symbol (whether or not it is an asterisk) at the end of a group name is problematic. We use smart groups heavily and some of our names are a bit long. I also agree that adding the asterisk at the front messes up the left alignment. Is the solution that @francescbassas suggested - have the symbol off to the left with padding on the elements that don't have the symbol to keep the text alignment consistent - not entertainable?

[monishdeb]

@lcdservices @colemanw @francescbassas @ray-wright if you all agree, should I keep asterisk only on smart group to the left (at start) of label and with some padding-left adjustment is the best solution here?

[lcdservices]

I'm fine with that. I don't think there's a perfect solution, but this solution would address the most concerns/issues.

[mattwire]

@monishdeb Needs rebase

[monishdeb]

I have rebased the PR and made the changes to show asterisk on left of smart group label.

[mlutfy]

Could you discard the as $id from the foreach loop above? It will avoid someone accidentally changing it back to what it was before.

[mlutfy]

Code seems OK, and I'll rally to the majority of placing an asterisk before.

I'm slightly concerned about changing the signature of CRM_Contact_BAO_Group::getGroupsHierarchy, since it could potentially break extensions?

For example, the Contact Layout Summary Editor does this:

      $groups = CRM_Contact_BAO_Group::getGroupsHierarchy(CRM_Core_PseudoConstant::group(), NULL, '  ', TRUE);
      $groupElement = $this->add('select', 'group', $label, $groups, FALSE, ['class' => 'crm-select2', 'multiple' => TRUE]);

And these might need a bit more testing, for example, on the search forms:

$ grep -r getGroupsHierarchy
[..]
CRM_Contact_BAO_Group::getGroupsHierarchy($params, NULL, '  ', TRUE);
CRM/Contact/BAO/GroupContact.php: $options = CRM_Contact_BAO_Group::getGroupsHierarchy($options, NULL, '- ', TRUE);
[..]
CRM/Contact/Form/Search/Basic.php: $groupHierarchy = CRM_Contact_BAO_Group::getGroupsHierarchy($this->_group, NULL, '  ', TRUE);
CRM/Contact/Form/Search/Criteria.php: $groupHierarchy = CRM_Contact_BAO_Group::getGroupsHierarchy($form->_group, NULL, '  ', TRUE);
CRM/Contact/Form/Edit/TagsAndGroups.php: $groups = CRM_Contact_BAO_Group::getGroupsHierarchy($ids);
CRM/Contact/Form/GroupContact.php: $heirGroups = CRM_Contact_BAO_Group::getGroupsHierarchy($ids);
CRM/Contact/Form/GroupContact.php: $groupHierarchy = CRM_Contact_BAO_Group::getGroupsHierarchy($allGroups, NULL, '  ', TRUE);
CRM/Core/PseudoConstant.php: return CRM_Contact_BAO_Group::getGroupsHierarchy($groups, NULL, '  ', TRUE);

[eileenmcnaughton]

@mlutfy the last comment on this (which seems blocking) is from you

"I'm slightly concerned about changing the signature of CRM_Contact_BAO_Group::getGroupsHierarchy, since it could potentially break extensions?"

• I don't see a change to that signature - although the title is 'tweaked'

Can you comment & maybe merge if you see no remaining blockers

[mlutfy]

hmm indeed, seems all fine. merging!

[eileenmcnaughton]

Unfortunately this caused fatal errors - see #17888 - for now it is reverted as I think revert & redo is a sensible policy when the alternative is not obvious

[francescbassas]

I think instead of putting the asterisk directly in the name of the smart groups it would be better to add a class to be able to define the visual variations at CSS level.