dev/core#812 - Fix null display of contribution row on contact summar…

[jitendrapurohit]

…y page

Overview

Null row is displayed on contact summary page if contact has 0 contribution.

Before

Screenshot from dmaster -

If any of the link is clicked on the row, it results into an error -

After

Fixed.

Technical Details

We removed the unnecessary financial type join from the contribution table in #13720

Before the above fix, the query included the financial type which unintentionally avoided the null row to be returned as output -

SELECT  civicrm_contribution.id, .......... 
FROM civicrm_contact contact_a 
    LEFT JOIN civicrm_contribution ON civicrm_contribution.contact_id = contact_a.id  
    INNER JOIN civicrm_financial_type ON civicrm_contribution.financial_type_id = civicrm_financial_type.id  
    LEFT  JOIN civicrm_contribution_product ON civicrm_contribution_product.contribution_id = civicrm_contribution.id 
    LEFT  JOIN civicrm_product ON civicrm_contribution_product.product_id =civicrm_product.id 
WHERE  ( contact_a.id = '202' )  AND (contact_a.is_deleted = 0)   
  GROUP BY civicrm_contribution.id  ORDER BY `receive_date` desc, `contact_a`.`id`  LIMIT 0, 50 

After the removal of financial type join, the query is -

SELECT  civicrm_contribution.id, ......
FROM civicrm_contact contact_a 
    LEFT JOIN civicrm_contribution   ON civicrm_contribution.contact_id = contact_a.id
WHERE  ( contact_a.id = '202' )  AND (contact_a.is_deleted = 0)   
GROUP BY civicrm_contribution.id  ORDER BY `receive_date` desc, `contact_a`.`id`  
LIMIT 0, 50 

As it is a left join and no other field, the contact null row gets returned and is displayed on the contribution tab. @eileenmcnaughton

Comments

Gitlab - https://lab.civicrm.org/dev/core/issues/812

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

[eileenmcnaughton]

@jitendrapurohit that spun up fails- perhaps we can add a where instead?

[eileenmcnaughton]

this should go a the rc when we get it figured out maybe

[eileenmcnaughton]

@jitendrapurohit what about this instead - I'm not sure if that selector is the best place to add the extra criteria but it seems to work & potentially avoids changing a scary join lower down

diff --git a/CRM/Contribute/Selector/Search.php b/CRM/Contribute/Selector/Search.php
index 0606f9a207..e17f043cb7 100644
--- a/CRM/Contribute/Selector/Search.php
+++ b/CRM/Contribute/Selector/Search.php
@@ -181,6 +181,7 @@ class CRM_Contribute_Selector_Search extends CRM_Core_Selector_Base implements C
     $this->_action = $action;
     $returnProperties = CRM_Contribute_BAO_Query::selectorReturnProperties($this->_queryParams);
     $this->_includeSoftCredits = CRM_Contribute_BAO_Query::isSoftCreditOptionEnabled($this->_queryParams);
+    $this->_queryParams[] = ['contribution_id', '!=', 0, 0, 0];
     $this->_query = new CRM_Contact_BAO_Query(
       $this->_queryParams,
       $returnProperties,

[jitendrapurohit]

Updated the changes which now uses where clause to avoid the NULL contact row to be returned as result. Also avoided this unnecessary filter to be displayed as qill value on search results.

[eileenmcnaughton]

Hmm the new test is failing now - also - perhaps the qill should also check for 0 since I can imagine if someone later wants to have a qill for 'contribution id = 89' it might be confusing why it's blocked

[jitendrapurohit]

The test which is failing is the same which is added in this PR. The problem was it directly used the BAO_Query object to generate the query. It is now modified as per the selector which should work fine in passing all the tests.

Also, modified the qill condition to be more restrictive and ignore the display for only this case.

[eileenmcnaughton]

@jitendrapurohit looks good - last thing - I think this should be on the rc?

[jitendrapurohit]

Yes, I've confirmed that this also affects 5.12-rc. Have raised the PR here - #13881 @eileenmcnaughton

[eileenmcnaughton]

ok - we can close this one in favour of that then I guess