CRM-20610 expose payment edit form on the contribution page

[eileenmcnaughton]

@Monish this is the portion of #10729 that replaces the PaymentDetails section of the Contribution form. I've put it in this PR for discussion but given the pretty pictures already on that PR we should probably take what we agree back over there

---

• CRM-20610: Replace payment details block with editable payment list on 'Edit Contribution' form

[eileenmcnaughton]

???

[monishdeb]

Ohh wait this was included because I didn't included the fix #10680 about removing check_number from payment details block

[monishdeb]

So we can simply remove check_number. trxn_date is handled below. See at submit() function

[eileenmcnaughton]

& remove receive_date & my comment too :-)

[eileenmcnaughton]

Actually - foreach might be working against legibility now

[eileenmcnaughton]

I left off the change to this as I want to understand it better - my understanding is that it pre-exists & is not made worse by this?

[monishdeb]

Nope, it's needed for live mode. And won't affect the offline mode in addition to the new changes

[eileenmcnaughton]

This is obviously the main part. I have mixed feelings about the re-emergence of the accordian - but I don't presume to be the best opinion on that & aesthetic changes can happen later

[eileenmcnaughton]

oh bugger I was wrong - this is all about contribution Mode - yuck. will rethink

[eileenmcnaughton]

Anything in this table will not show on the NEW contribution form (I actually think it's kinda tidy the way it works)

[eileenmcnaughton]

ok - transaction field

[eileenmcnaughton]

ok - transaction field

[eileenmcnaughton]

ok - transaction field, but I do wonder with this & check_number if we should 'bubble up' payment edits to the contribution column. Possibly multiple - ie. if there were 3 checks we could simply put 123|567|890 into civicrm_contribution.check_number. This feels compatible with existing stuff to me.

[eileenmcnaughton]

I think this needs to come out of the payment block - probably above it

[monishdeb]

hmm agree

[eileenmcnaughton]

Ok I've raised #10777 to move that out as a prelude. Also resulted in less code duplication & a nice UI tweak that was in one place & not the other being retained

[eileenmcnaughton]

Shouldn't we just determine if we are in edit mode - perhaps use Core_Action - seems like this is a round-about-way of saying that. I guess you think it should be suppressed if there are no payments because it might seem 'obvious' that there aren't any

[monishdeb]

It's for 'Pending' contribution where no actual payment(s) were made. So to ensure we expose payment details block for pending contributions

[eileenmcnaughton]

OK - so the upshot here is I think we are removing too many receipt type fields.

[eileenmcnaughton]

Hmm - maybe suppressed for me locally rather than a PR issue - let's see...

[eileenmcnaughton]

nope - it is suppressed. I think we should move the receipt fields above the block

[eileenmcnaughton]

Actually this is the same as if $contribID isn't it - although I would always prefer a var like $isEditMode

[monishdeb]

as $isPending is ONLY set if contribution status is Pending so I thought why not name it so to add meaning

[eileenmcnaughton]

ok I guess cancelled would be a case where it would NOT be present.

[monishdeb]

yes

[eileenmcnaughton]

is this url correct? - has view in it

[monishdeb]

yup it's just a cut paste from above https://github.com/civicrm/civicrm-core/pull/10776/files#diff-c901defeafe677e3a1c2e18b793d18bcL189

[eileenmcnaughton]

@monishdeb I'm off to bed but here is where I got to with this.

I think it is very nearly there!

Overall I really like it. I know it's not the perfect UI that one might draw from scratch but if we avoid that as our benchmark & aim for incremental improvement here is how it fits,

• it adds the ability to edit the details of how a payment was made (credit card type, check number, last 4 digits of credit card, trxn_id) on a per payment basis.
• it makes the payments against a contribution more visible & makes it easier to make a partial payment from the contribution edit form
• it improves the code (although some of those improvements have been pre-merged so that might be less obvious to someone just seeing this pr
• it helps to disambiguate payments from contributions, in the code and for the user.
• when I tried testing without this patch I wound up inadvertantly getting 'flapping' transactions where my payment_instrument_id didn't match my payment, if I then re-edited the contribution. This solved that.

As an incremental change it works & I think it should be fairly non-controversial - having said that it might be worth pinging people that I think are pretty good from a UI POV like @lcdservices @agh1 @Stoob & @colemanw - just to check it seems OK to them. It's actually not dissimilar to a part of a screen mock up I saw @jamienovick do (he had a bunch of other things going on - but this as a re-usable code chunk looked like a part of his grand plan)

A lot of the things I had in this comment have now been resolved (it started out a lot longer) but there are still a few things..

1. We should figure out how to get the pop-up to size more appropriately (tangental to this PR & should be resolved separately)

2. on the payment edit form both card_type_id & pan_truncation should be optional (they don't necessarily have that information) (tangental to this PR & should be resolved separately)

3. there is an underlying issue with many-2-one payment data being stored on the contribution record as if there could only be one - this applies to check_number, trxn_id, payment_instrument_id & we should discuss what happens to these as we get multiple payments. The current (weird) behaviour is that if there is one check then that number will wind up in the check_number field. If there are multiple you probably wind up with only the most recent. (ping @JoeMurray )

• I would say the payment form postProcess should do the following

1. check_number - update the contribution.check_number to be a concatenation of all check_numbers against the contribution. For the vast majority of situations this will simply retain the existing behaviour of recording the single-check-number against it.

2. trxn_id - we could possibly do the same - there is more chance people have done their own weird & wonderful things with that field, but those are probably not the same people working with multiple payments & it's not like any individual one of the trxn_ids is more logical to use. Notably, we don't really support IPN - type payment processors doing multiple contributions against a payment.

3. payment_instrument_id - update to match the first payment that comes in & leave at that.

It's possible we should provide a way to edit these on the contribution record & not on the payment, this should not be too obvious, perhaps an edit-in-place hidden in an accordian down the bottom. This isn't a new issue, but it becomes pertinent to discuss with this change.

[monishdeb]

As per the new spec let me present the new behavior for pending contribution :

[eileenmcnaughton]

For comparison - I have a completed contribution with 2 payments already made against it - here is the before & after form -

BEFORE

AFTER

If I fill in a check number in the first form (current master) then afterwards the payments look like

[xurizaemon]

These   will complicate translation? Better handled in CSS I think. (Is it really an odd-row or are we using that in a different sense?)

[xurizaemon]

Aside from my whitespace nitpicks above, this looks great!

It might be illuminating if the issue articulates a little about the intended purpose (rather than the functional description). You know - "our neighbourhood org has a weekly promise meeting, and people complete donation process during the week", tell a bit of a story?

(I see the benefit of this functionality, but don't fully understand why you'd want to record a donation before the person makes it. Help me see why?)

[eileenmcnaughton]

We should move this to the payment form once they are off the edit form (I'm ok with leaving that out of scope for this PR though)

[eileenmcnaughton]

@xurizaemon yeah there is a fundamental confusion here - around about 3.x we had a long discussion about the fact that people want to make multiple payments against (for example) an event. At that time there was a one to one relationship between contributions or events and we identified that you actually have 2 things in that transaction - an invoice and a payment and it was resolved that actually a contribution maps to an invoice rather than to a payment. Hence it is now possible to register someone for an event & they get one contribution against which they can make n payments. We never really updated the UI (& many code places) to reflect the changes to the data model that changed around 4.3 to reflect this interpretation of what business entity maps to what

[eileenmcnaughton]

Removed WIP as this is open for comment rather than WIP now

[lcdservices]

I think this looks great, and don't really have anything to add in terms of the UI. A few functionality things though (which maybe are better handled in a separate improvement PR, as some are not directly related):

1)- for non-CC-transaction payments, I think we should have the ability to delete the payment.
2) - we might also want to expose the ability to change the payment method (for non-CC-transaction payments). let's say someone mistakenly records a cash payment and later realizes it is a check payment. currently there is no recourse apart from deleting the entire contribution
3) - should fee amount and net amount be present on the payment edit form? they are present on the record new payment form. there's seems to be an inconsistency there. actually, aren't those contribution-specific fields? if so, they shouldn't be on any version of the payment form.
4) - it looks like the check number field has been returned to the main payment details block, rather than getting ajax loaded in the div below that. I'd like to see the CC details also in that main payment panel. I understand why they may need to be ajax loaded, but at the very least they should be included in payment details accordion. right now they look like an awkward afterthought instead of data directly tied to the payment data.

