Merge pull request #670 from magento-folks/totals-collector

[Folks] Collect totals logic optimization
magento · Oct 5, 2015 · 5d35a7b · ihor-sviziev · Oct 6, 2015 · 5d35a7b
2 parents c3ae167 + 02bbbc0
commit 5d35a7b
Show file tree

Hide file tree

Showing 99 changed files with 3,398 additions and 2,068 deletions.
diff --git a/app/code/Magento/Checkout/Block/Cart/AbstractCart.php b/app/code/Magento/Checkout/Block/Cart/AbstractCart.php
@@ -149,7 +149,11 @@ public function getTotals()
     public function getTotalsCache()
     {
         if (empty($this->_totals)) {
-            $this->_totals = $this->getQuote()->getTotals();
+            if ($this->getQuote()->isVirtual()) {
+                $this->_totals = $this->getQuote()->getBillingAddress()->getTotals();
+            } else {
+                $this->_totals = $this->getQuote()->getShippingAddress()->getTotals();
+            }
         }
         return $this->_totals;
     }

diff --git a/app/code/Magento/Checkout/Model/DefaultConfigProvider.php b/app/code/Magento/Checkout/Model/DefaultConfigProvider.php
@@ -562,7 +562,11 @@ private function getTotalsData()
         $totalSegmentsData = [];
         /** @var \Magento\Quote\Model\Cart\TotalSegment $totalSegment */
         foreach ($totals->getTotalSegments() as $totalSegment) {
-            $totalSegmentsData[] = $totalSegment->toArray();
+            $totalSegmentArray = $totalSegment->toArray();
+            if (is_object($totalSegment->getExtensionAttributes())) {
+                $totalSegmentArray['extension_attributes'] = $totalSegment->getExtensionAttributes()->__toArray();
+            }
+            $totalSegmentsData[] = $totalSegmentArray;
         }
         $totals->setItems($items);
         $totals->setTotalSegments($totalSegmentsData);

diff --git a/app/code/Magento/Checkout/Model/ShippingInformationManagement.php b/app/code/Magento/Checkout/Model/ShippingInformationManagement.php
@@ -62,6 +62,11 @@ class ShippingInformationManagement implements \Magento\Checkout\Api\ShippingInf
      */
     protected $scopeConfig;
 
