Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dev/core#2034 Fix paypal standard cancel url and action links in dashboard #18787

Closed
wants to merge 8 commits into from
Closed

dev/core#2034 Fix paypal standard cancel url and action links in dashboard #18787

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
eb1139e
Add missing "supports" methods to PayPal processor
webmaster-cses-org-uk Oct 16, 2020
22fafab
Add further missing "supports" methods to PayPal
webmaster-cses-org-uk Oct 16, 2020
faacfef
Correct logic in "supports" method
webmaster-cses-org-uk Oct 16, 2020
2478d17
Fix recurLinks to exclude inappropriate operations
webmaster-cses-org-uk Oct 16, 2020
3a3732a
Remove "view" link if no permissions
webmaster-cses-org-uk Oct 16, 2020
ede8ae1
Remove inappropriate link to instalments
webmaster-cses-org-uk Oct 16, 2020
400f826
Fixed formatting errors in contributions dashboard
webmaster-cses-org-uk Oct 16, 2020
2847b55
Removed trailing whitespace
webmaster-cses-org-uk Oct 18, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions CRM/Contribute/Page/Tab.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,15 +73,18 @@ public static function recurLinks($recurID = FALSE, $context = 'contribution') {
if ($paymentProcessorObj) {
if ($paymentProcessorObj->supports('cancelRecurring')) {
unset($links[CRM_Core_Action::DISABLE]['extra'], $links[CRM_Core_Action::DISABLE]['ref']);
$links[CRM_Core_Action::DISABLE]['url'] = "civicrm/contribute/unsubscribe";
$links[CRM_Core_Action::DISABLE]['url'] = $paymentProcessorObj->subscriptionURL(NULL, NULL, 'cancel');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mattwire @webmaster-cses-org-uk this link would be available to backoffice users and to people viewing their own dashboard.

I think the desired link & behaviour might be different - ie the front end user should be taken off the paypal whereas the backoffice user can only do an administrative cancel (which shouldn't be available to the front end user).

I have a feeling the code might be simpler if we didn't have one 'recurLinks' function try to deal with both scenarios

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree splitting it is probably better. It would be cleaner and easier to maintain: relying on the free-text $context argument leaves a bit too much to chance I think.

One other thing to consider: among the changes I've proposed, supports("cancelRecurring") will return FALSE for PayPal_Standard. This is because the method is currently defined (in the comments) as whether the processor supports cancellation "in code", which of course PayPal_Standard doesn't. This would mean that no link is present for back-office or front-end users.

I think we may therefore also need to split the "cancelRecurring" operation into two: back-office and front-end, and similarly have two modes for subscriptionURL (back-end and front-end cancel).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes - I think so - maybe we should supportUserCancelRecurring (or supportDonorCancelRecurring)

Copy link
Contributor Author

@webmaster-cses-org-uk webmaster-cses-org-uk Oct 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd second that. Either of those names would be fine. Then for subscriptionURL perhaps we could have a corresponding 'userCancel' (or 'donorCancel')?

$links[CRM_Core_Action::DISABLE]['qs'] = "reset=1&crid=%%crid%%&cid=%%cid%%&context={$context}";
}
else {
unset($links[CRM_Core_Action::DISABLE]);
}

if ($paymentProcessorObj->supports('UpdateSubscriptionBillingInfo')) {
$links[CRM_Core_Action::RENEW] = [
'name' => ts('Change Billing Details'),
'title' => ts('Change Billing Details'),
'url' => 'civicrm/contribute/updatebilling',
'url' => $paymentProcessorObj->subscriptionURL(NULL, NULL, 'billing'),
'qs' => "reset=1&crid=%%crid%%&cid=%%cid%%&context={$context}",
];
}
Expand All @@ -98,6 +101,10 @@ public static function recurLinks($recurID = FALSE, $context = 'contribution') {
unset($links[CRM_Core_Action::DISABLE]);
unset($links[CRM_Core_Action::UPDATE]);
}

if (!CRM_Core_Permission::check('view my contact') && $context === 'dashboard') {
unset($links[CRM_Core_Action::VIEW]);
}
}

return $links;
Expand Down
13 changes: 10 additions & 3 deletions CRM/Contribute/Page/UserDashboard.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ public function listContribution() {
// This is required for tpl logic. We should move away from hard-code this to adding an array of actions to the row
// which the tpl can iterate through - this should allow us to cope with competing attempts to add new buttons
// and allow extensions to assign new ones through the pageRun hook
// ALSO: Should 'id' be set to the contribution page of the original transaction?
if ('Pending' === CRM_Core_PseudoConstant::getName('CRM_Contribute_BAO_Contribution', 'contribution_status_id', $row['contribution_status_id'])) {
$row['buttons']['pay'] = [
'class' => 'button',
Expand Down Expand Up @@ -124,11 +125,17 @@ public function listContribution() {
}
if (is_array($recurIDs) && !empty($recurIDs)) {
$getCount = CRM_Contribute_BAO_ContributionRecur::getCount($recurIDs);
$isLinked = CRM_Core_Permission::check('access CiviContribute');
foreach ($getCount as $key => $val) {
$recurRow[$key]['completed'] = $val;
$recurRow[$key]['link'] = CRM_Utils_System::url('civicrm/contribute/search',
"reset=1&force=1&recur=$key"
);
if ($isLinked) {
$recurRow[$key]['link'] = CRM_Utils_System::url('civicrm/contribute/search',
"reset=1&force=1&recur=$key"
);
}
else {
$recurRow[$key]['link'] = '';
}
}
}

Expand Down
46 changes: 46 additions & 0 deletions CRM/Core/Payment/PayPalImpl.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,22 @@ protected function supportsBackOffice() {
return FALSE;
}

/**
* Does this processor support cancelling recurring contributions through code.
*
* If the processor returns true it must be possible to take action from within CiviCRM
* that will result in no further payments being processed. In the case of token processors (e.g
* IATS, eWay) updating the contribution_recur table is probably sufficient.
*
* @return bool
*/
protected function supportsCancelRecurring() {
if ($this->isPayPalType($this::PAYPAL_STANDARD)) {
return FALSE;
}
return parent::supportsCancelRecurring();
}

/**
* Does this processor support pre-approval.
*
Expand All @@ -125,6 +141,36 @@ protected function supportsPreApproval() {
return FALSE;
}

/**
* Does this processor support updating billing info for recurring contributions through code.
*
* If the processor returns true then it must be possible to update billing info from within CiviCRM
* that will be updated at the payment processor.
*
* @return bool
*/
protected function supportsUpdateSubscriptionBillingInfo() {
if (!$this->isPayPalType($this::PAYPAL_PRO)) {
return FALSE;
}
return parent::supportsUpdateSubscriptionBillingInfo();
}

/**
* Does this processor support changing the amount for recurring contributions through code.
*
* If the processor returns true then it must be possible to update the amount from within CiviCRM
* that will be updated at the payment processor.
*
* @return bool
*/
protected function supportsChangeSubscriptionAmount() {
if (!$this->isPayPalType($this::PAYPAL_PRO)) {
return FALSE;
}
return parent::supportsChangeSubscriptionAmount();
}

/**
* Opportunity for the payment processor to override the entire form build.
*
Expand Down
16 changes: 7 additions & 9 deletions templates/CRM/Contribute/Page/UserDashboard.tpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
<th></th>
{/if}
{foreach from=$row.buttons item=button}
{* @todo Fix this: doesn't work (doesn't add extra header cells) because $row does not exist in this context. See also comments below / in UserDashboard.php re changing to actions array *}
<th></th>
{/foreach}
</tr>
Expand Down Expand Up @@ -116,22 +117,19 @@
<div><label>{ts}Recurring Contribution(s){/ts}</label></div>
<table class="selector">
<tr class="columnheader">
<th>{ts}Terms:{/ts}</th>
<th>{ts}Status{/ts}</th>
<th>{ts}Amount{/ts}</th>
<th>{ts}Interval{/ts}</th>
<th>{ts}Installments{/ts}</th>
<th>{ts}Status{/ts}</th>
<th>{ts}Created{/ts}</th>
<th></th>
</tr>
{foreach from=$recurRows item=row key=id}
<tr class="{cycle values="odd-row,even-row"}">
<td><label>{$recurRows.$id.amount|crmMoney}</label>
every {$recurRows.$id.frequency_interval} {$recurRows.$id.frequency_unit}
for {$recurRows.$id.installments} installments
</td>
<td>{$recurRows.$id.amount|crmMoney}</td>
<td>{$recurRows.$id.frequency_interval} {$recurRows.$id.frequency_unit}</td>
<td>{if $recurRows.$id.link}<a href="{$recurRows.$id.link}">{/if}{if $recurRows.$id.completed}{$recurRows.$id.completed}{else}0{/if} / {if $recurRows.$id.installments}{$recurRows.$id.installments}{else}{ts}(Open-ended){/ts}{/if}{if $recurRows.$id.link}</a>{/if}</td>
<td>{$recurRows.$id.recur_status}</td>
<td>{if $recurRows.$id.completed}<a href="{$recurRows.$id.link}">{$recurRows.$id.completed}
/{$recurRows.$id.installments}</a>
{else}0/{$recurRows.$id.installments} {/if}</td>
<td>{$recurRows.$id.create_date|crmDate}</td>
<td>{$recurRows.$id.action|replace:'xx':$recurRows.id}</td>
</tr>
Expand Down