-
-
Notifications
You must be signed in to change notification settings - Fork 825
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#139 Fail more gracefully on contribution detail report with invalid combination #12205
Conversation
@jitendrapurohit are you able to review this? |
if (CRM_Utils_Array::value('contribution_or_soft_value', $this->_params) == | ||
'contributions_only' && | ||
!empty($this->_params['fields']['soft_credit_type_id']) | ||
|| !empty($this->_params['soft_credit_type_id_value']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems the ||
condition need to be inside brackets? Currently, the status message is shown if the soft credit type is selected in the filter with Contribution OR Soft Credit?
set to any value. Works fine with below if statement -
if (CRM_Utils_Array::value('contribution_or_soft_value', $this->_params) ==
'contributions_only' &&
(!empty($this->_params['fields']['soft_credit_type_id'])
|| !empty($this->_params['soft_credit_type_id_value']))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @jitendrapurohit - I just pushed a fix
7e9c917
to
a9cf37f
Compare
…not including soft credits. For better or worse this report munges soft credits & contributions in a very wierd way. It currently hard fails if you try to search for 'contributions only' but include a soft credit criteria, although there appears to have been an effort to block this in the past. It might be nice to add that filter but restructuring the report to make it testable should be a pre-requisite for that - so for now, let's address the fatal
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested contribution detail report with the patch, works as expected 👍
I still get an error when my criteria is Soft Credits Only and in columns I have selected all the soft credit fields |
@yashodha do you want to submit a patch that extends this to suppress those fields too when it is 'Contributions only' |
@yashodha Actually - we could probably use isTableSelected() rather than identifying fields? |
Overview
Currently it is not valid to view 'Contributions only' with soft credit filters in the Contribution Detail report. This fix changes a hard error to a soft error when this is attempted
Before
Fatal error when combining 'Contributions only' with a filter on 'soft_credit_type_id'
After
Filter ignored with a message - no hard error
Technical Details
For better or worse this report munges soft credits & contributions in a very weird way.
There is already code to prevent accessing the soft credit criteria when doing 'Contributions only' search - but it was not working in this case. This PR fixes that & adds a message.
In general this report needs to be made testable before any notable changes are made. Moving some code to beginPostProcessCommon is a small step towards that.
Comments