From 3e08b6ae49d5f38791dba4050be50c6f760275c3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Enrique=20Barbeito=20Garc=C3=ADa?= Date: Sun, 8 Sep 2024 09:42:23 +0200 Subject: [PATCH 1/7] The notification resolver throws its related empty exception When the request payload does not define any notification items --- src/Exception/NotificationItemsEmptyException.php | 2 +- src/Resolver/Notification/NotificationResolver.php | 8 +++++--- .../Notification/NotificationResolverInterface.php | 3 +++ 3 files changed, 9 insertions(+), 4 deletions(-) diff --git a/src/Exception/NotificationItemsEmptyException.php b/src/Exception/NotificationItemsEmptyException.php index 497ee912..63be03ed 100644 --- a/src/Exception/NotificationItemsEmptyException.php +++ b/src/Exception/NotificationItemsEmptyException.php @@ -11,6 +11,6 @@ namespace BitBag\SyliusAdyenPlugin\Exception; -class NotificationItemsEmptyException extends \InvalidArgumentException +final class NotificationItemsEmptyException extends \InvalidArgumentException { } diff --git a/src/Resolver/Notification/NotificationResolver.php b/src/Resolver/Notification/NotificationResolver.php index d44bd50b..8b41ad9e 100644 --- a/src/Resolver/Notification/NotificationResolver.php +++ b/src/Resolver/Notification/NotificationResolver.php @@ -11,6 +11,7 @@ namespace BitBag\SyliusAdyenPlugin\Resolver\Notification; +use BitBag\SyliusAdyenPlugin\Exception\NotificationItemsEmptyException; use BitBag\SyliusAdyenPlugin\Resolver\Notification\Struct\NotificationItemData; use Psr\Log\LoggerInterface; use Symfony\Component\HttpFoundation\Request; @@ -48,11 +49,12 @@ public function __construct( */ private function denormalizeRequestData(Request $request): array { + $result = []; $payload = $request->request->all(); /** @var array $notificationItems */ - $notificationItems = $payload['notificationItems']; - $result = []; + $notificationItems = $payload['notificationItems'] + ?? throw new NotificationItemsEmptyException(); /** @var array $notificationItem */ foreach ($notificationItems as $notificationItem) { @@ -70,7 +72,7 @@ private function denormalizeRequestData(Request $request): array } /** - * @return NotificationItemData[] + * @inheritDoc */ public function resolve(string $paymentCode, Request $request): array { diff --git a/src/Resolver/Notification/NotificationResolverInterface.php b/src/Resolver/Notification/NotificationResolverInterface.php index 064db7d9..8087884a 100644 --- a/src/Resolver/Notification/NotificationResolverInterface.php +++ b/src/Resolver/Notification/NotificationResolverInterface.php @@ -11,6 +11,7 @@ namespace BitBag\SyliusAdyenPlugin\Resolver\Notification; +use BitBag\SyliusAdyenPlugin\Exception\NotificationItemsEmptyException; use BitBag\SyliusAdyenPlugin\Resolver\Notification\Struct\NotificationItemData; use Symfony\Component\HttpFoundation\Request; @@ -18,6 +19,8 @@ interface NotificationResolverInterface { /** * @return NotificationItemData[] + * + * @throws NotificationItemsEmptyException */ public function resolve(string $paymentCode, Request $request): array; } From d9463b3ae94afecf6967753e3fb234099f35fc80 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Enrique=20Barbeito=20Garc=C3=ADa?= Date: Sun, 8 Sep 2024 09:45:04 +0200 Subject: [PATCH 2/7] Safer NotificationRequest initialization by preventing uneeded nullables --- src/Resolver/Notification/Struct/NotificationRequest.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Resolver/Notification/Struct/NotificationRequest.php b/src/Resolver/Notification/Struct/NotificationRequest.php index 7816539a..67677337 100644 --- a/src/Resolver/Notification/Struct/NotificationRequest.php +++ b/src/Resolver/Notification/Struct/NotificationRequest.php @@ -16,6 +16,6 @@ class NotificationRequest /** @var ?bool */ public $live; - /** @var ?NotificationItem[] */ - public $notificationItems; + /** @var NotificationItem[] */ + public $notificationItems = []; } From 6097f293323782a8269b94ec83237a5cc09ce405 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Enrique=20Barbeito=20Garc=C3=ADa?= Date: Sun, 8 Sep 2024 09:47:37 +0200 Subject: [PATCH 3/7] Allow to process undefined or empty notification items preventing PHP warnings This commit resolves the PHP Warning: Undefined array key "notificationItems" --- src/Controller/Shop/ProcessNotificationsAction.php | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/Controller/Shop/ProcessNotificationsAction.php b/src/Controller/Shop/ProcessNotificationsAction.php index e2cd8a52..48eeb9ec 100644 --- a/src/Controller/Shop/ProcessNotificationsAction.php +++ b/src/Controller/Shop/ProcessNotificationsAction.php @@ -49,7 +49,14 @@ public function __construct( public function __invoke(string $code, Request $request): Response { - foreach ($this->notificationResolver->resolve($code, $request) as $notificationItem) { + try { + $notifications = $this->notificationResolver->resolve($code, $request); + } catch (NotificationItemsEmptyException) { + $this->logger->error('Request payload did not contain any notification items'); + $notifications = []; + } + + foreach ($notifications as $notificationItem) { if (null === $notificationItem || false === $notificationItem->success) { $this->logger->error(\sprintf( 'Payment with pspReference [%s] did not return success', From b84192c1cd44b7b3eced1602e8fb8eb1e162cc98 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Enrique=20Barbeito=20Garc=C3=ADa?= Date: Sun, 8 Sep 2024 10:10:01 +0200 Subject: [PATCH 4/7] Unit test the undefined notification items HTTP request action --- .../Shop/ProcessNotificationsAction.php | 13 ++++---- .../Shop/ProcessNotificationsActionTest.php | 30 +++++++++++++++++++ tests/Unit/Mock/RequestMother.php | 13 ++++++-- 3 files changed, 47 insertions(+), 9 deletions(-) create mode 100644 tests/Unit/Controller/Shop/ProcessNotificationsActionTest.php diff --git a/src/Controller/Shop/ProcessNotificationsAction.php b/src/Controller/Shop/ProcessNotificationsAction.php index 48eeb9ec..70195c15 100644 --- a/src/Controller/Shop/ProcessNotificationsAction.php +++ b/src/Controller/Shop/ProcessNotificationsAction.php @@ -12,9 +12,10 @@ namespace BitBag\SyliusAdyenPlugin\Controller\Shop; use BitBag\SyliusAdyenPlugin\Bus\DispatcherInterface; -use BitBag\SyliusAdyenPlugin\Resolver\Notification\NotificationResolver; +use BitBag\SyliusAdyenPlugin\Exception\NotificationItemsEmptyException; use BitBag\SyliusAdyenPlugin\Resolver\Notification\NotificationResolver\NoCommandResolvedException; -use BitBag\SyliusAdyenPlugin\Resolver\Notification\NotificationToCommandResolver; +use BitBag\SyliusAdyenPlugin\Resolver\Notification\NotificationResolverInterface; +use BitBag\SyliusAdyenPlugin\Resolver\Notification\NotificationToCommandResolverInterface; use Psr\Log\LoggerInterface; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\Response; @@ -26,10 +27,10 @@ class ProcessNotificationsAction /** @var DispatcherInterface */ private $dispatcher; - /** @var NotificationToCommandResolver */ + /** @var NotificationToCommandResolverInterface */ private $notificationCommandResolver; - /** @var NotificationResolver */ + /** @var NotificationResolverInterface */ private $notificationResolver; /** @var LoggerInterface */ @@ -37,8 +38,8 @@ class ProcessNotificationsAction public function __construct( DispatcherInterface $dispatcher, - NotificationToCommandResolver $notificationCommandResolver, - NotificationResolver $notificationResolver, + NotificationToCommandResolverInterface $notificationCommandResolver, + NotificationResolverInterface $notificationResolver, LoggerInterface $logger, ) { $this->dispatcher = $dispatcher; diff --git a/tests/Unit/Controller/Shop/ProcessNotificationsActionTest.php b/tests/Unit/Controller/Shop/ProcessNotificationsActionTest.php new file mode 100644 index 00000000..3fa9f194 --- /dev/null +++ b/tests/Unit/Controller/Shop/ProcessNotificationsActionTest.php @@ -0,0 +1,30 @@ +createMock(DispatcherInterface::class), + $this->createMock(NotificationToCommandResolverInterface::class), + $this->createMock(NotificationResolverInterface::class), + new NullLogger() + ); + + $response = $action('dummy-code', RequestMother::createDummy()); + + self::assertSame(200, $response->getStatusCode()); + self::assertSame('[accepted]', $response->getContent()); + } +} diff --git a/tests/Unit/Mock/RequestMother.php b/tests/Unit/Mock/RequestMother.php index 02ef4bde..2d8e7040 100644 --- a/tests/Unit/Mock/RequestMother.php +++ b/tests/Unit/Mock/RequestMother.php @@ -18,14 +18,21 @@ final class RequestMother { - public const TEST_LOCALE = 'pl_PL'; + private const ROOT_PATH = '/'; + + private const TEST_LOCALE = 'pl_PL'; public const WHERE_YOUR_HOME_IS = '127.0.0.1'; + public static function createDummy(): Request + { + return Request::create(self::ROOT_PATH); + } + public static function createWithSession(): Request { $session = new Session(new MockArraySessionStorage()); - $request = Request::create('/'); + $request = Request::create(self::ROOT_PATH); $request->setSession($session); return $request; @@ -49,7 +56,7 @@ public static function createWithSessionForSpecifiedQueryToken(): Request public static function createWithLocaleSet(): Request { - $result = Request::create('/', 'GET', [], [], [], ['REMOTE_ADDR' => self::WHERE_YOUR_HOME_IS]); + $result = Request::create(self::ROOT_PATH, 'GET', [], [], [], ['REMOTE_ADDR' => self::WHERE_YOUR_HOME_IS]); $result->setLocale(self::TEST_LOCALE); return $result; From 2c9ef25da3b50f20bace149992b9bb00b462cb05 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Enrique=20Barbeito=20Garc=C3=ADa?= Date: Sun, 8 Sep 2024 10:39:10 +0200 Subject: [PATCH 5/7] Perform coding standard checks --- .../Controller/Shop/ProcessNotificationsActionTest.php | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/tests/Unit/Controller/Shop/ProcessNotificationsActionTest.php b/tests/Unit/Controller/Shop/ProcessNotificationsActionTest.php index 3fa9f194..5e302f22 100644 --- a/tests/Unit/Controller/Shop/ProcessNotificationsActionTest.php +++ b/tests/Unit/Controller/Shop/ProcessNotificationsActionTest.php @@ -1,4 +1,12 @@ createMock(DispatcherInterface::class), $this->createMock(NotificationToCommandResolverInterface::class), $this->createMock(NotificationResolverInterface::class), - new NullLogger() + new NullLogger(), ); $response = $action('dummy-code', RequestMother::createDummy()); From 29ed5c6abaa8bdd8a3e10c114874fe0aeada3bee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Enrique=20Barbeito=20Garc=C3=ADa?= Date: Sun, 8 Sep 2024 11:29:42 +0200 Subject: [PATCH 6/7] Fix constant visibility to hide implementation detail --- src/Controller/Shop/ProcessNotificationsAction.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Controller/Shop/ProcessNotificationsAction.php b/src/Controller/Shop/ProcessNotificationsAction.php index 70195c15..22b6c70b 100644 --- a/src/Controller/Shop/ProcessNotificationsAction.php +++ b/src/Controller/Shop/ProcessNotificationsAction.php @@ -22,7 +22,7 @@ class ProcessNotificationsAction { - public const EXPECTED_ADYEN_RESPONSE = '[accepted]'; + private const EXPECTED_ADYEN_RESPONSE = '[accepted]'; /** @var DispatcherInterface */ private $dispatcher; From 8c7dda61a6fc28b6f677a41fc8ceb5bfdcd55f84 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Enrique=20Barbeito=20Garc=C3=ADa?= Date: Sun, 8 Sep 2024 11:31:00 +0200 Subject: [PATCH 7/7] Add unit test for Notification Resolver --- .../Notification/NotificationResolverTest.php | 36 +++++++++++++++++++ 1 file changed, 36 insertions(+) create mode 100644 tests/Unit/Resolver/Notification/NotificationResolverTest.php diff --git a/tests/Unit/Resolver/Notification/NotificationResolverTest.php b/tests/Unit/Resolver/Notification/NotificationResolverTest.php new file mode 100644 index 00000000..e8bc42e1 --- /dev/null +++ b/tests/Unit/Resolver/Notification/NotificationResolverTest.php @@ -0,0 +1,36 @@ +createMock(DenormalizerInterface::class), + $this->createMock(ValidatorInterface::class), + new NullLogger(), + ); + + $this->expectException(NotificationItemsEmptyException::class); + + $resolver->resolve('dummy-payment-code', RequestMother::createDummy()); + } +}