re: @eileenmcnaughton comments concerning the shift in data structure from v4.3 --
it's a bigger discussion than we probably want to have here, but I would be a big advocate for changing the contribution terminology to invoice. it is, functionally speaking, an invoice record. and the "contribution" terminology has always been a cause for confusion. few organizations would call an event registration fee or membership dues a "contribution." it's a term that is imprecise, confusing, and inaccurate for describing what we're actually doing with the data. but I digress...

[eileenmcnaughton]

To your points

1. • yes good idea, separate issue
2. • wrt this there is a regression of sorts here in that regard, in that if there is only ONE payment then you can currently fix the payment type. I guess this would change that. I think you are suggesting it be added to the edit payment form - which makes sense to me & I guess it would resolve my concern about it not being possible to edit now. I think there needs to be a routine that updates the payment instrument on the contribution based on the payments against it, probably in the BAO create function on financial_trxn. The rule would need to be something like 'Never change the payment instrument if at least one payment still matches that instrument, if none match then change to match the one currently being edited'
3. should fee amount and net amount be present on the payment edit form? they are present on the record new payment form? Yes they should be - aren't they hidden in a nasty obscure accordian down the bottom? Anyway we need to confirm that before any merge.
4. let's address that one separately. I think check_number should be treated the same way as credit card type & pan_truncation & this PR doesn't actually change that - but I think there is some scope for reformatting & re-organisaion.

