CRM-21343: Add cancel filters for Contribution search

[sluc23]

Overview

Add cancel reason and cancel date filters in Contribution search form
https://issues.civicrm.org/jira/browse/CRM-21343

Before

No cancel reason or cancel date fields in the form to be used as contribution filters

After

[civicrm-builder]

Can one of the admins verify this patch?

[seamuslee001]

Jenkins ok to test

[monishdeb]

@sluc23 nice work. I have reviewed your patch and here's few point I like to add on top of your changes:

1. Can you shift the two fields to right, to utilize the space better?
2. Can you add UT (unit test) for these two new filters? See how UT for 'card_type' is added here

[sluc23]

@monishdeb regarding UT, the issue is that the demo database doesn't have any Canceled Contribution.
What do you think it's the best way to deal with this if I need to create the UT based on the civibuild database?

[monishdeb]

@sluc23 true. You might have noticed that in order to write UT for card-type filter, first a dummy contribution is created using card type - see https://github.com/civicrm/civicrm-core/blob/master/tests/phpunit/CRM/Contribute/Form/SearchTest.php#L218

So, in this case, you need to create a dummy canceled contribution(s) using Contribution.create API. Hint:

public function testCancelReasonFilter() {
  $contactID = $this->individualCreate();
  $this->contributionCreate(array(   // this is a helper fn provided by Civi UT to create dummy contribution or if you don't like it use civicrm_api3('contribution', 'create', $params) instead
   'contact_id' => $contactID,
    'contribution_status_id' => 'Cancelled',
    'cancel_reason' => 'oops transaction failed',
   'cancel_date' => date('YmdHis'),
  ));
  ...
 }

Hope this helps :)

[sluc23]

@monishdeb regarding your 1. before doing the commits, you think this looks better?

At the right of Currency field there is a conditional Batch field which is not always shown (like in my screenshot), so it is not easy to move both fields at the right column, at the same level of Currency

[monishdeb]

@sluc23 sorry for late reply. Can you bring 'Personal Campaign Page' in right and below of 'Personal Campaign Page Honor Roll?' field. In that the 2 PCP fields will be rendered jointly and this will adjust the space well. What you say?

[sluc23]

@monishdeb like this?

Anyway the layout of this form is not very user friendly, many controls/labels are not aligned, not very intuitive to find fields.. I think some cosmetic adjustments would be nice to have to make this form look better, what do you think?

[monishdeb]

I think some cosmetic adjustments would be nice to have to make this form look better, what do you think?

I agree, but I'ld prefer to create a separate issue for that because I think the required change in tpl will be big and doesn't fit in the purpose of this issue.

Back to the screenshot, I like the view now, but will it be a simple fix to remove space above currency field?

[yashodha]

@monishdeb @sluc23 while we are at moving around stuf, we should move the currency all the way up next to Contribution Amounts since it belongs there IMO. What do you guys think?

[sluc23]

@yashodha @monishdeb something like this?
remember that at the right of currency fields there is a hidden Contribution Batch field, which is shown conditionally

[monishdeb]

Yeah, it looks good now. :)

[mlutfy]

Can you wrap "Contribution Cancel Date" in ts() ? -- and also the 2-3 other occurrences of dateQueryBuilder() which should have been wrapped as well.

[mlutfy]

.. and possibly use the same label as "Cancelled / Refunded Date"? (This is shown in the search results, I think?)

[sluc23]

@mlutfy sure.. what do you think about here (line 1066):

    // we only have recurring dates using this ATM so lets' short cut to find the table name
    $table = 'contribution_recur';
    $fieldName = explode($table . '_', $field);
    $query->dateQueryBuilder($values,
      'civicrm_' . $table, $field, $fieldName[1], $title
    );
    return TRUE;
  }

should I wrap it as

$query->dateQueryBuilder($values,
      'civicrm_' . $table, $field, $fieldName[1], ts($title)
    );

?

[mlutfy]

@sluc23 No, because gettext/civistrings uses the ts() function to detect which strings to extract (to send to Transifex). So while ts($title) would send the text through gettext, the actual string would not be present in the translation dictionary.
In other words, ts() means:

• "lookup this string in the translation dictionary (civicrm.mo)"
• "and send this string to Transifex".
  Hence a string must not include a variable, since we do not know what the value of the variable is during extraction time (which is done as a c-i process outside program execution).

[eileenmcnaughton]

@monishdeb looks like you have mostly reviewed this?

[monishdeb]

@eileenmcnaughton yes and I am happy with the patch. @sluc23 did really good job in addressing all the additional issues raised during QA. Only thing remaining to get this PR merged is adding new/extended-existing UT on those two new search filters. @sluc23 are you going to add UT? Please ping me on mattermost (alias @Monish) if you need my help :)

[sluc23]

@monishdeb I've added unit test, and rebased into master.
I'm getting a conflict in the file I've edited to add the test.. I'm not sure why it's showing me that error..

any clue?

[monishdeb]

@sluc23 you might have git pull in an attempt to doing rebase, reason why you lots of false commits in your PR. Can you create a new PR and close this one?

[eileenmcnaughton]

In general to rebase I do

git pull --rebase origin master

& then I do a second one

git pull -i origin/master

(the second one allows for squashing of commits & tweaking commit messages)

You need to use the -f switch when you push

git push -f myrepo CRM-21343
(where CRM-21343 is your branch name - myrepo will need changing to your repo name)

[sluc23]

New PR created --> #11638