PAYOSWXP-158: Add PayPal v2 payment methods #331
Conversation
momocode-de
force-pushed
the
PAYOSWXP-158-PayPal-v2
branch
from
d2ca6d5
to
b70dc4e
Compare
| @@ -26,7 +26,7 @@ jobs: | |||
| matrix: | |||
| php-version: [ '8.2', '8.3' ] | |||
| mysql-version: [ '8.0' ] | |||
| shopware-version: [ 'v6.6.0.0', 'v6.6.1.0', 'v6.6.2.0', 'v6.6.3.0', 'v6.6.4.0', 'v6.6.5.0' ] | |||
| shopware-version: [ 'v6.6.0.0', 'v6.6.1.0', 'v6.6.2.0', 'v6.6.3.0', 'v6.6.4.0', 'v6.6.5.0', 'v6.6.6.0', 'v6.6.7.0' ] | |||
There was a problem hiding this comment.
please separate this in a own PR to make sure the pipelines does also work on these versions without the PayPal v2 adjustments.
| $isShippingAddressComplete = $this->hasAddressRequiredData($shippingAddress); | ||
|
|
||
| if (!$isBillingAddressComplete && !$isShippingAddressComplete) { | ||
| throw new RuntimeException($this->translator->trans('PayonePayment.errorMessages.genericError')); |
There was a problem hiding this comment.
please log the concrete error, or make the message more specific
There was a problem hiding this comment.
You did it exactly the same way in the “getSalutationId” function, so that's what I did. Was there a reason why you threw the generic error? Should both be adjusted then?
|
|
||
| private function hasAddressRequiredData(array $address): bool | ||
| { | ||
| $requiredFields = [ |
There was a problem hiding this comment.
just a side note: shopware might require the phone number, but PayPal will only provide a phone number, if the customer has set up one.
It is not possible to mark the phone number as required field for PayPal/Payone calls. maybe this has been changed during the PayPal v2. Please check this, and consider if we should add the check for the phone number here aswell.
There was a problem hiding this comment.
Ah yes, you can make phone, birthday, additionalAddress1 and additionalAddress2 mandatory fields via the configuration. They are certainly not all transferred by PayPal in every case.
Should I then query the configurations here and, depending on the configuration, make them mandatory fields and if they are not passed, the exception is also thrown, as with the other fields?
| defaults: ['XmlHttpRequest' => true], | ||
| methods: ['GET'] | ||
| )] | ||
| public function createSessionAction(SalesChannelContext $context, string $paymentMethodId): Response |
There was a problem hiding this comment.
is this also used by Amazon-Pay and PayPal (legacy)?
if note, does it make sense to separate this into a custom-controller, or add additional checks for the payment-method?
There was a problem hiding this comment.
No, so far only used by PayPal v2. Since the same thing happens as in the “redirectAction”, I think it should stay in this controller. The difference is simply the response, so instead of redirect, the orderId is returned as JSON, which was needed for PayPal v2, but could just as well be used by others.
| { | ||
| /** @var CustomerRegistrationUtil $util */ | ||
| $util = $this->getContainer()->get(CustomerRegistrationUtil::class); | ||
|
|
||
| $data = $util->getCustomerDataBagFromGetCheckoutSessionResponse([ | ||
| 'addpaydata' => [ | ||
| 'email' => '[email protected]', | ||
| 'telephonenumber' => '0123456789', |
There was a problem hiding this comment.
are these changes a mistake?
the method has tested shipping-data, and now it testing billing-data, but the method still is named to test shipping-data.
i am confused :P
There was a problem hiding this comment.
I had fundamentally renamed the tests and adapted them to the new logic. The diff may be a little confusing here, but it should be correct. This function tests whether all data is correct if no shipping data is passed.
| @@ -16,6 +16,7 @@ | |||
| class RedirectHandlerTest extends TestCase | |||
| { | |||
| use KernelTestBehaviour; | |||
| private const APP_SECRET = 's$cretf0rt3st'; | |||
There was a problem hiding this comment.
would love to see this change in a separate commit.
| ]); | ||
|
|
||
| $activePaymentMethodsLoader = $this->createMock(ActivePaymentMethodsLoaderInterface::class); | ||
| $activePaymentMethodsLoader |
There was a problem hiding this comment.
please keep the testcase simple.
remove the expect and the with
we wont expect any method calls. it is only required to make sure, the method has a return-value. And if the return value does not change between different runs, we only need to have
$activePaymentMethodsLoader->method('getActivePaymentMethodIds')->willReturn($activePaymentMethodIds);There was a problem hiding this comment.
please also apply this to the other statements/testcases
| $customerData = new RequestDataBag([ | ||
| 'guest' => true, | ||
| 'salutationId' => $salutationId, | ||
| 'email' => $response['addpaydata']['email'], | ||
| 'firstName' => $this->extractBillingData($response, 'firstname'), | ||
| 'lastName' => $this->extractBillingData($response, 'lastname'), | ||
| 'firstName' => $billingAddress['firstName'], /** @phpstan-ignore offsetAccess.notFound */ |
There was a problem hiding this comment.
yes, i unterstand, but if it would be still great, if we can get rid of an ignore.
as alternative: use the array_filter only for the parameter of method hasAddressRequiredData, or within the method itself - then you should not get an phpstan error.
|
|
||
| private function hasAddressRequiredData(array $address): bool | ||
| { | ||
| $requiredFields = [ |
|
|
||
| _renderButtons() { | ||
| paypal.Buttons({ | ||
|
|
There was a problem hiding this comment.
could you remove the many empty rows below, please? (till line 63)
No description provided.