dev/core#905 Separate contribution_recur status_id option group from contribution option group #14343

eileenmcnaughton · 2019-05-26T05:45:51Z

Overview

No user impact, splits contribution recur status option group from contribution

Before

One option group shared

After

Two separate ones

Technical Details

As we did earlier for pledge status this separates out the option groups - on the basis we have different statuses for the 2 things. In order to ensure we don't break anything
we give the new option group the exact same values as the old one

For new sites I have left off some statuses that I think would not be relevant - Refunded,
Partially Paid, Chargeback, Pending Refund

Comments

@mattwire @monishdeb @pradpnayak @adixon input welcome!

A couple of follow up notes

I think 'In Progress' is now not needed in the contribution_status option group & we might remove from new installs
@mattwire this also solves a problem I hit when trying to make the contribution recur search form metadata driven.
Regarding adding the statuses to add - there were some comments on this PR template but I've removed them from here as the discussion is in gitlab & I will add a follow up PR with them

https://lab.civicrm.org/dev/core/issues/905

civibot · 2019-05-26T05:45:54Z

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

seamuslee001 · 2019-05-26T05:51:16Z

@eileenmcnaughton don't we need the updated generated_data.sql?

eileenmcnaughton · 2019-05-26T05:53:54Z

@seamuslee001 good question - I guess I run setup.sh?

seamuslee001 · 2019-05-26T05:55:04Z

xml/schema/Contribute/ContributionRecur.xml

@@ -241,7 +241,7 @@
    <import>true</import>
    <add>1.6</add>
    <pseudoconstant>
-      <optionGroupName>contribution_status</optionGroupName>
+      <optionGroupName>contribution_recur_status</optionGroupName>


need to run ./bin/setup -g to update the DAO

eileenmcnaughton · 2019-05-26T05:56:20Z

Cool done.

eileenmcnaughton · 2019-05-26T07:49:24Z

test this please

mattwire · 2019-05-26T11:24:00Z

@eileenmcnaughton Thoughts added to the gitlab issue. Summary: Yes, but we need more thought on the options we provide before merging.

eileenmcnaughton · 2019-05-26T14:02:26Z

test this please

eileenmcnaughton · 2019-05-26T21:21:04Z

@mattwire I've altered this to NOT add the new status. I feel like this would be better merged as a PR that just adds the group & I can follow up & add the statuses. I think we have agreed on them but it feels like a separate PR would be cleaner

adixon · 2019-05-27T13:55:52Z

Good idea, for reasons already well noted, and I support the implementation strategy.

I'm only confused about the note about removing the "In Progress" status - are you talking about removing that from the non-recurring payment status?

eileenmcnaughton · 2019-05-27T21:21:09Z

@adixon when I wrote that I believed that 'In Progress' was relevant to pledges & recurring contributions - but not actual contributions. So I thought that with neither of those entities sharing the contribution status option group anymore it would not be needed in that group (I have seem it cause some confusion). However after doing some other stuff in BAO_Contribution yesterday I realise it IS referenced in there for contribution handling so I think I was wrong

pradpnayak · 2019-05-27T21:34:56Z

CRM/Upgrade/Incremental/sql/5.15.alpha1.mysql.tpl

@@ -1 +1,11 @@
 {* file to handle db changes in 5.15.alpha1 during upgrade *}
+--dev/core#905 Add contribution recur status option group
+INSERT INTO `civicrm_option_group`  ( `name`, {localize field='title'}`title`{/localize}, `is_active` ) VALUES ('contribution_recur_status', {localize}'{ts escape="sql"}Contribution Recur Status{/ts}'{/localize}, 1);


Contribution status option group has is_reserved and is_locked to TRUE, should this option should have same value for those fields?

@pradpnayak yes, thank you - I will fix!

pradpnayak · 2019-05-27T21:36:58Z

xml/templates/civicrm_data.tpl

@@ -212,6 +212,7 @@ VALUES
   ('wysiwyg_presets'               , '{ts escape="sql"}WYSIWYG Editor Presets{/ts}'             , NULL, 1, 1, 0),
   ('relative_date_filters'         , '{ts escape="sql"}Relative Date Filters{/ts}'              , NULL, 1, 1, 0),
   ('pledge_status'                 , '{ts escape="sql"}Pledge Status{/ts}'                      , NULL, 1, 1, 1),
+   ('contribution_recur_status'     , '{ts escape="sql"}Recurring Contribution Status{/ts}'      , NULL, 1, 1, 1),


Label is different here v/s in upgrade script.

…contribution option group As we did earlier for pledge status this separates out the option groups - on the basis we have different statuses for the 2 things. In order to ensure we don't break anything existing the values we give the new option group the exact same values as the old one> However, I HAVE added one more value - Processing - which is intended for the period between when a new contribution starts to be added and when it has been added. This would normally be a few seconds but when there is an error it would be left in this state - warding off future attempts. For new sites I have left off some statuses that I think would not be relevant - Refunded, Partially Paid, Chargeback, Pending Refund

