(dev/core#865) Result filter criteria doesn't show IS NULL/IS NOT NUL…

[yashodha]

…L for operations

Overview

We can not search all contributions that are not associated with a campaign currently in report. We should expose IS NULL/IS NOT NULL for other operators as well

Before

After

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

[demeritcowboy]

What's the reason for removing the $element assignment? It looks like it's used just below on line 1309.

[yashodha]

@demeritcowboy : good catch, will rectify.

[yashodha]

@demeritcowboy : can you please check now?

[demeritcowboy]

• General standards
  • [r-explain] PASS
  • [r-user] PASS
  • [r-doc] PASS
  • [r-run] PASS Since it's a global change that is available in all reports not just contribution campaigns, I tested a couple other reports too. Results all worked. An interesting nice side effect is that for example you can do something like run the constituent summary report to find all contacts that don't have the gender field filled in, etc.
• Developer standards
  • [r-tech] PASS
  • [r-code] PASS
  • [r-maint] COMMENTS Maybe a unit test where you select the not null filter. It could be as simple as some in tests/phpunit/api/v3/ReportTemplateTest.php, e.g.:

      $template = 'contribute/summary';
      $rows = $this->callAPISuccess('report_template', 'getrows', [
      'report_id' => $template,
      // pass in some filter set to Is Not Null
     ]); 
    

  • [r-test] Undecided The current failure seems unrelated.

[eileenmcnaughton]

I agree the change makes sense. I also agree the test fail is unrelated. I think we can arguably get away with adding a unit test to cover this since it is not really different to the logic for integers etc and since this is otherwise mergeable due to @demeritcowboy's review I'm going to proceed to merge

We hit this on wanting 'in' on strings - albeit via a url for us