core#1795: Searchable Parent tags

[monishdeb]

Overview

Steps to replicate the issue:

1. Create a parent tag A and child tag A1
2. Create/update contact and tag with A1
3. Go to 'Find Contacts' or 'Find Contributions' and search by Tag - A

Before

1. In 'Find Contact' result, it doesn't show contact which is tagged with child tag A1
2. In 'Find Contributions' result, it doesn't pull contribution(s) of contact tagged with child tag A1

After

Searching using a parent tag should automatically include the contacts which are in parent tag A and in its n child tag(s) - A(n). In this use-case, it should show contact tagged with child A1 tag

Technical Details

This improvement, in other words, the code fix, affects search forms only where Tag filter is used (doesn't affect API). This idea about making parent tag searchable is about making the tag hierarchy feature useful for searching purposes like we are already supporting searchable parent regular/smart group here

Comments

ping @JoeMurray @eileenmcnaughton @lcdservices @seamuslee001

[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).

[JoeMurray]

Could you do some QA on hierarchies that are at least 4 deep, checking what is returned for searches in the 2nd or 3rd levels of the hierarchy? I'm concerned on a quick scan that https://github.com/civicrm/civicrm-core/blob/master/CRM/Core/BAO/Tag.php#L554 is not finding all of the offspring of a tag, but all of the ancestors.

Consider this 5 level hierachy:
A

• A1
  -- A11
  -- A12
• A2
  -- A21
  --- A211
  --- A212
  ---- A2121
  ---- A2122
  ---- A2123

If we wanted to search for A21 and all its children, we should get
A21
A211
A212
A2121
A2122
A2123

I'm worried instead that we would get
A21
A211
A212
A2
A

[monishdeb]

@JoeMurray actually the two while loop in the getChildTags() fn do a recursive search for tags and format the tag result further in here So for
A1
-- A11
-- A12
A2
-- A21
--- A211
--- A212
---- A2121
---- A2122
---- A2123

getChildTags() will return

$tagTree = [
  A1 => [A11, A12],
  A2 => [A21, A211, A212, A2121, A2122, A2123], 
  A21 => [A211, A212, A2121, A2122, A2123],
  A211,
  A212 => [A2121, A2122, A2123],
];

In my unitest, I have included a level2 tag contact here and to confirm that the search works with the level1 and level2 child tags I have included some some combination of tag search criterias here . Should I extend my unit test to include levle2+ level3 tags in search criteria to show that the code fix will work for more complex tag hierarchy?

[jaapjansma]

Betty and I are reviewing PR's and we came across this one. It is unclear to us why this has the label schedule-discussion. Could you explain that @eileenmcnaughton? Because we are a bit confused on whether we should review this PR or not.

[seamuslee001]

@jaapjansma I suspect because at the time there was a feeling on the lab ticket https://lab.civicrm.org/dev/core/-/issues/1795 that there still need to be agreement on the proposed change

[jaapjansma]

@seamuslee001 ah okay. I will remove the ready for review tag as this is not yet ready for review.

[eileenmcnaughton]

@jaapjansma I think you could still review this - we may try to give people more chance to comment - but nothing in the feedback so far looks like it will block this one

[lcdservices]

I've tested this and it works as expected. There's been no further comment in the lab ticket. So perhaps it's ready to push forward?

[eileenmcnaughton]

@lcdservices OK - I'll go with your testing - this has a test etc & no-one raised concerns