eileenmcnaughton · 2019-05-27T21:44:38Z

@pradpnayak thank you ! I think I've fixed those now

pradpnayak · 2019-05-27T22:03:57Z

Tested this PR on fresh install and upgrade.
On fresh install - New recurring contribution status option group is created with options

On upgrade - New recurring contribution status option group is created with options of Contribution status copied over.

The options doesn't match on Fresh install v/s upgrade since on fresh install it creates 6 options while during upgrade it creates 10 options(by default contribution statuses). This does make sense inorder to keep the consistency of option.value.

Good to merge.

eileenmcnaughton · 2019-05-28T04:56:40Z

test this please

eileenmcnaughton · 2019-05-28T19:57:56Z

test this please

eileenmcnaughton · 2019-05-28T22:48:30Z

test this please

yashodha · 2019-05-29T04:36:10Z

@eileenmcnaughton : the tests have all passed.
Can this be merged or does it need further inputs/discussions?

eileenmcnaughton · 2019-05-29T05:21:54Z

@yashodha I think it's good to merge - I kept all the stuff that might need discussion for a follow up PR - which I'll do once this is merged

civibot bot added the master label May 26, 2019

eileenmcnaughton changed the title ~~dev/core#905 Separate contribution_recur status_id option group from …~~ dev/core#905 Separate contribution_recur status_id option group from contribution option group May 26, 2019

seamuslee001 reviewed May 26, 2019

View reviewed changes

eileenmcnaughton force-pushed the recur_status branch from 99e56ab to e6279dd Compare May 26, 2019 05:56

eileenmcnaughton force-pushed the recur_status branch from e6279dd to 70e12d9 Compare May 26, 2019 21:18

eileenmcnaughton force-pushed the recur_status branch from 70e12d9 to 6c2643f Compare May 26, 2019 21:21

pradpnayak reviewed May 27, 2019

View reviewed changes

eileenmcnaughton force-pushed the recur_status branch from 2794852 to 0dc36ab Compare May 27, 2019 21:43

eileenmcnaughton merged commit 86ad0bd into civicrm:master May 29, 2019

eileenmcnaughton deleted the recur_status branch May 29, 2019 05:22

mattwire mentioned this pull request Jun 1, 2019

Set new inactive statuses as 'Chargeback' does not exist on new installs and that breaks Contributionrecur.create API #14398

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dev/core#905 Separate contribution_recur status_id option group from contribution option group #14343

dev/core#905 Separate contribution_recur status_id option group from contribution option group #14343

eileenmcnaughton commented May 26, 2019 •

edited

Loading

civibot bot commented May 26, 2019

seamuslee001 commented May 26, 2019

eileenmcnaughton commented May 26, 2019

seamuslee001 May 26, 2019

eileenmcnaughton commented May 26, 2019

eileenmcnaughton commented May 26, 2019

mattwire commented May 26, 2019

eileenmcnaughton commented May 26, 2019

eileenmcnaughton commented May 26, 2019

adixon commented May 27, 2019

eileenmcnaughton commented May 27, 2019

pradpnayak May 27, 2019

eileenmcnaughton May 27, 2019

pradpnayak May 27, 2019

eileenmcnaughton commented May 27, 2019

pradpnayak commented May 27, 2019

eileenmcnaughton commented May 28, 2019

eileenmcnaughton commented May 28, 2019

eileenmcnaughton commented May 28, 2019

yashodha commented May 29, 2019

eileenmcnaughton commented May 29, 2019

dev/core#905 Separate contribution_recur status_id option group from contribution option group #14343

dev/core#905 Separate contribution_recur status_id option group from contribution option group #14343

Conversation

eileenmcnaughton commented May 26, 2019 • edited Loading

Overview

Before

After

Technical Details

Comments

civibot bot commented May 26, 2019

seamuslee001 commented May 26, 2019

eileenmcnaughton commented May 26, 2019

seamuslee001 May 26, 2019

Choose a reason for hiding this comment

eileenmcnaughton commented May 26, 2019

eileenmcnaughton commented May 26, 2019

mattwire commented May 26, 2019

eileenmcnaughton commented May 26, 2019

eileenmcnaughton commented May 26, 2019

adixon commented May 27, 2019

eileenmcnaughton commented May 27, 2019

pradpnayak May 27, 2019

Choose a reason for hiding this comment

eileenmcnaughton May 27, 2019

Choose a reason for hiding this comment

pradpnayak May 27, 2019

Choose a reason for hiding this comment

eileenmcnaughton commented May 27, 2019

pradpnayak commented May 27, 2019

eileenmcnaughton commented May 28, 2019

eileenmcnaughton commented May 28, 2019

eileenmcnaughton commented May 28, 2019

yashodha commented May 29, 2019

eileenmcnaughton commented May 29, 2019

eileenmcnaughton commented May 26, 2019 •

edited

Loading