Update docblock for doPayment function #13844
Conversation
|
(Standard links)
|
CRM/Core/Payment.php
Outdated
| * by those workflows. Ideally all workflows would create a pending contribution BEFORE calling doPayment (eg. https://github.com/civicrm/civicrm-core/pull/13763 for events) | ||
| * @fixme Creating a contribution record is inconsistent! We should always create a contribution BEFORE calling doPayment. However currently: | ||
| * * Contribution Pages: Creates a contribution before calling doPayment. | ||
| * * Contribution Pages with Membership: Does NOT create a contribution. UNLESS auto-renew=TRUE in which case both a recurring contribution |
There was a problem hiding this comment.
@mattwire I think that when there is no recurring it does?
There was a problem hiding this comment.
@eileenmcnaughton When there is no recurring what does? contribution page with membership does not create a contribution if auto renew is disabled.
There was a problem hiding this comment.
Hmm - I'd have to spend a while digging to check but we have
- membership with separate contribution
- membership without separate contribution
- the above with combinations of renew
My other worry about documenting the gaps here is we might not remember to remove them - perhaps we should instead be more aggressively documenting the places that should, but don't, create pending contributions or using an issue in gitlab to track the last status of the meta project 'make all places create pending & then call doPayment & then call Payment.create' and just point to that from here?
There was a problem hiding this comment.
@eileenmcnaughton Every time I work on / develop a payment processor I have to go through and guess what might be returned and what should happen. I'm proposing adding these code comments because it's the only place a developer will actually find them! And better to start by documentation what is actually happening (with lot's of @fixme tags indicating this is not the preferred way). If we were going to fix all the "known" and the "unknown" workflows in the next release I'd agree, but realistically it's a year or two away and in the mean time we still need payment processors to try and actually work.
CRM/Core/Payment.php
Outdated
| * 'payment_status_id' => {as above} | ||
| * 'trxn_id' => {trxn ref to link to record at payment processor} | ||
| * 'fee_amount' => {optional fee charged by processor} | ||
| * 'net_amount' => {optional net amount (ie. amount minus fee))} |
There was a problem hiding this comment.
I don't think we should encourage returning net_amount - that can be calculated & will be elsewhere if not returned
There was a problem hiding this comment.
Ideally we should be returning nothing but payment_status_id and the payment processor should be saving the other parameters. The workflows will only save certain parameters, net_amount is one of them. The comment states that these should only be passed if a contribution ID is not created - and it's marked as optional. In the long run when all flows are fixed this won't matter.
There was a problem hiding this comment.
I still don't think we should document net_amount as we don't want to be recommending something that is not preferred
There was a problem hiding this comment.
Problem we've got is that it's the only way you can actually record the amount - see eg. https://github.com/civicrm/civicrm-core/blob/master/CRM/Event/Form/Registration/Confirm.php#L1002
This comment is not meant to be "how it should be done" as that requires refactoring all these workflows so contribution is created first, at which point the payproc can take care of any/all of these params. But this is how it "has to be done" currently if you actually want to save these params.
There was a problem hiding this comment.
if net_amount is empty it will still be calculated in the BAO no ?
mattwire
added
the
needs-work-not-review-ready
mattwire
changed the title
|
Need to decide whether this sits in the code, the docs or a gitlab issue as it needs to be updated each time we improve the code. |
mattwire
force-pushed
the
corepaymentmore_essay
branch
from
d257800 to
278c5e1
Compare
mattwire
changed the title
|
@eileenmcnaughton I've add the detail to a gitlab issue and linked it from the docblock. Also cleaned up the existing docblock somewhat. If you are happy please merge :-) |
eileenmcnaughton
removed
the
needs-work-not-review-ready
Overview
Further investigation and explanation into how and when contributions are created in relation to doPayment
Before
Less clear
After
More clear
Technical Details
Comments
Comments
@eileenmcnaughton This extends what I wrote, as I found out some more.