bugfix #15250 [Admin][Country] Add violation for adding provinces with duplicated codes or names (NoResponseMate)

This PR was merged into the 1.12 branch.

Discussion
----------

| Q               | A                                                            |
|-----------------|--------------------------------------------------------------|
| Branch?         | 1.12 |
| Bug fix?        | yes                                                       |
| New feature?    | no                                                       |
| BC breaks?      | no                                                       |
| Deprecations?   | no |
| Related tickets | -                      |
| License         | MIT                                                          |

Commits
-------
  [Country] Add violation for adding provinces with duplicated codes
  [Behat][Admin][Country] Tests for saving duplicated provinces
  Update phpstan baseline
  [Country] Additional validation for duplicated provinces names
  [Behat][Admin][Country] Validation of saving duplicated province names

features/addressing/managing_countries/province_unique_code_validation.feature

@@ -1,18 +0,0 @@
-@managing_countries
-Feature: Province unique code validation
-    In order to uniquely identify provinces
-    As an Administrator
-    I want to be prevented from adding two provinces with the same code
-
-    Background:
-        Given the store has country "United Kingdom"
-        And this country has the "Northern Ireland" province with "GB-NIR" code
-        And I am logged in as an administrator
-
-    @ui @javascript @api
-    Scenario: Trying to add a new province with taken code
-        When I want to edit this country
-        And I add the "Scotland" province with "GB-NIR" code
-        And I try to save changes
-        Then I should be notified that province code must be unique
-        And province with name "Scotland" should not be added in this country

features/addressing/managing_countries/province_unique_fields_validation.feature

@@ -0,0 +1,41 @@
+@managing_countries
+Feature: Province unique fields validation
+    In order to uniquely identify provinces
+    As an Administrator
+    I want to be prevented from adding two provinces with the same code or name
+
+    Background:
+        Given the store has country "United Kingdom"
+        And this country has the "Northern Ireland" province with "GB-NIR" code
+        And I am logged in as an administrator
+
+    @ui @javascript @api
+    Scenario: Trying to add a new province with a taken code
+        When I want to add a new country
+        And I choose "Gibraltar"
+        And I add the "Scotland" province with "GB-NIR" code
+        And I try to add it
+        Then I should be notified that province code must be unique
+
+    @ui @javascript @api
+    Scenario: Trying to add a new province with a taken name
+        When I want to edit this country
+        And I add the "Northern Ireland" province with "GB-NI" code
+        And I try to save changes
+        Then I should be notified that province name must be unique
+
+    @ui @javascript @api
+    Scenario: Trying to add new provinces with duplicated codes
+        When I want to edit this country
+        And I add the "Scotland" province with "GB-SCO" code
+        And I add the "Not Scotland" province with "GB-SCO" code
+        And I try to save changes
+        Then I should be notified that all province codes and names within this country need to be unique
+
+    @ui @javascript @api
+    Scenario: Trying to add new provinces with duplicated names
+        When I want to edit this country
+        And I add the "Scotland" province with "GB-SC" code
+        And I add the "Scotland" province with "GB-SCO" code
+        And I try to save changes
+        Then I should be notified that all province codes and names within this country need to be unique

phpstan-baseline.neon

@@ -70,6 +70,11 @@ parameters:
 			count: 1
 			path: src/Sylius/Bundle/AddressingBundle/Validator/Constraints/ProvinceAddressConstraintValidator.php
 
+		-
+			message: "#^Method Sylius\\\\Bundle\\\\AddressingBundle\\\\Validator\\\\Constraints\\\\UniqueProvinceCollectionValidator\\:\\:validate\\(\\) has parameter \\$value with no type specified\\.$#"
+			count: 1
+			path: src/Sylius/Bundle/AddressingBundle/Validator/Constraints/UniqueProvinceCollectionValidator.php
+
 		-
 			message: "#^Method Sylius\\\\Bundle\\\\AddressingBundle\\\\Validator\\\\Constraints\\\\ZoneCannotContainItselfValidator\\:\\:validate\\(\\) has parameter \\$value with no type specified\\.$#"
 			count: 1

src/Sylius/Behat/Context/Api/Admin/ManagingCountriesContext.php

@@ -52,6 +52,7 @@ final class ManagingCountriesContext implements Context
 
     /**
      * @When I add it
+     * @When I try to add it
      */
     public function iAddIt(): void
     {
@@ -312,13 +313,24 @@ final class ManagingCountriesContext implements Context
     }
 
     /**
-     * @Then I should be notified that province code must be unique
+     * @Then /^I should be notified that province (code|name) must be unique$/
      */
-    public function iShouldBeNotifiedThatProvinceCodeMustBeUnique(): void
+    public function iShouldBeNotifiedThatProvinceCodeMustBeUnique(string $field): void
     {
-        Assert::same(
+        Assert::regex(
             $this->responseChecker->getError($this->client->getLastResponse()),
-            'provinces[1].code: Province code must be unique.',
+            sprintf('/provinces\[[\d+]\]\.%1$s: Province %1$s must be unique\./', $field),
+        );
+    }
+
+    /**
+     * @Then I should be notified that all province codes and names within this country need to be unique
+     */
+    public function iShouldBeNotifiedThatAllProvinceCodesAndNamesWithinThisCountryNeedToBeUnique(): void
+    {
+        Assert::contains(
+            $this->responseChecker->getError($this->client->getLastResponse()),
+            'provinces: All provinces within this country need to have unique codes and names.',
         );
     }
 

src/Sylius/Behat/Context/Ui/Admin/ManagingCountriesContext.php

@@ -74,6 +74,7 @@ final class ManagingCountriesContext implements Context
 
     /**
      * @When I add it
+     * @When I try to add it
      */
     public function iAddIt()
     {
@@ -265,11 +266,22 @@ final class ManagingCountriesContext implements Context
     }
 
     /**
-     * @Then I should be notified that province code must be unique
+     * @Then /^I should be notified that province (code|name) must be unique$/
      */
-    public function iShouldBeNotifiedThatProvinceCodeMustBeUnique(): void
+    public function iShouldBeNotifiedThatProvinceCodeMustBeUnique(string $field): void
     {
-        Assert::same($this->updatePage->getValidationMessage('code'), 'Province code must be unique.');
+        Assert::same($this->updatePage->getValidationMessage($field), sprintf('Province %s must be unique.', $field));
+    }
+
+    /**
+     * @Then I should be notified that all province codes and names within this country need to be unique
+     */
+    public function iShouldBeNotifiedThatAllProvinceCodesAndNamesWithinThisCountryNeedToBeUnique(): void
+    {
+        Assert::inArray(
+            'All provinces within this country need to have unique codes and names.',
+            $this->updatePage->getFormValidationErrors(),
+        );
     }
 
     /**

src/Sylius/Behat/Page/Admin/Country/UpdatePage.php

@@ -105,6 +105,13 @@ class UpdatePage extends BaseUpdatePage implements UpdatePageInterface
         $provinceForm->fillField('Code', $code);
     }
 
+    public function getFormValidationErrors(): array
+    {
+        $errors = $this->getElement('form')->findAll('css', '.sylius-validation-error:not(.pointing)');
+
+        return array_map(fn (NodeElement $element) => $element->getText(), $errors);
+    }
+
     public function getValidationMessage(string $element): string
     {
         $provinceForm = $this->getLastProvinceElement();
@@ -127,6 +134,7 @@ class UpdatePage extends BaseUpdatePage implements UpdatePageInterface
         return array_merge(parent::getDefinedElements(), [
             'code' => '#sylius_country_code',
             'enabled' => '#sylius_country_enabled',
+            'form' => 'form',
             'provinces' => '#sylius_country_provinces',
         ]);
     }

src/Sylius/Behat/Page/Admin/Country/UpdatePageInterface.php

@@ -38,4 +38,7 @@ interface UpdatePageInterface extends BaseUpdatePageInterface
     public function removeProvinceName(string $provinceName): void;
 
     public function specifyProvinceCode(string $provinceCode): void;
+
+    /** @return array<array-key, string> */
+    public function getFormValidationErrors(): array;
 }

src/Sylius/Bundle/AddressingBundle/Resources/config/services.xml

@@ -62,5 +62,9 @@
         <service id="sylius.validator.zone_cannot_contain_itself" class="Sylius\Bundle\AddressingBundle\Validator\Constraints\ZoneCannotContainItselfValidator">
             <tag name="validator.constraint_validator" alias="sylius_zone_cannot_contain_itself_validator" />
         </service>
+
+        <service id="sylius.validator.unique_province_collection" class="Sylius\Bundle\AddressingBundle\Validator\Constraints\UniqueProvinceCollectionValidator">
+            <tag name="validator.constraint_validator" alias="sylius_unique_province_collection_validator" />
+        </service>
     </services>
 </container>

src/Sylius/Bundle/AddressingBundle/Resources/config/validation/Country.xml

@@ -36,6 +36,9 @@
             </constraint>
         </property>
         <property name="provinces">
+            <constraint name="Sylius\Bundle\AddressingBundle\Validator\Constraints\UniqueProvinceCollection">
+                <option name="groups">sylius</option>
+            </constraint>
             <constraint name="Valid" />
         </property>
     </class>

src/Sylius/Bundle/AddressingBundle/Resources/translations/validators.en.yml

@@ -31,6 +31,7 @@ sylius:
             not_blank: Please enter country ISO code.
             regex: Country code can only be comprised of letters, numbers, dashes and underscores.
             unique: Country ISO code must be unique.
+        unique_provinces: All provinces within this country need to have unique codes and names.
     province:
         code:
             min_length: Province code must be at least 5 characters long|Province code must be at least 5 characters long.

src/Sylius/Bundle/AddressingBundle/Validator/Constraints/UniqueProvinceCollection.php

@@ -0,0 +1,17 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Sylius\Bundle\AddressingBundle\Validator\Constraints;
+
+use Symfony\Component\Validator\Constraint;
+
+final class UniqueProvinceCollection extends Constraint
+{
+    public string $message = 'sylius.country.unique_provinces';
+
+    public function validatedBy(): string
+    {
+        return 'sylius_unique_province_collection_validator';
+    }
+}

src/Sylius/Bundle/AddressingBundle/Validator/Constraints/UniqueProvinceCollectionValidator.php

@@ -0,0 +1,46 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Sylius\Bundle\AddressingBundle\Validator\Constraints;
+
+use Doctrine\Common\Collections\Collection;
+use Sylius\Component\Addressing\Model\ProvinceInterface;
+use Symfony\Component\Validator\Constraint;
+use Symfony\Component\Validator\ConstraintValidator;
+use Webmozart\Assert\Assert;
+
+final class UniqueProvinceCollectionValidator extends ConstraintValidator
+{
+    public function validate($value, Constraint $constraint): void
+    {
+        /** @var Collection<array-key, ProvinceInterface> $value */
+        Assert::allIsInstanceOf($value, ProvinceInterface::class);
+        /** @var UniqueProvinceCollection $constraint */
+        Assert::isInstanceOf($constraint, UniqueProvinceCollection::class);
+
+        if ($value->isEmpty()) {
+            return;
+        }
+
+        $provincesWithAnyRequiredData = $value->filter(
+            fn(ProvinceInterface $province): bool => null !== $province->getCode() || null !== $province->getName()
+        );
+
+        $codes = [];
+        $names = [];
+        foreach ($provincesWithAnyRequiredData as $province) {
+            $code = $province->getCode();
+            $name = $province->getName();
+
+            if (isset($code) && in_array($code, $codes) || isset($name) && in_array($name, $names)) {
+                $this->context->addViolation($constraint->message);
+
+                return;
+            }
+
+            $codes[] = $code;
+            $names[] = $name;
+        }
+    }
+}

src/Sylius/Bundle/AddressingBundle/spec/Validator/Constraints/UniqueProvinceCollectionValidatorSpec.php

@@ -0,0 +1,199 @@
+<?php
+
+/*
+ * This file is part of the Sylius package.
+ *
+ * (c) Sylius Sp. z o.o.
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+declare(strict_types=1);
+
+namespace spec\Sylius\Bundle\AddressingBundle\Validator\Constraints;
+
+use Doctrine\Common\Collections\ArrayCollection;
+use PhpSpec\ObjectBehavior;
+use Prophecy\Argument;
+use Sylius\Bundle\AddressingBundle\Validator\Constraints\UniqueProvinceCollection;
+use Sylius\Component\Addressing\Model\ProvinceInterface;
+use Symfony\Component\Validator\Constraint;
+use Symfony\Component\Validator\ConstraintValidatorInterface;
+use Symfony\Component\Validator\Context\ExecutionContextInterface;
+
+final class UniqueProvinceCollectionValidatorSpec extends ObjectBehavior
+{
+    function let(ExecutionContextInterface $executionContext): void
+    {
+        $this->beConstructedWith();
+        $this->initialize($executionContext);
+    }
+
+    function it_is_a_constraint_validator(): void
+    {
+        $this->shouldImplement(ConstraintValidatorInterface::class);
+    }
+
+    function it_throws_exception_when_value_is_not_a_collection(): void
+    {
+        $this
+            ->shouldThrow(\InvalidArgumentException::class)
+            ->during('validate', [
+                new \stdClass(),
+                new UniqueProvinceCollection(),
+            ])
+        ;
+    }
+
+    function it_throws_exception_when_collection_contains_something_other_than_provinces(): void
+    {
+        $this
+            ->shouldThrow(\InvalidArgumentException::class)
+            ->during('validate', [
+                new ArrayCollection([new \stdClass()]),
+                new UniqueProvinceCollection(),
+            ])
+        ;
+    }
+
+    function it_throws_exception_when_constraint_is_not_a_unique_province_collection_codes(
+        Constraint $constraint,
+    ): void {
+        $this
+            ->shouldThrow(\InvalidArgumentException::class)
+            ->during('validate', [
+                new ArrayCollection(),
+                $constraint,
+            ])
+        ;
+    }
+
+    function it_does_nothing_when_collection_is_empty(ExecutionContextInterface $context): void
+    {
+        $context->addViolation(Argument::any())->shouldNotBeCalled();
+
+        $this->validate(new ArrayCollection(), new UniqueProvinceCollection());
+    }
+
+    function it_does_nothing_when_all_provinces_have_unique_codes(
+        ExecutionContextInterface $executionContext,
+        ProvinceInterface $firstProvince,
+        ProvinceInterface $secondProvince,
+    ): void {
+        $firstProvince->getCode()->willReturn('first');
+        $firstProvince->getName()->willReturn('first');
+        $secondProvince->getCode()->willReturn('second');
+        $secondProvince->getName()->willReturn('second');
+
+        $executionContext->addViolation(Argument::any())->shouldNotBeCalled();
+
+        $this->validate(
+            new ArrayCollection([
+                $firstProvince->getWrappedObject(),
+                $secondProvince->getWrappedObject(),
+            ]),
+            new UniqueProvinceCollection(),
+        );
+    }
+
+    function it_checks_uniqueness_with_incomplete_codes(
+        ExecutionContextInterface $executionContext,
+        ProvinceInterface $province,
+        ProvinceInterface $sameProvinceWithCode,
+    ): void {
+        $constraint = new UniqueProvinceCollection();
+
+        $province->getCode()->willReturn(null);
+        $province->getName()->willReturn('name');
+        $sameProvinceWithCode->getCode()->willReturn('code');
+        $sameProvinceWithCode->getName()->willReturn('name');
+
+        $executionContext->addViolation($constraint->message)->shouldBeCalled();
+
+        $this->validate(
+            new ArrayCollection([
+                $province->getWrappedObject(),
+                $sameProvinceWithCode->getWrappedObject(),
+            ]),
+            $constraint,
+        );
+    }
+
+    function it_checks_uniqueness_with_incomplete_names(
+        ExecutionContextInterface $executionContext,
+        ProvinceInterface $province,
+        ProvinceInterface $sameProvinceWithName,
+    ): void {
+        $constraint = new UniqueProvinceCollection();
+
+        $province->getCode()->willReturn('code');
+        $province->getName()->willReturn(null);
+        $sameProvinceWithName->getCode()->willReturn('code');
+        $sameProvinceWithName->getName()->willReturn('name');
+
+        $executionContext->addViolation($constraint->message)->shouldBeCalled();
+
+        $this->validate(
+            new ArrayCollection([
+                $province->getWrappedObject(),
+                $sameProvinceWithName->getWrappedObject(),
+            ]),
+            $constraint,
+        );
+    }
+
+    function it_adds_violation_when_codes_are_duplicated(
+        ExecutionContextInterface $executionContext,
+        ProvinceInterface $firstProvince,
+        ProvinceInterface $secondProvince,
+        ProvinceInterface $thirdProvince,
+    ): void {
+        $constraint = new UniqueProvinceCollection();
+
+        $firstProvince->getCode()->willReturn('same');
+        $firstProvince->getName()->willReturn('first');
+        $secondProvince->getCode()->willReturn('same');
+        $secondProvince->getName()->willReturn('second');
+        $thirdProvince->getCode()->willReturn('different');
+        $thirdProvince->getName()->willReturn('third');
+
+        $executionContext->addViolation($constraint->message)->shouldBeCalled();
+
+        $this->validate(
+            new ArrayCollection([
+                $firstProvince->getWrappedObject(),
+                $secondProvince->getWrappedObject(),
+                $thirdProvince->getWrappedObject(),
+            ]),
+            $constraint,
+        );
+    }
+
+    function it_adds_violation_when_names_are_duplicated(
+        ExecutionContextInterface $executionContext,
+        ProvinceInterface $firstProvince,
+        ProvinceInterface $secondProvince,
+        ProvinceInterface $thirdProvince,
+    ): void {
+        $constraint = new UniqueProvinceCollection();
+
+        $firstProvince->getCode()->willReturn('first');
+        $firstProvince->getName()->willReturn('first');
+        $secondProvince->getCode()->willReturn('second');
+        $secondProvince->getName()->willReturn('same');
+        $thirdProvince->getCode()->willReturn('third');
+        $thirdProvince->getName()->willReturn('same');
+
+        $executionContext->addViolation($constraint->message)->shouldBeCalled();
+
+        $this->validate(
+            new ArrayCollection([
+                $firstProvince->getWrappedObject(),
+                $secondProvince->getWrappedObject(),
+                $thirdProvince->getWrappedObject(),
+            ]),
+            $constraint,
+        );
+    }
+}

src/Sylius/Bundle/AdminBundle/Resources/views/Country/_form.html.twig

@@ -1,5 +1,5 @@
+{{ form_errors(form) }}
 <div class="ui segment">
-    {{ form_errors(form) }}
     {{ form_row(form.code) }}
     {{ form_row(form.enabled) }}
 </div>