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#987 Reporthook #14320

Merged
merged 2 commits into from
Aug 1, 2019
Merged

dev/core#987 Reporthook #14320

merged 2 commits into from
Aug 1, 2019

Conversation

sushantpaste
Copy link
Contributor

Overview

Currently alterReportVars hook is invoked after grand total and section total calculations.
When alterReportVars used for altering any values in report rows , calculations does not take that into
consideration.
alterReportVars hook should invoked before grand total and section total calculations.

Before

When any rows data is changed in alterReportVars hook , those data changes is not reflecting into grand total or other calculations.

After

After applying patch data changes is not reflecting into grand total or other calculations.

Technical Details

In CRM/Report/Form.php hook is not invoke before calculations in formatDisplay().

@civibot
Copy link

civibot bot commented May 24, 2019

(Standard links)

@civibot civibot bot added the master label May 24, 2019
@yashodha
Copy link
Contributor

yashodha commented Jun 5, 2019

@sushantpaste Can you squash your commits?

@jaapjansma
Copy link
Contributor

See the discussion in: https://lab.civicrm.org/dev/core/issues/987

@mlutfy mlutfy changed the title Reporthook dev/core#987 Reporthook Jun 24, 2019
@eileenmcnaughton
Copy link
Contributor

I've come back to this & feel it needs an up or down decision. My general feeling is those report hooks are poorly designed & relying on them is a recipe for trouble - creating custom reports is a safer approach IMHO.

However, I agree that at the end of the day the grandTotal is more logically done on the 'final rows' and I feel like the risk is low- so I'm going to merge this despite my hesitations

@eileenmcnaughton eileenmcnaughton merged commit 4e74399 into civicrm:master Aug 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants