Resolve conflict after upmerging changing processors priorities

popatkaran · Mar 18, 2022 · e6017a5 · e6017a5
2 parents 42703d7 + 6c674b2
commit e6017a5
Show file tree

Hide file tree

Showing 9 changed files with 209 additions and 11 deletions.
diff --git a/UPGRADE-1.10.md b/UPGRADE-1.10.md
@@ -1,3 +1,29 @@
+# UPGRADE FROM `v1.10.x` TO `v1.10.12`
+1. Order Processors' priorities have changed and `sylius.order_processing.order_prices_recalculator` has now a higher priority than `sylius.order_processing.order_shipment_processor`.
+
+Previous priorities:
+```shell
+sylius.order_processing.order_adjustments_clearer          60         Sylius\Component\Core\OrderProcessing\OrderAdjustmentsClearer   
+sylius.order_processing.order_shipment_processor           50         Sylius\Component\Core\OrderProcessing\OrderShipmentProcessor    
+sylius.order_processing.order_prices_recalculator          40         Sylius\Component\Core\OrderProcessing\OrderPricesRecalculator   
+...     
+```
+
+Current priorities:
+```shell
+sylius.order_processing.order_adjustments_clearer          60         Sylius\Component\Core\OrderProcessing\OrderAdjustmentsClearer   
+sylius.order_processing.order_prices_recalculator          50         Sylius\Component\Core\OrderProcessing\OrderPricesRecalculator   
+sylius.order_processing.order_shipment_processor           40         Sylius\Component\Core\OrderProcessing\OrderShipmentProcessor    
+...     
+```
+
+If you rely on previous priorities, you can bring them back by setting flag ``sylius_core.process_shipments_before_recalculating_prices`` to ``true`` in ``config/packages/_sylius.yaml``:
+```yaml
+sylius_core:
+    process_shipments_before_recalculating_prices: true
+```
+However, it is not recommended because new priorities fix [invalid estimated shipping costs](https://github.com/Sylius/Sylius/pull/13769).
+
 # UPGRADE FROM `v1.10.8` TO `v1.10.10`
 
 1. Field `createdByGuest` has been added to `Sylius\Component\Core\Model\Order`, this change will allow us to distinguish carts 

diff --git a/...pplying_shipping_method_rules/seeing_estimated_shipping_cost_based_on_order_total.feature b/...pplying_shipping_method_rules/seeing_estimated_shipping_cost_based_on_order_total.feature
@@ -0,0 +1,25 @@
+@applying_shipping_method_rules
+Feature: Seeing estimated shipping costs based on order total
+    In order to be aware of estimated shipping costs
+    As a Customer
+    I want to see estimated shipping costs that match the shipping method rule
+
+    Background:
+        Given the store operates on a single channel in "United States"
+        And the store has a product "Cheap Jacket" priced at "$20.00"
+        And the store has a product "Expensive Jacket" priced at "$200.00"
+        And the store has "Ship with us, ship with pride" shipping method with "$200" fee
+        And this shipping method is only available for orders over or equal to "$30"
+        And the store has "We delivery cheap goodz" shipping method with "$2" fee
+        And this shipping method is only available for orders under or equal to "$29.99"
+        And I am a logged in customer
+
+    @ui @api
+    Scenario: Seeing valid estimated shipping cost for the cart with a value over minimum price configured on the shipping method
+        When I add product "Expensive Jacket" to the cart
+        Then my cart estimated shipping cost should be "$200.00"
+
+    @ui @api
+    Scenario: Seeing valid estimated shipping cost for the cart with a value under maximum price configured on the shipping method
+        When I add product "Cheap Jacket" to the cart
+        Then my cart estimated shipping cost should be "$2.00"
diff --git a/...plying_shipping_method_rules/seeing_estimated_shipping_cost_based_on_total_weight.feature b/...plying_shipping_method_rules/seeing_estimated_shipping_cost_based_on_total_weight.feature
@@ -0,0 +1,27 @@
+@applying_shipping_method_rules
+Feature: Seeing estimated shipping costs based on total weight
+    In order to be aware of estimated shipping costs
+    As a Customer
+    I want to see estimated shipping costs that match the shipping method rule
+
+    Background:
+        Given the store operates on a single channel in "United States"
+        And the store has a product "Jacket for the Lochness Monster" priced at "$1337.00"
+        And this product's weight is 200
+        And the store has a product "T-Shirt for Tinkerbell" priced at "$1.00"
+        And this product's weight is 0.1
+        And the store has "Heavy Duty Courier" shipping method with "$200" fee
+        And this shipping method is only available for orders with a total weight greater or equal to 100.0
+        And the store has "Fairytale Delivery Service" shipping method with "$2" fee
+        And this shipping method is only available for orders with a total weight less or equal to 1.0
+        And I am a logged in customer
+
+    @ui @api
+    Scenario: Seeing valid estimated shipping cost for the cart with a total weight over minimum total weight configured on the shipping method
+        When I add product "Jacket for the Lochness Monster" to the cart
+        Then my cart estimated shipping cost should be "$200.00"
+
+    @ui @api
+    Scenario: Seeing valid estimated shipping cost for the cart with a total weight under maximum total weight configured on the shipping method
+        When I add product "T-Shirt for Tinkerbell" to the cart
+        Then my cart estimated shipping cost should be "$2.00"
diff --git a/src/Sylius/Behat/Context/Api/Shop/CartContext.php b/src/Sylius/Behat/Context/Api/Shop/CartContext.php
@@ -275,9 +275,9 @@ public function myCartLocaleShouldBe(LocaleInterface $locale): void
     {
         Assert::same(
             $this->responseChecker->getValue(
-            $this->cartsClient->getLastResponse(),
-            'localeCode'
-        ),
+                $this->cartsClient->getLastResponse(),
+                'localeCode'
+            ),
             $locale->getCode()
         );
     }
@@ -308,7 +308,7 @@ public function myCartShouldBeCleared(): void
      * @Then /^my (cart) total should be ("[^"]+")$/
      * @Then /^the (cart) total should be ("[^"]+")$/
      */
-    public function myCartSTotalShouldBe(string $tokenValue, int $total): void
+    public function myCartTotalShouldBe(string $tokenValue, int $total): void
     {
         $response = $this->cartsClient->show($tokenValue);
         $responseTotal = $this->responseChecker->getValue(
@@ -602,6 +602,7 @@ public function myCartShouldHaveItems(int $quantity, ProductInterface $product):
     /**
      * @Then /^my cart shipping total should be ("[^"]+")$/
      * @Then I should not see shipping total for my cart
+     * @Then /^my cart estimated shipping cost should be ("[^"]+")$/
      */
     public function myCartShippingFeeShouldBe(int $shippingTotal = 0): void
     {

diff --git a/src/Sylius/Bundle/CoreBundle/DependencyInjection/Configuration.php b/src/Sylius/Bundle/CoreBundle/DependencyInjection/Configuration.php
@@ -15,7 +15,6 @@
 
 use Sylius\Bundle\CoreBundle\Controller\ProductTaxonController;
 use Sylius\Bundle\CoreBundle\Doctrine\ORM\AvatarImageRepository;
-use Sylius\Bundle\CoreBundle\Doctrine\ORM\ChannelPricingRepository;
 use Sylius\Bundle\CoreBundle\Form\Type\Product\ChannelPricingType;
 use Sylius\Bundle\CoreBundle\Form\Type\ShopBillingDataType;
 use Sylius\Bundle\ResourceBundle\Controller\ResourceController;
@@ -51,6 +50,7 @@ public function getConfigTreeBuilder(): TreeBuilder
                 ->scalarNode('driver')->defaultValue(SyliusResourceBundle::DRIVER_DOCTRINE_ORM)->end()
                 ->booleanNode('prepend_doctrine_migrations')->defaultTrue()->end()
                 ->booleanNode('shipping_address_based_taxation')->defaultFalse()->end()
+                ->booleanNode('process_shipments_before_recalculating_prices')->defaultFalse()->end()
                 ->arrayNode('catalog_promotions')
                     ->addDefaultsIfNotSet()
                     ->children()
@@ -101,7 +101,7 @@ private function addResourcesSection(ArrayNodeDefinition $node): void
                                     ->addDefaultsIfNotSet()
                                     ->children()
                                         ->scalarNode('model')->defaultValue(AvatarImage::class)->cannotBeEmpty()->end()
-										->scalarNode('interface')->defaultValue(AvatarImageInterface::class)->cannotBeEmpty()->end()
+                                        ->scalarNode('interface')->defaultValue(AvatarImageInterface::class)->cannotBeEmpty()->end()
                                         ->scalarNode('repository')->defaultValue(AvatarImageRepository::class)->cannotBeEmpty()->end()
                                         ->scalarNode('factory')->defaultValue(Factory::class)->end()
                                     ->end()

diff --git a/src/Sylius/Bundle/CoreBundle/DependencyInjection/SyliusCoreExtension.php b/src/Sylius/Bundle/CoreBundle/DependencyInjection/SyliusCoreExtension.php
@@ -16,6 +16,7 @@
 use Sylius\Bundle\ResourceBundle\DependencyInjection\Extension\AbstractResourceExtension;
 use Symfony\Component\Config\FileLocator;
 use Symfony\Component\DependencyInjection\ContainerBuilder;
+use Symfony\Component\DependencyInjection\Definition;
 use Symfony\Component\DependencyInjection\Extension\PrependExtensionInterface;
 use Symfony\Component\DependencyInjection\Loader\XmlFileLoader;
 
@@ -60,6 +61,13 @@ public function load(array $configs, ContainerBuilder $container): void
         if ('test' === $env || 'test_cached' === $env) {
             $loader->load('test_services.xml');
         }
+
+        if ($config['process_shipments_before_recalculating_prices']) {
+            $this->switchOrderProcessorsPriorities(
+                $container->getDefinition('sylius.order_processing.order_shipment_processor'),
+                $container->getDefinition('sylius.order_processing.order_prices_recalculator')
+            );
+        }
     }
 
     public function prepend(ContainerBuilder $container): void
@@ -138,4 +146,24 @@ private function prependJmsSerializerIfAdminApiBundleIsNotPresent(ContainerBuild
             ],
         ]);
     }
+
+    private function switchOrderProcessorsPriorities(
+        Definition $firstServiceDefinition,
+        Definition $secondServiceDefinition
+    ) {
+        $firstServicePriority = $firstServiceDefinition->getTag('sylius.order_processor')[0]['priority'];
+        $secondServicePriority = $secondServiceDefinition->getTag('sylius.order_processor')[0]['priority'];
+
+        $firstServiceDefinition->clearTag('sylius.order_processor');
+        $secondServiceDefinition->clearTag('sylius.order_processor');
+
+        $firstServiceDefinition->addTag(
+            'sylius.order_processor',
+            ['priority' => $secondServicePriority]
+        );
+        $secondServiceDefinition->addTag(
+            'sylius.order_processor',
+            ['priority' => $firstServicePriority]
+        );
+    }
 }
diff --git a/src/Sylius/Bundle/CoreBundle/Resources/config/services/order_processing.xml b/src/Sylius/Bundle/CoreBundle/Resources/config/services/order_processing.xml
@@ -27,15 +27,15 @@
             <tag name="sylius.order_processor" priority="60"/>
         </service>
 
+        <service id="sylius.order_processing.order_prices_recalculator" class="Sylius\Component\Core\OrderProcessing\OrderPricesRecalculator">
+            <argument type="service" id="sylius.calculator.product_variant_price" />
+            <tag name="sylius.order_processor" priority="50"/>
+        </service>
+
         <service id="sylius.order_processing.order_shipment_processor" class="Sylius\Component\Core\OrderProcessing\OrderShipmentProcessor">
             <argument type="service" id="sylius.shipping_method_resolver.default" />
             <argument type="service" id="sylius.factory.shipment" />
             <argument type="service" id="sylius.shipping_methods_resolver" />
-            <tag name="sylius.order_processor" priority="50"/>
-        </service>
-
-        <service id="sylius.order_processing.order_prices_recalculator" class="Sylius\Component\Core\OrderProcessing\OrderPricesRecalculator">
-            <argument type="service" id="sylius.calculator.product_variant_price" />
             <tag name="sylius.order_processor" priority="40"/>
         </service>
 

