-
-
Notifications
You must be signed in to change notification settings - Fork 825
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.
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
CRM-18854 [needs-review] Added support for recurring contributions for online pledges #8558
Conversation
Edzelopez
commented
Jun 14, 2016
•
edited by civicrm-builder
Loading
edited by civicrm-builder
- CRM-18854: Recurring Payments for Pledges - front end
'id' => $contribution->id, | ||
'contribution_recur_id' => $recurringPledgeID, | ||
); | ||
CRM_Contribute_BAO_Contribution::add($contribParams); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use the api from the form layer as much as possible - there was a general feeling at CiviCon in favour of that
I made some comments about code approach (ie. use the api more & don't throw exceptions at the BAO layer.) Obviously you know I'm going to say something about adding a test. But on the bigger picture I don't understand the use of pledge_start_date. It seems like a fixed date set on the contribution page rather than one entered on the front end? I was thinking is_show_start_date would be the difference between 'start right now' & 'start at a date I specify' but I'm not quite sure about it in this context |
Thanks for the suggestions, will use the API instead of BAO functions. I included the WIP in the subject as I am still writing the test. ;) For the pledge_start_date, this will be a new field in the civicrm_pledge_block table. The purpose of this field is to allow the admin to sort of define a default start date while creating the contribution page. If the admin sets a start date, this field will be visible (but frozen) on the online contribution page which will allow users to see when the recurring pledge payments start. If the admin however does not set a default start date, then there is a field to allow users to specify their own start date (which will be stored in civicrm_pledge.start_date) while creating the pledge on the online contribution form. |
Hmm - so it's important to note that it is only possible to have a future start date if the paymentProcessorObject->supports('futureStartDate'); (e.g Authorize.net does & paypal doesn't.) I feel like on top of that there are the questions
Your pledge_start_date field is the answer to 2 but my concern is that people will want more flexibility than just defining a fixed date - probably things like '1st date of next month' will be common preferences. Pledges manage this (not that well) by having a frequency_date field - which you will need to ensure is set correctly. I feel like we need a more nuanced way to define the start_date - perhaps with a 2 part format. |
* Create recur record for pledge. | ||
* | ||
*/ | ||
public static function createRecurRecord($pledge, $params, $form) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Passing the $form object to the BAO is a really bad pattern we are trying to deprecate Try to figure out a way to avoid it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can be avoided by passing the payment instrument id. I will make the change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool
@@ -10,3 +10,6 @@ UPDATE civicrm_payment_processor pp, civicrm_payment_processor_type ppt SET pp.i | |||
-- CRM-17815 | |||
{include file='../CRM/Upgrade/4.7.beta8.msg_template/civicrm_msg_template.tpl'} | |||
|
|||
-- CRM-18854 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is in the incorrect file
My biggest reservation here remains the use of a hard-coded start date which just feels too restrictive to be of real use (vs something that would allow you to define the day of the month that it should start on or one month after the page is filled in). I feel like to put it in core it should be a good enough schema design to fill these equally common scenarios. @JoeMurray @colemanw @guyiac was this schema aspect discussed? |
A few thoughts. I see where you are going with this - relative dates would be be nice for the start date, in that it would be consistent with many of the date fields in Civi. However, I will note that even in the back end, on the New Pledge screen, the “Payments Start” field is an absolute date field with no relative dates available. I am taking on ultimate financial responsibility for this work because my client needs the functionality and the date isn’t really negotiable - I need it on or before 7/1. Can I push it to 7/4 or 7/6? Yeah, I can - but I can’t really push it beyond that. Now, whether that functionality is delivered in core now, or delivered in an extension now and put into core later, I don’t really care (and I’ll note that the latter would fit with iterate by month, leap by extension). But that’s a call for you guys - I just need the functionality. I will say this functionality as specified is of real use to our client. The way the pledges are done in UCC churches in the states fits this functionality. Church members pledge during the pledge drive for the upcoming year (which is done several months before the new pledge year is scheduled to start), and the user will know the date they want the new pledge payments to start - which is usually when the current pledge year ends - but doesn't necessarily have to be exactly. |
The model I like for something like this is what Jon did with relative date filters - which uses an option set to define a whole lot of possible configs. If we agreed that was a good model then the start_date field would be a text field & it would hold values like fixed_date.2017-09-09 Apart from the fixed_date prefix the rest would be defined in an option group. I would not say the option group & formats would need to be defined at this stage - but if that was the 'right' way to go it would affect how we store pledge_payment_block.start_date now & we would need to agree the plan & heavily comment the code to make that plan clear. Possibly we would rename to pledge_payment_block.start_date to pledge_payment_block.start_configuration or pledge_payment_block.start_description if we go that way. |
My sense of use case is that folks want to specify a) a delay to when they want to start making payment, so month/year is more important than day, and b) probably day of month for the start so that contributions are paid on a particular day. I think it is reasonable to ask to expose existing backend functionality in front end without making it more elaborate. The relative date filters used on reports are defined at https://github.com/civicrm/civicrm-core/blob/master/xml/templates/civicrm_data.tpl#L960, and include lots of stuff but none of it meets Guy's use case of an absolute start date. The recur options for contributions also don't include anything (at present) in terms of an absolute date for starting the series (https://github.com/civicrm/civicrm-core/blob/master/CRM/Contribute/Form/Contribution/Main.php#L517). Accepting for the moment that there is a need to expand scope, and that we want to support user specifying day of month, start a month from now, or some date in the future. All of these can and should be changed into an absolute start date for storage in the schema. If I were to create a maximal specification that provides all reasonable options, I'd likely include a option group with 2 options, enabling secondary widgets when the option is selected
Thinking about this, I do think that widget 2 is useful and slightly more like how people think about what they are doing in most use cases. But the widget for 2 can be added later as a separate enhancement with no change in schema. So I'd like to let this go through. |
"All of these can and should be changed into an absolute start date for storage in the schema." Yes they have to be changed into an absolute date on the pledge object. And in some cases the person creating the pledge would be entering their own start data - which would need to be passed straight through and would be a data picker on the contribution page - which is all good. The schema change I'm concerned about is adding start_date to the pledge_block. The pledge_block is the configuration for what the start date would default to for any given contribution page. I's the contribution-page-specific configuration of this default start date that I think would be too unflexible for most use cases if we just add a date field to pledge_block. If I configure a contribution page for people's payments to start in one month's time then I'm going to set the date to 24 July 2016. Anyone who signs up on my contribution page today will get that start date. Tomorrow I have to edit my contribution page config to change it or they will get less than a month. Note I don't especially care if the UI provides flexibility between a delay vs a fixed date - only whether the schema permits it and it can be added later without an upgrade script. As you point out the start_date / first_payment_date should possibly be an option for recurring and indeed for normal contributions - I hadn't thought of that & I'm not sure if it makes more sense to put the field in the main civicrm_contribution_page table instead. |
Reading through this more closely I'm changing my mind. Let's change So when a pledge is made and a backoffice contribution is to be made against it (http://dmaster.demo.civicrm.org/civicrm/contact/view/contribution?reset=1&action=add&cid=202&ppid=9&context=pledge&mode=live) there is a date field for the contribution that will determine when it is made. One can make this the start of a recurring series of contributions, and normally this would be monthly recurrence every month after the start date. So far so good. The spec agreed on was a minimum viable product approach to adding the ability to make a contribution page: in the original email thread it was "add the front end functionality including adding start date option to contribution page and setting up pledge and associated recurring payment from front end", which became https://docs.google.com/document/d/1lsvbhPQEdFuBki7jJKcftYTXIBiURZq00vFX-Qw7quA/edit. Eileen's raised various options to improve the flexibility beyond spec of a single absolute date reference on the contribution page config. I responded above looking only to the most recent of her points on this thread by speaking about options for user choosing date, not administrator on contribution page config, emphasizing ability of a date field for the contribution to do what Eileen wanted, rather than the more complex suggestions she had been making, quite rightly, about how to store the options on the contribution page config. See the google doc which contains a revised and expanded spec. I see the value of allowing administrator to default the date to various relative dates from the date user is creating donation: for example, specific day of month is most obvious. We could support the following as storage formats in a varchar(64) field: |
Edsel, could you go ahead and start work on this spec please. Thanks. |
We will also be adding a future_start_date field to the civicrm_payment_processor & civicrm_payment_processor_type tables which will allow devs to specify if the payment processor allows setting payments for future start dates. |
@@ -149,7 +149,7 @@ public function testGetPledgeStartDate() { | |||
|
|||
// Try with fixed date | |||
$params = array( | |||
'pledge_start_date' => serialize(array('contribution_date' => '2016-06-10')), | |||
'pledge_start_date' => serialize(array('contribution_date' => '2016-06-10')), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think as preference we are switching to json_encode now rather than serialize @colemanw is that your feeling ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eileenmcnaughton should I go ahead and switch to using json_encode rather than serialize?
MISC-113 Added JS for adjust recurring start date MISC-110 Added future start date values on install MISC-113 Added script on install and upgrade MISC-110 Added form rule for pledge start date MISC-110 Removed explicit create of recur record -- minor change -- minor change -- minor change MISC-113 Added unit test MISC-113 Removed old webtest MISC-113 Resolved jenkins errors MISC-110 Style changes MISC-113 Allowed NULL values for future start date MISC-110 Removed future_start_date field from payment processor MISC-110 Removed future_start_date field from data
MISC-113 Jenkins style fixes
…h future start date MISC-110 Bug fix
Conflicts: CRM/Contribute/BAO/Contribution/Utils.php
Rebased PR. |
Thanks a lot @colemanw! Really appreciate the help. Looks like all tests have passed :D |
jenkins, test this please |
Code and functional testing look good to me. |