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

Payment.create should not set contribution date to today #17688

Merged

Conversation

artfulrobot
Copy link
Contributor

@artfulrobot artfulrobot commented Jun 24, 2020

Overview

I noticed that calling the payment.create API resets the contribution date.

Before

  1. create a pending contribution with a historical date.
  2. create a payment on it also with a historical date
  3. contribution receive_date gets reset to today's date.

After

  1. create a pending contribution with a historical date.
  2. create a payment on it also with a historical date
  3. contribution receive_date gets update to the payment's date.

Technical Details

Has test.

@eileenmcnaughton @JoeMurray @mattwire @adixon may be interested

@civibot
Copy link

civibot bot commented Jun 24, 2020

(Standard links)

@adixon
Copy link
Contributor

adixon commented Jun 24, 2020

Nice catch, thanks.

@eileenmcnaughton
Copy link
Contributor

I agree this is better - my first thought is we shouldn't change it at all

@artfulrobot
Copy link
Contributor Author

@eileenmcnaughton yes, I can see the appeal of that.

But I think this pr "fixed"what the previous design behaviour was: the code looks for trxn_date but it just wasn't being passed in, so I passed it in.

@eileenmcnaughton
Copy link
Contributor

@artfulrobot so I agree trxn_date should be passed in & I agree it's better. My worry is simply that that if we lock in this change then it will seem like there is agreement that receive_date should be updated - which I'm not sure about. Code comments indicating that it's still an open question would mitigate that - in the test would be enough as that is the part locking in the behaviour -the other part is just passing an obviously sensible param

@artfulrobot artfulrobot force-pushed the artfulrobot-trxn-id-and-date branch from 4a19cb6 to 4fa3b81 Compare June 25, 2020 06:18
@artfulrobot
Copy link
Contributor Author

@eileenmcnaughton I've added a descriptive docblock, altered the test so that it's a one-word change to test either behaviour, and set up an issue at https://lab.civicrm.org/dev/financial/-/issues/139 to discuss this.

@eileenmcnaughton eileenmcnaughton merged commit a5e01e6 into civicrm:master Jun 25, 2020
@eileenmcnaughton
Copy link
Contributor

@artfulrobot perfect - thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants