dev/core#1118 correct filteration by case type, re-use parent where()

[deepak-srivastava]

Overview

Applying the case type filter in the case summary report CRM_Report_Form_Case_Summary is resulting in two case types being reported where only one is selected in the case type filter.

Before

After

Technical Details

REGEXP filtering for case type:

WHERE case_civireport.case_type_id REGEXP '[[:cntrl:]]5[[:cntrl:]]'

Results in records where case type id consists of 5 (for this example).

The clause looks old and appears to be based on assumption that case could be based on multiple case types. Case type however is an integer column and could use where() clause of CRM/Report/Form.php.

Comments

Anything else you would like the reviewer to note

[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]

@deepak-srivastava the test failure relates -

Stacktrace
api_v3_ReportTemplateTest::testReportTemplateGetRowsAllReports with data set #23 ('case/summary')
Failure in api call for report_template getrows:  Type is not defined for field case_type_id
#0 [internal function]: PHPUnit\Util\ErrorHandler::handleError(512, 'Type is not def...', '/home/jenkins/b...', 1949, Array)

I would say you have exposed the fact that the metadata in the _construct function is not defining 'type' for one or more fields.

On the bright side that tells use the report has test cover & I think this can get through without a test if we can get someone to review - maybe @demeritcowboy or @mattwire will once you've got tests passing

[demeritcowboy]

I'm happy to review once the tests pass - I remember the stackexchange post.

[deepak-srivastava]

Test case seems to have passed.

[demeritcowboy]

• General standards
  • [r-explain] PASS
  • [r-user] PASS
  • [r-doc] PASS
  • [r-run] PASS
    • In the test site here I added a new case type and using some magic changed its ID to 13 so that the original problem could be verified.
    • Tested various filters, not just the case type.
    • The patch removes the _having = '' line, but I'm thinking that was old and leftover anyway. Group by with sorting seems to all work. It doesn't give you section totals in the section headers it just shows () but it didn't before either (as compared to e.g. the constituent summary report which does).
    • Any glitches or weirdness I found was pre-existing.
• Developer standards
  • [r-tech] PASS
    • There's an alternate PR (dev/core#1118 Case summary report filters incorrectly for case type #14842) that was added after which I haven't tested, but I'm thinking since this one centralizes things this one might be preferable?
    • And it seems like it would be rare where a site that was first installed before 4.5 has a mix of case types some that have CHAR(1) and some that don't, because it would mean something like they have custom code that is still creating cases using CHAR(1), which is possible but then stop doing that.
  • [r-code] PASS
  • [r-maint] Undecided I didn't look into what coverage the earlier test failure was actually covering.
  • [r-test] PASS

[eileenmcnaughton]

Merging based on @demeritcowboy review - note that I agree with the analysis that this is simplifying the code & re-using handling & avoiding reinventing the wheel