Switch addActivity to use API4 #17274

mattwire · 2020-05-10T10:55:54Z

Overview

Debugging various issues with payments keeps leading me back to this addActivity function. In a number of cases source_contact_id is not being set and this triggers the fatal in addActivity which triggers a rollback of the transaction.
What that usually means is that payment was taken but doesn't get recorded in CiviCRM because fatal() rolls back the transaction.

Before

Fatal error causes important information to be lost (such as contribution records) which upsets people because they've actually paid...

After

Switch to calling API4 which throws an exception which can be handled at the form layer, easily debugged and doesn't rollback the transaction.

Technical Details

If the PR involves technical details/changes/considerations which would not be manifest to a casual developer skimming the above sections, please describe the details here.

Comments

Adding activities is important but not as important as the actual contribution/membership/participant data..

@eileenmcnaughton @artfulrobot I come across a lot of sites where for some users (normally anonymous - ie. creating a contact) the site crashes on the thankyou page and then information is not recorded properly in CiviCRM. This fatal is the cause of a lot of those problems.

civibot · 2020-05-10T10:55:57Z

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

CRM/Activity/BAO/Activity.php

eileenmcnaughton · 2020-05-10T23:33:34Z

@mattwire I definitely agree in principle - a bit of a war with jenkins to be fought to get the tests to pass. Superficially it seems maybe the api is not setting all the required defaults but I didn't dig

eileenmcnaughton · 2020-05-10T23:34:43Z

CRM/Activity/BAO/Activity.php

