CRM-18502 - Fix payment reminder activity creation

[mjwright]

Call to civicrm_api() fails due to missing "version" parameter, but is undetected due to test for is_a 'CRM_Core_Error'. Changed to use civicrm_api3() and properly test result for civicrm_error.

---

• CRM-18502: Pledge payment reminder activity fails to record

[eileenmcnaughton]

since you are calling the api to create this it could be simplified to

'activity_date_time' => 'now',

or simply omit the line since I am pretty sure that's the default for new activities via the api.

[eileenmcnaughton]

Have I ever tried to sell you on the joys of writing unit tests....?

[mjwright]

I haven't gotten the sales pitch, but I'm listening. Should there be a unit test for this PR?

[eileenmcnaughton]

In a perfect world there would be a test on every pr... We'll never get to 100% - but tests are your guarantee that what you fix stays fixed.

There are no existing tests on that function :-(

I just added a test for another job here https://github.com/civicrm/civicrm-core/pull/8293/files#diff-a95d6214aaf5b8f4e1cdb78f8b95d676R300

(& am building up tests on top of that that will increase the coverage from there)

[eileenmcnaughton]

In terms of getting started - there are a few resources - generally starting from buildkit is easiest - this stuff is still relevant https://wiki.civicrm.org/confluence/display/CRMDOC/Testing#Testing-PHPunittests - although I think there are a few more methods available now too.

[JoeMurray]

Marty as Release Manager this month, I'm trying to recruit people to help pare down the backlog of almost 100 PRs, some going back to last summer. I'm wondering if you would be able to help QA another PR if I got someone to QA this PR?

[monishdeb]

Tested and it works fine. @eileenmcnaughton @mjwright is there any followup pr for unit test or this PR can be merged for now?

[monishdeb]

Jenkin, test this please

[JoeMurray]

@PalanteJon could you help @mjwright get started with a test for this?

[MegaphoneJon]

@mjwright I'm going to try to help! That said, I've only written API tests - and while this is API-related, the API is working fine. That said, I've been studying up on unit tests, so I think I understand what's needed here. So this is us learning together.

The unit test should be testing the function that's affected. Unfortunately, updatePledgeStatus is a long function that does a lot of stuff, which is annoying for reasons I'll get to. So I'll outline two approaches:

test as is

In your dev environment, navigate to <civiroot>/tests/phpunit/CRM/Pledge/BAO/PledgeTest.php. Create a function called testCreatePledgeActivity or similar. Your test will look something like this. Note that I didn't run any of this code - it's pseudo-pseudocde:

  /** 
   *  Test that pledges create an activity.
   */  
  public function testCreatePledgeActivity() {
    // These are the wrong params - put whatever legit params make sense here to pass to updatePledgeStatus!  If you don't know, add a debug statement to dump params at the top of updatePledgeStatus to screen and/or log, then run manually.  Then copy/paste those params.
    $params = array(
      'contact_id' => $this->_contactId,
      'frequency_unit' => 'month',
      'frequency_interval' => 1,
      etc.
    );  

    // Now call the function!
    CRM_Core_BAO_Pledge::updatePledgeStatus($params);

    // Now test that the Activity was created.
    $this->callAPISuccess('Activity', 'get', array(
      'subject' => $params['subject'],
      'source_contact_id' => $params['contactId'],
      'source_record_id' => $params['paymentId'],
      'assignee_contact_id' => $params['contactId'],
      'activity_type_id' => CRM_Core_OptionGroup::getValue('activity_type',
        'Pledge Reminder',
        'name'
      ),
    ));

  }

You can run the test with:

cd /path-to/civicrm/tools
scripts/phpunit --filter testCreatePledgeActivity CRM_Pledge_BAO_PledgeTest

Alternative approach: refactor

• Make a new function in this file (maybe named createPledgeActivity).
• Throw the code from lines 1072-1094 into it.
  So now line 1072 might look something more like:

self::createPledgeActivity($subject, $contactId, $paymentId, $details);

If you've refactored correctly, everything should work exactly as it had before you changed it. But now you can write a test that tests ONLY those 20 lines, not the 300+ lines of the entire function. That prevents the test failing if something breaks elsewhere in that function.

Hope that this is enough to get you started! Let me know if you have any other questions - you can find me on chat.civicrm.org as @JungleBird.

[eileenmcnaughton]

nice write-up @PalanteJon

[JoeMurray]

Great stuff @PalanteJon - thanks! For some general background see https://wiki.civicrm.org/confluence/display/CRM/Writing+a+PHPUnit+testcase+HOWTO and even https://wiki.civicrm.org/confluence/display/CRMDOC/Testing#Testing-PHPunittests

@eileenmcnaughton would you see a place to add some of this 'how to' guidelines stuff in terms of refactoring and even directories?

[monishdeb]

@mjwright are you going to update this PR or create a new, to add a unit test for it ?

[MegaphoneJon]

@monishdeb This isn't my PR, so no. I think you meant to ask @mjwright.

[monishdeb]

Sorry my bad :( edited.

[eileenmcnaughton]

I think you should merge this - I am very keen to help @mjwright to get his first unit test written - but lets not keep this one stuck because it is a clear cut improvement