Of notes above I think number 2 is the one where we probably need to confirm the spec for.

So to clarify here is my proposal

• add a routine to the FinancialTrxn.create function that edits the parent contribution's check_number, trxn_id an payment_instrument_id. In the case of the first 2, edit to be a concatenation of those values for payments against the contribution. For the latter if should be unchanged if the payment instrument still matches at least one payment, otherwise change to match the one currently being edited.

[eileenmcnaughton]

@guanhuan any chance you can chase this up internally & see if we can unblock it - there are other bug fixes blocked by this

[jamienovick]

@eileenmcnaughton @JoeMurray @monishdeb

Sorry for slow response. I've spent some (a lot!) of time thinking about this and trying to abstract out what we are trying to achieve.

The overall objective is to clearly separate out the contribution from the payment entities and trying to reflect better the 1 to many relationship between them (or should that be many to many??). I feel that (from what I can see) this PR achieves some of this under certain specific circumstances.

My concern is that we are definitely only doing half of the job, and as such are leading to inconsistencies:

• When I first create a contribution there is nothing to explain that the contribution is obligation to pay and a payment will be created for me unless I set it to pending. Whilst I know this is "current behaviour" that doesn't make it correct behaviour - especially if our goal is to to clearly separate out the contribution from the payment. This leads to other problems as to what the fields on the contribution really mean in that context. Fields almost change meaning once we have a pending contribution. I feel this is the biggest problem with things as it stands.
• So what if I half complete the contribution payment fields when I first create it. Are these ignored?
• I'm a bit confused about what then payment status is - is it always confirmed?
• Dates are now a bit confusing. We have received date for the contribution at the start (and also revenue recognition date I think if we switch this on - which is month and year only) separate. We should have the invoice date (i.e. the date for revenue recognition) and the payment date (date payment received).
• Have the cancelled / refund states of the contribution been considered? Could we end up with some wierd behaviour of completed contributions with payments that are not 100% of the contribution value?