+    /**
+     * @var \Magento\Quote\Model\Quote\TotalsCollector
+     */
+    protected $totalsCollector;
+
     /**
      * @param \Magento\Quote\Api\PaymentMethodManagementInterface $paymentMethodManagement
      * @param \Magento\Checkout\Model\PaymentDetailsFactory $paymentDetailsFactory
@@ -71,6 +76,7 @@ class ShippingInformationManagement implements \Magento\Checkout\Api\ShippingInf
      * @param Logger $logger
      * @param \Magento\Customer\Api\AddressRepositoryInterface $addressRepository
      * @param \Magento\Framework\App\Config\ScopeConfigInterface $scopeConfig
+     * @param \Magento\Quote\Model\Quote\TotalsCollector $totalsCollector
      * @codeCoverageIgnore
      */
     public function __construct(
@@ -81,7 +87,8 @@ public function __construct(
         QuoteAddressValidator $addressValidator,
         Logger $logger,
         \Magento\Customer\Api\AddressRepositoryInterface $addressRepository,
-        \Magento\Framework\App\Config\ScopeConfigInterface $scopeConfig
+        \Magento\Framework\App\Config\ScopeConfigInterface $scopeConfig,
+        \Magento\Quote\Model\Quote\TotalsCollector $totalsCollector
     ) {
         $this->paymentMethodManagement = $paymentMethodManagement;
         $this->paymentDetailsFactory = $paymentDetailsFactory;
@@ -91,6 +98,7 @@ public function __construct(
         $this->logger = $logger;
         $this->addressRepository = $addressRepository;
         $this->scopeConfig = $scopeConfig;
+        $this->totalsCollector = $totalsCollector;
     }
 
     /**
@@ -132,9 +140,7 @@ public function saveAddressInformation(
         $address->setShippingMethod($carrierCode . '_' . $methodCode);
 
         try {
-            /** TODO: refactor this code. Eliminate save operation */
-            $address->save();
-            $address->collectTotals();
+            $this->totalsCollector->collectAddressTotals($quote, $address);
         } catch (\Exception $e) {
             $this->logger->critical($e);
             throw new InputException(__('Unable to save address. Please, check input data.'));

diff --git a/app/code/Magento/Checkout/Model/Type/Onepage.php b/app/code/Magento/Checkout/Model/Type/Onepage.php
@@ -165,6 +165,11 @@ class Onepage
      */
     protected $dataObjectHelper;
 
+    /**
+     * @var \Magento\Quote\Model\Quote\TotalsCollector
+     */
+    protected $totalsCollector;
+
     /**
      * @param \Magento\Framework\Event\ManagerInterface $eventManager
      * @param \Magento\Checkout\Helper\Data $helper
@@ -192,6 +197,7 @@ class Onepage
      * @param \Magento\Framework\Api\ExtensibleDataObjectConverter $extensibleDataObjectConverter
      * @param \Magento\Quote\Model\QuoteManagement $quoteManagement
      * @param \Magento\Framework\Api\DataObjectHelper $dataObjectHelper
+     * @param \Magento\Quote\Model\Quote\TotalsCollector $totalsCollector
      * @codeCoverageIgnore
      * @SuppressWarnings(PHPMD.ExcessiveParameterList)
      */
@@ -221,7 +227,8 @@ public function __construct(
         \Magento\Quote\Model\QuoteRepository $quoteRepository,
         \Magento\Framework\Api\ExtensibleDataObjectConverter $extensibleDataObjectConverter,
         \Magento\Quote\Model\QuoteManagement $quoteManagement,
-        \Magento\Framework\Api\DataObjectHelper $dataObjectHelper
+        \Magento\Framework\Api\DataObjectHelper $dataObjectHelper,
+        \Magento\Quote\Model\Quote\TotalsCollector $totalsCollector
     ) {
         $this->_eventManager = $eventManager;
         $this->_customerUrl = $customerUrl;
@@ -249,6 +256,7 @@ public function __construct(
         $this->extensibleDataObjectConverter = $extensibleDataObjectConverter;
         $this->quoteManagement = $quoteManagement;
         $this->dataObjectHelper = $dataObjectHelper;
+        $this->totalsCollector = $totalsCollector;
     }
 
     /**
@@ -490,8 +498,8 @@ public function saveBilling($data, $customerAddressId)
                         ->setSameAsBilling(1)
                         ->setSaveInAddressBook(0)
                         ->setShippingMethod($shippingMethod)
-                        ->setCollectShippingRates(true)
-                        ->collectTotals();
+                        ->setCollectShippingRates(true);
+                    $this->totalsCollector->collectAddressTotals($this->getQuote(), $shipping);
 
                     if (!$this->isCheckoutMethodRegister()) {
                         $shipping->save();
@@ -690,7 +698,8 @@ public function saveShipping($data, $customerAddressId)
             return ['error' => 1, 'message' => $validateRes];
         }
 
-        $address->collectTotals()->save();
+        $this->totalsCollector->collectAddressTotals($this->getQuote(), $address);
+        $address->save();
 
         $this->getCheckout()->setStepData('shipping', 'complete', true)->setStepData('shipping_method', 'allow', true);
 

diff --git a/app/code/Magento/Checkout/Test/Unit/Block/Cart/AbstractCartTest.php b/app/code/Magento/Checkout/Test/Unit/Block/Cart/AbstractCartTest.php
@@ -109,4 +109,38 @@ public function testGetItemRendererThrowsExceptionForNonexistentRenderer()
 
         $block->getItemRenderer('some-type');
     }
+
+    /**
+     * @param array $expectedResult
+     * @param bool $isVirtual
+     * @dataProvider getTotalsCacheDataProvider
+     */
+    public function testGetTotalsCache($expectedResult, $isVirtual)
+    {
+        $totals = $isVirtual ? ['billing_totals'] : ['shipping_totals'];
+        $addressMock = $this->getMock('Magento\Quote\Model\Quote\Address', [], [], '', false);
+        $checkoutSessionMock = $this->getMock('Magento\Checkout\Model\Session', [], [], '', false);
+        $quoteMock = $this->getMock('Magento\Quote\Model\Quote', [], [], '', false);
+        $checkoutSessionMock->expects($this->once())->method('getQuote')->willReturn($quoteMock);
+
+        $quoteMock->expects($this->once())->method('isVirtual')->willReturn($isVirtual);
+        $quoteMock->expects($this->any())->method('getShippingAddress')->willReturn($addressMock);
+        $quoteMock->expects($this->any())->method('getBillingAddress')->willReturn($addressMock);
+        $addressMock->expects($this->once())->method('getTotals')->willReturn($totals);
+
+        /** @var \Magento\Checkout\Block\Cart\AbstractCart $model */
+        $model = $this->_objectManager->getObject(
+            'Magento\Checkout\Block\Cart\AbstractCart',
+            ['checkoutSession' => $checkoutSessionMock]
+        );
+        $this->assertEquals($expectedResult, $model->getTotalsCache());
+    }
+
+    public function getTotalsCacheDataProvider()
+    {
+        return [
+            [['billing_totals'], true],
+            [['shipping_totals'], false]
+        ];
+    }
 }
diff --git a/app/code/Magento/Checkout/Test/Unit/Model/ShippingInformationManagementTest.php b/app/code/Magento/Checkout/Test/Unit/Model/ShippingInformationManagementTest.php
@@ -57,6 +57,11 @@ class ShippingInformationManagementTest extends \PHPUnit_Framework_TestCase
      */
     protected $quoteMock;
 
+    /**
+     * @var \PHPUnit_Framework_MockObject_MockObject
+     */
+    protected $totalsCollectorMock;
+
     /**
      * @var \Magento\Checkout\Model\ShippingInformationManagement
      */
@@ -80,7 +85,8 @@ protected function setUp()
         $this->loggerMock = $this->getMock('\Psr\Log\LoggerInterface');
         $this->addressRepositoryMock = $this->getMock('\Magento\Customer\Api\AddressRepositoryInterface');
         $this->scopeConfigMock = $this->getMock('\Magento\Framework\App\Config\ScopeConfigInterface');
-
+        $this->totalsCollectorMock =
+            $this->getMock('Magento\Quote\Model\Quote\TotalsCollector', [], [], '', false);
         $this->model = $objectManager->getObject(
             '\Magento\Checkout\Model\ShippingInformationManagement',
             [
@@ -91,7 +97,8 @@ protected function setUp()
                 'addressValidator' => $this->addressValidatorMock,
                 'logger' => $this->loggerMock,
                 'addressRepository' => $this->addressRepositoryMock,
-                'scopeConfig' => $this->scopeConfigMock
+                'scopeConfig' => $this->scopeConfigMock,
+                'totalsCollector' => $this->totalsCollectorMock
             ]
         );
 
@@ -109,7 +116,6 @@ protected function setUp()
                 'getCountryId',
                 'importCustomerAddressData',
                 'save',
-                'collectTotals',
                 'getShippingRateByCode',
                 'getShippingMethod'
             ],
@@ -294,8 +300,10 @@ public function testSaveAddressInformationThrowExceptionWhileAddressSaving()
             ->with(true)
             ->willReturnSelf();
         $this->shippingAddressMock->expects($this->once())->method('getCountryId')->willReturn(1);
-        $this->shippingAddressMock->expects($this->once())->method('save')->willThrowException($exception);
-
+        $this->totalsCollectorMock
+            ->expects($this->once())
+            ->method('collectAddressTotals')
+            ->willThrowException($exception);
         $this->addressValidatorMock->expects($this->once())
             ->method('validate')
             ->with($this->shippingAddressMock)
@@ -386,8 +394,10 @@ public function testSaveAddressInformationIfCarrierCodeIsInvalid()
             ->with(true)
             ->willReturnSelf();
         $this->shippingAddressMock->expects($this->once())->method('getCountryId')->willReturn(1);
-        $this->shippingAddressMock->expects($this->once())->method('save')->willReturnSelf();
-        $this->shippingAddressMock->expects($this->once())->method('collectTotals')->willReturnSelf();
+        $this->totalsCollectorMock
+            ->expects($this->once())
+            ->method('collectAddressTotals')
+            ->with($this->quoteMock, $this->shippingAddressMock);
         $this->shippingAddressMock->expects($this->once())->method('getShippingMethod')->willReturn($shippingMethod);
         $this->shippingAddressMock->expects($this->once())
             ->method('getShippingRateByCode')
@@ -464,8 +474,10 @@ public function testSaveAddressInformationIfMinimumAmountIsNotValid()
             ->with(true)
             ->willReturnSelf();
         $this->shippingAddressMock->expects($this->once())->method('getCountryId')->willReturn(1);
-        $this->shippingAddressMock->expects($this->once())->method('save')->willReturnSelf();
-        $this->shippingAddressMock->expects($this->once())->method('collectTotals')->willReturnSelf();
+        $this->totalsCollectorMock
+            ->expects($this->once())
+            ->method('collectAddressTotals')
+            ->with($this->quoteMock, $this->shippingAddressMock);
         $this->shippingAddressMock->expects($this->once())->method('getShippingMethod')->willReturn($shippingMethod);
         $this->shippingAddressMock->expects($this->once())
             ->method('getShippingRateByCode')
@@ -506,7 +518,6 @@ public function testSaveAddressInformationIfCanNotSaveQuote()
             ->method('save')
             ->with($this->quoteMock)
             ->willThrowException($exception);
-
         $addressInformationMock = $this->getMock('\Magento\Checkout\Api\Data\ShippingInformationInterface');
         $addressInformationMock->expects($this->once())
             ->method('getShippingAddress')
@@ -550,8 +561,11 @@ public function testSaveAddressInformationIfCanNotSaveQuote()
             ->with(true)
             ->willReturnSelf();
         $this->shippingAddressMock->expects($this->once())->method('getCountryId')->willReturn(1);
-        $this->shippingAddressMock->expects($this->exactly(2))->method('save')->willReturnSelf();
-        $this->shippingAddressMock->expects($this->once())->method('collectTotals')->willReturnSelf();
+        $this->shippingAddressMock->expects($this->once())->method('save')->willReturnSelf();
+        $this->totalsCollectorMock
+            ->expects($this->once())
+            ->method('collectAddressTotals')
+            ->with($this->quoteMock, $this->shippingAddressMock);
         $this->shippingAddressMock->expects($this->once())->method('getShippingMethod')->willReturn($shippingMethod);
         $this->shippingAddressMock->expects($this->once())
             ->method('getShippingRateByCode')
@@ -626,8 +640,10 @@ public function testSaveAddressInformation()
             ->with(true)
             ->willReturnSelf();
         $this->shippingAddressMock->expects($this->once())->method('getCountryId')->willReturn(1);
-        $this->shippingAddressMock->expects($this->exactly(2))->method('save')->willReturnSelf();
-        $this->shippingAddressMock->expects($this->once())->method('collectTotals')->willReturnSelf();
+        $this->totalsCollectorMock
+            ->expects($this->once())
+            ->method('collectAddressTotals')
+            ->with($this->quoteMock, $this->shippingAddressMock);
         $this->shippingAddressMock->expects($this->once())->method('getShippingMethod')->willReturn($shippingMethod);
         $this->shippingAddressMock->expects($this->once())
             ->method('getShippingRateByCode')

diff --git a/app/code/Magento/Checkout/Test/Unit/Model/Type/OnepageTest.php b/app/code/Magento/Checkout/Test/Unit/Model/Type/OnepageTest.php
@@ -98,6 +98,9 @@ class OnepageTest extends \PHPUnit_Framework_TestCase
     /** @var \Magento\Framework\Api\ExtensibleDataObjectConverter|\PHPUnit_Framework_MockObject_MockObject */
     protected $extensibleDataObjectConverterMock;
 
+    /** @var \PHPUnit_Framework_MockObject_MockObject */
+    protected $totalsCollectorMock;
+
     /**
      * @SuppressWarnings(PHPMD.ExcessiveMethodLength)
      */
@@ -195,6 +198,7 @@ protected function setUp()
             ->method('toFlatArray')
             ->will($this->returnValue([]));
         $this->objectManagerHelper = new ObjectManagerHelper($this);
+        $this->totalsCollectorMock = $this->getMock('Magento\Quote\Model\Quote\TotalsCollector', [], [], '', false);
         $this->onepage = $this->objectManagerHelper->getObject(
             'Magento\Checkout\Model\Type\Onepage',
             [
@@ -222,7 +226,8 @@ protected function setUp()
                 'customerRepository' => $this->customerRepositoryMock,
                 'extensibleDataObjectConverter' => $this->extensibleDataObjectConverterMock,
                 'quoteRepository' => $this->quoteRepositoryMock,
-                'quoteManagement' => $this->quoteManagementMock
+                'quoteManagement' => $this->quoteManagementMock,
+                'totalsCollector' => $this->totalsCollectorMock
             ]
         );
     }
