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

[jitendrapurohit]

…ions

Overview

Online paynow functionality for partially paid contributions.

Before

Partial Paid contribution can only be completed from backend pages.

After

Partial Paid has a pay now link on user dashboard which navigates to a payment page for online completion.

Payment page list all the price field and the balance due amount.

Comments

Added unit test.

---

Gitlab issue https://lab.civicrm.org/dev/financial/issues/20

[civibot]

(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).

[jitendrapurohit]

ping @monishdeb @eileenmcnaughton @magnolia61

[monishdeb]

@jitendrapurohit there is a style warning, can you please fix that?

[jitendrapurohit]

@monishdeb Done 👍

[magnolia61]

I will test now and feedback in a bit

[magnolia61]

Very nice and works mostly as expected, I have just a few suggestions.

1. For a clear user interface and maintainability + because this block became recently available I suggest the use of the Payment Summary block which is also in the contribution summary view (or otherwise add the 'amount paid' somewhere, but using the available block is easier I guess).
   
2. The Contribution information shows a full history of all changes, I believe it should only show items that add to the total amount (would that be the same as QTY > 0? or amount > 0)
   
3. When a users clicks the PayNow link after it has already been paid a confusing screen is shown. But an error message is shown AND the price set of the Contribution page that was configured.
   
   I think the previous behavior (transparent redirect to the homepage) also is confusing. Instead a helpful message should be shown to the end-user. A related solution to this problem has been submitted as a PR in https://lab.civicrm.org/feature-requests/user-interface/issues/1 which dealt with expired checksum links.

I think 1 & 2 would be very easy fixes. I'm not sure if the example error message would be easy to use.
With those suggestions being made, I want to compliment Jitendra for this wonderfull PR.

[eileenmcnaughton]

@jitendrapurohit related test fails?

[magnolia61]

BTW. of my earlier 3 remarks 1 and 2 are easy I think but also 'nice to have' so no problem if they are just documented here. Only #3 is really in need of improvement as both transparent redirect to the homepage as the mix of a message and a price set are both confusing for end users.

[jitendrapurohit]

Thanks for the review and UI improvement suggestion @magnolia61 👍

Re 2/ we don't list the price field on the main page if the user has not opted for it. You screenshot seems to be a result of Change Selection made by the user so that the one which is removed is shown as a disabled row in the price field section. I think it adds more info on the page to let user know of the selections updated by them. Same is done on the view screen at the backend.

Can anyone @eileenmcnaughton @colemanw do a +1 to @magnolia61 suggestion so that I can proceed in implementing 1 and 2 in a separate ticket?

For 3 - PayNow link is mostly clicked by the user from user dashboard page. If an anonymous visits the paynow link - the error is simply not shown to the user as civicrm doesn't display errors on non-civi pages. Hence, a most common public page is chosen here(default invoice payment page URL without ccid or cid) for the redirection so that the user know what was wrong with their page load.

[jitendrapurohit]

@monishdeb @eileenmcnaughton @pradpnayak seems calculating total payment from financial_trxn table should also consider the negative amount value with refund status? We do the same in getPartialPaymentWithType function. Thoughts - if this seems fine to you?

[monishdeb]

Apprently it doesn't seem odd to me consider Refunded status. But I checked the code for its impact and this function get called at two places:

CRM//Core/BAO/FinancialTrxn.php:536:    $paid = CRM_Core_BAO_FinancialTrxn::getTotalPayments($params['contribution_id']);
CRM//Price/BAO/LineItem.php:1176:    $paidAmount = CRM_Core_BAO_FinancialTrxn::getTotalPayments($contributionId);

As per the usage I played with some Event's change-fee scenario and line-item editor to check how it behaves but didn't found any unintended behaivour. So this seems fine to me.

