CRM-13918 add report_template.getrows, report_template.getstatistics api functions

[eileenmcnaughton]

Tests effectively create an audit of existing reports & highlight one issue specific to the report apis - if reports put important functionality into the postProcess function this is not available to the report api. I have added a new beginPostProcessCommon function (which seemed consistent with preProcessCommon) which is called after params are set & is where I recommend setting variables preparatory to generating the sql. (even without the api consideration I'm not keen on the postProcess as a choice of function to over-ride as it controls the flow).

Note that I had inconsistencies around setup & tear down of test data which meant I had to comment out the tests that checked the actual output.

The tests also identified several e-Notices, function inconsistencies & a couple of fatals that pre-exist in the report classes. I fixed one class but left the rest. This list is documented in the test itself ie.

$reportsToSkip = array(
    'activity' =>  'does not respect function signature on from clause',
    'walklist' => 'Notice: Undefined index: type in CRM_Report_Form_Walklist_Walklist line 155.
                   (suspect the select function should be removed in favour of the parent (state province field)
                  also, type should be added to state province & others? & potentially getAddressColumns fn should be
                  used per other reports',
    'contribute/repeat' => 'Reports with important functionality in postProcess are not callable via the api. For variable setting recommend beginPostProcessCommon, for temp table creation recommend From fn',
    'contribute/organizationSummary' => 'Failure in api call for report_template getrows:  Only variables should be assigned by reference line 381',
    'contribute/householdSummary' => '(see contribute/repeat) Undefined property: CRM_Report_Form_Contribute_HouseholdSummary::$householdContact LINE 260, property should be declared on class, for api accessibility should be set in beginPreProcess common',
    'contribute/topDonor' => 'construction of query in postprocess makes inaccessible ',
    'contribute/sybunt' => 'e notice - (ui gives fatal error at civicrm/report/contribute/sybunt&reset=1&force=1
                            e-notice is on yid_valueContribute/Sybunt.php(214) because at the force url "yid_relative" not "yid_value" is defined',
    'contribute/lybunt' => 'same as sybunt - fatals on force url & test identifies why',
    'event/income' => 'I do no understant why but error is Call to undefined method CRM_Report_Form_Event_Income::from() in CRM/Report/Form.php on line 2120',
    'contact/relationship' => '(see contribute/repeat), property declaration issue, Undefined property: CRM_Report_Form_Contact_Relationship::$relationType in /Contact/Relationship.php(486):',
    'case/demographics' => 'Undefined index: operatorType Case/Demographics.php(319)',
    'activitySummary' => 'Undefined index: group_bys_freq m/ActivitySummary.php(191)',
    'event/incomesummary' => 'Undefined index: title, Report/Form/Event/IncomeCountSummary.php(187)',
    'logging/contact/summary' => '(likely to be test releated) probably logging off Undefined index: Form/Contact/LoggingSummary.php(231): PHP',
    'logging/contact/detail' => '(likely to be test releated) probably logging off  DB Error: no such table',
    'logging/contribute/summary' => '(likely to be test releated) probably logging off DB Error: no such table',
    'logging/contribute/detail' => '(likely to be test releated) probably logging off DB Error: no such table',
    'survey/detail' => '(likely to be test releated)  Undefined index: CiviCampaign civicrm CRM/Core/Component.php(196)',
    'contribute/history' => 'Declaration of CRM_Report_Form_Contribute_History::buildRows() should be compatible with CRM_Report_Form::buildRows($sql, &$rows)',
);

if(in_array($reportID , array('contribute/softcredit', 'contribute/bookkeeping'))) {
  $this->markTestIncomplete($reportID . " has non enotices when calling statistics fn");

[deepak-srivastava]

Looks good. Merging it anyway since shouldn't delay this getting into core.

Few comments:

1. It seems limit is always applied. May be api user should know this, and have something to override it ?
   Sth like:
   $sql = $reportInstance->buildQuery(CRM_Utils_Array::value('is_apply_limit', $params, TRUE));
2. I think the reason postprocess has been overridden most of the time is, to modify sql and generate $rows. If we could combine buildQuery and buildRows into another func (say buildQueryAndRows), can be overridden without touching the flow, and probably could also be used to alter params?
3. Re: inconsistencies around test data - sth inline with https://github.com/civicrm/civicrm-core/blob/4.4/tests/phpunit/CRM/Report/Form/Contribute/DetailTest.php would work a lot better.
4. Not sure how CRM/Report/Form/Contribute/Detail.php fix is related here. May be fixes sth else.

[eileenmcnaughton]

1. Hmm - the standard api limit is 25 by default & the way to over-ride it is to set $params['options'] => array('limit' => 50). I'm guessing we are currently defaulting to 50? & there is a disconnect between api standard default (25) & report standard default (50). I guess as this is an api call we should default to 25 & accept default as a param using the above syntax. I would probably add a setter function for setting limit rather than making it a param on buildQuery

2. I guess we need to look at a few specific reports on that (ie. the reports identified above). It might take a little wading through. If I look at CRM_Report_Form_Contribute_Repeat I see the postProcess is about 50% over-riding buildQuery & about 50% compensating for not having declared the contribution table twice with different aliases in the _construnct fn. But there is also some special data formatting which I'm not sure where else it would obviously belong

3. test data - well I wasn't even trying to set up data - just to run it with the standard civicrm.mysql data & at least check the contact reports & I wasn't even finding the standard domain contact was reliably there - so I didn't want to go too much further. I hadn't started to think as ambitiously as testing complex data sets.

4. the fix on the specific report is because the phpunit tests all enforce e-Notice compliance - so I had to make those changes to make the test pass. I didn't do all the other minor changes required for the tests.

[deepak-srivastava]

Re: 1 - Ok i'll leave that to you. My main concern was user should have a choice to retrieve limit less result.

Re: 2 - I would say repeat is an exception and doesn't really fit into the framework.

Re: 3 - OK. Thats fine for now.

Re: 4 - Ok.