[WIP] Modify backend Record Payment forms to use payment create API. #13330

jitendrapurohit · 2018-12-20T10:45:32Z

Overview

Modify backend Record Payment forms to use payment create API and move API code to BAO layer.

Before

payment create api exists but is not used by backend record payment forms.
Email receipt is only accessible by backend forms.

After

Payment create API is used to records payments for pending or partially paid contributions.
Email receipt function is separated so that it can be also used by front-end payments.

Comments

This is a part of #12319 PR and it only contains fix for the backend payment forms. I'll create another PR to fix partial payment in front end contribution pages.

@magnolia61 Can you confirm if this works fine in your testing? Note that front end fix is yet to be created separately. Thanks.

ping @monishdeb @eileenmcnaughton

civibot · 2018-12-20T10:45:34Z

(Standard links)

If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

magnolia61 · 2018-12-21T12:47:26Z

I tested this PR on the RC and the scenario of a 50 euro event fee paid for and changed to 30 extra so the balance is 30.
Result when I use 'record payment' from the backend:

The contribution as a whole changes to 'Completed'
The line-item for the 30 euro extra keeps the status 'partially paid'
An extra line item is created with status Complete
Event status changes from Pending to Registered
No confirmation email is being sent (maybe this is current and expected behavior). And by this I mean no confirmation email of the payment, nor of the event registration status change.

Note about nr.5 I think when using the backend it would be ideal if one submits a payment that completes the contribution for an event or membership, an option is given to either send an confirmation email or not. Sometimes am administrator just wants to register payments without sending emails to the participants. So either an optional confirmation mail or the current behavior: no mail after backend completion of an event contribution is ok I think.

This is a partial of civicrm#13330 - trying to separate out the main code move from the changes. My only change was to change the name to addPayment since that is what we are doing but here are some additionaly thoughts. 1) I'm inclined to put the class onto a new pseudo-bao CRM_Finanial_BAO_Payment. We already have this pseudo-entity on the api level & it gives us a way to build up a place for the functions we are migrating towards. 2) I think we should consider changing the input parameters to this function to be - contributionID, total_amount & line_items or in some way limiting the parameters tha t get passed through. We have historically gotten ourselves in trouble by having people hacking in another parameter to go through whereas the strength of the api is clear inputs & outputs. I note the api will need to support 'is_email_receipt' as will this fn & the CRM_Contribute_Form_AdditionalPayment::emailReceipt should be moved off the form layer - possibly to our new BAO 3) I think the exceptions need to be CRM_Core_Exceptions if thrown in the BAO although this is kinda cosmetic

This is a partial of civicrm#13330 - trying to separate out the main code move from the changes. My only change was to change the name to addPayment since that is what we are doing but here are some additionaly thoughts. 1) I'm inclined to put the class onto a new pseudo-bao CRM_Finanial_BAO_Payment. We already have this pseudo-entity on the api level & it gives us a way to build up a place for the functions we are migrating towards. 2) I think we should consider changing the input parameters to this function to be - contributionID, total_amount & line_items or in some way limiting the parameters tha t get passed through. We have historically gotten ourselves in trouble by having people hacking in another parameter to go through whereas the strength of the api is clear inputs & outputs. I note the api will need to support 'is_email_receipt' as will this fn & the CRM_Contribute_Form_AdditionalPayment::emailReceipt should be moved off the form layer - possibly to our new BAO 3) I think the exceptions need to be CRM_Core_Exceptions if thrown in the BAO although this is kinda cosmetic Change-Id: Ib5caf0d878e9c5cc735cb1a20c640f61da2dbfc6

This is a partial of civicrm#13330 - trying to separate out the main code move from the changes. My only change was to change the name to addPayment since that is what we are doing but here are some additionaly thoughts. 1) I'm inclined to put the class onto a new pseudo-bao CRM_Finanial_BAO_Payment. We already have this pseudo-entity on the api level & it gives us a way to build up a place for the functions we are migrating towards. 2) I think we should consider changing the input parameters to this function to be - contributionID, total_amount & line_items or in some way limiting the parameters tha t get passed through. We have historically gotten ourselves in trouble by having people hacking in another parameter to go through whereas the strength of the api is clear inputs & outputs. I note the api will need to support 'is_email_receipt' as will this fn & the CRM_Contribute_Form_AdditionalPayment::emailReceipt should be moved off the form layer - possibly to our new BAO 3) I think the exceptions need to be CRM_Core_Exceptions if thrown in the BAO although this is kinda cosmetic