(Although it would be ok to get rid of the status subclause, but let's not tweak this rule in this PR and it might cause regression)

[magnolia61]

@jitendrapurohit About #3: We mostly use the constructed Pay Now link that we send by email of have available in a view(block) for them. So for us there are not really anonymous users, because people click the link with the checksum.I'll try to describe what happens in real-life and do some suggestions in order of preferability:

A users clicks Pay now link from email: sees price set and message (confusing, maybe the price set does not have anything to do with the payment (there is only 1 default contribution page but many payment scenarios, different events, memberships),
Result: the user does not know what to do in this case (see screenshot above at #3). Especially the price set that is shown is confusion because the users does not know what to do with that. Should he/she select something, and what happens when he/she does?

Suggestion #1: Best would be only the message (no confusing price set that should be ignored)
Suggestion #2: Otherwise: redirect to homepage without a message (also bad but less confusing)

Is it really not possible to just show a message screen with:

Error: This contribution has already been handled.

And show that only when the contribution is completed, so allow payments for pending, partial and failed contributions.

[magnolia61]

Suggestion #1: Best would be only the message (no confusing price set that should be ignored)
Suggestion #2: Otherwise: redirect to homepage without a message (also bad but less confusing)

Would it be possible to implement one of my two suggestions to proceed. The solution to show the default contrib page is really too confusing for end users. If my 1st suggestion would require too much work please just revert it to redirect to the homepage in order for this PR to be able to get merged :-)

[jitendrapurohit]

@magnolia61 I've updated the PR to display an error message instead of redirecting to any page. Can you pls give it a final testing?

Thanks :-)

[magnolia61]

@jitendrapurohit Thank you Jitendra. It now shows an error message when logged in, and transparently redirects to the homepage (without a message) when a Pay Now checksum is used. Altough this solution lacks communication to the anonymous checksumlink users, I believe for the time being this is the best solution.

(BTW. Interesting work on a alert message solution for anonymous users has successfully been done at #12724 (yet to be merged). That might be a nice way to at least show the anonymous users the errormessage). For our purposes an 'empty' public civicrm page would have to be available for the alert to show up. For this I will open a separate issue

The only thing I think is still missing now is a column with the Balance in the user dashboard (like the pledge table has, see screenshot) . For Partial paid contributions it now looks pretty scary for a user. For them it seems the whole amount this needs to be paid. (see screenshot). If you would you have the opportunity to add this to the dashboard, then it would make the partial payment user experience complete :-)

[eileenmcnaughton]

We shouldn't be adding in fatals. It's a deprecated function.

[jitendrapurohit]

Modified it to use an exception instead of the above 👍

[magnolia61]

To include balance on the user dashboard contribution block I added the following in
templates/CRM/Contribute/Page/UserDashboard.tpl
ln34: <th>{ts}Balance Due{/ts}</th>
ln56: <td>{$row.balance_due}</td>
Can you agree and would you be ok to change those files or do I need to submit a PR? Thanks, Richard

[jitendrapurohit]

@magnolia61 Yes pls, make a new PR as I'm not sure if it is fine to show balance due as a standard column for all the contribution rows. It only makes sense for the partially paid record and quite predictable for other statuses. Eg empty for completed, equal to total amount for pending, etc.

[JoeMurray]

So I don't think @magnolia61 suggestion 2 is something that should go into this. It's a bit of a preference thing that deserves a separate discussion.

I like the idea of having a balance due field displayed somewhere close to the Pay Now button. I'm of two minds about whether it should be included. On balance, I think it makes sense to include in the same PR.

I don't think the Pay Now should be inside list of payments for a contribution since it will require opening that, and mostly people will not have partial payments, and if they do then they still don't have a need to see the list of payments.

I have not setup a test site with the PR. Could a screenshot from #3 without error be posted here? I just want to ensure that the general layout of a CiviCRM page is being respected. Might be easiest to assess with a default theme from Drupal.

[jitendrapurohit]

Thanks @JoeMurray

Could a screenshot from #3 without error be posted here?

So if a user tries to pay for a contribution which has already completed, an error message is displayed on the screen. Similar to other errors that a user gets on a public page.

If the contribution id is valid, the payment page loads the amount that needs to be paid along with the line items -

I'll add the changes to include the balance column on the user dashboard soon 👍

[JoeMurray]

Thanks, @jitendrapurohit .

The non-error form looks fine.

Regarding error handling, the general pattern is that friendly messages should be displayed for non-programming errors, and ugly program level error feedback is acceptable for bugs. The scenario of a user attempting to pay for a partial amount on fully paid contribution is a bit in the middle: it's possible to get there through inappropriate use of back button or multiple tabs. I suppose it's acceptable to get away with an unfriendly display, though a friendly message display format would be preferred. More appropriate text would be 'This contribution is already fully paid.'

[mattwire]

More appropriate text would be 'This contribution is already fully paid.'

@JoeMurray @jitendrapurohit I agree. Is there any need to get rid of the statusBounce - much more user friendly, I dread when demo-ing CiviCRM and the "Sorry due to an error..." screen appears.

[magnolia61]

I tested out the latest PR with the payment api functionality.
I found several bugs worth mentioning.

scenario 1:
contribution 50 euro. with (frontend) Pay Now checksum link I pay 30.
result: the total sum of the contribution changes from 50 to 30.
desired result: contribution total should remain 50, but a payment of 30 should be recorded

scenario 2:
event price set
a) basic fee (50 euro)
b) optional fee (30 euro)

