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

Sagepay one-off payments #147

Closed
wants to merge 8 commits into from
Closed

Sagepay one-off payments #147

wants to merge 8 commits into from

Conversation

elcapo
Copy link
Contributor

@elcapo elcapo commented Jan 23, 2020

Hello @eileenmcnaughton,

Here is my first attempt to make Sagepay payments work. It only enables one-off payments (I'll be working on recurring ones but they're not covered yet).

Can you let me know if these changes look correct to you?

Notification from different session

I added a notification_from_different_session payment processor metadata to identify a Sagepay behavior: contributions are completed by their notification services and they redirect the user to CiviCRM after the payment is completed.

As the qfKey is stored in the user's browser session and Sagepay also requires us remembering a SecurityKey, both the qfKey and the SecurityKey are (pre)-stored in the contribution and then queried from there during the notification process.

Shipping fields can't be empty

To prevent a 3140 : The DeliveryCountry value is invalid, I copied them from the billing data, just in case they are missing.

@civibot civibot bot added the master label Jan 23, 2020
@mattwire mattwire mentioned this pull request Jan 27, 2020
@@ -512,6 +577,8 @@ private function getCreditCardObjectParams($params) {
$cardFields['billingState'] = CRM_Core_PseudoConstant::stateProvinceAbbreviation($cardFields['billingState']);
}

$this->copyShippingFieldsFromBillingIfEmpty($cardFields);
Copy link
Contributor

Choose a reason for hiding this comment

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

@elcapo At least with sagepay there are two ways to handle the missing shipping fields. Either copy them or set billingForShipping to 1 in the omnipay sagepay gateway params - see https://github.com/eileenmcnaughton/nz.co.fuzion.omnipaymultiprocessor/pull/42/files#diff-71ff8f521b77ef1a6c6643105f8b24d5R60

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've just tried removing the copyShippingFieldsFromBillingIfEmpty function in favor of the gateway_params you suggest but that takes me again to the 3140 : The DeliveryCountry value is invalid error message. Also, I didn't find any usage of the gateway_params in the extension. Maybe it has been deprecated?

Copy link
Contributor

Choose a reason for hiding this comment

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

It wasn't fully implemented in this extension yet - see #81

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Good to know! I'll do that your way when your PR is accepted. It looks cleaner than mine.

Copy link
Contributor

@mattwire mattwire left a comment

Choose a reason for hiding this comment

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

Also, @elcapo can you confirm if this PR can be closed or would still be needed with your changes? #82

@elcapo
Copy link
Contributor Author

elcapo commented Jan 27, 2020

Also, @elcapo can you confirm if this PR can be closed or would still be needed with your changes? #82

It can be closed from my point of view. Only thing I see I'm not doing yet is in this PR is to complete an upgrade to Sagepay 3 by removing the send part from:

$this->gateway->acceptNotification($params)->send();

But I guess that can be done in a different PR (fully compliance with Omnipay 3.0 or something like that).

By the way, I'll be sending (hopefully this week) a PR with support for repeated transactions using continuous authority support.

@elcapo
Copy link
Contributor Author

elcapo commented Jan 27, 2020

@mattwire Just in case you want to start testing repeated payments through continuous authority, I've a "working draft" in this continuous_authority branch. I'll be working on it to have ready a PR soon. Any feedback is welcome.

@elcapo
Copy link
Contributor Author

elcapo commented Jan 28, 2020

@eileenmcnaughton If you see this, please consider accepting @mattwire 's Pull Request about gateway params via metadata first. I've an update ready for this one where, assuming Matt's PR is there, I removed the copyShippingFieldsFromBillingIfEmpty function (which was actually breaking some tests).

@mattwire
Copy link
Contributor

@elcapo This needs a rebase - maybe best to include #81 in this PR?

@eileenmcnaughton
Copy link
Owner

I'm closing this out now as the functionality for it has been merged in as well as the functionality to make recurring payments work - it seems to work fine in my tests but needs more testing before I tag a release

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.

3 participants