@@ -492,8 +497,17 @@ public function testSaveBilling(
             ->method('setCollectShippingRates')
             ->will($this->returnSelf());
 
-        $shippingAddressMock->expects($useForShipping ? $this->once() : $this->never())
-            ->method('collectTotals');
+        if ($useForShipping === \Magento\Checkout\Model\Type\Onepage::USE_FOR_SHIPPING) {
+            $this->totalsCollectorMock
+                ->expects($this->once())
+                ->method('collectAddressTotals')
+                ->with($quoteMock, $shippingAddressMock);
+        } else {
+            $this->totalsCollectorMock
+                ->expects($this->never())
+                ->method('collectAddressTotals')
+                ->with($quoteMock, $shippingAddressMock);
+        }
 
         $quoteMock->expects($this->any())->method('setPasswordHash')->with($passwordHash);
         $quoteMock->expects($this->any())->method('getCheckoutMethod')->will($this->returnValue($checkoutMethod));

diff --git a/app/code/Magento/Msrp/Block/Total.php b/app/code/Magento/Msrp/Block/Total.php
@@ -10,21 +10,31 @@
  */
 class Total extends \Magento\Framework\View\Element\Template
 {
-    /** @var \Magento\Msrp\Model\Config */
+    /**
+     * @var \Magento\Msrp\Model\Config
+     */
     protected $config;
 
+    /**
+     * @var \Magento\Msrp\Model\Quote\Msrp
+     */
+    protected $msrp;
+
     /**
      * @param \Magento\Framework\View\Element\Template\Context $context
      * @param \Magento\Msrp\Model\Config $config
+     * @param \Magento\Msrp\Model\Quote\Msrp $msrp
      * @param array $data
      */
     public function __construct(
         \Magento\Framework\View\Element\Template\Context $context,
         \Magento\Msrp\Model\Config $config,
+        \Magento\Msrp\Model\Quote\Msrp $msrp,
         array $data = []
     ) {
         parent::__construct($context, $data);
         $this->config = $config;
+        $this->msrp = $msrp;
     }
 
     /**
@@ -35,10 +45,10 @@ protected function _toHtml()
         /** @var \Magento\Checkout\Block\Cart\AbstractCart $originalBlock */
         $originalBlock = $this->getLayout()->getBlock($this->getOriginalBlockName());
         $quote = $originalBlock->getQuote();
-        if (!$quote->hasCanApplyMsrp() && $this->config->isEnabled()) {
+        if (!$this->msrp->getCanApplyMsrp($quote->getId()) && $this->config->isEnabled()) {
             $quote->collectTotals();
         }
-        if ($quote->getCanApplyMsrp()) {
+        if ($this->msrp->getCanApplyMsrp($quote->getId())) {
             $originalBlock->setTemplate('');
             return parent::_toHtml();
         } else {