Payment is complete for basic fee. Event price set is changed (backend) to include the optional fee. There is now 30 euro (the optional fee) still to be paid.
I pay with the checksum Pay Now link.
result: an extra contribution of 30 euro is recorded.
desired result: a payment towards the original contribution is expected.

I will test some more scenarios but i thought I'd let you know this already

[eileenmcnaughton]

@jitendrapurohit can we break any refactorings into small PRs that only do the refactorings & merge those first to get the amount of stuff happening in here down. I'd rather chip away at this a bit - e.g try out getting something like this down eileenmcnaughton@431a590 first with changes in payment.create first & mature the payment.create api as step one

[jitendrapurohit]

Sorry - back from a small break and looking into this comment. @magnolia61 Seems like the usecase replicates for "pending" payments only and works fine for partial payment I tested in my above comment. I've pushed a minor update to this PR and it fixes your cases too.

@eileenmcnaughton Sure. I'll create some PR in batches and lets merge them first.

[magnolia61]

Hello @jitendrapurohit Thank you for your work. I hope to continue to contribute with testing different (real-life) scenarios. I just tested your updated PR and it indeed prevents a new contribution to be created (yeah!). Unfortunately the Pay Now screen now shows the full amount instead of the Balance

SCENARIO 2:
INITIAL ACTION: Registered for event, basic fee of 50 euro selected and paid
INITIAL RESULT: Contribution recorded, status: Completed
BACKEND ACTION: Backend registration edited to include the options.
BACKEND RESULT:Contribution Status changes to: Partially Paid. Total Fee: 80 euro. Balance: 30 euro
USER ACTION: User uses Pay Now Checksum link to pay for remaining 30 euro
ACTUAL RESULT: User sees payment screen for the full 80 euro (not just the balance)

CONTRIBUTION:

PAY NOW SCREEN

PS. SCENARIO 1 remains the same. The total contribution fee changes to the chosen amount paid in the frontend Pay Now form.

Glad to help testing!

[jitendrapurohit]

@magnolia61 This does not seem to be true on my testing. See the below screenshot where a contribution is created for $50(completed) -> selections changed to include $30 (status changes to partially paid) -> Navigating to the pay now link gives the below screen. Do you see any points missing here?

Also scenario 1 works fine on my end.

Side Note: I recall you had problems in fetching the correct patch before, so just need to confirm if you have the correct updated patch applied on your review setup, i.e, https://patch-diff.githubusercontent.com/raw/civicrm/civicrm-core/pull/12319.diff?

[magnolia61]

Maybe @jitendrapurohit I will double-doublecheck. What I did was a

wget https://github.com/civicrm/civicrm-core/pull/12319.patch
patch -p1 < 12319.patch

Will let you know my results in a few moments

[monishdeb]

@magnolia61 sometimes if there's a patch file, present with the same name then it creates 12319.patch.1 without raising a warning on wget. Please ensure that there is no 12319.patch present in Civi before executing wget.

[jitendrapurohit]

I've raised a PR #13330 which only contains fix for the backend forms and uses payment create API for the payment recorded via admin interface. It moves the code from API -> BAO level and also focuses on making the emailReceipt() as a common function which can also be accessed by front end contribution pages to send receipts.