eileenmcnaughton · 2018-12-31T06:23:12Z

Seamus merged my 'move code only' sub-cut of this - so https://github.com/civicrm/civicrm-core/compare/master...eileenmcnaughton:payment_j?expand=1 is the rebased commit from this PR

I feel like the 2 things I feel I want clarity on at this stage are

should we create a new class - per comments on the on [REF] extract internals of Payment.create into function on BAO class. #13370 for the payments functions we re migrating towards
what is the correct email to send when a payment is received - ie. I assume that when a payment is received and we are fully paid we send the same email as we do for a normal contribution receipt but we use a separate email for a part payment? It might make sense to wrap this action in a sendconfirmation action like we do on Contribution - I see @magnolia61 has suggested we add a checkbox to determine whether to send an email on the form.

monishdeb · 2019-01-04T14:24:14Z

@jitendrapurohit can you please rebase your PR

eileenmcnaughton · 2019-01-04T21:16:22Z

@jitendrapurohit @monishdeb I actually did a rebase - there was a link to it from the other PR - I didn't open it because I wanted to discuss the question of a new class for these functions first - per the #13370 comments & also the question about the email will affect a lot of things

… it. This is per discussions coming off civicrm#13330 Note that this ONLY moves the code - that PR does other things as well that still need to be worked through

jitendrapurohit · 2019-01-21T11:31:25Z

I've rebased the PR with the current status of master. This now contains fixes related to

migration of email confirmation code to the newly added financial bao payment.php
Uses payment create API for backend payments.

monishdeb · 2019-01-23T10:20:34Z

I like this patch as it moves the code to process refund or owed payment into Payment.create API. Did a quick test with partial and refund payment in UI and it works fine, also with the Send Email reciept. Added UT looks good. @eileenmcnaughton @mattwire @magnolia61 any final thoughts before merging this PR?

On a separate note, how about moving CRM_Contribute_BAO_Contribution::recordAdditionalPayment to CRM_Financial_BAO_Payment::recordAdditionalPayment in a separate PR

mattwire · 2019-01-23T14:59:44Z

CRM/Contribute/Form/AdditionalPayment.php

-      if ($sendReceipt) {
-        $statusMsg .= ' ' . ts('A receipt has been emailed to the contributor.');
-      }
+    if (empty($payment['is_error']) && !empty($this->_params['is_email_receipt'])) {


So I think the payment params could be modified by hooks once Payment.create has been called but we are relying on the value of is_email_receipt from before that function is called. Are we able to use the returned parameters of Payment.create to check if is_email_receipt stayed TRUE and therefore an email actually got sent?

@mattwire I don't think payment params can be modified by any hooks, as I don't see pre/post hook are invoked inside Payment::create() BAO fn, or is there any at API level which I am unaware of?

ok, then I'm ok with this.

mattwire

Just one comment about the is_email_receipt parameter. Apart from that all fine from me - nice cleanup too!

eileenmcnaughton · 2019-01-31T06:00:05Z

So where I'm at with this in general is that the Payment.create function should send appropriate email receipts parameter dependent & I want to clarify that & get the tests sorted for that .

The current blocker for me is clarifying exactly what emails should go out (and getting that separately committed & tested in preparation). The Payment.create api would ideally be called from many forms - ie. we should anticipate that eventually we would replace all the places in the code that call Contribution.completetransaction with Payment.create (with the latter being called if the payment does complete a transaction.)

It seems to me we have 3 scenarios

we don't want any emails to go out - the current bool seems adequate for this one :-)
we want emails to go out and we are adding a payment but it does NOT complete the contribution
we want emails to go out and we are adding a payment but it DOES complete the contribution - in this case what combination of payment receipt, contribution completed receipt, event receipt do we send out?

In the latter case do we send out the online invoice receipt as we would when calling Contribution.completeorder or some other one or do they get 2 receipts. What about in case 2? Is the off/on is_email_receipt the right parameter to pass in

@JoeMurray I pinged the above question to you somewhere else in the discussions around this but it got lost along the way

Note
At the moment 'payment_type' and 'participant_id' are being added to what is passed into payment.create but these seem like things that could be calculated from Payment create.

monishdeb · 2019-01-31T06:12:51Z

@eileenmcnaughton I have a concern in handling both partial and refund payment in a single Payment::create API rather than handling them in Payment::partial and Payment::refund, BAO fns and APIs respectively. Actually as per the refund spec here I suppose to have a separate API but if we I go with this PR fix then I shouldn't need to have separate API. What do you think? Or we should bring the separation in a separate PR ?

NOTE: The Payment::create should be restricted to Contribution.completetransaction + handling financial record on Complete payment.

JoeMurray · 2019-02-06T17:29:27Z

Re: separate API for refund vs include in existing payment.create as a negative payment.

We're moving from merely recording refunds in CiviCRM to actually sending the money out in some current work.

Sending money out tends to need a separate permission. I would prefer for refunds to be in a separate BAO / api to facilitate this.

JoeMurray · 2019-02-06T17:40:48Z

Eileen wrote:

3. we want emails to go out and we are adding a payment but it DOES complete the contribution - in this case what combination of payment receipt, contribution completed receipt, event receipt do we send out?
In the latter case do we send out the online invoice receipt as we would when calling Contribution.completeorder or some other one or do they get 2 receipts.

Reviewing the message templates, I don't see one that is specific to just a payment being received (not that we shouldn't have one, it's just that we don't at present):

