dev/core#212 - Contribution Details report fails when 'Is not one of'…

[jitendrapurohit]

… condition is used for Groups field

Overview

Fix DB error when contribution detail report is filtered by is not one of Group operator

Before

• open Report > Contribution Reports > Contribution Details
• click on Filters tab
• scroll down to Groups field
• change condition to "Is not one of"
• choose at least one group
• click Refresh results
• result: DB Error: no such field

After

Filtering on is not one of group operator correctly displays all contacts which are not in any selected group.

Technical Details

Contribuiton Detail Report uses a custom from clause to build the soft credit rows. It missed the group join if there is a temp table already created.

Comments

Added a small unit test to check for any DB failure when group by filter is applied.

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

[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 I want to suggest an alternate unit test approach - this one is an adjusted one that can work in the ReportTemplateTest class

  /**
   * Test the group filter works on the various reports.
   *
   * @dataProvider getMembershipAndContributionReportTemplatesForGroupTests
   *
   * @param string $template
   *   Report template unique identifier.
   */
  public function testReportsWithNoTInSmartGroupFilter($template) {
    $groupID = $this->setUpPopulatedGroup();
    $rows = $this->callAPISuccess('report_template', 'getrows', array(
      'report_id' => $template,
      'gid_value' => array($groupID),
      'gid_op' => 'notin',
      'options' => array('metadata' => array('sql')),
    ));
    $this->assertNumberOfContactsInResult(2, $rows, $template);
  }

[eileenmcnaughton]

I also tried this

  /**
   * Generate the from clause as it relates to the soft credits.
   */
  public function softCreditFrom() {
    $this->setFromBase('civireport_contribution_detail_temp1', 'id', 'temp1_civireport');
    $this->_from .= "
      INNER JOIN civicrm_contribution {$this->_aliases['civicrm_contribution']}
        ON temp1_civireport.civicrm_contribution_contribution_id = {$this->_aliases['civicrm_contribution']}.id
      INNER JOIN civicrm_contribution_soft contribution_soft_civireport
        ON contribution_soft_civireport.contribution_id = {$this->_aliases['civicrm_contribution']}.id
      INNER JOIN civicrm_contact      {$this->_aliases['civicrm_contact']}
        ON {$this->_aliases['civicrm_contact']}.id = contribution_soft_civireport.contact_id
    ";

    $this->appendAdditionalFromJoins();
  }

It's not quite working - but I think I may have the id field wrong. That setFromBase adds the acl & group join so I feel like that is nearly the same

[jitendrapurohit]

Ah, thanks for the above test. It actually found another similar issue with repeat contribution report. I've updated the PR to include the fix for the same 👍

[eileenmcnaughton]

Remaining fail is just about the need to clean up temporary tables. @totten is there any recommendation for doing this differently now that we have some new routines around temp tables - e.g something cleverer than adding to the tearDown

CRM_Core_DAO::executeQuery("DROP TEMPORARY TABLE IF EXISTS 'civireport_contribution_detail_temp1');

[eileenmcnaughton]

@jitendrapurohit TBH I think the fix might be a bit narrow (one operator only - notin but not in) - I don't need that changed since we have a unit test but maybe a code note to say that line is covered by tests to give people confidence to alter later.

[jitendrapurohit]

The failing test is due to this whitespace removal by my editor 🤦‍♂️ . Will update the PR with the fix.

[eileenmcnaughton]

OK - I'm happy with this - thanks @jitendrapurohit !