Skip to content
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.

Sign up for GitHub

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

Jump to bottom

CRM-20913 - Add separate option group for pledge status #10737

Merged
merged 5 commits into from
Jul 29, 2017
Merged

CRM-20913 - Add separate option group for pledge status #10737

merged 5 commits into from
Jul 29, 2017

Conversation

jitendrapurohit
Copy link
Contributor

@jitendrapurohit jitendrapurohit commented Jul 24, 2017 •
edited
Loading

Tagging as wip since I haven't yet performed any initial testing for this or written any unit test..

  1. Creates a new option_group for pledge similar to contribution_status except Failed, Chargeback, Pending Refund, Partially paid and Refunded.
  2. Remove In Progress value from contribution status. - reverted this as In Progress gets set for recurring contribution.

Comments welcome @eileenmcnaughton

eileenmcnaughton
eileenmcnaughton reviewed Jul 24, 2017
View reviewed changes
CRM/Pledge/BAO/Query.php Outdated
@@ -405,7 +405,7 @@ public static function from($name, $mode, $side) {
break;

case 'pledge_status':
$from .= " $side JOIN civicrm_option_group option_group_pledge_status ON (option_group_pledge_status.name = 'contribution_status')";
$from .= " $side JOIN civicrm_option_group option_group_pledge_status ON (option_group_pledge_status.name = {$name})";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does using $name add anything here - seems it would always be that

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$name is always pledge_status. But, just for readability, I'll update this to text instead of variable.

@eileenmcnaughton
Copy link
Contributor

Not having tested it either, the code looks about right - looks like Jenkins has been busy testing it :-)

@jitendrapurohit jitendrapurohit force-pushed the CRM-20913 branch 3 times, most recently from 8b0d660 to 035788c Compare July 25, 2017 07:22
@jitendrapurohit
Copy link
Contributor Author

jitendrapurohit commented Jul 26, 2017 •
edited
Loading

Added unit test for updatePledgeStatus(). Also done manual testing for following scenarios with the updated changes and worked fine.

  1. Online pledge contribution with pay later(pledge_status = pending) and CC(pledge status = inprogress for multiple installments).
  2. Backend pledge creation and recording payments(pledge status after each payments looked fine).
  3. Cancellation of pledge.
  4. Search related to Pledge in Advanced Search, Find Pledge with various status options and check built qill message.
  5. Pledge Details Report with filtering on status and other options.

To-do - Test Upgrade code to assert whether new option group gets created after upgradation.

@jitendrapurohit
Copy link
Contributor Author

Tested and confirmed pledge_status gets correctly created after upgrade. Removing wip tag as I've done writing unit test and manual testing from my side.

@jitendrapurohit jitendrapurohit changed the title WIP - CRM-20913 - Add separate option group for pledge status CRM-20913 - Add separate option group for pledge status Jul 28, 2017
@eileenmcnaughton
Copy link
Contributor

@monishdeb if you are able to take a look at this it should simplify some of the other contribution things

@jitendrapurohit
Copy link
Contributor Author

Note: I haven't yet pushed the generated data changes, as PR is more likely to get stale due to them.

@eileenmcnaughton eileenmcnaughton mentioned this pull request Jul 28, 2017
Merged
@monishdeb
Copy link
Member

Tested the PR, working as expected. @jitendrapurohit can you add the generated data changes. So I could merge then?

@jitendrapurohit
Copy link
Contributor Author

Done @monishdeb 👍

@monishdeb
Copy link
Member

Tested all the five scenarios/fixes mentioned above #10737 (comment) . Working fine as expected

@monishdeb monishdeb merged commit 8bd0b01 into civicrm:master Jul 29, 2017
monishdeb
monishdeb reviewed Jul 29, 2017
View reviewed changes
xml/templates/civicrm_data.tpl
(@option_group_id_ps, '{ts escape="sql"}Pending{/ts}' , 2, 'Pending' , NULL, 0, NULL, 2, NULL, 0, 1, 1, NULL, NULL, NULL),
(@option_group_id_ps, '{ts escape="sql"}Cancelled{/ts}' , 3, 'Cancelled' , NULL, 0, NULL, 3, NULL, 0, 1, 1, NULL, NULL, NULL),
(@option_group_id_ps, '{ts escape="sql"}In Progress{/ts}', 4, 'In Progress', NULL, 0, NULL, 4, NULL, 0, 1, 1, NULL, NULL, NULL),
(@option_group_id_ps, '{ts escape="sql"}Overdue{/ts}' , 5, 'Overdue' , NULL, 0, NULL, 5, NULL, 0, 1, 1, NULL, NULL, NULL);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jitendrapurohit @eileenmcnaughton I forgot to mention, do we need to delete In Progress and Overdue status from Contribution Status's option values too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are pledge status where In Progress and Overdue status are required. I haven't removed any existing status in this PR. See point 2 in first comment where I initially removed Inprogress from contribution status but it looks like some unit test do want to set this status for recur contribution - Eg. - ContributionTest and ContributionPageTest.

@jitendrapurohit jitendrapurohit deleted the CRM-20913 branch July 30, 2017 06:15
@eileenmcnaughton
Copy link
Contributor

Well done guys - I think @jitendrapurohit said 'In Progress' is needed for contribution recur (which suggests to me it should have it's own set too)

@eileenmcnaughton eileenmcnaughton mentioned this pull request Jul 31, 2017
Merged
@jitendrapurohit jitendrapurohit mentioned this pull request Aug 2, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eileenmcnaughton eileenmcnaughton eileenmcnaughton left review comments

@monishdeb monishdeb monishdeb left review comments

Assignees
No one assigned
Labels
master
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jitendrapurohit @eileenmcnaughton @monishdeb @civicrm-builder