report#24 - Case Detail report - fix 'Active role?' when no relationships

[aydun]

Overview

The 'Active Role?' filter assumes relationships will exist and wrongly excludes them

Before

The 'Active Role?' filter only offers 'Yes' or 'No' - but both options exclude cases without relationships.

After

The 'Active Role?' filter offers 'Yes', 'No' or 'Any' - 'Yes' & 'No' operate as before when relationships exist. 'Any' does not exclude cases where no relationships exist.

Technical Details

Changes 'Active Role?' and 'Is Deleted?' to use default behaviour for booleans.

Comments

https://lab.civicrm.org/dev/report/issues/24

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

[seamuslee001]

ping @demeritcowboy @alifrumin

[seamuslee001]

Jenkins re test this please

[demeritcowboy]

I can give it a spin. The case detail report has the problem but the case summary one seems like it was updated a while ago to allow "neither" and I can't reproduce on the summary report even with a case that has no relationships, but anyway it makes sense to standardize the code in both.

[demeritcowboy]

It's good just have two easily fixable things.

• General standards
  • [r-explain] PASS
    • Just the note that it's only the case detail report where I can reproduce it since the summary report was already updated a while ago.
  • [r-user] PASS
  • [r-doc] PASS
  • [r-run] PASS
    • There's a pre-existing issue where you can end up with duplicate rows in the summary report, but it's not because of the PR.
• Developer standards
  • [r-tech] PASS
  • [r-code] Issue
    • This line also needs updating similar to line 377 in the summary report otherwise you get a slew of notices if you choose to display the deleted column: https://github.com/civicrm/civicrm-core/blob/5.19.3/CRM/Report/Form/Case/Detail.php#L696
    • But then that made me realize if you run a report and then export and you have 100,000 cases it's going to call that enhanced_boolean() function 100,000 times. For both reports, could the return value from enhanced_boolean() be put in a local variable and then use the local variable in alterDisplay?
  • [r-maint] Undecided
  • [r-test] PASS

[demeritcowboy]

Actually the function call thing is probably a micro-optimization that's not necessary but the line in the detail report does need updating.

[yashodha]

@aydun Do you think similar changes need to be made in other reports as well for deleted contacts ?

[aydun]

Thanks for testing @demeritcowboy I've changed the description to only report an issue in the Detail report.

The default behaviour for CRM_Utils_Type::T_BOOLEAN is ok, except that the reports were overriding some of that. This simpler version uses the defaults and also avoids rewriting the 'is_deleted'.

@yashodha - no, the revised changes bring this more into line with other reports.

[demeritcowboy]

Looks good! It just creates one minor visual change in the output from before, but as noted above it's consistent with BOOLEAN in other reports.

• General standards
  • [r-explain] PASS
  • [r-user] PASS
  • [r-doc] PASS
  • [r-run] PASS
    • Also checked that if you have a saved instance from before using these fields it still retains it.
    • (Minor Change) If you include the deleted column in the output, it no longer outputs the word "No" for non-deleted cases.
• Developer standards
  • [r-tech] PASS
  • [r-code] PASS
  • [r-maint] Undecided
  • [r-test] PASS

[demeritcowboy]

@yashodha or anyone are there any issues - this seems good to go?

[yashodha]

@demeritcowboy Looks good, I am happy to merge.