From d2460a891988b7a97c85b134c7567a4a7bd2d6f9 Mon Sep 17 00:00:00 2001 From: Monish Deb Date: Thu, 1 Dec 2016 20:29:43 +0530 Subject: [PATCH] =?utf8?q?optimization=20-=20move=20addActivity=20call=20f?= =?utf8?q?rom=20Membership's=20create=20to=20add=20=E2=80=A6?= MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit --- CRM/Activity/BAO/Activity.php | 121 ++++++++++++---------- CRM/Contribute/BAO/Contribution.php | 2 - CRM/Member/BAO/Membership.php | 110 ++++++-------------- tests/phpunit/api/v3/ContributionTest.php | 16 +-- 4 files changed, 109 insertions(+), 140 deletions(-) diff --git a/CRM/Activity/BAO/Activity.php b/CRM/Activity/BAO/Activity.php index 1811b2e206..95f646d201 100644 --- a/CRM/Activity/BAO/Activity.php +++ b/CRM/Activity/BAO/Activity.php @@ -2041,57 +2041,22 @@ WHERE activity.id IN ($activityIds)"; * @param string $activityType * For Membership Signup or Renewal. * @param int $targetContactID + * @param array $params + * Activity params to override * * @return bool|NULL */ public static function addActivity( &$activity, $activityType = 'Membership Signup', - $targetContactID = NULL + $targetContactID = NULL, + $params = array() ) { + $date = date('YmdHis'); if ($activity->__table == 'civicrm_membership') { - $membershipType = CRM_Member_PseudoConstant::membershipType($activity->membership_type_id); - - if (!$membershipType) { - $membershipType = ts('Membership'); - } - - $subject = "{$membershipType}"; - - if (!empty($activity->source) && $activity->source != 'null') { - $subject .= " - {$activity->source}"; - } - - if ($activity->owner_membership_id) { - $query = " -SELECT display_name - FROM civicrm_contact, civicrm_membership - WHERE civicrm_contact.id = civicrm_membership.contact_id - AND civicrm_membership.id = $activity->owner_membership_id -"; - $displayName = CRM_Core_DAO::singleValueQuery($query); - $subject .= " (by {$displayName})"; - } - - $subject .= " - Status: " . CRM_Core_DAO::getFieldValue('CRM_Member_DAO_MembershipStatus', $activity->status_id, 'label'); - // CRM-72097 changed from start date to today - $date = date('YmdHis'); $component = 'Membership'; } elseif ($activity->__table == 'civicrm_participant') { - $event = CRM_Event_BAO_Event::getEvents(1, $activity->event_id, TRUE, FALSE); - - $roles = CRM_Event_PseudoConstant::participantRole(); - $status = CRM_Event_PseudoConstant::participantStatus(); - - $subject = $event[$activity->event_id]; - if (!empty($roles[$activity->role_id])) { - $subject .= ' - ' . $roles[$activity->role_id]; - } - if (!empty($status[$activity->status_id])) { - $subject .= ' - ' . $status[$activity->status_id]; - } - $date = date('YmdHis'); if ($activityType != 'Email') { $activityType = 'Event Registration'; } @@ -2103,35 +2068,36 @@ SELECT display_name return NULL; } - $subject = NULL; - - $subject .= CRM_Utils_Money::format($activity->total_amount, $activity->currency); - if (!empty($activity->source) && $activity->source != 'null') { - $subject .= " - {$activity->source}"; - } $date = CRM_Utils_Date::isoToMysql($activity->receive_date); $activityType = $component = 'Contribution'; } + $activityParams = array( 'source_contact_id' => $activity->contact_id, 'source_record_id' => $activity->id, 'activity_type_id' => CRM_Core_PseudoConstant::getKey('CRM_Activity_BAO_Activity', 'activity_type_id', $activityType), - 'subject' => $subject, + 'target_contact_id' => $activity->contact_id, 'activity_date_time' => $date, 'is_test' => $activity->is_test, 'status_id' => CRM_Core_PseudoConstant::getKey('CRM_Activity_BAO_Activity', 'activity_status_id', 'Completed'), 'skipRecentView' => TRUE, 'campaign_id' => $activity->campaign_id, ); + $activityParams = array_merge($activityParams, $params); + + if (empty($activityParams['subject'])) { + $activityParams['subject'] = self::getActivitySubject($activity); + } if (!empty($activity->activity_id)) { $activityParams['id'] = $activity->activity_id; } // create activity with target contacts - $id = CRM_Core_Session::getLoggedInContactID(); - if ($id) { - $activityParams['source_contact_id'] = $id; - $activityParams['target_contact_id'][] = $activity->contact_id; + if (!empty($activityParams['source_contact_id'])) { + $id = CRM_Core_Session::getLoggedInContactID(); + if ($id) { + $activityParams['source_contact_id'] = $id; + } } // CRM-14945 @@ -2140,7 +2106,7 @@ SELECT display_name } //CRM-4027 if ($targetContactID) { - $activityParams['target_contact_id'][] = $targetContactID; + $activityParams['target_contact_id'] = $targetContactID; } // @todo - use api - remove lots of wrangling above. Remove deprecated fatal & let form layer // deal with any exceptions. @@ -2150,6 +2116,57 @@ SELECT display_name } } + /** + * Get activity subject on basis of component object + * + * @param object $entityObj + * (reference) particular component object. + * + * @return string + */ + public static function getActivitySubject($entityObj) { + switch ($entityObj->__table) { + case 'civicrm_membership': + $membershipType = CRM_Member_PseudoConstant::membershipType($entityObj->membership_type_id); + $subject = $membershipType ? $membershipType : ts('Membership'); + + if (!CRM_Utils_System::isNull($entityObj->source)) { + $subject .= " - {$entityObj->source}"; + } + + if ($entityObj->owner_membership_id) { + list($displayName) = CRM_Contact_BAO_Contact::getDisplayAndImage(CRM_Core_DAO::getFieldValue('CRM_Member_DAO_Membership', $entityObj->owner_membership_id, 'contact_id')); + $subject .= sprintf(' (by %s)', $displayName); + } + + $subject .= " - Status: " . CRM_Core_DAO::getFieldValue('CRM_Member_DAO_MembershipStatus', $entityObj->status_id, 'label'); + return $subject; + + case 'civicrm_participant': + $event = CRM_Event_BAO_Event::getEvents(1, $entityObj->event_id, TRUE, FALSE); + $roles = CRM_Event_PseudoConstant::participantRole(); + $status = CRM_Event_PseudoConstant::participantStatus(); + $subject = $event[$entityObj->event_id]; + + if (!empty($roles[$entityObj->role_id])) { + $subject .= ' - ' . $roles[$entityObj->role_id]; + } + if (!empty($status[$entityObj->status_id])) { + $subject .= ' - ' . $status[$entityObj->status_id]; + } + + return $subject; + + case 'civicrm_contribution': + $subject = CRM_Utils_Money::format($entityObj->total_amount, $entityObj->currency); + if (!CRM_Utils_System::isNull($entityObj->source)) { + $subject .= " - {$entityObj->source}"; + } + + return $subject; + } + } + /** * Get Parent activity for currently viewed activity. * diff --git a/CRM/Contribute/BAO/Contribution.php b/CRM/Contribute/BAO/Contribution.php index 6b88720554..22c3e625f7 100644 --- a/CRM/Contribute/BAO/Contribution.php +++ b/CRM/Contribute/BAO/Contribution.php @@ -4608,8 +4608,6 @@ LIMIT 1;"; $var = TRUE; CRM_Member_BAO_Membership::createRelatedMemberships($var, $var, TRUE); civicrm_api3('Membership', 'create', $membershipParams); - //CRM-19600: Add Membership Renewal activity - CRM_Activity_BAO_Activity::addActivity($membership, 'Membership Renewal', $membership->contact_id); } } } diff --git a/CRM/Member/BAO/Membership.php b/CRM/Member/BAO/Membership.php index 419effbd5c..03b70d128c 100644 --- a/CRM/Member/BAO/Membership.php +++ b/CRM/Member/BAO/Membership.php @@ -139,38 +139,45 @@ class CRM_Member_BAO_Membership extends CRM_Member_DAO_Membership { // reset the group contact cache since smart groups might be affected due to this CRM_Contact_BAO_GroupContactCache::opportunisticCacheFlush(); + $targetContactID = $membership->contact_id; + if (!empty($params['is_for_organization'])) { + $targetContactID = CRM_Utils_Array::value('userId', $ids); + } if ($id) { if ($membership->status_id != $oldStatus) { $allStatus = CRM_Member_BAO_Membership::buildOptions('status_id', 'get'); - $activityParam = array( - 'subject' => "Status changed from {$allStatus[$oldStatus]} to {$allStatus[$membership->status_id]}", - 'source_contact_id' => $membershipLog['modified_id'], - 'target_contact_id' => $membership->contact_id, - 'source_record_id' => $membership->id, - 'activity_type_id' => CRM_Core_PseudoConstant::getKey('CRM_Activity_BAO_Activity', 'activity_type_id', 'Change Membership Status'), - 'status_id' => 2, - 'priority_id' => 2, - 'activity_date_time' => date('Y-m-d H:i:s'), + CRM_Activity_BAO_Activity::addActivity($membership, + 'Change Membership Status', + NULL, + array( + 'subject' => "Status changed from {$allStatus[$oldStatus]} to {$allStatus[$membership->status_id]}", + 'source_contact_id' => $membershipLog['modified_id'], + 'priority_id' => 2, + ) ); - civicrm_api3('activity', 'create', $activityParam); } if (isset($membership->membership_type_id) && $membership->membership_type_id != $oldType) { $membershipTypes = CRM_Member_BAO_Membership::buildOptions('membership_type_id', 'get'); - $activityParam = array( - 'subject' => "Type changed from {$membershipTypes[$oldType]} to {$membershipTypes[$membership->membership_type_id]}", - 'source_contact_id' => $membershipLog['modified_id'], - 'target_contact_id' => $membership->contact_id, - 'source_record_id' => $membership->id, - 'activity_type_id' => CRM_Core_PseudoConstant::getKey('CRM_Activity_BAO_Activity', 'activity_type_id', 'Change Membership Type'), - 'status_id' => 2, - 'priority_id' => 2, - 'activity_date_time' => date('Y-m-d H:i:s'), + CRM_Activity_BAO_Activity::addActivity($membership, + 'Change Membership Type', + NULL, + array( + 'subject' => "Type changed from {$membershipTypes[$oldType]} to {$membershipTypes[$membership->membership_type_id]}", + 'source_contact_id' => $membershipLog['modified_id'], + 'priority_id' => 2, + ) ); - civicrm_api3('activity', 'create', $activityParam); + } + // via Contribution.completeOrder membership.create is called again to ensure correct data has been + // copied to the related membership. but there is a sideffect of this new membership workflow as a + // 'Membership Renewal' Activity is created. We can avoid this on basis of is_override=FALSE condition + if (CRM_Utils_Array::value('is_override', $params) != 'null') { + CRM_Activity_BAO_Activity::addActivity($membership, 'Membership Renewal', $targetContactID); } CRM_Utils_Hook::post('edit', 'Membership', $membership->id, $membership); } else { + CRM_Activity_BAO_Activity::addActivity($membership, 'Membership Signup', $targetContactID); CRM_Utils_Hook::post('create', 'Membership', $membership->id, $membership); } @@ -229,13 +236,12 @@ class CRM_Member_BAO_Membership extends CRM_Member_DAO_Membership { * @param array $ids * Deprecated parameter The array that holds all the db ids. * @param bool $skipRedirect - * @param string $activityType * * @throws CRM_Core_Exception * * @return CRM_Member_BAO_Membership|CRM_Core_Error */ - public static function create(&$params, &$ids, $skipRedirect = FALSE, $activityType = 'Membership Signup') { + public static function create(&$params, &$ids, $skipRedirect = FALSE) { // always calculate status if is_override/skipStatusCal is not true. // giving respect to is_override during import. CRM-4012 @@ -378,54 +384,6 @@ class CRM_Member_BAO_Membership extends CRM_Member_DAO_Membership { )); } - // add activity record only during create mode and renew mode - // also add activity if status changed CRM-3984 and CRM-2521 - if (empty($ids['membership']) || - $activityType == 'Membership Renewal' || !empty($params['createActivity']) - ) { - if (!empty($ids['membership'])) { - $data = array(); - CRM_Core_DAO::commonRetrieveAll('CRM_Member_DAO_Membership', - 'id', - $membership->id, - $data, - array('contact_id', 'membership_type_id', 'source') - ); - - $membership->contact_id = $data[$membership->id]['contact_id']; - $membership->membership_type_id = $data[$membership->id]['membership_type_id']; - $membership->source = CRM_Utils_Array::value('source', $data[$membership->id]); - } - - // since we are going to create activity record w/ - // individual contact as a target in case of on behalf signup, - // so get the copy of organization id, CRM-5551 - $realMembershipContactId = $membership->contact_id; - - // create activity source = individual, target = org CRM-4027 - $targetContactID = NULL; - if (!empty($params['is_for_organization'])) { - $targetContactID = $membership->contact_id; - $membership->contact_id = CRM_Utils_Array::value('userId', $ids); - } - - if (empty($membership->contact_id) && (!empty($membership->owner_membership_id))) { - $membership->contact_id = $realMembershipContactId; - } - - if (!empty($ids['membership']) && $activityType != 'Membership Signup') { - CRM_Activity_BAO_Activity::addActivity($membership, $activityType, $targetContactID); - } - elseif (empty($ids['membership'])) { - CRM_Activity_BAO_Activity::addActivity($membership, $activityType, $targetContactID); - } - - // we might created activity record w/ individual - // contact as target so update membership object w/ - // original organization id, CRM-5551 - $membership->contact_id = $realMembershipContactId; - } - $transaction->commit(); self::createRelatedMemberships($params, $membership); @@ -1854,7 +1812,6 @@ INNER JOIN civicrm_contact contact ON ( contact.id = membership.contact_id AND $is_test, $membershipID, TRUE ); if ($currentMembership) { - $activityType = 'Membership Renewal'; $renewalMode = TRUE; // Do NOT do anything. @@ -1880,11 +1837,8 @@ INNER JOIN civicrm_contact contact ON ( contact.id = membership.contact_id AND if ($contributionRecurID) { $memParams['contribution_recur_id'] = $contributionRecurID; } - //CRM-19678 : Pay later chosen on current membership then create 'Membership Signup' Activity to track - if ($isPayLater) { - $activityType = 'Membership Signup'; - } - $membership = self::create($memParams, $ids, FALSE, $activityType); + + $membership = self::create($memParams, $ids, FALSE); return array($membership, $renewalMode, $dates); } @@ -1978,8 +1932,6 @@ INNER JOIN civicrm_contact contact ON ( contact.id = membership.contact_id AND } else { // NEW Membership - - $activityType = 'Membership Signup'; $memParams = array( 'contact_id' => $contactID, 'membership_type_id' => $membershipTypeID, @@ -2058,7 +2010,7 @@ INNER JOIN civicrm_contact contact ON ( contact.id = membership.contact_id AND // Load all line items & process all in membership. Don't do in contribution. // Relevant tests in api_v3_ContributionPageTest. $memParams['line_item'] = $lineItems; - $membership = self::create($memParams, $ids, FALSE, $activityType); + $membership = self::create($memParams, $ids, FALSE); // not sure why this statement is here, seems quite odd :( - Lobo: 12/26/2010 // related to: http://forum.civicrm.org/index.php/topic,11416.msg49072.html#msg49072 diff --git a/tests/phpunit/api/v3/ContributionTest.php b/tests/phpunit/api/v3/ContributionTest.php index 1c5dcac1d3..d7f79e519a 100644 --- a/tests/phpunit/api/v3/ContributionTest.php +++ b/tests/phpunit/api/v3/ContributionTest.php @@ -2739,7 +2739,7 @@ class api_v3_ContributionTest extends CiviUnitTestCase { )); $this->assertEquals(1, $logs['count']); $this->assertEquals($stateOfGrace, $membership['status_id']); - $contribution = $this->callAPISuccess('contribution', 'completetransaction', array('id' => $this->_ids['contribution'])); + $this->callAPISuccess('contribution', 'completetransaction', array('id' => $this->_ids['contribution'])); $membership = $this->callAPISuccess('membership', 'getsingle', array('id' => $this->_ids['membership'])); $this->assertEquals(date('Y-m-d', strtotime('yesterday + 1 year')), $membership['end_date']); $this->callAPISuccessGetSingle('LineItem', array( @@ -2749,12 +2749,6 @@ class api_v3_ContributionTest extends CiviUnitTestCase { $logs = $this->callAPISuccess('MembershipLog', 'get', array( 'membership_id' => $this->_ids['membership'], )); - //CRM-19600: Ensure that 'Membership Renewal' activity is created after successful membership regsitration - $activity = $this->callAPISuccess('Activity', 'get', array( - 'activity_type_id' => 'Membership Renewal', - 'source_record_id' => $contribution['id'], - )); - $this->assertEquals(1, $activity['count']); $this->assertEquals(2, $logs['count']); $this->assertNotEquals($stateOfGrace, $logs['values'][2]['status_id']); $this->cleanUpAfterPriceSets(); @@ -2832,6 +2826,14 @@ class api_v3_ContributionTest extends CiviUnitTestCase { 'source_record_id' => $this->_ids['contribution'], )); $this->assertEquals(0, $activity['count']); + + //Case 4 (CRM-19600): Assert that Membership Renewal Activity is created via online contribution + $this->callAPISuccess('contribution', 'completetransaction', array('id' => $this->_ids['contribution'])); + $activity = $this->callAPISuccess('Activity', 'get', array( + 'activity_type_id' => 'Membership Renewal', + 'source_record_id' => $this->_ids['contribution'], + )); + $this->assertEquals(1, $activity['count']); } /** -- 2.25.1