dev/report/15 Add fix and tests for contact subtype report filter

[elisseck]

Overview

Adding a PR for the fix by @MegaphoneJon along with some tests for the filter options that are now available.

Before

On the contact summary report (or other reports), using the 'IS NULL' or 'IS NOT NULL' filters for "Contact Subtype" would result in a DB error.

After

Filters are working as expected and tests have been added to keep it that way.

Technical Details

This is a WIP because i've written the tests how I believe they're supposed to go (I've also tried the /api/v3/ReportTemplateTest.php style tests) ... I cannot seem to get them to pass. I've tried adding the null and not null filters a bunch of ways but the rows returned are always 2 (all) instead of the expected 1.

The functionality from Jon's commit, however, is working exactly as it should if I actually go run the report through the GUI and use the filters... so this is just a problem with the tests i've written.

[civibot]

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

[eileenmcnaughton]

So as a test this worked for me in terms of hitting the right places in the code (it didn't actually pass but that was because it's not handling NULL in contact_sub_type)

diff --git a/tests/phpunit/api/v3/ReportTemplateTest.php b/tests/phpunit/api/v3/ReportTemplateTest.php
index beae3d5b16..d98f11ff08 100644
--- a/tests/phpunit/api/v3/ReportTemplateTest.php
+++ b/tests/phpunit/api/v3/ReportTemplateTest.php
@@ -1258,4 +1258,24 @@ class api_v3_ReportTemplateTest extends CiviUnitTestCase {
     ]);
   }
 
+  /**
+   * Test contact subtype filter on summary report.
+   *
+   * @throws \CRM_Core_Exception
+   */
+  public function testContactSubtypeNotIn() {
+    $this->individualCreate(['contact_sub_type' => ['Student', 'Parent']]);
+    $this->individualCreate();
+
+    $rows = $this->callAPISuccess('report_template', 'getrows', [
+      'report_id' => 'contact/summary',
+      'contact_sub_type_op' => 'notin',
+      'contact_sub_type_value' => ['Parent'],
+      'fields' => [
+        'sort_name',
+      ],
+    ]);
+    $this->assertEquals(1, $rows['count']);
+  }
+
 }

Captured query

SELECT contact_civireport.sort_name as civicrm_contact_sort_name, contact_civireport.id as civicrm_contact_id, (address_civireport.street_number % 2) as civicrm_address_address_odd_street_number  
        FROM civicrm_contact contact_civireport  
                 LEFT JOIN civicrm_address address_civireport
                           ON (contact_civireport.id =
                               address_civireport.contact_id) AND
                               address_civireport.is_primary = 1
 WHERE ( contact_civireport.contact_sub_type NOT LIKE '%Parent%' )   ORDER BY contact_civireport.sort_name ASC  LIMIT 25

[eileenmcnaughton]

Just a note that we need to drop 5.15.1 - if this is ready in time it should be included - I don't know exactly when it will drop - need to fit around @totten

[elisseck]

@eileenmcnaughton maybe i'm just being dense but what i'm having trouble with is that the tests for the null and not null filters (the ones that were causing db errors so we'd want to test that they're working) don't seem to line up with what i'm seeing in the GUI i.e. when I put "IS NOT NULL" to simulate someone using that filter, for example

/**
   * Test contact subtype filter on summary report.
   *
   * @throws \CRM_Core_Exception
   */
  public function testContactSubtypeNotNull() {
    $this->individualCreate(['contact_sub_type' => ['Student', 'Parent']]);
    $this->individualCreate();

    $rows = $this->callAPISuccess('report_template', 'getrows', [
      'report_id' => 'contact/summary',
      'contact_sub_type_op' => 'IS NOT NULL', 
(I have also tried 1, true, '', or omitting the contact_sub_type_value line altogether for the same result or a worse result where it's just an error where the query generated is as below. 
What's supposed to go here for a null filter?)
      'contact_sub_type_value' => NULL,
      'fields' => [
        'sort_name',
      ],
    ]);
    $this->assertEquals(1, $rows['count']);
  }

The query generated by 'contact_sub_type_op' => 'IS NOT NULL', 'contact_sub_type_value' => TRUE looks like this when generated by the API (I copied this from an sql error log):

SELECT contact_civireport.sort_name as civicrm_contact_sort_name, contact_civireport.id as civicrm_contact_id, (address_civireport.street_number % 2) as civicrm_address_address_odd_street_number FROM civicrm_contact contact_civireport LEFT JOIN civicrm_address address_civireport ON (contact_civireport.id = address_civireport.contact_id) AND address_civireport.is_primary = 1 WHERE ( contact_civireport.contact_sub_type LIKE '%'%' ) ORDER BY contact_civireport.sort_name ASC LIMIT 25

Any other "IS NOT NULL" query i've tried seems to bring back all contacts as result rows whether subtype actually is null or not.

However when I use the "IS NOT NULL" filter from the actual report screen, I get the correct results from the report and the generated query looks like:

SELECT SQL_CALC_FOUND_ROWS contact_civireport.sort_name as civicrm_contact_sort_name, contact_civireport.id as civicrm_contact_id, (address_civireport.street_number % 2) as civicrm_address_address_odd_street_number FROM civicrm_contact contact_civireport LEFT JOIN civicrm_address address_civireport ON (contact_civireport.id = address_civireport.contact_id) AND address_civireport.is_primary = 1 WHERE ( contact_civireport.contact_sub_type IS NOT NULL ) AND ( contact_civireport.is_deceased = 0 ) AND ( contact_civireport.is_deleted = 0 ) ORDER BY contact_civireport.sort_name ASC LIMIT 0, 50

Which is what I would expect from either method.

[eileenmcnaughton]

@elisseck I think you want 'nnll' don't you?

civicrm-core/CRM/Report/Form.php

Line 1782 in 306f077

'nnll' => ts('Is not empty (Null)'),

FWIW the way I normally do this is I submit the form through the UI & capture the values in $this->_params from there (I use a breakpoint in phpstorm & the copy as var_dump function but at a pinch you could var_dump + die)

[elisseck]

@eileenmcnaughton Thank you!! That was the piece I was missing. I just pushed with tests for the null and not null filters that should be passing on top of Jon's commit.

[eileenmcnaughton]

@elisseck can you rebase this onto the 5.16 branch so it can go into the rc since it was a regression

[elisseck]

That can't be right...

[elisseck]

@eileenmcnaughton I am unsure if this is correct or not since I haven't done it before. I rebased my changes on upstream/5.16. Do I also change where this pull request merges to I guess?

[eileenmcnaughton]

I changed the base & it still just shows 2 commits - so that is good

[eileenmcnaughton]

unrelated test fail @MegaphoneJon @elisseck I feel like your collaboration on this constitutes review - if you both confirm you are happy with this PR as it stands I will merge

[elisseck]

@eileenmcnaughton I'm happy with Jon's changes and we've gotten rid of the db error that was happening with the "empty" and "not empty" filters on contact sub type. I think your non-passing test above for "not in" alludes to the fact that "not in" is still not quite working the way you'd expect with contacts that don't have a subtype at all - but there isn't a db error so that might warrant it's own issue for a different set of changes and a test at a later time?

[eileenmcnaughton]

OK, I'm going to merge this now then - since you are the reviewer for @MegaphoneJon's change