@@ -1737,10 +1735,14 @@ public static function addActivity(
      'is_test' => $activity->is_test,
      'status_id' => CRM_Core_PseudoConstant::getKey('CRM_Activity_BAO_Activity', 'activity_status_id', 'Completed'),


Note APIv4 pseudoconstant syntax is 'status_id:name' => 'Completed'

artfulrobot · 2020-05-11T06:42:02Z

CRM/Activity/BAO/Activity.php

-   * @return bool|NULL
+   * @throws \API_Exception
+   * @throws \CiviCRM_API3_Exception
+   * @throws \Civi\API\Exception\UnauthorizedException


Is this still true if we add setcheckpermissions(FALSE) as suggested.

Well phpstorm gets upset if you don't have that there....

I meant, won't it always return NULL, and never a bool? Surely your ~~proprietary corporate overlords~~helpful developer assistant can allow you to have a function that does not return a value? ;-)

artfulrobot · 2020-05-11T06:43:50Z

CRM/Activity/BAO/Activity.php

@@ -1684,7 +1684,9 @@ public static function getContactActivity($contactId) {
   * @param array $params
   *   Activity params to override.
   *
-   * @return bool|NULL
+   * @throws \API_Exception
+   * @throws \CiviCRM_API3_Exception


I couldn't see where it calls api3 any more?

artfulrobot · 2020-05-11T06:46:18Z

CRM/Activity/BAO/Activity.php

@@ -1684,7 +1684,9 @@ public static function getContactActivity($contactId) {
   * @param array $params
   *   Activity params to override.
   *
-   * @return bool|NULL


Just on this cleanup: It seems weird to me that a create function doesn't at least return the ID of the thing it created. I know the original function doesn't, so maybe we should have a @todo comment in here?

artfulrobot · 2020-05-11T06:47:49Z

This looks sensible to me, though I still struggle getting my head around the function and role of API4 because I tend to think of it as

High level » API4 » BAO » DAO » Low level

So seeing code that jumps to the lower level BAO then to jump back to API4 seems jarring. I guess it's historical and that code that calls this function should not be doing so and should be calling API4 directly? If this is correct we should put a note to this effect in the docblock.

a war with jenkins to be fought to get the tests to pass.

Why is that?

eileenmcnaughton · 2020-05-18T22:15:09Z

I think this one might be a clue to the failing tests

https://test.civicrm.org/job/CiviCRM-Core-PR/34077/testReport/junit/(root)/CRM_Contribute_Form_ContributionTest/testPartialPaymentWithCreditCard/

"Cannot pass id to Create action. Use Update action instead."

I think using 'Save' is a reasonable workaround - although the syntax is a bit different

mattwire · 2020-05-26T15:18:19Z

@eileenmcnaughton Tests are now passing. I don't know if you can make sense of the changes I had to make to SearchTest? I couldn't work out how this would be related to this change or if it matters.

The other test failures were fixed by switching to Create/Update as appropriate.

colemanw · 2020-05-26T20:36:57Z

CRM/Activity/BAO/Activity.php

+    // Despite being called "addActivity" this function actually both adds and updates activities
+    // So we have to select the right Api4 action depending on if we have an activity ID.
+    if (!empty($activityParams['id'])) {
+      $activityApi4 = \Civi\Api4\Activity::update();
+    }
+    else {
+      $activityApi4 = \Civi\Api4\Activity::create();
    }
+    $activityApi4
+      ->setValues($activityParams)
+      ->setCheckPermissions(FALSE)
+      ->execute();


I think this can be simplified to:

Suggested change

// Despite being called "addActivity" this function actually both adds and updates activities

// So we have to select the right Api4 action depending on if we have an activity ID.

if (!empty($activityParams['id'])) {

$activityApi4 = \Civi\Api4\Activity::update();

}

else {

$activityApi4 = \Civi\Api4\Activity::create();

}

$activityApi4

->setValues($activityParams)

->setCheckPermissions(FALSE)

->execute();

\Civi\Api4\Activity::save()

->addRecord($activityParams)

->setCheckPermissions(FALSE)

->execute();

eileenmcnaughton · 2020-05-26T20:48:50Z

tests/phpunit/CRM/Activity/Form/SearchTest.php

@@ -49,7 +49,6 @@ public function testSearch() {
        'source_record_id' => '1',
        'activity_type_id' => '6',
        'activity_type' => 'Contribution',
-        'activity_is_test' => '0',


@colemanw this suggests that passing

'is_test' => $activity->is_test,

is passing null or something & resulting in the db default not being applied? I can't quite think my way through what's happening here

eileenmcnaughton · 2020-05-27T03:11:22Z

@mattwire I think the requirement for test changes IS a problem - is_test is winding up empty not 0. I also noticed today that apiv4 activity puts NULL in activity_date_time (https://lab.civicrm.org/dev/core/-/issues/1782)

@colemanw might have some thoughts but I think we need to tweak the api not the test.

mattwire · 2020-06-01T12:23:46Z

See #17450

…so the form can handle the error)

…ction I took a look at civicrm#17274 which has blocking test failures - but felt that the shared function was adding nothing and simply using the api to create the activity made more sense. The shared function does a lot of silly wrangling for very little shared functionality and is hard to read. In this call only 2 params are passed in - so most of the wranglingg doesn't apply anyway. I ensured the 2 JIRA issues referenced in the removed code have test cover (one already had a test written by Monish & I added in the campaign check

eileenmcnaughton · 2020-07-18T02:59:42Z

@mattwire I took a look at this & managed to get past the test fail & to extend the cover a bit - but I felt much happier by-passing this function than altering it
#17881

…ction I took a look at civicrm#17274 which has blocking test failures - but felt that the shared function was adding nothing and simply using the api to create the activity made more sense. The shared function does a lot of silly wrangling for very little shared functionality and is hard to read. In this call only 2 params are passed in - so most of the wranglingg doesn't apply anyway. I ensured the 2 JIRA issues referenced in the removed code have test cover (one already had a test written by Monish & I added in the campaign check

mattwire · 2020-07-18T11:00:24Z

Closing in favour of #17881

…ction I took a look at civicrm#17274 which has blocking test failures - but felt that the shared function was adding nothing and simply using the api to create the activity made more sense. The shared function does a lot of silly wrangling for very little shared functionality and is hard to read. In this call only 2 params are passed in - so most of the wranglingg doesn't apply anyway. I ensured the 2 JIRA issues referenced in the removed code have test cover (one already had a test written by Monish & I added in the campaign check

civibot bot added the master label May 10, 2020

eileenmcnaughton reviewed May 10, 2020

View reviewed changes

CRM/Activity/BAO/Activity.php Outdated Show resolved Hide resolved

eileenmcnaughton reviewed May 10, 2020

View reviewed changes

artfulrobot reviewed May 11, 2020

View reviewed changes

mattwire force-pushed the addactivitytoapi4 branch from 8334029 to 97084be Compare May 18, 2020 09:26

mattwire force-pushed the addactivitytoapi4 branch from 97084be to a32bea7 Compare May 25, 2020 13:08

colemanw reviewed May 26, 2020

View reviewed changes

eileenmcnaughton reviewed May 26, 2020

View reviewed changes

mattwire mentioned this pull request Jun 1, 2020

dev/core#1782 Set activity_date_time and created_date to default to current_timestamp #17450

Merged

mattwire changed the title ~~Switch addActivity to use API4 (and throw exception instead of fatal so the form can handle the error)~~ Pending #17450 Switch addActivity to use API4 (and throw exception instead of fatal so the form can handle the error) Jun 6, 2020

mattwire added the needs-work-not-review-ready Submitter required to take action, will be closed after 1-2 weeks in this state label Jun 6, 2020

mattwire added 2 commits June 24, 2020 13:06

Switch addActivity to use API4 (and throw exception instead of fatal …

7186ebc

…so the form can handle the error)

Tweak SearchTest

35c7c15

mattwire force-pushed the addactivitytoapi4 branch from 9eed097 to 35c7c15 Compare June 24, 2020 12:06

mattwire changed the title ~~Pending #17450 Switch addActivity to use API4 (and throw exception instead of fatal so the form can handle the error)~~ Switch addActivity to use API4 (and throw exception instead of fatal so the form can handle the error) Jun 24, 2020

mattwire changed the title ~~Switch addActivity to use API4 (and throw exception instead of fatal so the form can handle the error)~~ Switch addActivity to use API4 Jun 24, 2020

Break tests

b26da89

mattwire force-pushed the addactivitytoapi4 branch from 4b3569c to b26da89 Compare June 24, 2020 15:55

eileenmcnaughton mentioned this pull request Jul 18, 2020

Call apiv4 from Contribution create rather than fugly addActivity function #17881

Merged

mattwire closed this Jul 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch addActivity to use API4 #17274

Switch addActivity to use API4 #17274

mattwire commented May 10, 2020

civibot bot commented May 10, 2020

eileenmcnaughton commented May 10, 2020

eileenmcnaughton May 10, 2020 •

edited by colemanw

Loading

artfulrobot May 11, 2020

eileenmcnaughton May 18, 2020

artfulrobot May 19, 2020

artfulrobot May 11, 2020

artfulrobot May 11, 2020

artfulrobot commented May 11, 2020 •

edited

Loading

eileenmcnaughton commented May 18, 2020

mattwire commented May 26, 2020

colemanw May 26, 2020

eileenmcnaughton May 26, 2020

eileenmcnaughton commented May 27, 2020

mattwire commented Jun 1, 2020

eileenmcnaughton commented Jul 18, 2020

mattwire commented Jul 18, 2020

		@@ -1737,10 +1735,14 @@ public static function addActivity(
		'is_test' => $activity->is_test,
		'status_id' => CRM_Core_PseudoConstant::getKey('CRM_Activity_BAO_Activity', 'activity_status_id', 'Completed'),

Switch addActivity to use API4 #17274

Switch addActivity to use API4 #17274

Conversation

mattwire commented May 10, 2020

Overview

Before

After

Technical Details

Comments

civibot bot commented May 10, 2020

eileenmcnaughton commented May 10, 2020

eileenmcnaughton May 10, 2020 • edited by colemanw Loading

Choose a reason for hiding this comment

artfulrobot May 11, 2020

Choose a reason for hiding this comment

eileenmcnaughton May 18, 2020

Choose a reason for hiding this comment

artfulrobot May 19, 2020

Choose a reason for hiding this comment

artfulrobot May 11, 2020

Choose a reason for hiding this comment

artfulrobot May 11, 2020

Choose a reason for hiding this comment

artfulrobot commented May 11, 2020 • edited Loading

eileenmcnaughton commented May 18, 2020

mattwire commented May 26, 2020

colemanw May 26, 2020

Choose a reason for hiding this comment

eileenmcnaughton May 26, 2020

Choose a reason for hiding this comment

eileenmcnaughton commented May 27, 2020

mattwire commented Jun 1, 2020

eileenmcnaughton commented Jul 18, 2020

mattwire commented Jul 18, 2020

eileenmcnaughton May 10, 2020 •

edited by colemanw

Loading

artfulrobot commented May 11, 2020 •

edited

Loading