In reality the question here is whether to:

a) See this as an improvement that does 2 or 3 out of the 10 things we want to do, perhaps well enough to put this in
b) Say that this doesn't reach the bar of a core improvement that we should be releasing to 10k sites at this time without any consultation as it doesn't fully solve all the issues and may lead to some inconsistencies.

What I would like is that we have a really clear spec for this - a lot of what Im doing here is inferring from the screens above and the ticket, but I think a set of wireframes that covers all the screens and all the states of those screens is the correct way to attack this.

I've started to do that here:

https://wiki.civicrm.org/confluence/display/CRM/Separating+Contributions+and+Payments

but of course this slows things down and perhaps may increase the scope of works.

So what I would like to see (but understand that this may not be feasible) before this is approved:

1. A solution for the "new contribution screen" that is more robust then just ignoring the fact that we are separating out the payment and contribution. Please see ideas from the wireframes I've made.

2. Some detailed analysis for what the impact on payments section is for each of the contribution status's in the workflow.

Is that feasible?

J

[eileenmcnaughton]

So with regards to the change in scope - this issue is about progressing changes to the edit form, which will allow us to resolve some ongoing bugs / issues around payment (e.g. the 'flapping payment instrument' problem & the 'yo extra financial transactions where did you come from' problem).

There is a larger scope about further changes that might be made in the future and I think your wireframes are a good step towards planning them. I broadly agree with the suggestions you have made, and some of those could be made fairly easily.

I'm going to focus in on this change in my comments here because I making this incremental change is a good code, testing, & UI step forwards. I think if we are going to reject it because it doesn't do everything then we need to be clear about the ways in which it is.

a) making it worse and /or
b) making it harder to reach our longer term objectives.

I have pinged a number of specific people who I feel have good insight into UI starting from July (above) and emailed the dev-money list in July, so I have certainly tried to consult & elicit feedback. This has also been in the PR q in one form or another for most of this year.

The scope of my comments is thus on the 'EDIT contribution' form. I'm going to assume any suggested changes to the 'Add contribution' or 'Add payment' form remain out of scope.

On the edit form there are some specific suggestions

