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.

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

Merged
merged 27 commits into from
Jul 8, 2016

Conversation

Edzelopez
Copy link
Contributor

@Edzelopez Edzelopez commented Jun 14, 2016

@Edzelopez Edzelopez changed the title CRM-18854 Added support for recurring contributions for online pledges CRM-18854 [DO NOT MERGE - WIP] Added support for recurring contributions for online pledges Jun 14, 2016
'id' => $contribution->id,
'contribution_recur_id' => $recurringPledgeID,
);
CRM_Contribute_BAO_Contribution::add($contribParams);
Copy link
Contributor

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

@eileenmcnaughton
Copy link
Contributor

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

@Edzelopez
Copy link
Contributor Author

Hi @eileenmcnaughton,

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.

@eileenmcnaughton
Copy link
Contributor

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

  1. do you want to allow the person entering the pledge to set their own start date (e.g a field is_choose_start_date
  2. what should the default be (which would be over-ridable if they can set their own).

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) {
Copy link
Contributor

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!

Copy link
Contributor Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

cool

@Edzelopez Edzelopez changed the title CRM-18854 [DO NOT MERGE - WIP] Added support for recurring contributions for online pledges CRM-18854 [needs-review] Added support for recurring contributions for online pledges Jun 21, 2016
@@ -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
Copy link
Contributor

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

@eileenmcnaughton
Copy link
Contributor

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?

@guyiac
Copy link
Contributor

guyiac commented Jun 22, 2016

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.

@eileenmcnaughton
Copy link
Contributor

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
day_of_month.1_next
delay_by.30_days

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.

@JoeMurray
Copy link
Contributor

JoeMurray commented Jun 23, 2016

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

  1. option for absolute date to start, defaulting to today, validating as today or later
  2. options for day of month, 1 - 28 and last day of month; storing as next date >= today that matches
  3. mmmaybe, start a month from today
    My issue is that 2 is pretty much a duplication of choosing date in next 30 days from the calendar, so it doesn't seem to really improve usability.

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.

@eileenmcnaughton
Copy link
Contributor

"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.

@JoeMurray
Copy link
Contributor

Reading through this more closely I'm changing my mind.

Let's change
+ALTER TABLE civicrm_pledge_block ADD pledge_start_date DATE NULL DEFAULT NULL AFTER additional_reminder_day;
to
+ALTER TABLE civicrm_pledge_block ADD pledge_start_date varchar(64) NULL DEFAULT NULL AFTER additional_reminder_day;

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:
no_adjustment - means date contribution recur is setup
absolute_date.yyyy-mm-dd - means yyyy-mm-dd date, or if that is in past, date contribution recur is setup
day_of_month.dd where 1 <= dd <= 31 - means dd of month contribution recur is setup if dd >= current day of month, else dd of next month.

@JoeMurray
Copy link
Contributor

Edsel, could you go ahead and start work on this spec please. Thanks.

@Edzelopez Edzelopez changed the title CRM-18854 [needs-review] Added support for recurring contributions for online pledges CRM-18854 [DO NOT MERGE - WIP] Added support for recurring contributions for online pledges Jun 24, 2016
@Edzelopez
Copy link
Contributor Author

Edzelopez commented Jun 25, 2016

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.

@Edzelopez Edzelopez changed the title CRM-18854 [DO NOT MERGE - WIP] Added support for recurring contributions for online pledges CRM-18854 [needs-review] Added support for recurring contributions for online pledges Jun 28, 2016
@@ -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')),
Copy link
Contributor

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 ?

Copy link
Contributor Author

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?

Edzelopez added 22 commits July 7, 2016 16:51
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
Conflicts:
	CRM/Contribute/BAO/Contribution/Utils.php
@Edzelopez
Copy link
Contributor Author

Rebased PR.

@Edzelopez
Copy link
Contributor Author

Thanks a lot @colemanw! Really appreciate the help. Looks like all tests have passed :D

@Edzelopez
Copy link
Contributor Author

jenkins, test this please

@colemanw
Copy link
Member

colemanw commented Jul 8, 2016

Code and functional testing look good to me.

@colemanw colemanw merged commit dccd9f4 into civicrm:master Jul 8, 2016
@Edzelopez Edzelopez deleted the MISC-110 branch March 6, 2017 10:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants