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

Set api default for contributionRecur to Pending #19512

Closed

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Feb 3, 2021

Overview

Set api default for contributionRecur to Pending - note this is one options - see techical details

Before

A contribution created with the v3 ContributionRecur.create api will have a status of 'Completed' if contribution_status_id is not specified

After

A contribution created with the v3 ContributionRecur.create api will have a status of 'Pending' if contribution_status_id is not specified.

Per existing behaviour adding a completed contribution record (through contribution.create) to a recurring contribution or (better) updating an already attached contribution record to completed will update the recurring contribution record to 'In Progress' or to 'Completed' depending on the number of installments vs the number of contributions

Technical Details

This has a DB default of 'Completed' which is not right as far as I'm concerned - the right
default is 'Pending' IMHO.

There are 2 ways we could address this

  1. We could update the DB default via a schema change
  2. add defaults on both v3 & v4 api. (this is a placeholder for the latter approach but I increasingly prefer the former)

I think we would need to advise of this change via change log, release notes, dev-digest
but I doubt it would be that controversial since contributions need to be
add to to contribution recur after it is created & if enough
contributions are added to complete it it would be updated to Completed

Comments

@mattwire @seamuslee001 @colemanw @artfulrobot @adixon @agh1 @KarinG - thoughts

This has a DB default of 'Completed' which is not right as far as I'm concerned - the right
default is 'Pending'. We could update the DB default or add defaults on both
v3 & v4 api.

I think we would need to advise of this change via change log, release notes, dev-digest
but I doubt it would be that controversial since contributions need to be
add to to contribution recur after it is created & if enough
contributions are added to complete it it would be updated to Completed
@civibot
Copy link

civibot bot commented Feb 3, 2021

(Standard links)

@civibot civibot bot added the master label Feb 3, 2021
@eileenmcnaughton
Copy link
Contributor Author

Test fail relates but I'll worry about concept approval first

@adixon
Copy link
Contributor

adixon commented Feb 3, 2021

I'd also be happy with setting the status to be a required field, but pending is fine as a default. Recurring schedules cover so many different mechanisms that setting any defaults or making assumptions is bound to cause trouble (and already has for so long ...). Setting a recurring schedule status to 'completed' as a default is truly wacky (since that normally means that there would be no more contributions in that schedule). 'In progress' would be another reasonable default, and probably what this was intended to be, but I'm just throwing that in as evidence of the dangers of setting defaults.

@agh1
Copy link
Contributor

agh1 commented Feb 3, 2021

@adixon has a good point. I don't know off the top of my head, but if a Pending contributionRecur automatically becomes In Progress when a contribution is recorded against it, I think it would be reasonable to default to Pending. If not, I think it would probably be safer to just make it required in the API. (Or to provide some backwards compatibility, we could default but complain about it with a deprecation notice.)

@adixon
Copy link
Contributor

adixon commented Feb 3, 2021

@adixon has a good point. I don't know off the top of my head, but if a Pending contributionRecur automatically becomes In Progress when a contribution is recorded against it, I think it would be reasonable to default to Pending. If not, I think it would probably be safer to just make it required in the API. (Or to provide some backwards compatibility, we could default but complain about it with a deprecation notice.)

In fact, that's a good example of what not to do in core. Payment processors need to handle those changes, it absolutely should not be in core. "Reasonable" is very contextual and has been what got us into much of the current mess of assumptions in core.

But on rereading, I can see there is confusion here about what we're even talking about. @Eileen, can you update your issue to be more clear about the recurring record (schedule) vs the contribution itself which may or may not be generated as a side effect? Are you actually saying that if I create a new recurring schedule a contribution will also get created?

@eileenmcnaughton
Copy link
Contributor Author

eileenmcnaughton commented Feb 3, 2021

@adixon no - I'm saying that if you create a new pending recurring contribution record and then you attach a completed contribution record (or update an existing one to be complete) the core code base will alter the recurring contribution record status to in progress.

@eileenmcnaughton
Copy link
Contributor Author

@adixon @agh1 it's worth noting that most calls to create a recurring contribution DO pass in status. Everywhere in core does. However, I found some custom code where status was not passed in & it actually causes other problems in terms of the editability of the recurrings.

@mattwire
Copy link
Contributor

mattwire commented Feb 3, 2021

I'm saying that if you create a new pending recurring contribution record and then you attach a completed contribution record (or update an existing one to be complete) the core code base will alter the recurring contribution record status to pending.

@eileenmcnaughton The principal seems fine but I assume this comment should say "In Progress" not pending?

@eileenmcnaughton
Copy link
Contributor Author

@mattwire you are imagining it - look it says 'In progress' (where's that gaslighting emoji)

wmfgerrit pushed a commit to wikimedia/wikimedia-fundraising-crm that referenced this pull request Feb 9, 2021
…tatus_id to Pending.

On an ongoing basis we have been creating recurring contributions with
a status of completed, which means we need a UI hack to prevent usability
 issues for Donor services.

This has been a sin of omission - we have not been specifying contribution_status_id
 and the default is Completed not pending.

 I have raised civicrm/civicrm-core#19512 to
  discuss upstream but I think we should proceed with this
 in the meantime, and hopefully remove the hack when we deploy (I'll
 re-evaluate that once this is out & we have stopped digging).

Bug T272061

Change-Id: I206bdd71738b7b8b35cc256b717a4555148386da
@eileenmcnaughton
Copy link
Contributor Author

I've created #19934 & corresponding gitlab to alter the db default as that appears to have been agreed (once people were clear about the proposal and the fact the BAO would manage the status transition from pending when contributions were added).

Closing this out now

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.

4 participants