1. Extra 'add payment' button. I agree. At the moment I think it shows the link when there are no payments but not when there are payments (it goes to the existing 'add payment' form & I don't think we should change that). I had thought we should add an additional button or 2 as a follow up pr & where you put it is fine, I had been imagining it either at the bottom of the payment block or next to contribution status, or both.
2. net amount & fee amount - are both already on the 'record payment' form
3. refund payments - currently the record payment form supports adding a negative payment so I think that is fine but the button messaging could be 'add payment/ refund payment' for clarity
4. contribution Status I've left this until last because it is the one that I believe must be resolved before this can be merged. I can't really resolve myself to fully removing because it's totally possible that people will have altered the statuses/ done weird & wonderful things. I agree with you we should change the contribution statuses based on the amount paid - when the 'record payment form' is used it will actually update the status. So as long as we can refresh the page once that is done (would have to ask @colemanw for suggestions) we can update that. So we could make Completed, Pending, unavailable as user selections. However, cancelled, chargeback, refunded are tricky because they are a change at the Contribution level ( a cancellation) and potentially at the payment level. I feel like we need to make that flow a little more intuitive.

The following items / suggestions can stand alone from this change & be discussed separately and happen or not independent of this (although only once this has been merged or abandoned or we will find it hard to manage)

1. Rename total amount to amount due - I don't fully understand that one because that implies the total amount would no longer be visible & only the amount outstanding would? Or that the amount due actually means the total amount but we are now calling it total amount.
2. allowing amount change on edit payment - in general we should be adding additional payments & refunds rather than editing the amount. If we can agree the transactions / concerns for editing the amount we could add it to the edit payment form (we can do that separately whether or not we proceed with this)
3. moving financial type I think that repositioning makes sense - I would suggest doing that through a separate issue as I think it's independent. I note some of your comments are about removing financial type from the contribution & something to do with date fields & I think those things are out of scope.
4. Revenue recognition date - I have no opinion on that - it's a feature I don't use & which is turned off on all sites I have dealt with,but @JoeMurray will have an opinion as to whether it applies to the whole contribution or to the payment. If the question is about the whole concept of revenue_recognition_date it needs to be out of scope.
5. Renaming contribution statuses (paid/unpaid/overpaid) - I'd need to see community feedback on this.

Note I don't believe anyone working on this is customer funded - I would really like it to go in because I think it moves us in the right direction and if we let this go stale again now I think we will be starting from scratch again on this piece of the puzzle.

[JoeMurray]

This smells bad to me. It's pulling in one value for payment instrument and check number when there may be many associated with a contribution. We should eliminate that expectation from the values submitted when editting a contribution.

[eileenmcnaughton]

yep agree - the bao should load them if it needs them. There is a bit of an issue in some of the BAO functions where they are not properly interpretting 'this var is not in $params so we should understand it is not being changed'.

(I'm not going to try to improve this ATM as I think the PR is more seriously blocked at the moment)

[JoeMurray]

I think the contribution status should remain out of scope for this PR. For me, it's something I've wanted for a few years but it was too big to tackle without a good budget and a lot of patience for community interactions. Looking over Eileen's latest comments, I'm not sure that anything more is required of this PR before merging, except to resolve branch conflicts on the .tpl due to delay in merging I presume.

[eileenmcnaughton]

@JoeMurray apart from this being stale I'd kinda given up on it as blocked by @jamienovick

From my point of view the minimum to get it merged would be

1. make sure that contribution status is correctly updated & form refreshed when a payment is added (ignoring user input & setting an appropriate status).
2. fix merge conflices

I've been thinking this could go in an extension as getting it into core is too difficult. We would want to freeze developing various areas of code in core (which would be confusing) since we'd be moving the primary technique (not having a many to one form that pretends to be a one to one form) for fixing code issues relating to partial payments being an outside core thing, and we should enable the new one by default.

Perhaps @colemanw you want to make a call

[eileenmcnaughton]

@monishdeb I got a further comment from @jamienovick saying

"Hi @Eileen sorry I didnt see this - I did leave a last comment to say go ahead if you thought it was ok and whilst it stops short of what I think we need I guess it does move us on the pathway there. If we did want to put it on hold however we could include it as part of a bigger piece of work to really look at splitting payments and contributions. We are now some way towards documenting this. See here: https://civicrm.atlassian.net/wiki/spaces/PPI/pages/2261058/1.+Split+Contribution+and+Payment+fully. It would be great to get your feedback on this as we work thought it."

I think we should revisit this.

I think it would be good to touch this file
templates/CRM/Contribute/Form/PaymentInfoBlock.tpl
first & alter the change slightly so the add payment link it always there if the contribution is not fully paid. We could get that merged fairly quickly.

[JoeMurray]

Actually, I would leave it there even if fully paid or overpaid, as clients report sometimes receiving a second cheque in mail for an invoice, sometimes one that has since been modified.

[eileenmcnaughton]

@JoeMurray I don't know off the top of my head whether the 'Add payment' form copes OK with that or not - not that part is in #11432 which I think we should review & merge before assessing this

[monishdeb]

@eileenmcnaughton it's strange there aren't any merge conflicts after I merged #11432 . Isn't that PR created after pulling out most of the changes from current PR ?

[eileenmcnaughton]

I rebased it over the top - will re-rebase it now

[monishdeb]

@eileenmcnaughton I am getting a notice error on backoffice contribution edit form

I have prepared a patch for it, considering context as an alternative to component:

diff --git a/CRM/Contribute/Form/AbstractEditPayment.php b/CRM/Contribute/Form/AbstractEditPayment.php
index c09e51d..f2ab9c0 100644
--- a/CRM/Contribute/Form/AbstractEditPayment.php
+++ b/CRM/Contribute/Form/AbstractEditPayment.php
@@ -707,9 +707,10 @@ WHERE  contribution_id = {$id}
    *   Block title.
    */
   protected function assignPaymentInfoBlock() {
-    $paymentInfo = CRM_Contribute_BAO_Contribution::getPaymentInfo($this->_id, $this->_component, TRUE);
+    $component = empty($this->_component) ? $this->_context : $this->_component;
+    $paymentInfo = CRM_Contribute_BAO_Contribution::getPaymentInfo($this->_id, $component, TRUE);
     $title = ts('View Payment');
-    if (!empty($this->_component) && $this->_component == 'event') {
+    if (!empty($component) && $component == 'event') {
       $info = CRM_Event_BAO_Participant::participantDetails($this->_id);
       $title .= " - {$info['title']}";
     }
diff --git a/CRM/Contribute/Form/Contribution.php b/CRM/Contribute/Form/Contribution.php
index 70306d2..58237af 100644
--- a/CRM/Contribute/Form/Contribution.php
+++ b/CRM/Contribute/Form/Contribution.php
@@ -227,6 +227,9 @@ class CRM_Contribute_Form_Contribution extends CRM_Contribute_Form_AbstractEditP
 
     $this->assign('action', $this->_action);
 
+    $this->_context = CRM_Utils_Request::retrieve('context', 'String', $this);
+    $this->assign('context', $this->_context);
+
     // Get the contribution id if update
     $this->_id = CRM_Utils_Request::retrieve('id', 'Positive', $this);
     if (!empty($this->_id)) {
@@ -235,9 +238,6 @@ class CRM_Contribute_Form_Contribution extends CRM_Contribute_Form_AbstractEditP
       $this->assign('isUsePaymentBlock', TRUE);
     }
 
-    $this->_context = CRM_Utils_Request::retrieve('context', 'String', $this);
-    $this->assign('context', $this->_context);
-
     $this->_compId = CRM_Utils_Request::retrieve('compId', 'Positive', $this);
 
     $this->_compContext = CRM_Utils_Request::retrieve('compContext', 'String', $this);

[eileenmcnaughton]

@monishdeb I have been thinking about this - on one hand I think it's an important step forwards. I don't think we can properly handle issues like https://issues.civicrm.org/jira/browse/CRM-21756 without resolving the ambiguity between payment & amount due on back office forms and I feel slightly nauseous that we may have spent dozens of hours fixing similar bugs that may not have been required had we bedded this in.

On the other hand - this went cold in Sep last year & I think both of us have been reluctant to put the work in to get ourselves mentally back into the space of this enough to get it finished (I would say there is at least a day's work just to mentally 'uncold' it).

However I also sense that this could be merged with an on/off switch to get us past our nerves a little. ie. unless $isUsePaymentBlock', TRUE is assigned to the template I think the existing one contribution-to-one payment block will appear & if it is the multiple payment block will appear. I'm inclined to leverage this & add a temporary setting is_display_non_legacy_payment_block & put in a call back that sets it to TRUE is isDevelopment. After JMA have deployed it in production for a few cycles we can look at changing more aggressively. The immediate impact of the change will be in developer mode as it will change how we address bugs but eventually we'll need to make it compulsory.

[JoeMurray]

@monishdeb please make this a priority in coming days. Thx!

[JoeMurray]

Ooops, sorry for temporarily closing.

[eileenmcnaughton]

This has been heavily discussed - including on dev list. I followed @agh1 suggestion here https://lab.civicrm.org/dev/financial/issues/3 and emailed dev list to say that this would be merge once the PR is cut, with the option of reverting if people hit issues in the nearly 2 months until release