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

Fix GroupNesting, GroupOrganization, Domain to work with singleValueAlter #11689

Merged
merged 1 commit into from
Feb 20, 2018

Conversation

eileenmcnaughton
Copy link
Contributor

Overview

Standardise apis for GroupNesting, GroupOrganization, Domain and extend unit testing to cover them

Before

per overview

After

per overview

Technical Details

ContactType - $params was being overwritten (passed by reference) for no good reason
Group Nesting - did not support 'id' as a parameter. I added a create function which takes predecence over add in the api
GroupOrganization - did not really support id as a parameter. Did weird things when passed incl e-notice
Domain - now using dao metadata serialization handling

Comments

The GroupOrganization api test needed to be altered, indicating the return array was originally not correctly formatted. I feel Ok about fixing this as the format is so clearly established in general.

@@ -75,7 +75,6 @@ function civicrm_api3_contact_type_create($params) {
* Array of matching contact_types
*/
function civicrm_api3_contact_type_get($params) {
civicrm_api3_verify_mandatory($params);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does mandatory params happen as part of basic get?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it happens before it reaches that function in the api wrapper

@@ -142,7 +142,7 @@ public static function edit(&$params, &$id) {
*/
public static function create($params) {
$domain = new CRM_Core_DAO_Domain();
$domain->copyValues($params);
$domain->copyValues($params, TRUE);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remind me what the second parameter does here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's new - we added array handling to that function - providing the metadata is defined - new in the xml

@eileenmcnaughton
Copy link
Contributor Author

@colemanw are you able to review this one? Just cleanup really

@colemanw
Copy link
Member

This is all code cleanup on tested apis. The code looks good and the tests are passing.

@colemanw colemanw merged commit 159a0a9 into civicrm:master Feb 20, 2018
@eileenmcnaughton eileenmcnaughton deleted the groupOrg branch February 20, 2018 19:16
@mlutfy mlutfy added this to the 4.7.32 milestone Mar 6, 2018
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.

5 participants