dev/core#521: PCP Send notification after successful contribution payment #19096

nishant-bhorodia · 2020-12-03T13:22:55Z

Overview

Owner notification email sending before payment. When you are the owner of a fundraising page, you are supposed to receive a notification email after a successful donation has been made to your fundraising page. However when you are using a payment processor that navigates you out from civicrm to its own payment page (like sagepay, or paypal standard) the owner notification is sent before the payment has been made.
Core issue: https://lab.civicrm.org/dev/core/-/issues/521

Before

PCP Owner received notification for contribution before the payment was done.

After

PCP Owner receives notification only after successful contribution payment is done.

Technical Details

CRM_Contribute_Form_Contribution_Confirm::pcpNotifyOwner function will be called at two places now.

in protected static function processPCP($pcp, $contribution) for backend pcp contributions.
in public static function completeOrder for contributions made by using PCP page.

civibot · 2020-12-03T13:22:58Z

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

yashodha · 2020-12-04T03:53:43Z

@nishant-bhorodia

Are the failing tests related?

eileenmcnaughton · 2020-12-04T05:24:12Z

CRM/Contribute/BAO/Contribution.php

@@ -4340,6 +4340,16 @@ public static function completeOrder($input, $ids, $contribution, $isPostPayment
    $contribution->contribution_status_id = $contributionParams['contribution_status_id'];

    CRM_Core_Error::debug_log_message('Contribution record updated successfully');
+    $contributionSoft = civicrm_api3('ContributionSoft', 'get', [


Since the v4 api for ContributionSoft is about to be merged let's use that

#19083

Thanks @eileenmcnaughton
I will update the code to use api4 then :)

eileenmcnaughton · 2020-12-04T05:25:27Z

CRM/Contribute/BAO/Contribution.php

+    $contributionSoft = (object) $contributionSoft['values'][0];
+    //Send notification to owner for PCP
+    if ($contributionSoft->pcp_id) {
+      CRM_Contribute_Form_Contribution_Confirm::pcpNotifyOwner($contribution, $contributionSoft);


This function is only called from one place in the code so we can easily change it to receive an array rather than cast to an object

eileenmcnaughton · 2020-12-04T05:29:00Z

Test fails are enotices as $contributionSoft['values'][0] may not be set - but I think it is better to switch to apiv4 here anyway as we are trying to use apiv4 on new code

eileenmcnaughton · 2020-12-04T05:47:26Z

Also - I should say - I think this is the right thing in terms of moving the code to a more appropriate location. I am trying to find out if there is any test cover in #19113

nishant-bhorodia · 2020-12-04T10:12:44Z

Hey @yashodha @eileenmcnaughton
Thanks for reviewing, I think in terms of changes, we want to do the following:

Use API4 for contributioSoft
Add a check to see if the value exists
Change signature for pcpNotifyOwner function to use array instead of object

Please let me know if we need any other changes?
Thanks!!

I wound up adding PcpBlock v4 api to write the test In support of civicrm#19096

eileenmcnaughton · 2020-12-04T10:48:14Z

@nishant-bhorodia that sounds right - might need to pull #19083 in or wait for it to be merged first

I added a test here #19117

nishant-bhorodia · 2020-12-04T10:52:39Z

Many Thanks @eileenmcnaughton :)

eileenmcnaughton · 2020-12-04T20:41:31Z

the soft credit api is merged now

I wound up adding PcpBlock v4 api to write the test In support of civicrm#19096

nishant-bhorodia · 2020-12-15T12:16:19Z

Thanks @eileenmcnaughton 😄
Updated the code to use civicrm api 4 instead.
thanks!

eileenmcnaughton · 2020-12-15T19:12:10Z

thanks @nishant-bhorodia - still test fails :-(

nishant-bhorodia · 2020-12-17T08:53:36Z

oh :( checking!

This is a partial of civicrm#19096 which requires contributionSoft to be an array & just simplifies that commit (which is still failing tests

eileenmcnaughton · 2020-12-30T19:53:11Z

@nishant-bhorodia ideally completeOrder should be hit in both flows. The problem I see in create is that an unrelated edit could trigger it too

nishant-bhorodia · 2021-03-24T12:19:21Z

Hello @eileenmcnaughton
Hope you are well.
Sorry for delay in getting back to this, got side tracked!!
I have made few changes to this please can you have a look when you get a chance?
thanks!!

nishant-bhorodia · 2021-03-28T12:44:06Z

gentle reminder @eileenmcnaughton :)
Many thanks!!

eileenmcnaughton · 2021-04-05T07:33:57Z

test this please

eileenmcnaughton · 2021-04-05T07:48:17Z

CRM/Contribute/BAO/ContributionSoft.php

-      //Send notification to owner for PCP
-      if ($contributionSoft->pcp_id && empty($pcpId)) {
+      // Send notification to owner for PCP.
+      if ($contributionSoft->pcp_id && empty($pcpId) && empty($contribution->contribution_page_id)) {


@nishant-bhorodia I feel like this might be the wrong extra criteria to add. Does it work if instead we change the contribution status? (if it's still pending we should not send)

Hello @eileenmcnaughton
thanks for reviewing!

I have added another condition to check if the status is completed.
The two conditions I added does the following:

Check if the contribution is made without contribution page i.e., from the admin area.

Check if the status is completed.

Thanks!

Hello @eileenmcnaughton ,
Hope you are well!!
Please can I ask if the change looks fine?
thanks!!

eileenmcnaughton · 2021-04-13T06:10:58Z

There is a test failure

CRM_Core_Payment_AuthorizeNetIPNTest::testIPNPaymentMembershipRecurSuccessNoLeakage
Undefined variable: contribution

/home/jenkins/bknix-dfl/build/core-19096-8dde/web/sites/all/modules/civicrm/CRM/Contribute/BAO/Contribution.php:4290
/home/jenkins/bknix-dfl/build/core-19096-8dde/web/sites/all/modules/civicrm/CRM/Core/Payment/AuthorizeNetIPN.php:177
/home/jenkins/bknix-dfl/build/core-19096-

ahed-compucorp · 2021-06-01T09:17:04Z

Updated the PR to pass the failing tests which includes :

The result of calling civicrm_api4 is an instance of ArrayObject which evaluates to true when it is casted to a boolean or false when it is used with empty() construct . Instead I checked against the first element $result->first() which returns null for when the civicrm_api4 returns no items.
The first parameter for pcpNotifyOwner($contribution, $contributionSoft) must be contribution object.

     $transaction->commit();
     \Civi::log()->info("Contribution {$contributionParams['id']} updated successfully");
-    $contributionSoft = civicrm_api4('ContributionSoft', 'get', [
+    $result = civicrm_api4('ContributionSoft', 'get', [
...
+    $contributionSoft = $result->first();

-    if (!empty($contributionSoft)) {
+    if ($contributionSoft && $contributionSoft['pcp_id']) {
       //Send notification to owner for PCP
-      if ($contributionSoft->column('pcp_id')) {
-        CRM_Contribute_Form_Contribution_Confirm::pcpNotifyOwner($contribution, $contributionSoft[0]);
-      }
+      $contributionDetails = $contributionResult['values'][$contributionResult['id']];
+      $contribution = new CRM_Contribute_BAO_Contribution();
+      $contribution->copyValues($contributionDetails);
+      CRM_Contribute_Form_Contribution_Confirm::pcpNotifyOwner($contribution, $contributionSoft);
     }

This is cleanup towards civicrm#19096 in that it gets the function out of the form layer and also makes it so it does not require a BAO. This does add one extra db lookup in some cases but I think it's pretty low volume and will mean this is not required https://github.com/civicrm/civicrm-core/pull/19096/files#diff-4c9d0b1abe07057a4eea2b47bc627eecb95face8ed8d86c1c005312a52cca811R4271-R4273

This is a reviewer's cut of civicrm#19096 the only substantive difference is the check around the contribution_status_id in this line https://github.com/civicrm/civicrm-core/pull/19096/files#diff-9f33a2073e9f844dbb842e0737b7df4bce13db31eabe13b9bdd10e1035dbf6d3R612 has had the empty(->contribution_page_id) bit removed - I can 't think why that would be added in & I think it was discussed out but not removed

This is cleanup towards civicrm#19096 in that it gets the function out of the form layer and also makes it so it does not require a BAO. This does add one extra db lookup in some cases but I think it's pretty low volume and will mean this is not required https://github.com/civicrm/civicrm-core/pull/19096/files#diff-4c9d0b1abe07057a4eea2b47bc627eecb95face8ed8d86c1c005312a52cca811R4271-R4273

This is a reviewer's cut of civicrm#19096 the only substantive difference is the check around the contribution_status_id in this line https://github.com/civicrm/civicrm-core/pull/19096/files#diff-9f33a2073e9f844dbb842e0737b7df4bce13db31eabe13b9bdd10e1035dbf6d3R612 has had the empty(->contribution_page_id) bit removed - I can 't think why that would be added in & I think it was discussed out but not removed

This is cleanup towards civicrm#19096 in that it gets the function out of the form layer and also makes it so it does not require a BAO. This does add one extra db lookup in some cases but I think it's pretty low volume and will mean this is not required https://github.com/civicrm/civicrm-core/pull/19096/files#diff-4c9d0b1abe07057a4eea2b47bc627eecb95face8ed8d86c1c005312a52cca811R4271-R4273

This is a reviewer's cut of civicrm#19096 the only substantive difference is the check around the contribution_status_id in this line https://github.com/civicrm/civicrm-core/pull/19096/files#diff-9f33a2073e9f844dbb842e0737b7df4bce13db31eabe13b9bdd10e1035dbf6d3R612 has had the empty(->contribution_page_id) bit removed - I can 't think why that would be added in & I think it was discussed out but not removed

demeritcowboy · 2021-08-22T12:21:45Z

@nishant-bhorodia @ahed-compucorp This now has file conflicts.

eileenmcnaughton · 2021-08-28T02:25:32Z

I'm pretty sure this has been otherwise fixed

ahed-compucorp · 2021-09-20T15:12:35Z

Yes, you have created pull request #20523 and merged it.

eileenmcnaughton · 2021-09-20T19:43:40Z

yay

civibot bot added the master label Dec 3, 2020

eileenmcnaughton reviewed Dec 4, 2020

View reviewed changes

eileenmcnaughton added a commit to eileenmcnaughton/civicrm-core that referenced this pull request Dec 4, 2020

Add test to check pcp notification is sent

265de86

I wound up adding PcpBlock v4 api to write the test In support of civicrm#19096

eileenmcnaughton added a commit to eileenmcnaughton/civicrm-core that referenced this pull request Dec 4, 2020

Add test to check pcp notification is sent

03983c2

I wound up adding PcpBlock v4 api to write the test In support of civicrm#19096

eileenmcnaughton mentioned this pull request Dec 4, 2020

Add test to check pcp notification is sent #19117

Merged

eileenmcnaughton added a commit to eileenmcnaughton/civicrm-core that referenced this pull request Dec 5, 2020

Add test to check pcp notification is sent

da508f6

I wound up adding PcpBlock v4 api to write the test In support of civicrm#19096

eileenmcnaughton added a commit to eileenmcnaughton/civicrm-core that referenced this pull request Dec 5, 2020

Add test to check pcp notification is sent

f6992e7

I wound up adding PcpBlock v4 api to write the test In support of civicrm#19096

nishant-bhorodia force-pushed the Issue#521-send-owner-notification-after-successful-payment branch 5 times, most recently from fbba50e to d185010 Compare December 15, 2020 12:09

eileenmcnaughton mentioned this pull request Dec 23, 2020

Convert contributionSoft to an array #19259

Merged

nishant-bhorodia force-pushed the Issue#521-send-owner-notification-after-successful-payment branch from 923d84b to ab5f980 Compare December 28, 2020 10:37

mattwire changed the title ~~Issue#521: Send notification after successful contribution payment~~ dev/core#521: Send notification after successful contribution payment Dec 28, 2020

nishant-bhorodia force-pushed the Issue#521-send-owner-notification-after-successful-payment branch 2 times, most recently from da706e9 to ea8190f Compare December 30, 2020 10:27

nishant-bhorodia force-pushed the Issue#521-send-owner-notification-after-successful-payment branch 2 times, most recently from b6038fc to 731c3be Compare March 24, 2021 10:45

eileenmcnaughton reviewed Apr 5, 2021

View reviewed changes

nishant-bhorodia force-pushed the Issue#521-send-owner-notification-after-successful-payment branch from 731c3be to d5bd28f Compare April 7, 2021 12:29

ahed-compucorp force-pushed the Issue#521-send-owner-notification-after-successful-payment branch from d5bd28f to 09e3798 Compare June 1, 2021 09:05

Issue#521: Send notification after successful contribution payment

9fb8ac2

ahed-compucorp force-pushed the Issue#521-send-owner-notification-after-successful-payment branch from 09e3798 to 9fb8ac2 Compare June 1, 2021 09:06

mattwire changed the title ~~dev/core#521: Send notification after successful contribution payment~~ dev/core#521: PCP Send notification after successful contribution payment Jun 1, 2021

eileenmcnaughton mentioned this pull request Jun 7, 2021

[REF] Move pcpNotifyOwner to ContributionSoft #20522

Merged

eileenmcnaughton mentioned this pull request Jun 7, 2021

Send pcp notification only when the contribution is completed #20523

Merged

ahed-compucorp mentioned this pull request Jun 28, 2021

MC-204: Send pcp notification when contribution is completed compucorp/civicrm-core#60

Merged

eileenmcnaughton closed this Aug 28, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dev/core#521: PCP Send notification after successful contribution payment #19096

dev/core#521: PCP Send notification after successful contribution payment #19096

nishant-bhorodia commented Dec 3, 2020 •

edited

Loading

civibot bot commented Dec 3, 2020

yashodha commented Dec 4, 2020

eileenmcnaughton Dec 4, 2020

nishant-bhorodia Dec 4, 2020

eileenmcnaughton Dec 4, 2020

eileenmcnaughton commented Dec 4, 2020

eileenmcnaughton commented Dec 4, 2020

nishant-bhorodia commented Dec 4, 2020 •

edited

Loading

eileenmcnaughton commented Dec 4, 2020

nishant-bhorodia commented Dec 4, 2020

eileenmcnaughton commented Dec 4, 2020

nishant-bhorodia commented Dec 15, 2020

eileenmcnaughton commented Dec 15, 2020

nishant-bhorodia commented Dec 17, 2020

eileenmcnaughton commented Dec 30, 2020

nishant-bhorodia commented Mar 24, 2021

nishant-bhorodia commented Mar 28, 2021

eileenmcnaughton commented Apr 5, 2021

eileenmcnaughton Apr 5, 2021

nishant-bhorodia Apr 7, 2021 •

edited

Loading

nishant-bhorodia Apr 12, 2021

eileenmcnaughton commented Apr 13, 2021

ahed-compucorp commented Jun 1, 2021 •

edited

Loading

demeritcowboy commented Aug 22, 2021

eileenmcnaughton commented Aug 28, 2021

ahed-compucorp commented Sep 20, 2021

eileenmcnaughton commented Sep 20, 2021

dev/core#521: PCP Send notification after successful contribution payment #19096

dev/core#521: PCP Send notification after successful contribution payment #19096

Conversation

nishant-bhorodia commented Dec 3, 2020 • edited Loading

Overview

Before

After

Technical Details

civibot bot commented Dec 3, 2020

yashodha commented Dec 4, 2020

eileenmcnaughton Dec 4, 2020

Choose a reason for hiding this comment

nishant-bhorodia Dec 4, 2020

Choose a reason for hiding this comment

eileenmcnaughton Dec 4, 2020

Choose a reason for hiding this comment

eileenmcnaughton commented Dec 4, 2020

eileenmcnaughton commented Dec 4, 2020

nishant-bhorodia commented Dec 4, 2020 • edited Loading

eileenmcnaughton commented Dec 4, 2020

nishant-bhorodia commented Dec 4, 2020

eileenmcnaughton commented Dec 4, 2020

nishant-bhorodia commented Dec 15, 2020

eileenmcnaughton commented Dec 15, 2020

nishant-bhorodia commented Dec 17, 2020

eileenmcnaughton commented Dec 30, 2020

nishant-bhorodia commented Mar 24, 2021

nishant-bhorodia commented Mar 28, 2021

eileenmcnaughton commented Apr 5, 2021

eileenmcnaughton Apr 5, 2021

Choose a reason for hiding this comment

nishant-bhorodia Apr 7, 2021 • edited Loading

Choose a reason for hiding this comment

nishant-bhorodia Apr 12, 2021

Choose a reason for hiding this comment

eileenmcnaughton commented Apr 13, 2021

ahed-compucorp commented Jun 1, 2021 • edited Loading

demeritcowboy commented Aug 22, 2021

eileenmcnaughton commented Aug 28, 2021

ahed-compucorp commented Sep 20, 2021

eileenmcnaughton commented Sep 20, 2021

nishant-bhorodia commented Dec 3, 2020 •

edited

Loading

nishant-bhorodia commented Dec 4, 2020 •

edited

Loading

nishant-bhorodia Apr 7, 2021 •

edited

Loading

ahed-compucorp commented Jun 1, 2021 •

edited

Loading