[magnolia61]

Hello Jitendra, I am aware of your new backend only PR (I have tested it and commented) and also Eileens recent work.
Still I have some feedback for testing this PR which is mostly working ok for us, and I think my experience might help development.

When I use this PR 12319 on civicrm 5.9 or master 'normal' contributions work fine. It is the partial contributions that when correctly processed by the payment processor returns to the payment screen with the message: "Your payment was not successful. Please try again".

[magnolia61]

I am very happy to see all that is going on with the payment.create function which has it's origin in this mother-issue on front end partial payments. What I am missing is a gltlab issue to track all the sub-PRs for a bit of overview. I am collecting all relevant PR's in this place. @eileenmcnaughton : is there already a gltlab issue for this that I oversaw? Are all steps to create the payment.create api now merged. If so: bless you for your hard work on that. Would this mean a first approach can be made to have the front end payment screen also use the new payment.create api?

Originating issue:
#12319 Extend pay now functionality for partially paid contribution

Steps to first create a proper Payment api
#13330 [WIP] Modify backend Record Payment forms to use payment create API
#13370 extract internals of Payment.create into function on BAO class
#13647 Modify backend Record Payment forms to use payment create API (not merged yet)
#13689 Call Payment.create from payment.cancel
#13690 Clean up Payment.create function
#13756 separate financial handling & component transitioning in Payment.create

Issues on payment sendconfirmation:
#13561 Add new Payment.sendconfirmation api
#13609 Payment.sendconfirmation api - add further tpl variables
#13610 Payment.sendconfirmation api - add further tpl variables
#13649 Switch additional payment form to use Payment.sendconfirmation api (merged)

[eileenmcnaughton]

@magnolia61 thanks for bringing them all together! Yes, we are just trying to clean up the code first so we can do this properly - there is a long legacy of difficult code here :-( Over probably the past 2 years I think Monish, Pradeep & I have built up a pretty good amount of tests on the code which means we are in a position to tidy it up now - we're not quite there with the payment.create fixes yet though - a few more rounds left

[eileenmcnaughton]

I'm going to close this as it is stale & has become more of an issue tracker for 'cleanup payment code to the point where we can fix this and related things reliably'

Currently the open work on this is this PR #13689

& once that is merged I'll look at finishing off the next parts of the payment create consolidation

[magnolia61]

@jitendrapurohit Would it be possible to rebase this patch to work with 5.14? Or would the whole frontend partial payment patch need to be rewritten using all latest payment create api functionality?

[eileenmcnaughton]

@magnolia61 the changes needed will have changed quite a bit by now. This patch here is the next one unmerged to take us forward in this process #14408

Do we need to load a contribution page for pay now or could we just load the add payment form (as in the backend) which would be easier & less risky

[magnolia61]

Do we need to load a contribution page for pay now or could we just load the add payment form (as in the backend) which would be easier & less risky

I think the lean and mean add payment form would be very much ok for this.
There are different possibilities to use it:
a) The enduser could be able to enter an amount to pay
b) The amount could be locked readonly to be the full amount
c) The full amount would be entered, but the user would be able to change it

I think I would prefer option c) Probably changing from a full contribution page to the add payment form makes all of this much less complicated :-)

(But would the add payment form enable 'online' processing of a payment processor?)

[magnolia61]

I have opened up a new pr that enables the Pay Now button and payment via checksum link for partially paid contributions. Still a work in progress as there are a few minor issues. But the main nice preliminary conclusion is that all of the hard work Eileen put in crafting the payment create api makes it kinda work out of the box. Please take a look at #15696 and if possible help me move this forward as it would be great to be able to include it in 5.20 which includes a lot of financial cleanup and improvements ;-)

[magnolia61]

I opened an issue to advocate for an improvement to proceed with using Record Payment form for the Pay Now functionality
https://lab.civicrm.org/dev/core/-/issues/2095 I cannot code this myself (no skills) but hope someone working on related stuff can drop by and make the Pay Now use this function properly. I am available to help test this stuff anytime :-)