So my sense at this point is that we should have a receipt go out for any payment received, and I bet it will be the same template whether the payment is partial or complete or overpaid. For events and memberships, I think those should go out only when the object is completely paid for. So at most two emails currently unless there is a hack to produce price sets that include events and memberships, in which case it's conceivable there could be three eventually.

NB: I haven't reviewed code for this; these are my thoughts on business process objectives given my understanding of how things are.

JoeMurray · 2019-02-06T17:41:46Z

Eileen wrote:

At the moment 'payment_type' and 'participant_id' are being added to what is passed into payment.create but these seem like things that could be calculated from Payment create.

+1

This adds a new api Payment.sendconfirmation intended to be an equivalent of Contribution.sendconfirmation. The key thing this needs to be able to do is to load up all related information to assign to the template - this is not all covered in this PR & until it is functionally equivalent to AdditionalPayment::sendEmail we can't switch over but since it's new functionality we should be able to merge & keep working on it. Note that there is discussion on the PR this relates to (civicrm#13330) about what should happen if 'Payment.create' api accepts 'is_email_receipt'. I think the most logical outcome is that receipts would go out if it caused the contribution to be completed (ie. we pass is_email_receipt into completetransaction api). However, I don't feel like that is 'settled' yet - separating into a second api (Payment.sendnotification) means we will have 2 generic function which can be called from multiple places in the code rather than 2 functions tightly tied to the form layer. I think it's OK if it is 2 not one

monishdeb · 2019-02-19T14:24:13Z

@eileenmcnaughton @jitendrapurohit I have rebased this PR. Can you please indicate what is the outstanding issue remaining in this patch?

eileenmcnaughton · 2019-02-19T20:00:20Z

@monishdeb I think we should pull all the email receipt changes out of it - with that last email receipt change I made I think we can simply call the api from the form & then remove all the entire email function - which will simplify this PR a lot

monishdeb · 2019-02-20T02:59:47Z

Hmm seems like I need to flesh out those code in a separate PR which only contains the Payment.create call on forms w/o email receipt changes. And after that PR is merged, we can rebase this PR

eileenmcnaughton · 2019-02-20T03:01:04Z

@monishdeb I'm happy to throw up a PR doing the send change - if you merge the pre-requisite PR first

eileenmcnaughton · 2019-02-20T03:01:36Z

(that might make what I mean clearer too :-)

monishdeb · 2019-02-20T03:01:47Z

Will do 👍

eileenmcnaughton · 2019-02-20T07:39:25Z

just adding the link for 'the prerequisit one' #13610

monishdeb · 2019-02-20T09:16:54Z

@eileenmcnaughton submitted a sub-PR #13647

eileenmcnaughton · 2019-07-27T05:03:21Z

Closing this as it has been superceded by later work - currently stalled pending review on #14673

civibot bot added the master label Dec 20, 2018

jitendrapurohit mentioned this pull request Dec 20, 2018

dev/financial#20 - Extend pay now functionality for partially paid contribut… #12319

Closed

eileenmcnaughton mentioned this pull request Dec 30, 2018

[REF] extract internals of Payment.create into function on BAO class. #13370

Merged

jitendrapurohit force-pushed the partial-1 branch 2 times, most recently from cb4345f to 2678bd4 Compare January 9, 2019 11:40

jitendrapurohit referenced this pull request Jan 11, 2019

Use payment create API for backend record payment forms

570243c

eileenmcnaughton mentioned this pull request Jan 13, 2019

[REF] Create pseudo bao CRM_Financial_BAO_Payment & move create function to it #13443

Merged

jitendrapurohit force-pushed the partial-1 branch 3 times, most recently from 4a7cac0 to 69bec4b Compare January 21, 2019 11:20

jitendrapurohit force-pushed the partial-1 branch 2 times, most recently from 18f934e to 1e80516 Compare January 21, 2019 13:50

mattwire reviewed Jan 23, 2019

View reviewed changes

eileenmcnaughton mentioned this pull request Feb 11, 2019

Add new Payment.sendconfirmation api #13561

Merged

Use payment create API for backend record payment forms

d5ea31f

monishdeb force-pushed the partial-1 branch from 1e80516 to c3f5ea1 Compare February 19, 2019 14:07

Move addPayment to CRM_Financial_BAO_Payment

ceeacdf

monishdeb force-pushed the partial-1 branch from c3f5ea1 to ceeacdf Compare February 19, 2019 17:44

monishdeb mentioned this pull request Feb 20, 2019

Modify backend Record Payment forms to use payment create API #13647

Closed

monishdeb changed the title ~~Modify backend Record Payment forms to use payment create API.~~ [WIP] Modify backend Record Payment forms to use payment create API. Feb 20, 2019

eileenmcnaughton closed this Jul 27, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Modify backend Record Payment forms to use payment create API. #13330

[WIP] Modify backend Record Payment forms to use payment create API. #13330

jitendrapurohit commented Dec 20, 2018

civibot bot commented Dec 20, 2018

magnolia61 commented Dec 21, 2018 •

edited

Loading

eileenmcnaughton commented Dec 31, 2018

monishdeb commented Jan 4, 2019

eileenmcnaughton commented Jan 4, 2019 •

edited

Loading

jitendrapurohit commented Jan 21, 2019

monishdeb commented Jan 23, 2019 •

edited

Loading

mattwire Jan 23, 2019

monishdeb Jan 23, 2019

mattwire Jan 23, 2019

mattwire left a comment

eileenmcnaughton commented Jan 31, 2019

monishdeb commented Jan 31, 2019 •

edited

Loading

JoeMurray commented Feb 6, 2019

JoeMurray commented Feb 6, 2019

JoeMurray commented Feb 6, 2019

monishdeb commented Feb 19, 2019

eileenmcnaughton commented Feb 19, 2019

monishdeb commented Feb 20, 2019

eileenmcnaughton commented Feb 20, 2019

eileenmcnaughton commented Feb 20, 2019

monishdeb commented Feb 20, 2019

eileenmcnaughton commented Feb 20, 2019

monishdeb commented Feb 20, 2019

eileenmcnaughton commented Jul 27, 2019

[WIP] Modify backend Record Payment forms to use payment create API. #13330

[WIP] Modify backend Record Payment forms to use payment create API. #13330

Conversation

jitendrapurohit commented Dec 20, 2018

Overview

Before

After

Comments

civibot bot commented Dec 20, 2018

magnolia61 commented Dec 21, 2018 • edited Loading

eileenmcnaughton commented Dec 31, 2018

monishdeb commented Jan 4, 2019

eileenmcnaughton commented Jan 4, 2019 • edited Loading

jitendrapurohit commented Jan 21, 2019

monishdeb commented Jan 23, 2019 • edited Loading

mattwire Jan 23, 2019

Choose a reason for hiding this comment

monishdeb Jan 23, 2019

Choose a reason for hiding this comment

mattwire Jan 23, 2019

Choose a reason for hiding this comment

mattwire left a comment

Choose a reason for hiding this comment

eileenmcnaughton commented Jan 31, 2019

monishdeb commented Jan 31, 2019 • edited Loading

JoeMurray commented Feb 6, 2019

JoeMurray commented Feb 6, 2019

JoeMurray commented Feb 6, 2019

monishdeb commented Feb 19, 2019

eileenmcnaughton commented Feb 19, 2019

monishdeb commented Feb 20, 2019

eileenmcnaughton commented Feb 20, 2019

eileenmcnaughton commented Feb 20, 2019

monishdeb commented Feb 20, 2019

eileenmcnaughton commented Feb 20, 2019

monishdeb commented Feb 20, 2019

eileenmcnaughton commented Jul 27, 2019

magnolia61 commented Dec 21, 2018 •

edited

Loading

eileenmcnaughton commented Jan 4, 2019 •

edited

Loading

monishdeb commented Jan 23, 2019 •

edited

Loading

monishdeb commented Jan 31, 2019 •

edited

Loading