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

CRM-21831 & dev/report/issues/1 Fix regressions in contribution detail report relating to soft credits #11917

Merged
merged 1 commit into from
Apr 2, 2018

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Apr 1, 2018

Overview

This fixes 3 regressions in the contribution report, all of which relate to use of soft credits

  1. a fatal error on soft credit fields https://lab.civicrm.org/dev/report/issues/1
  2. total amount not displayed - https://issues.civicrm.org/jira/browse/CRM-21831
  3. the total amount column no longer has a link

Before

  1. when soft credit column is on display the report does not render with a fatal error
  2. as described in Resolve issue CRM-21831 on https://issues.civicrm.org/jira/browse/CRM-21831 #11897
  3. no link on amount column

After

  1. Note the soft credit column renders without error

screenshot 2018-04-02 10 22 47

  1. The total amount is shown on the section break
    image

  2. Total amount link is re-instated
    screenshot 2018-04-02 10 56 28

Technical Details

Per @mlutfy investigation this line caused the regressions 4b885f8#diff-d355cdb00cea3915a3cf306c6c08f6a6R2314

The reason is that this line was causing the select query to be treated as a group by query when statistics were present. In this report stats were present but there is no group by tab or option (there is an option in the order by to do sections but that is a different concept from a code POV)

screenshot 2018-04-02 10 59 35

At some early point the fact the addStatisticsToSelect was altering the column names to 'group by style' was adapted to, not by fixing the assumption, but by series of hacks in the report code. This PR removes them.

@mlutfy @lcdservices @elgui02 - this takes #11897 fix & extends it to also address the fatal issue. As a recent regression I believe this needs to go into the rc - but it will need a quick turn-around on feedback for that

Comments

I checked to see what would happen in the soft credit rendering if a contact had more than one soft credit from the same contribution. It turned out to be UI blocked.
screenshot 2018-04-02 10 22 32

@eileenmcnaughton eileenmcnaughton changed the base branch from master to 5.0 April 1, 2018 22:49
@eileenmcnaughton
Copy link
Contributor Author

test this please

@eileenmcnaughton eileenmcnaughton changed the title CRM-21831 & dev/report/issues/1Fix bugs in contribution detail report relating to soft credits CRM-21831 & dev/report/issues/1 Fix regressions in contribution detail report relating to soft credits Apr 1, 2018
@eileenmcnaughton
Copy link
Contributor Author

test this please

@lcdservices
Copy link
Contributor

@eileenmcnaughton I applied the PR and ran through tests and confirmed all three issues are fixed. Looks good to me.

@eileenmcnaughton eileenmcnaughton merged commit 0542d40 into civicrm:5.0 Apr 2, 2018
@eileenmcnaughton eileenmcnaughton deleted the cont_report branch April 2, 2018 19:28
@eileenmcnaughton
Copy link
Contributor Author

thanks @lcdservices - merging based on your testing

@freeform-sg
Copy link
Contributor

Thanks everyone, my client will be very pleased to see this fix :)

@jamienovick
Copy link

@eileenmcnaughton just confirming this was a compucorp application task

@eileenmcnaughton
Copy link
Contributor Author

@jamienovick huh?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants