CRM-21354 : Allow "Advanced Search" to search for recurring contribution status. #11080

waddyvic · 2017-10-07T15:59:21Z

Overview

Advanced search wasn't capable of searching for contacts with a specific recurring contribution status. I've added this field in Advanced Search to find people that are currently a monthly donors (status In Progress) or has been a monthly donors before (status "Cancelled" or "Completed").

CRM-21354: Allow "Advanced Search" to search for recurring contribution status

civicrm-builder · 2017-10-07T15:59:22Z

Can one of the admins verify this patch?

seamuslee001 · 2017-10-07T16:42:20Z

Jenkins ok to test

MegaphoneJon · 2017-10-09T21:18:51Z

Jenkins ok to retest

monishdeb · 2017-10-13T06:06:10Z

CRM/Contribute/BAO/ContributionRecur.php

+    // Add field for contribution status
+    $contributionStatus = CRM_Contribute_PseudoConstant::contributionStatus();
+    $form->addSelect('contribution_recur_contribution_status_id',
+      array('entity' => 'contribution', 'multiple' => 'multiple', 'context' => 'search', 'options' => $contributionStatus)


Can you directly use the function in options i.e.
..., 'options' => CRM_Contribute_PseudoConstant::contributionStatus()
because $contributionStatus isn't used anywhere else.

monishdeb · 2017-10-13T06:08:30Z

CRM/Contribute/BAO/Query.php

+      case 'contribution_recur_contribution_status_id':
+        $query->_where[$grouping][] = CRM_Contact_BAO_Query::buildClause("civicrm_contribution_recur.contribution_status_id", $op, $value, 'String');
+        list($op, $value) = CRM_Contact_BAO_Query::buildQillForFieldValue('CRM_Contribute_DAO_ContributionRecur', 'contribution_status_id', $value, $op, $pseudoExtraParam);
+        $query->_qill[$grouping][] = ts('%1 %2 %3', array(1 => 'Recurring Contribution Status', 2 => $op, 3 => $value));


Please add single quotation around status value to highlight the statuses, because it qill odd for In progress status :

Sure thing.

I was wondering if I should quote the status values when I developed this field, but when I checked the output for normal contribution status (not recurring), there was no quote. So I just followed core behaviour.

monishdeb · 2017-10-13T06:23:40Z

Great work @waddyvic 👍 Apart from the two changes I have mentioned in the patch, I need point out some general issue with this PR:

Is there a JIRA ticket raised for this change? If not please create one in https://issues.civicrm.org and mention the ticket no. in PR title as CRM-# : Allow "Advanced Search" to search for recurring contribution status.
I can see a false commit 6f00ce3 it seems like you tried to pull the latest change in your branch. To avoid this try always to rebase against pull. Here are the steps:

$ git fetch upstream master // if upstream is pointing at [email protected]:civicrm/civicrm-core.git
$ git pull --rebase upstream master // if you got any merge conflict resolve them manually
$ git push -f origin advanced-search-recur-status //if origin is pointing at [email protected]: waddyvic/civicrm-core.git

Can you add a UT (unit test) for this new field? Let me show an example how UTfor contribution filters were added, see - https://github.com/civicrm/civicrm-core/blob/master/tests/phpunit/CRM/Contribute/Form/SearchTest.php
Ping me (alias - @Monish) on MM https://chat.civicrm.org/civicrm/channels/dev if you need help :)

waddyvic · 2017-10-13T16:10:17Z

Hey @monishdeb thx for reviewing my PR. To respond to your previous comments:

I just signed up an account on civicrm.org, pending approval. Will create an issue as soon as I can login.
Thx for pointing out best practices of PR. I didn't do this enough to know exactly what steps to follow. I'll use rebase from now on before creating PR.
I'll add a UT for this field as soon as possible.

monishdeb · 2017-10-13T18:15:03Z

CRM/Contribute/BAO/ContributionRecur.php

+
+    // Add field for contribution status
+    $form->addSelect('contribution_recur_contribution_status_id',
+      array('entity' => 'contribution', 'multiple' => 'multiple', 'context' => 'search', 'options' => CRM_Contribute_PseudoConstant::contributionStatus() )


See the related test build failure https://test.civicrm.org/job/CiviCRM-Core-PR/17604/checkstyleResult/new/

It says There should be no white space before a closing ")" so I would say that's a style error. As per coding standard there shouldn't be any trailing space before/after parenthesis.

LOL my supervisor had his own coding guidelines which goes against the community. It's fixed now.

monishdeb · 2017-10-13T20:27:04Z

CRM/Contribute/BAO/ContributionRecur.php

+
+    // Add field for contribution status
+    $form->addSelect('contribution_recur_contribution_status_id',
+      array('entity' => 'contribution', 'multiple' => 'multiple', 'context' => 'search', 'options' => CRM_Contribute_PseudoConstant::contributionStatus()),


Oops you left a comma ...CRM_Contribute_PseudoConstant::contributionStatus()), at then end. Causes test failure again :(

Sounds like I need to install PHP code sniffer after all

waddyvic · 2017-10-23T16:58:51Z

Hi @monishdeb, I've added a unit test for the recurring contribution status search field. I'm still waiting for my account on issues.civicrm.org approved, so couldn't create an issue on there yet. I couldn't find how to contact the site's admin to approve my registration. Do you have any lead on this?

monishdeb · 2017-10-23T18:41:43Z

Great work. @colemanw can you please approve the JIRA account of @waddyvic ?

monishdeb · 2017-10-25T12:37:59Z

@waddyvic any luck? Or can you ask in mattermost dev-channel https://chat.civicrm.org/civicrm/channels/dev to approve your JIRA account

waddyvic · 2017-10-25T16:47:33Z

@monishdeb Still waiting. I've joined mattermost to ask for help.

waddyvic · 2017-10-25T17:38:02Z

@monishdeb OK got it sorted out. issue number is CRM-21354

monishdeb · 2017-10-25T17:59:19Z

CRM/Contribute/BAO/Query.php

+      case 'contribution_recur_contribution_status_id':
+        $query->_where[$grouping][] = CRM_Contact_BAO_Query::buildClause("civicrm_contribution_recur.contribution_status_id", $op, $value, 'String');
+        list($op, $value) = CRM_Contact_BAO_Query::buildQillForFieldValue('CRM_Contribute_DAO_ContributionRecur', 'contribution_status_id', $value, $op, $pseudoExtraParam);
+        $query->_qill[$grouping][] = ts("%1 %2 '%3'", array(1 => 'Recurring Contribution Status', 2 => $op, 3 => $value));


Can you change it to

$query->_qill[$grouping][] = ts("Recurring Contribution Status %1 '%2'", array(1 => $op, 2 => $value));

monishdeb · 2017-10-25T18:02:14Z

Great.. one last change. Can you squash your 8 commits into 1? Here's the steps:

$ git fetch upstream master // if upstream is pointing to civicrm/civicrm-core.git
$ git pull --rebase upstream master
$ git rebase -i //this will open a vi edit screen
// now on vi edit screen, except 1st commit, change 'pick' to 'squash' against all subsequent commits, then save
$ git push -f origin advanced-search-recur-status

…ecurring contribution status(es). CRM_Contribute_BAO_ContributionRecur: add multi-select dropdown field to select recurring contribution status. CRM_Contribute_BAO_Query: add case to construct where clause for recurring contribution status. templates/CRM/Contribute/Form/Search/ContributionRecur.tpl: add recurring contribution status field in template. #11080 Add quotes around recurring contribution status qill. #11080 Directly use CRM_Contribute_PseudoConstant::contributionStatus() instead of creating variable that is not used anywhere else. Remove space after closing ) to comply with coding standard Remove comma after closing ) to comply with coding standard Add unit test for recurring contribution status filter Fix coding style error: The closing brace for the class must have an empty line before it Do not use variable replacement in qill for static text "Recurring Contribution Status".

waddyvic · 2017-10-27T19:04:06Z

Hi @monishdeb , I did what you requested and it's pushed now.

monishdeb · 2017-10-27T19:15:04Z

Perfect 👍 Tested the latest patch and it worked for me. Happy with the final patch. Hence merging.

monishdeb · 2017-10-27T19:16:08Z

Congrats @waddyvic .. Keep up the good work :D

waddyvic · 2017-10-27T19:17:32Z

Thx for all your help on this @monishdeb

…status CRM-21354 : Allow "Advanced Search" to search for recurring contribution status.

civicrm-builder added the master label Oct 7, 2017

monishdeb reviewed Oct 13, 2017

View reviewed changes

waddyvic changed the title ~~Allow "Advanced Search" to search for recurring contribution status.~~ CRM-21354: Allow "Advanced Search" to search for recurring contribution status. Oct 25, 2017

waddyvic changed the title ~~CRM-21354: Allow "Advanced Search" to search for recurring contribution status.~~ CRM-21354 : Allow "Advanced Search" to search for recurring contribution status. Oct 25, 2017

monishdeb reviewed Oct 25, 2017

View reviewed changes

monishdeb merged commit 08c2d3c into civicrm:master Oct 27, 2017

waddyvic deleted the advanced-search-recur-status branch October 27, 2017 19:16

sluc23 pushed a commit to ixiam/civicrm-core that referenced this pull request Jan 10, 2018

Merge pull request civicrm#11080 from waddyvic/advanced-search-recur-…

0800d51

…status CRM-21354 : Allow "Advanced Search" to search for recurring contribution status.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CRM-21354 : Allow "Advanced Search" to search for recurring contribution status. #11080

CRM-21354 : Allow "Advanced Search" to search for recurring contribution status. #11080

waddyvic commented Oct 7, 2017 •

edited by civicrm-builder

Loading

civicrm-builder commented Oct 7, 2017

seamuslee001 commented Oct 7, 2017

MegaphoneJon commented Oct 9, 2017

monishdeb Oct 13, 2017 •

edited

Loading

monishdeb Oct 13, 2017

waddyvic Oct 13, 2017

monishdeb commented Oct 13, 2017 •

edited

Loading

waddyvic commented Oct 13, 2017

monishdeb Oct 13, 2017

waddyvic Oct 13, 2017

monishdeb Oct 13, 2017

waddyvic Oct 13, 2017

waddyvic commented Oct 23, 2017

monishdeb commented Oct 23, 2017

monishdeb commented Oct 25, 2017

waddyvic commented Oct 25, 2017

waddyvic commented Oct 25, 2017

monishdeb Oct 25, 2017

monishdeb commented Oct 25, 2017 •

edited

Loading

waddyvic commented Oct 27, 2017

monishdeb commented Oct 27, 2017 •

edited

Loading

monishdeb commented Oct 27, 2017

waddyvic commented Oct 27, 2017

CRM-21354 : Allow "Advanced Search" to search for recurring contribution status. #11080

CRM-21354 : Allow "Advanced Search" to search for recurring contribution status. #11080

Conversation

waddyvic commented Oct 7, 2017 • edited by civicrm-builder Loading

Overview

civicrm-builder commented Oct 7, 2017

seamuslee001 commented Oct 7, 2017

MegaphoneJon commented Oct 9, 2017

monishdeb Oct 13, 2017 • edited Loading

Choose a reason for hiding this comment

monishdeb Oct 13, 2017

Choose a reason for hiding this comment

waddyvic Oct 13, 2017

Choose a reason for hiding this comment

monishdeb commented Oct 13, 2017 • edited Loading

waddyvic commented Oct 13, 2017

monishdeb Oct 13, 2017

Choose a reason for hiding this comment

waddyvic Oct 13, 2017

Choose a reason for hiding this comment

monishdeb Oct 13, 2017

Choose a reason for hiding this comment

waddyvic Oct 13, 2017

Choose a reason for hiding this comment

waddyvic commented Oct 23, 2017

monishdeb commented Oct 23, 2017

monishdeb commented Oct 25, 2017

waddyvic commented Oct 25, 2017

waddyvic commented Oct 25, 2017

monishdeb Oct 25, 2017

Choose a reason for hiding this comment

monishdeb commented Oct 25, 2017 • edited Loading

waddyvic commented Oct 27, 2017

monishdeb commented Oct 27, 2017 • edited Loading

monishdeb commented Oct 27, 2017

waddyvic commented Oct 27, 2017

waddyvic commented Oct 7, 2017 •

edited by civicrm-builder

Loading

monishdeb Oct 13, 2017 •

edited

Loading

monishdeb commented Oct 13, 2017 •

edited

Loading

monishdeb commented Oct 25, 2017 •

edited

Loading

monishdeb commented Oct 27, 2017 •

edited

Loading