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.

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

[WIP] Fix subscription urls #17445

Closed
wants to merge 5 commits into from

Conversation

Edzelopez
Copy link
Contributor

Overview

This is an initial attempt to fix the errors when cancelling and updating billing details for a recurring contribution using self serve links. #127

Before

  1. Trying to cancel a recurring payment using the self serve link results in an error:
    InvalidArgumentException: "processorID field has max length of 255"

#0 /xxx/civicrm/CRM/Contribute/Form/CancelSubscription.php(217): Civi\Payment\PropertyBag->setRecurProcessorID(NULL)

This was due to the absence of the processor_id in the civicrm_contribution_recur table.

  1. Trying to update billing details, gives the following error:
    Exception: "This system is unable to perform self-service updates to credit cards. Please contact the administrator of this site."

#0 /xxx/civicrm/CRM/Contribute/Form/UpdateBilling.php(207): CRM_Core_Payment_iATSService->updateSubscriptionBillingInfo(NULL, (Array:21))

This was due to the absence of the crid(recur_id) param in the URL.

After

The links now function as expected, allowing one to cancel or update the billing details for a recurring contribution.

Comments

The fix around passing the extra recur_id param may likely need to be expanded to handle the various other calls to this function from different instances. Hence, marked this as WIP, to open discussions for the same.

@civibot civibot bot added the master label Jun 1, 2020
@civibot
Copy link

civibot bot commented Jun 1, 2020

(Standard links)

@Edzelopez Edzelopez marked this pull request as draft June 1, 2020 09:58
@Edzelopez Edzelopez marked this pull request as ready for review June 1, 2020 10:01
@mattwire
Copy link
Contributor

mattwire commented Jun 1, 2020

I would prefer to see these moved to a token implementation rather than being fixed via the smarty route. We've made some good progress on tokens recently.

I'd suggest:

  1. If we can get Standardise what we pass to tokenProcessor so we don't have to add specific handling in each class for actionSchedule #16983 reviewed and merged then it becomes simpler to implement a contributionRecur tokenProcessor class.
  2. You can more or less copy CRM_Activity_Tokens to create CRM_Contribute_Recurtokens (or similar).
  3. Trigger tokenProcessor when sending the message template so we parse contributionrecur tokens. (This also gives us a path to deprecate the older token methods later). I can help with this part.
  4. Pass the recur ID through with message template params so that it's picked up by tokenProcessor.

@mattwire
Copy link
Contributor

mattwire commented Jun 1, 2020

Additional benefit would be that we'd get recurring contribution tokens that we don't currently have.

@JoeMurray
Copy link
Contributor

JoeMurray commented Jun 1, 2020

It seems like you're asking for a fair bit of work here, @mattwire . @seamuslee001 is going to take a look too and provide feedback. We've got a limited budget and even more limited timeframe for completing this. Which isn't to say it wouldn't be nice to have all the stuff you're suggesting.

@mattwire
Copy link
Contributor

mattwire commented Jun 1, 2020

@JoeMurray I know the feeling :-) In that case let's fix the bug but see if we can do anything to make future cleanup easier. Just improving the comments on the params to subscriptionURL would certainly help.

@@ -1088,7 +1088,7 @@ public static function processRecurringContribution(&$form, &$params, $contactID
}
$recurParams['invoice_id'] = $params['invoiceID'] ?? NULL;
$recurParams['contribution_status_id'] = CRM_Core_PseudoConstant::getKey('CRM_Contribute_BAO_Contribution', 'contribution_status_id', 'Pending');
$recurParams['payment_processor_id'] = $params['payment_processor_id'] ?? NULL;
$recurParams['payment_processor_id'] = $recurParams['processor_id'] = $params['payment_processor_id'] ?? NULL;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not 100% sure this is correct,

The processor_id is meant to be the unique token or similar that is used when charging the payment. The payment_processor_id = the id of the PaymentProcessor that is used from civicrm_payment_processor table

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. I figured the processor_id == payment_processor_id, which is not true.

@eileenmcnaughton
Copy link
Contributor

I'm pretty sure this was otherwise fixed as a regression a while back

"#0 /xxx/civicrm/CRM/Contribute/Form/CancelSubscription.php(217): Civi\Payment\PropertyBag->setRecurProcessorID(NULL)"

  • now treats NULL as acceptable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants