Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dev/core#1366 - case audit printReport parameter is no longer used #16670

Merged
merged 1 commit into from Mar 29, 2020

Conversation

demeritcowboy
Copy link
Contributor

@demeritcowboy demeritcowboy commented Mar 2, 2020

Overview

The printReport parameter to run() is no longer used because it's now always TRUE.

If you look at the files changed tab with the ignore whitespace setting turned on it's a little cleaner.

Technical Details

To see that it's no longer used, start by grepping for CRM_Case_Audit_Audit (exclude AuditConfig: grep -r CRM_Case_Audit_Audit * | grep -v CRM_Case_Audit_AuditConfig)

  • CRM/Case/XMLProcessor/Report.php: calls it with TRUE.
  • Within its own run function: Instantiates itself but doesn't call run.

Then to understand why it's never used, it's because the feature it supported was purposely bypassed in https://github.com/civicrm/civicrm-core/pull/15998/files#diff-fdb4ad333e7ba641428562199adc7dee, and thru some intermediate cleanup all other references to it have also been removed.

Comments

It's now also easier to see why PR #16660 was able to remove the Audit.tpl file since there's no references to it. The order of these PRs was backwards but it's the same result.

There's still more that can be removed but will be separate PRs.

@civibot
Copy link

civibot bot commented Mar 2, 2020

(Standard links)

@civibot civibot bot added the master label Mar 2, 2020
@demeritcowboy demeritcowboy changed the title [PENDING PR 16669] dev/core#1366 - case audit printReport parameter is no longer used dev/core#1366 - case audit printReport parameter is no longer used Mar 2, 2020
@demeritcowboy
Copy link
Contributor Author

jenkins test this please

1 similar comment
@demeritcowboy
Copy link
Contributor Author

jenkins test this please

@demeritcowboy
Copy link
Contributor Author

jenkins retest this please

@jaapjansma
Copy link
Contributor

Betty and I are reviewing PR's and we came across this one.

@demeritcowboy we have a couple of questions:

  1. How can we test this as a user? Could you help us with a step-by-step instruction?
  2. What is the before and after scenario of this PR?
  3. How does this PR relates to Alternate to #16650 - On Case Audit/Print Report richtext details field is getting escaped when system is non-english #16659

@demeritcowboy
Copy link
Contributor Author

Hi,
This one is more of a code review so you would do the grep mentioned above and confirm the bullet points listed in the description. The only thing you could run is to check that nothing has changed when you do an audit report from manage case, i.e. create a case, then on manage case click on "print report" in the third row of the header and see that it still does the same as before, and similarly on manage case choose the dropdown for Activity Audit in the second row of the header and see that it does the same as before.

@eileenmcnaughton
Copy link
Contributor

I did a code search & this is only called from one place & only with the param being TRUE so the parameter is not needed & hence this makes sense

@demeritcowboy looks like it's not the only unused param on that function!

@eileenmcnaughton eileenmcnaughton merged commit 8d557f6 into civicrm:master Mar 29, 2020
@demeritcowboy
Copy link
Contributor Author

Thanks. Yes I think there's a lot of audit-related code that can be removed. This is the last in the sequence related to the ticket.

@demeritcowboy demeritcowboy deleted the audit-tpl-3 branch March 29, 2020 22:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants