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

Implement Orders API for pay later payment methods #22

Open
wants to merge 8 commits into
base: 2018.10
Choose a base branch
from

Conversation

nvindice
Copy link

@nvindice nvindice commented Jun 4, 2019

This patch overrides a couple of Omnipay methods to implement the Mollie Orders API, which is needed for Klarna Pay later and Klarna Slice payment options.

I was not sure if it would be cleaner to modify some methods at Aimeos\MShop\Service\Provider\Payment\Omnipay directly, but ultimately decided to do it like this. Feel free to adapt or modify.

@coveralls
Copy link

coveralls commented Jun 4, 2019

Coverage Status

Coverage decreased (-11.5%) to 61.923% when pulling a3d9f79 on nvindice:2018.10 into b4a5103 on aimeoscom:2018.10.

$response = $provider->completeAuthorize( $params )->send();
$status = \Aimeos\MShop\Order\Item\Base::PAY_AUTHORIZED;
}
elseif( $this->getValue('apiType', 'payment') == 'order' )
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't this be the first if-clause?

Copy link
Author

Choose a reason for hiding this comment

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

Agreed, will change that.

$response = $provider->authorize( $data )->send();
$status = \Aimeos\MShop\Order\Item\Base::PAY_AUTHORIZED;
}
elseif( $this->getValue( 'apiType', '' ) === 'order' )
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same order here?

Copy link
Author

Choose a reason for hiding this comment

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

Sorry, it was a long day. Yes, same apiType in request and sync methods. I will remove the default in the other clause and switch positions here.

* @param array $params POST parameters passed to the provider
* @return \Omnipay\Common\CreditCard Credit card object
*/
protected function getCardDetails( \Aimeos\MShop\Order\Item\Base\Iface $base, array $params )
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why did you overwrite that method and removed some values?

Copy link
Author

Choose a reason for hiding this comment

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

Mollie API / Klarna is very strict on the phone number format. I'm not sure if an automatic transformation is a good idea when it comes to the payment process. So I removed the phone numbers altogether, Klarna will ask for them at a later stage if necessary.

Addiontally, GDPR makes it difficult to pass this information, IMHO.

Copy link
Collaborator

Choose a reason for hiding this comment

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

From the GDPR point of view, the shipping address and the e-mail should be removed as well

Copy link
Author

Choose a reason for hiding this comment

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

Right, but they are necessary for the Orders API. Phone number is not. So I think a shop provider could argue with Article 6 here.

@nvindice
Copy link
Author

nvindice commented Jun 5, 2019

What do you think of this solution altogether? It's not beautiful, because we have a lot of code duplicates here. However, the "order" methods might be too specific to add them to Aimeos\MShop\Service\Provider\Payment\Omnipay IMHO. Not sure how to do it best. What do you think?

EDIT: I will upload the changes later with some additional fixes for coupon handling.

@aimeos
Copy link
Collaborator

aimeos commented Jun 5, 2019

It's hard to split the updateSync and processOrder methods further so this is currently the best option.
Can you please add tests so the code coverage doesn't decrease?

@nvindice
Copy link
Author

nvindice commented Jun 5, 2019

Sorry, I'm not able to write the tests right now. Most of the tests from Omnipay should work, as the methods are copied with minor adaptions. The apiType = order is needed. Can you help me with that?

@aimeos
Copy link
Collaborator

aimeos commented Jun 6, 2019

Not at the moment

@aimeos
Copy link
Collaborator

aimeos commented Sep 9, 2019

@nvindice Any update on this?

@nvindice
Copy link
Author

nvindice commented Sep 9, 2019

Well, we are using it in production without any issues - I'll try to build some tests in the next couple of weeks...

@nvindice
Copy link
Author

Writing tests is more work than expected, because Omnipay/Dummy does not support orders, as Omnipay/omnipay-mollie does. Not sure how to continue best, as I'm not used to work with PHPUnit tests. Any tips for me?

@nvindice
Copy link
Author

nvindice commented Dec 5, 2019

(Push)

@aimeos
Copy link
Collaborator

aimeos commented Dec 13, 2019

Are you going to add the missing tests for your code in the near future?
Then, we could merge your PR

@nvindice
Copy link
Author

As said, I need some help on how to write those tests without a fitting Omnipay/Dummy method. Can you help me with this?

IMO, this PR brings a valuable new component to Aimeos: paying with Mollie/Klarna-PayLater, Apple Pay (?) and other advanced payment methods.

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

Successfully merging this pull request may close these issues.

3 participants