diff --git a/src/Sylius/Bundle/CoreBundle/Tests/DependencyInjection/SyliusCoreConfigurationTest.php b/src/Sylius/Bundle/CoreBundle/Tests/DependencyInjection/SyliusCoreConfigurationTest.php
@@ -0,0 +1,62 @@
+<?php
+
+/*
+ * This file is part of the Sylius package.
+ *
+ * (c) Paweł Jędrzejewski
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+declare(strict_types=1);
+
+namespace Sylius\Bundle\CoreBundle\Tests\DependencyInjection;
+
+use Matthias\SymfonyConfigTest\Partial\PartialProcessor;
+use Matthias\SymfonyConfigTest\PhpUnit\ConfigurationTestCaseTrait;
+use PHPUnit\Framework\TestCase;
+use Sylius\Bundle\CoreBundle\DependencyInjection\Configuration;
+use Symfony\Component\Config\Definition\Exception\InvalidTypeException;
+
+final class SyliusCoreConfigurationTest extends TestCase
+{
+    use ConfigurationTestCaseTrait;
+
+    /** @test */
+    public function it_does_not_bring_back_previous_priorities_for_order_processing_by_default(): void
+    {
+        $this->assertProcessedConfigurationEquals(
+            [[]],
+            ['process_shipments_before_recalculating_prices' => false],
+            'process_shipments_before_recalculating_prices',
+        );
+    }
+
+    /** @test */
+    public function it_allows_to_define_that_previous_priorities_should_be_brought_back_for_order_processing(): void
+    {
+        $this->assertProcessedConfigurationEquals(
+            [['process_shipments_before_recalculating_prices' => true]],
+            ['process_shipments_before_recalculating_prices' => true],
+            'process_shipments_before_recalculating_prices',
+        );
+    }
+
+    /** @test */
+    public function it_does_not_allow_to_define_previous_priorities_with_values_other_then_bool(): void
+    {
+        $this->expectException(InvalidTypeException::class);
+
+        (new PartialProcessor())->processConfiguration(
+            $this->getConfiguration(),
+            'process_shipments_before_recalculating_prices',
+            [['process_shipments_before_recalculating_prices' => 'yolo']]
+        );
+    }
+
+    protected function getConfiguration(): Configuration
+    {
+        return new Configuration();
+    }
+}
diff --git a/src/Sylius/Bundle/CoreBundle/Tests/DependencyInjection/SyliusCoreExtensionTest.php b/src/Sylius/Bundle/CoreBundle/Tests/DependencyInjection/SyliusCoreExtensionTest.php
@@ -15,11 +15,40 @@
 
 use Doctrine\Bundle\MigrationsBundle\DependencyInjection\DoctrineMigrationsExtension;
 use Matthias\SymfonyDependencyInjectionTest\PhpUnit\AbstractExtensionTestCase;
+use Matthias\SymfonyDependencyInjectionTest\PhpUnit\DefinitionHasTagConstraint;
 use Sylius\Bundle\CoreBundle\DependencyInjection\SyliusCoreExtension;
 use SyliusLabs\DoctrineMigrationsExtraBundle\DependencyInjection\SyliusLabsDoctrineMigrationsExtraExtension;
 
 final class SyliusCoreExtensionTest extends AbstractExtensionTestCase
 {
+    /** @test */
+    public function it_brings_back_previous_order_processing_priorities(): void
+    {
+        $this->container->setParameter('kernel.environment', 'dev');
+
+        $this->load(['process_shipments_before_recalculating_prices' => true]);
+
+        $this->assertThat(
+            $this->container->findDefinition('sylius.order_processing.order_prices_recalculator'),
+            new DefinitionHasTagConstraint('sylius.order_processor', ['priority' => 40])
+        );
+
+        $this->assertThat(
+            $this->container->findDefinition('sylius.order_processing.order_prices_recalculator'),
+            $this->logicalNot(new DefinitionHasTagConstraint('sylius.order_processor', ['priority' => 50]))
+        );
+
+        $this->assertThat(
+            $this->container->findDefinition('sylius.order_processing.order_shipment_processor'),
+            new DefinitionHasTagConstraint('sylius.order_processor', ['priority' => 50])
+        );
+
+        $this->assertThat(
+            $this->container->findDefinition('sylius.order_processing.order_shipment_processor'),
+            $this->logicalNot(new DefinitionHasTagConstraint('sylius.order_processor', ['priority' => 40]))
+        );
+    }
+
     /** @test */
     public function it_autoconfigures_prepending_doctrine_migrations_with_proper_migrations_path_for_test_env(): void
     {