Skip to content

Commit 547f453

Browse files
viktymviktym
committed
MAGETWO-99383: Empty email in quote prevent order creation even if payment success
- Added order address validation before order placing - Added logging of failed order saving attempts
1 parent 932acf9 commit 547f453

File tree

7 files changed

+195
-28
lines changed

7 files changed

+195
-28
lines changed

‎app/code/Magento/Quote/Model/QuoteManagement.php

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -37,9 +37,9 @@ class QuoteManagement implements \Magento\Quote\Api\CartManagementInterface
3737
protected $eventManager;
3838

3939
/**
40-
* @var QuoteValidator
40+
* @var SubmitQuoteValidator
4141
*/
42-
protected $quoteValidator;
42+
private $submitQuoteValidator;
4343

4444
/**
4545
* @var OrderFactory
@@ -148,7 +148,7 @@ class QuoteManagement implements \Magento\Quote\Api\CartManagementInterface
148148

149149
/**
150150
* @param EventManager $eventManager
151-
* @param QuoteValidator $quoteValidator
151+
* @param SubmitQuoteValidator $submitQuoteValidator
152152
* @param OrderFactory $orderFactory
153153
* @param OrderManagement $orderManagement
154154
* @param CustomerManagement $customerManagement
@@ -173,7 +173,7 @@ class QuoteManagement implements \Magento\Quote\Api\CartManagementInterface
173173
*/
174174
public function __construct(
175175
EventManager $eventManager,
176-
QuoteValidator $quoteValidator,
176+
SubmitQuoteValidator $submitQuoteValidator,
177177
OrderFactory $orderFactory,
178178
OrderManagement $orderManagement,
179179
CustomerManagement $customerManagement,
@@ -196,7 +196,7 @@ public function __construct(
196196
\Magento\Customer\Api\AddressRepositoryInterface $addressRepository = null
197197
) {
198198
$this->eventManager = $eventManager;
199-
$this->quoteValidator = $quoteValidator;
199+
$this->submitQuoteValidator = $submitQuoteValidator;
200200
$this->orderFactory = $orderFactory;
201201
$this->orderManagement = $orderManagement;
202202
$this->customerManagement = $customerManagement;
@@ -281,6 +281,7 @@ public function assignCustomer($cartId, $customerId, $storeId)
281281
throw new StateException(
282282
__("The customer can't be assigned to the cart because the customer already has an active cart.")
283283
);
284+
// phpcs:ignore Magento2.CodeAnalysis.EmptyBlock
284285
} catch (\Magento\Framework\Exception\NoSuchEntityException $e) {
285286
}
286287

@@ -457,7 +458,7 @@ protected function resolveItems(QuoteEntity $quote)
457458
protected function submitQuote(QuoteEntity $quote, $orderData = [])
458459
{
459460
$order = $this->orderFactory->create();
460-
$this->quoteValidator->validateBeforeSubmit($quote);
461+
$this->submitQuoteValidator->validateQuote($quote);
461462
if (!$quote->getCustomerIsGuest()) {
462463
if ($quote->getCustomerId()) {
463464
$this->_prepareCustomerQuote($quote);
@@ -512,6 +513,7 @@ protected function submitQuote(QuoteEntity $quote, $orderData = [])
512513
$order->setCustomerFirstname($quote->getCustomerFirstname());
513514
$order->setCustomerMiddlename($quote->getCustomerMiddlename());
514515
$order->setCustomerLastname($quote->getCustomerLastname());
516+
$this->submitQuoteValidator->validateOrder($order);
515517

516518
$this->eventManager->dispatch(
517519
'sales_model_service_quote_submit_before',
@@ -627,12 +629,13 @@ private function rollbackAddresses(
627629
'exception' => $e,
628630
]
629631
);
632+
// phpcs:ignore Magento2.Exceptions.ThrowCatch
630633
} catch (\Exception $consecutiveException) {
631634
$message = sprintf(
632635
"An exception occurred on 'sales_model_service_quote_submit_failure' event: %s",
633636
$consecutiveException->getMessage()
634637
);
635-
638+
// phpcs:ignore Magento2.Exceptions.DirectThrow
636639
throw new \Exception($message, 0, $e);
637640
}
638641
}

‎app/code/Magento/Quote/Model/SubmitQuoteValidator.php

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,71 @@
1+
<?php
2+
/**
3+
* Copyright © Magento, Inc. All rights reserved.
4+
* See COPYING.txt for license details.
5+
*/
6+
declare(strict_types=1);
7+
8+
namespace Magento\Quote\Model;
9+
10+
use Magento\Framework\Exception\LocalizedException;
11+
use Magento\Sales\Model\Order;
12+
use Magento\Sales\Model\Order\Address\Validator as OrderAddressValidator;
13+
14+
/**
15+
* Validates quote and order before quote submit.
16+
*/
17+
class SubmitQuoteValidator
18+
{
19+
/**
20+
* @var QuoteValidator
21+
*/
22+
private $quoteValidator;
23+
24+
/**
25+
* @var OrderAddressValidator
26+
*/
27+
private $orderAddressValidator;
28+
29+
/**
30+
* @param QuoteValidator $quoteValidator
31+
* @param OrderAddressValidator $orderAddressValidator
32+
*/
33+
public function __construct(
34+
QuoteValidator $quoteValidator,
35+
OrderAddressValidator $orderAddressValidator
36+
) {
37+
$this->quoteValidator = $quoteValidator;
38+
$this->orderAddressValidator = $orderAddressValidator;
39+
}
40+
41+
/**
42+
* Validates quote.
43+
*
44+
* @param Quote $quote
45+
* @return void
46+
* @throws LocalizedException
47+
*/
48+
public function validateQuote(Quote $quote): void
49+
{
50+
$this->quoteValidator->validateBeforeSubmit($quote);
51+
}
52+
53+
/**
54+
* Validates order.
55+
*
56+
* @param Order $order
57+
* @return void
58+
* @throws LocalizedException
59+
*/
60+
public function validateOrder(Order $order): void
61+
{
62+
foreach ($order->getAddresses() as $address) {
63+
$errors = $this->orderAddressValidator->validate($address);
64+
if (!empty($errors)) {
65+
throw new LocalizedException(
66+
__("Failed address validation:\n%1", implode("\n", $errors))
67+
);
68+
}
69+
}
70+
}
71+
}

‎app/code/Magento/Quote/Test/Unit/Model/QuoteManagementTest.php

Lines changed: 19 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,8 @@
1414
/**
1515
* @SuppressWarnings(PHPMD.CouplingBetweenObjects)
1616
* @SuppressWarnings(PHPMD.TooManyFields)
17+
* @SuppressWarnings(PHPMD.TooManyPublicMethods)
18+
* @SuppressWarnings(PHPMD.ExcessiveClassLength)
1719
*/
1820
class QuoteManagementTest extends \PHPUnit\Framework\TestCase
1921
{
@@ -23,9 +25,9 @@ class QuoteManagementTest extends \PHPUnit\Framework\TestCase
2325
protected $model;
2426

2527
/**
26-
* @var \Magento\Quote\Model\QuoteValidator|\PHPUnit_Framework_MockObject_MockObject
28+
* @var \Magento\Quote\Model\SubmitQuoteValidator|\PHPUnit_Framework_MockObject_MockObject
2729
*/
28-
protected $quoteValidator;
30+
protected $submitQuoteValidator;
2931

3032
/**
3133
* @var \Magento\Framework\Event\ManagerInterface|\PHPUnit_Framework_MockObject_MockObject
@@ -144,7 +146,7 @@ protected function setUp()
144146
{
145147
$objectManager = new \Magento\Framework\TestFramework\Unit\Helper\ObjectManager($this);
146148

147-
$this->quoteValidator = $this->createMock(\Magento\Quote\Model\QuoteValidator::class);
149+
$this->submitQuoteValidator = $this->createMock(\Magento\Quote\Model\SubmitQuoteValidator::class);
148150
$this->eventManager = $this->getMockForAbstractClass(\Magento\Framework\Event\ManagerInterface::class);
149151
$this->orderFactory = $this->createPartialMock(
150152
\Magento\Sales\Api\Data\OrderInterfaceFactory::class,
@@ -212,7 +214,7 @@ protected function setUp()
212214
\Magento\Quote\Model\QuoteManagement::class,
213215
[
214216
'eventManager' => $this->eventManager,
215-
'quoteValidator' => $this->quoteValidator,
217+
'submitQuoteValidator' => $this->submitQuoteValidator,
216218
'orderFactory' => $this->orderFactory,
217219
'orderManagement' => $this->orderManagement,
218220
'customerManagement' => $this->customerManagement,
@@ -564,7 +566,9 @@ public function testSubmit()
564566
$shippingAddress
565567
);
566568

567-
$this->quoteValidator->expects($this->once())->method('validateBeforeSubmit')->with($quote);
569+
$this->submitQuoteValidator->expects($this->once())
570+
->method('validateQuote')
571+
->with($quote);
568572
$this->quoteAddressToOrder->expects($this->once())
569573
->method('convert')
570574
->with($shippingAddress, $orderData)
@@ -658,7 +662,7 @@ public function testPlaceOrderIfCustomerIsGuest()
658662
->setConstructorArgs(
659663
[
660664
'eventManager' => $this->eventManager,
661-
'quoteValidator' => $this->quoteValidator,
665+
'quoteValidator' => $this->submitQuoteValidator,
662666
'orderFactory' => $this->orderFactory,
663667
'orderManagement' => $this->orderManagement,
664668
'customerManagement' => $this->customerManagement,
@@ -716,7 +720,7 @@ public function testPlaceOrder()
716720
->setConstructorArgs(
717721
[
718722
'eventManager' => $this->eventManager,
719-
'quoteValidator' => $this->quoteValidator,
723+
'quoteValidator' => $this->submitQuoteValidator,
720724
'orderFactory' => $this->orderFactory,
721725
'orderManagement' => $this->orderManagement,
722726
'customerManagement' => $this->customerManagement,
@@ -938,6 +942,9 @@ protected function prepareOrderFactory(
938942
return $order;
939943
}
940944

945+
/**
946+
* @throws NoSuchEntityException
947+
*/
941948
public function testGetCartForCustomer()
942949
{
943950
$customerId = 100;
@@ -982,6 +989,9 @@ protected function setPropertyValue(&$object, $property, $value)
982989
return $object;
983990
}
984991

992+
/**
993+
* @throws \Magento\Framework\Exception\LocalizedException
994+
*/
985995
public function testSubmitForCustomer()
986996
{
987997
$orderData = [];
@@ -1014,7 +1024,8 @@ public function testSubmitForCustomer()
10141024
$shippingAddress
10151025
);
10161026

1017-
$this->quoteValidator->expects($this->once())->method('validateBeforeSubmit')->with($quote);
1027+
$this->submitQuoteValidator->method('validateQuote')
1028+
->with($quote);
10181029
$this->quoteAddressToOrder->expects($this->once())
10191030
->method('convert')
10201031
->with($shippingAddress, $orderData)

‎app/code/Magento/Sales/Model/Service/OrderService.php

Lines changed: 26 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77

88
use Magento\Sales\Api\OrderManagementInterface;
99
use Magento\Payment\Gateway\Command\CommandException;
10+
use Psr\Log\LoggerInterface;
1011

1112
/**
1213
* Class OrderService
@@ -55,6 +56,11 @@ class OrderService implements OrderManagementInterface
5556
*/
5657
private $paymentFailures;
5758

59+
/**
60+
* @var LoggerInterface
61+
*/
62+
private $logger;
63+
5864
/**
5965
* Constructor
6066
*
@@ -65,7 +71,8 @@ class OrderService implements OrderManagementInterface
6571
* @param \Magento\Sales\Model\OrderNotifier $notifier
6672
* @param \Magento\Framework\Event\ManagerInterface $eventManager
6773
* @param \Magento\Sales\Model\Order\Email\Sender\OrderCommentSender $orderCommentSender
68-
* @param \Magento\Sales\Api\PaymentFailuresInterface|null $paymentFailures
74+
* @param \Magento\Sales\Api\PaymentFailuresInterface $paymentFailures
75+
* @param LoggerInterface $logger
6976
*/
7077
public function __construct(
7178
\Magento\Sales\Api\OrderRepositoryInterface $orderRepository,
@@ -75,7 +82,8 @@ public function __construct(
7582
\Magento\Sales\Model\OrderNotifier $notifier,
7683
\Magento\Framework\Event\ManagerInterface $eventManager,
7784
\Magento\Sales\Model\Order\Email\Sender\OrderCommentSender $orderCommentSender,
78-
\Magento\Sales\Api\PaymentFailuresInterface $paymentFailures = null
85+
\Magento\Sales\Api\PaymentFailuresInterface $paymentFailures,
86+
LoggerInterface $logger
7987
) {
8088
$this->orderRepository = $orderRepository;
8189
$this->historyRepository = $historyRepository;
@@ -84,8 +92,8 @@ public function __construct(
8492
$this->notifier = $notifier;
8593
$this->eventManager = $eventManager;
8694
$this->orderCommentSender = $orderCommentSender;
87-
$this->paymentFailures = $paymentFailures ? : \Magento\Framework\App\ObjectManager::getInstance()
88-
->get(\Magento\Sales\Api\PaymentFailuresInterface::class);
95+
$this->paymentFailures = $paymentFailures;
96+
$this->logger = $logger;
8997
}
9098

9199
/**
@@ -189,25 +197,31 @@ public function unHold($id)
189197
}
190198

191199
/**
200+
* Perform place order.
201+
*
192202
* @param \Magento\Sales\Api\Data\OrderInterface $order
193203
* @return \Magento\Sales\Api\Data\OrderInterface
194204
* @throws \Exception
195205
*/
196206
public function place(\Magento\Sales\Api\Data\OrderInterface $order)
197207
{
198-
// transaction will be here
199-
//begin transaction
200208
try {
201209
$order->place();
202-
return $this->orderRepository->save($order);
203-
//commit
210+
} catch (CommandException $e) {
211+
$this->paymentFailures->handle((int)$order->getQuoteId(), __($e->getMessage()));
212+
throw $e;
213+
}
214+
215+
try {
216+
$order = $this->orderRepository->save($order);
204217
} catch (\Exception $e) {
205-
if ($e instanceof CommandException) {
206-
$this->paymentFailures->handle((int)$order->getQuoteId(), __($e->getMessage()));
207-
}
218+
$this->logger->critical(
219+
'Saving order ' . $order->getIncrementId() . ' failed: ' . $e->getMessage()
220+
);
208221
throw $e;
209-
//rollback;
210222
}
223+
224+
return $order;
211225
}
212226

213227
/**

‎app/code/Magento/Sales/Test/Unit/Model/Service/OrderServiceTest.php

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,9 @@
55
*/
66
namespace Magento\Sales\Test\Unit\Model\Service;
77

8+
use Magento\Sales\Api\PaymentFailuresInterface;
9+
use Psr\Log\LoggerInterface;
10+
811
/**
912
* Class OrderUnHoldTest
1013
*
@@ -140,14 +143,22 @@ protected function setUp()
140143
->disableOriginalConstructor()
141144
->getMock();
142145

146+
/** @var PaymentFailuresInterface|\PHPUnit_Framework_MockObject_MockObject $paymentFailures */
147+
$paymentFailures = $this->createMock(PaymentFailuresInterface::class);
148+
149+
/** @var LoggerInterface|\PHPUnit_Framework_MockObject_MockObject $logger */
150+
$logger = $this->createMock(LoggerInterface::class);
151+
143152
$this->orderService = new \Magento\Sales\Model\Service\OrderService(
144153
$this->orderRepositoryMock,
145154
$this->orderStatusHistoryRepositoryMock,
146155
$this->searchCriteriaBuilderMock,
147156
$this->filterBuilderMock,
148157
$this->orderNotifierMock,
149158
$this->eventManagerMock,
150-
$this->orderCommentSender
159+
$this->orderCommentSender,
160+
$paymentFailures,
161+
$logger
151162
);
152163
}
153164

0 commit comments

Comments
 (0)