From 22f80e87d0f9172c4ef44486ef5131c390d6b53f Mon Sep 17 00:00:00 2001 From: Eileen McNaughton Date: Thu, 15 Jan 2015 15:07:19 +1300 Subject: [PATCH] a few more tidy ups --- CRM/Contact/BAO/Contact.php | 97 +++++++++++-------- api/v3/Im.php | 1 - tests/phpunit/CRM/Contact/BAO/ContactTest.php | 52 +++++----- tests/phpunit/api/v3/ContributionTest.php | 41 +++++--- 4 files changed, 104 insertions(+), 87 deletions(-) diff --git a/CRM/Contact/BAO/Contact.php b/CRM/Contact/BAO/Contact.php index bc43f7c1b5..10fd7477bd 100644 --- a/CRM/Contact/BAO/Contact.php +++ b/CRM/Contact/BAO/Contact.php @@ -30,7 +30,6 @@ * @package CRM * @copyright CiviCRM LLC (c) 2004-2014 * $Id$ - * */ class CRM_Contact_BAO_Contact extends CRM_Contact_DAO_Contact { @@ -64,14 +63,24 @@ class CRM_Contact_BAO_Contact extends CRM_Contact_DAO_Contact { * * @var array */ - static $_commPrefs = array('do_not_phone', 'do_not_email', 'do_not_mail', 'do_not_sms', 'do_not_trade'); + static $_commPrefs = array( + 'do_not_phone', + 'do_not_email', + 'do_not_mail', + 'do_not_sms', + 'do_not_trade', + ); /** * Types of greetings * * @var array */ - static $_greetingTypes = array('addressee', 'email_greeting', 'postal_greeting'); + static $_greetingTypes = array( + 'addressee', + 'email_greeting', + 'postal_greeting', + ); /** * Static field for all the contact information that we can potentially import @@ -88,27 +97,25 @@ class CRM_Contact_BAO_Contact extends CRM_Contact_DAO_Contact { static $_exportableFields = NULL; /** - * Class constructor - * - * @return \CRM_Contact_DAO_Contact - */ - /** + * Class constructor. */ public function __construct() { parent::__construct(); } /** - * Takes an associative array and creates a contact object + * Takes an associative array and creates a contact object. * - * the function extract all the params it needs to initialize the create a + * The function extracts all the params it needs to initialize the create a * contact object. the params array could contain additional unused name/value * pairs * * @param array $params - * (reference ) an assoc array of name/value pairs. + * (reference) an assoc array of name/value pairs. * - * @return CRM_Contact_BAO_Contact + * @return CRM_Contact_BAO_Contact|CRM_Core_Error + * Created or updated contact object or error object. + * (error objects are being phased out in favour of exceptions) */ public static function add(&$params) { $contact = new CRM_Contact_DAO_Contact(); @@ -117,7 +124,7 @@ class CRM_Contact_BAO_Contact extends CRM_Contact_DAO_Contact { return; } - //fix for validate contact sub type CRM-5143 + // Fix for validate contact sub type CRM-5143. if (isset($params['contact_sub_type'])) { if (empty($params['contact_sub_type'])) { $params['contact_sub_type'] = 'null'; @@ -140,17 +147,17 @@ class CRM_Contact_BAO_Contact extends CRM_Contact_DAO_Contact { } } else { - // reset the value - // CRM-101XX + // Reset the value. + // CRM-101XX. $params['contact_sub_type'] = 'null'; } - //fixed contact source + // Fixed contact source. if (isset($params['contact_source'])) { $params['source'] = $params['contact_source']; } - //fix for preferred communication method + // Fix for preferred communication method. $prefComm = CRM_Utils_Array::value('preferred_communication_method', $params); if ($prefComm && is_array($prefComm)) { unset($params['preferred_communication_method']); @@ -179,7 +186,7 @@ class CRM_Contact_BAO_Contact extends CRM_Contact_DAO_Contact { if ($contact->contact_type == 'Individual') { $allNull = FALSE; - //format individual fields + // Format individual fields. CRM_Contact_BAO_Individual::format($params, $contact); } elseif ($contact->contact_type == 'Household') { @@ -195,7 +202,6 @@ class CRM_Contact_BAO_Contact extends CRM_Contact_DAO_Contact { } } - // privacy block $privacy = CRM_Utils_Array::value('privacy', $params); if ($privacy && is_array($privacy) && @@ -207,9 +213,9 @@ class CRM_Contact_BAO_Contact extends CRM_Contact_DAO_Contact { } } - // since hash was required, make sure we have a 0 value for it, CRM-1063 - // fixed in 1.5 by making hash optional - // only do this in create mode, not update + // Since hash was required, make sure we have a 0 value for it (CRM-1063). + // @todo - does this mean we can remove this block? + // Fixed in 1.5 by making hash optional, only do this in create mode, not update. if ((!array_key_exists('hash', $contact) || !$contact->hash) && !$contact->id) { $allNull = FALSE; $contact->hash = md5(uniqid(rand(), TRUE)); @@ -230,7 +236,7 @@ class CRM_Contact_BAO_Contact extends CRM_Contact_DAO_Contact { } if ($contact->contact_type == 'Individual' && (isset($params['current_employer']) || isset($params['employer_id']))) { - // create current employer + // Create current employer. $newEmployer = !empty($params['employer_id']) ? $params['employer_id'] : CRM_Utils_Array::value('current_employer', $params); $newContact = FALSE; @@ -241,14 +247,13 @@ class CRM_Contact_BAO_Contact extends CRM_Contact_DAO_Contact { CRM_Contact_BAO_Contact_Utils::createCurrentEmployerRelationship($contact->id, $newEmployer, $employerId, $newContact); } else { - //unset if employer id exits if ($employerId) { CRM_Contact_BAO_Contact_Utils::clearCurrentEmployer($contact->id, $employerId); } } } - //update cached employee name + // Update cached employer name. if ($contact->contact_type == 'Organization') { CRM_Contact_BAO_Contact_Utils::updateCurrentEmployer($contact->id); } @@ -257,7 +262,8 @@ class CRM_Contact_BAO_Contact extends CRM_Contact_DAO_Contact { } /** - * Create contact + * Create contact. + * * takes an associative array and creates a contact object and all the associated * derived objects (i.e. individual, location, email, phone etc) * @@ -271,9 +277,14 @@ class CRM_Contact_BAO_Contact extends CRM_Contact_DAO_Contact { * If we need to invoke hooks. * * @param bool $skipDelete + * Unclear parameter, passed to website create + * + * @todo explain this parameter * * @throws Exception - * @return CRM_Contact_BAO_Contact + * @return CRM_Contact_BAO_Contact|CRM_Core_Error + * Created or updated contribution object. We are deprecating returning an error in + * favour of exceptions */ public static function &create(&$params, $fixAddress = TRUE, $invokeHooks = TRUE, $skipDelete = FALSE) { $contact = NULL; @@ -294,17 +305,17 @@ class CRM_Contact_BAO_Contact extends CRM_Contact_DAO_Contact { $config = CRM_Core_Config::singleton(); - // CRM-6942: set preferred language to the current language if it’s unset (and we’re creating a contact) + // CRM-6942: set preferred language to the current language if it’s unset (and we’re creating a contact). if (empty($params['contact_id']) && empty($params['preferred_language'])) { $params['preferred_language'] = $config->lcMessages; } - // CRM-9739: set greeting & addressee if unset and we’re creating a contact + // CRM-9739: set greeting & addressee if unset and we’re creating a contact. if (empty($params['contact_id'])) { foreach (self::$_greetingTypes as $greeting) { if (empty($params[$greeting . '_id'])) { - if ($defaultGreetingTypeId = - CRM_Contact_BAO_Contact_Utils::defaultGreeting($params['contact_type'], $greeting) + if ($defaultGreetingTypeId + = CRM_Contact_BAO_Contact_Utils::defaultGreeting($params['contact_type'], $greeting) ) { $params[$greeting . '_id'] = $defaultGreetingTypeId; } @@ -316,7 +327,7 @@ class CRM_Contact_BAO_Contact extends CRM_Contact_DAO_Contact { $contact = self::add($params); if (!$contact) { - // not dying here is stupid, since we get into wierd situation and into a bug that + // Not dying here is stupid, since we get into wierd situation and into a bug that // is impossible to figure out for the user or for us // CRM-7925 CRM_Core_Error::fatal(); @@ -325,7 +336,7 @@ class CRM_Contact_BAO_Contact extends CRM_Contact_DAO_Contact { $params['contact_id'] = $contact->id; if (CRM_Core_BAO_Setting::getItem(CRM_Core_BAO_Setting::MULTISITE_PREFERENCES_NAME, 'is_enabled')) { - // Enabling multisite causes the contact to be added to the domain group + // Enabling multisite causes the contact to be added to the domain group. $domainGroupID = CRM_Core_BAO_Domain::getGroupId(); if (!empty($domainGroupID)) { if (!empty($params['group']) && is_array($params['group'])) { @@ -349,7 +360,7 @@ class CRM_Contact_BAO_Contact extends CRM_Contact_DAO_Contact { } } - //add location Block data + // Add location Block data. $blocks = CRM_Core_BAO_Location::create($params, $fixAddress); foreach ($blocks as $name => $value) { $contact->$name = $value; @@ -405,7 +416,6 @@ class CRM_Contact_BAO_Contact extends CRM_Contact_DAO_Contact { } } - // update the UF user_unique_id if that has changed CRM_Core_BAO_UFMatch::updateUFName($contact->id); @@ -459,17 +469,18 @@ class CRM_Contact_BAO_Contact extends CRM_Contact_DAO_Contact { } /** - * Get the display name and image of a contact + * Get the display name and image of a contact. * * @param int $id * The contactId. * - * @param bool $type + * @param bool $includeTypeInReturnParameters + * Should type be part of the returned array? * * @return array * the displayName and contactImage for this contact */ - public static function getDisplayAndImage($id, $type = FALSE) { + public static function getDisplayAndImage($id, $includeTypeInReturnParameters = FALSE) { //CRM-14276 added the * on the civicrm_contact table so that we have all the contact info available $sql = " SELECT civicrm_contact.*, @@ -498,7 +509,7 @@ WHERE civicrm_contact.id = " . CRM_Utils_Type::escape($id, 'Integer'); CRM_Utils_Hook::alterDisplayName($displayName, $id, $dao); - return $type ? array( + return $includeTypeInReturnParameters ? array( $displayName, $image, $dao->contact_type, @@ -510,6 +521,8 @@ WHERE civicrm_contact.id = " . CRM_Utils_Type::escape($id, 'Integer'); } /** + * Create last viewed link to recently updated contact. + * * @param array $crudLinkSpec * With keys:. * - action: int, CRM_Core_Action::UPDATE or CRM_Core_Action::VIEW [default: VIEW] @@ -553,11 +566,9 @@ WHERE civicrm_contact.id = " . CRM_Utils_Type::escape($id, 'Integer'); * (reference) the default values, some of which need to be resolved. * @param bool $reverse * True if we want to resolve the values in the reverse direction (value -> name). - * - * @return void */ public static function resolveDefaults(&$defaults, $reverse = FALSE) { - // hack for birth_date + // Hack for birth_date. if (!empty($defaults['birth_date'])) { if (is_array($defaults['birth_date'])) { $defaults['birth_date'] = CRM_Utils_Date::format($defaults['birth_date'], '-'); @@ -671,7 +682,7 @@ WHERE civicrm_contact.id = " . CRM_Utils_Type::escape($id, 'Integer'); ); } - //kill the reference. + // Kill the reference. unset($values); } } diff --git a/api/v3/Im.php b/api/v3/Im.php index f117aec300..39dcb176af 100644 --- a/api/v3/Im.php +++ b/api/v3/Im.php @@ -64,7 +64,6 @@ function _civicrm_api3_im_create_spec(&$params) { * Deletes an existing IM * * @param array $params - * {@getfields im_delete} * * @return array * API result Array diff --git a/tests/phpunit/CRM/Contact/BAO/ContactTest.php b/tests/phpunit/CRM/Contact/BAO/ContactTest.php index 192482f65c..19b46231f1 100644 --- a/tests/phpunit/CRM/Contact/BAO/ContactTest.php +++ b/tests/phpunit/CRM/Contact/BAO/ContactTest.php @@ -7,26 +7,31 @@ require_once 'CiviTest/Custom.php'; * Class CRM_Contact_BAO_ContactTest */ class CRM_Contact_BAO_ContactTest extends CiviUnitTestCase { + + /** + * Set up function. + */ public function setUp() { parent::setUp(); } /** - * Test case for add( ) + * Test case for add( ). + * * test with empty params. */ public function testAddWithEmptyParams() { $params = array(); $contact = CRM_Contact_BAO_Contact::add($params); - //Now check Contact object + // Now check Contact object. $this->assertNull($contact); } /** - * Test case for add( ) - * test with names - * (create and update modes) + * Test case for add( ). + * + * Test with names (create and update modes) */ public function testAddWithNames() { $firstName = 'Shane'; @@ -39,14 +44,14 @@ class CRM_Contact_BAO_ContactTest extends CiviUnitTestCase { $contact = CRM_Contact_BAO_Contact::add($params); - //Now check $contact is object of contact DAO.. + // Now check $contact is object of contact DAO. $this->assertInstanceOf('CRM_Contact_DAO_Contact', $contact, 'Check for created object'); $this->assertEquals($firstName, $contact->first_name, 'Check for first name creation.'); $this->assertEquals($lastName, $contact->last_name, 'Check for last name creation.'); $contactId = $contact->id; - //update and change first name and last name, using add( ) + // Update and change first name and last name, using add( ). $firstName = 'Jane'; $params = array( 'first_name' => $firstName, @@ -56,7 +61,7 @@ class CRM_Contact_BAO_ContactTest extends CiviUnitTestCase { $contact = CRM_Contact_BAO_Contact::add($params); - //Now check $contact is object of contact DAO.. + // Now check $contact is object of contact DAO. $this->assertInstanceOf('CRM_Contact_DAO_Contact', $contact, 'Check for created object'); $this->assertEquals($firstName, $contact->first_name, 'Check for updated first name.'); @@ -65,25 +70,22 @@ class CRM_Contact_BAO_ContactTest extends CiviUnitTestCase { } /** - * Test case for add( ) - * test with all contact params - * (creat and update modes) + * Test case for add. + * + * Test with all contact params + * (create and update modes) */ public function testAddWithAll() { - //take the common contact params + // Take the common contact params. $params = $this->contactParams(); unset($params['location']); $prefComm = $params['preferred_communication_method']; - - //create the contact using add() $contact = CRM_Contact_BAO_Contact::add($params); $contactId = $contact->id; - //Now check $contact is object of contact DAO.. $this->assertInstanceOf('CRM_Contact_DAO_Contact', $contact, 'Check for created object'); - //Now check values of object with params. $this->assertEquals($params['first_name'], $contact->first_name, 'Check for first name creation.'); $this->assertEquals($params['last_name'], $contact->last_name, 'Check for last name creation.'); $this->assertEquals($params['middle_name'], $contact->middle_name, 'Check for middle name creation.'); @@ -121,7 +123,6 @@ class CRM_Contact_BAO_ContactTest extends CiviUnitTestCase { } $this->assertAttributesEquals($checkPrefComm, $prefComm); - //now update the contact using add( ) $updateParams = array( 'contact_type' => 'Individual', 'first_name' => 'Jane', @@ -162,14 +163,11 @@ class CRM_Contact_BAO_ContactTest extends CiviUnitTestCase { $prefComm = $updateParams['preferred_communication_method']; $updateParams['contact_id'] = $contactId; - //create the contact using add() $contact = CRM_Contact_BAO_Contact::create($updateParams); $contactId = $contact->id; - //Now check $contact is object of contact DAO.. $this->assertInstanceOf('CRM_Contact_DAO_Contact', $contact, 'Check for created object'); - //Now check values of object with params. $this->assertEquals($updateParams['first_name'], $contact->first_name, 'Check for first name creation.'); $this->assertEquals($updateParams['last_name'], $contact->last_name, 'Check for last name creation.'); $this->assertEquals($updateParams['middle_name'], $contact->middle_name, 'Check for middle name creation.'); @@ -211,13 +209,11 @@ class CRM_Contact_BAO_ContactTest extends CiviUnitTestCase { } $this->assertAttributesEquals($checkPrefComm, $prefComm); - //cleanup DB by deleting the contact Contact::delete($contactId); } /** - * Test case for add( ) - * test with All contact types. + * Test case for add( ) with All contact types. */ public function testAddWithAllContactTypes() { $firstName = 'Bill'; @@ -484,16 +480,16 @@ class CRM_Contact_BAO_ContactTest extends CiviUnitTestCase { ); $compareParams = array('phone' => '9766323895'); $this->assertDBCompareValues('CRM_Core_DAO_Phone', $searchParams, $compareParams); - //As we are not updating note - //Now check DB for New Note - $noteId = $this->assertDBNotNull('CRM_Core_DAO_Note', $updateParams['note'], 'id', 'note', + // As we are not updating note. + // Now check DB for New Note. + $this->assertDBNotNull('CRM_Core_DAO_Note', $updateParams['note'], 'id', 'note', 'Database check for New created note ' ); - //delete all notes related to contact + // Delete all notes related to contact. CRM_Core_BAO_Note::cleanContactNotes($contactId); - //cleanup DB by deleting the contact + // Cleanup DB by deleting the contact. Contact::delete($contactId); $this->quickCleanup(array('civicrm_contact', 'civicrm_note')); } diff --git a/tests/phpunit/api/v3/ContributionTest.php b/tests/phpunit/api/v3/ContributionTest.php index 5fecd0634b..46e55e16d3 100644 --- a/tests/phpunit/api/v3/ContributionTest.php +++ b/tests/phpunit/api/v3/ContributionTest.php @@ -50,6 +50,9 @@ class api_v3_ContributionTest extends CiviUnitTestCase { protected $_ids = array(); protected $_pageParams = array(); + /** + * Setup function. + */ public function setUp() { parent::setUp(); @@ -91,10 +94,16 @@ class api_v3_ContributionTest extends CiviUnitTestCase { ); } + /** + * Clean up after each test. + */ public function tearDown() { $this->quickCleanUpFinancialEntities(); } + /** + * Test Get. + */ public function testGetContribution() { $p = array( 'contact_id' => $this->_individualId, @@ -122,8 +131,9 @@ class api_v3_ContributionTest extends CiviUnitTestCase { $this->assertEquals(1, $contribution['count']); $this->assertEquals($contribution['values'][$contribution['id']]['contact_id'], $this->_individualId); - // note there was an assertion converting financial_type_id to 'Donation' which wasn't working. - // passing back a string rather than an id seems like an error / cruft - & if it is to be introduced we should discuss + // Note there was an assertion converting financial_type_id to 'Donation' which wasn't working. + // Passing back a string rather than an id seems like an error/cruft. + // If it is to be introduced we should discuss. $this->assertEquals($contribution['values'][$contribution['id']]['financial_type_id'], 1); $this->assertEquals($contribution['values'][$contribution['id']]['total_amount'], 100.00); $this->assertEquals($contribution['values'][$contribution['id']]['non_deductible_amount'], 10.00); @@ -133,34 +143,34 @@ class api_v3_ContributionTest extends CiviUnitTestCase { $this->assertEquals($contribution['values'][$contribution['id']]['invoice_id'], 78910); $this->assertEquals($contribution['values'][$contribution['id']]['contribution_source'], 'SSF'); $this->assertEquals($contribution['values'][$contribution['id']]['contribution_status'], 'Completed'); - //create a second contribution - we are testing that 'id' gets the right contribution id (not the contact id) + // Create a second contribution - we are testing that 'id' gets the right contribution id (not the contact id). $p['trxn_id'] = '3847'; $p['invoice_id'] = '3847'; $contribution2 = $this->callAPISuccess('contribution', 'create', $p); - // now we have 2 - test getcount + // Now we have 2 - test getcount. $contribution = $this->callAPISuccess('contribution', 'getcount', array()); $this->assertEquals(2, $contribution); - //test id only format + // Test id only format. $contribution = $this->callAPISuccess('contribution', 'get', array( 'id' => $this->_contribution['id'], 'format.only_id' => 1, )); - $this->assertEquals($this->_contribution['id'], $contribution, print_r($contribution, TRUE) . " in line " . __LINE__); - //test id only format + $this->assertEquals($this->_contribution['id'], $contribution, print_r($contribution, TRUE)); + // Test id only format. $contribution = $this->callAPISuccess('contribution', 'get', array( 'id' => $contribution2['id'], 'format.only_id' => 1, )); $this->assertEquals($contribution2['id'], $contribution); - //test id as field + // Test id as field. $contribution = $this->callAPISuccess('contribution', 'get', array( 'id' => $this->_contribution['id'], )); $this->assertEquals(1, $contribution['count']); - //test get by contact id works + // Test get by contact id works. $contribution = $this->callAPISuccess('contribution', 'get', array('contact_id' => $this->_individualId)); $this->assertEquals(2, $contribution['count']); @@ -173,7 +183,7 @@ class api_v3_ContributionTest extends CiviUnitTestCase { } /** - * We need to ensure previous tested behaviour still works as part of the api contract + * We need to ensure previous tested behaviour still works as part of the api contract. */ public function testGetContributionLegacyBehaviour() { $p = array( @@ -211,7 +221,8 @@ class api_v3_ContributionTest extends CiviUnitTestCase { $this->assertEquals($contribution['values'][$contribution['id']]['invoice_id'], 78910); $this->assertEquals($contribution['values'][$contribution['id']]['contribution_source'], 'SSF'); $this->assertEquals($contribution['values'][$contribution['id']]['contribution_status'], 'Completed'); - //create a second contribution - we are testing that 'id' gets the right contribution id (not the contact id) + + // Create a second contribution - we are testing that 'id' gets the right contribution id (not the contact id). $p['trxn_id'] = '3847'; $p['invoice_id'] = '3847'; @@ -225,7 +236,7 @@ class api_v3_ContributionTest extends CiviUnitTestCase { 'id' => $this->_contribution['id'], 'format.only_id' => 1, )); - $this->assertEquals($this->_contribution['id'], $contribution, print_r($contribution, TRUE) . " in line " . __LINE__); + $this->assertEquals($this->_contribution['id'], $contribution, print_r($contribution, TRUE)); //test id only format $contribution = $this->callAPISuccess('contribution', 'get', array( 'id' => $contribution2['id'], @@ -299,7 +310,7 @@ class api_v3_ContributionTest extends CiviUnitTestCase { )); $this->customFieldDelete($ids['custom_field_id']); $this->customGroupDelete($ids['custom_group_id']); - $this->assertEquals("custom string", $check['values'][$check['id']]['custom_' . $ids['custom_field_id']], ' in line ' . __LINE__); + $this->assertEquals("custom string", $check['values'][$check['id']]['custom_' . $ids['custom_field_id']]); } /** @@ -1168,7 +1179,7 @@ class api_v3_ContributionTest extends CiviUnitTestCase { ); $result = $this->callAPISuccess('contribution', 'delete', $params); - $this->assertAPISuccess($result, 'in line' . __LINE__); + $this->assertAPISuccess($result); } ///////////////// civicrm_contribution_delete methods @@ -1587,7 +1598,7 @@ class api_v3_ContributionTest extends CiviUnitTestCase { $params['contribution_source'] = $params['source']; unset($params['source']); foreach ($params as $key => $value) { - $this->assertEquals($value, $values[$key], $key . " value: $value doesn't match " . print_r($values, TRUE) . 'in line' . __LINE__); + $this->assertEquals($value, $values[$key], $key . " value: $value doesn't match " . print_r($values, TRUE)); } } -- 2.25.1