Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CRM-20091 - Add customValue.gettree api #10460

Merged
merged 2 commits into from
Jun 3, 2017
Merged

Conversation

colemanw
Copy link
Member

@colemanw colemanw commented Jun 2, 2017

This adds a CustomValue.gettree action which returns the entire tree of custom data for a specified entity -
groups, fields, and values, including the raw and display values. It's intended for use on Angular screens that do not have access to core BAO functions like CRM_Core_BAO_CustomGroup::getTree.

@totten
Copy link
Member

totten commented Jun 2, 2017

Generally, getTree() has been pretty important, so it makes sense that ends up needing API exposure.

My only real concern is that getTree() has a fairly big surface-area.

  • It would be really nice if we could somehow flag the API as experimental.
  • Really glad there's a unit-test. Given the surface-area, I'd be inclined to add a little more test-coverage. Some candidates: (a) CustomValue.getfields action=gettree (b) check_permissions behavior (c) just add more random assertions about the returned data.

Those aren't hard-blockers. My opinion is ~1.9/2.0.

@colemanw
Copy link
Member Author

colemanw commented Jun 2, 2017

@totten I've pushed some improvements which aren't as scary as they look, I promise. It expands the test coveage with a bit of (a) and (c) per your suggestion, and it also makes the query more efficient by allowing a $toReturn param to be passed into the bao::getTree function. I recycled an old unused param for this, and I think this is safe because 1) I removed all the spots in the code which used it and 2) even if I missed one (e.g. in an extension) that param was always an object, the new param is an array and I added a sanity check to ignore that value if an array isn't passed in.

@totten
Copy link
Member

totten commented Jun 2, 2017

Yeah, CustomGroup::getTree() has been an internal function, so (overall) it seems fair to re-purpose the second parameter. The search/cleanup in civicrm-core seems pretty thorough.

To identify/mitigate impacts on contrib, I grepped universe for CustomGroup::getTree. Pretty much all of them were safe (passing either NULL or CRM_Core_DAO::$_nullObject for the second option). Three files seemed like they should be updated:

@colemanw
Copy link
Member Author

colemanw commented Jun 2, 2017

Ok I've updated that one core line that I had missed, and I sincerely doubt that the backdrop version of CiviEngage is working/tested, but here's this anyway: civicrm/civicrm-backdrop#30

@totten
Copy link
Member

totten commented Jun 3, 2017

Thanks.

I've done some spot-checking in a few random UI's that display custom-data. ("View Contact", "View Contact: Edit custom block", "Edit Profile", "CiviEvent: Edit Event", "CiviEvent: Event Info") They seem to be working the same.

@seamuslee001
Copy link
Contributor

Jenkins re test this please

@colemanw colemanw merged commit dd285a5 into civicrm:master Jun 3, 2017
@colemanw colemanw deleted the CRM-20091 branch June 3, 2017 13:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants