[FIX][Core][Checkout] Adding the isEnabled filter to the countries list in checkout while addressing.

[christopherhero]

This change restricts the list of available countries in the checkout->addressing step to those which are enabled.
Currently, when completing the address in checkout, all countries that are assigned to a channel are displayed, regardless of whether the country is enabled (country.enabled).

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

[vvasiloi]

Hey @christopherhero, could you please also cover this:

Sylius/src/Sylius/Bundle/ApiBundle/Doctrine/QueryCollectionExtension/CountryCollectionExtension.php

Lines 53 to 57 in baa1311

	if ($channel->getCountries()->count() > 0) {
	$rootAlias = $queryBuilder->getRootAliases()[0];
	$queryBuilder
	->andWhere(sprintf('%s.id in (:%s)', $rootAlias, $countriesParameterName))
	->setParameter($countriesParameterName, $channel->getCountries())

[vvasiloi]

How about using criteria matching?

Suggested change

	return $this->getCountries()->filter(fn (CountryInterface $country): bool => true === $country->isEnabled());
	return $this->getCountries()->matching(new Criteria(Criteria::expr()->eq('enabled', true)));

[vvasiloi]

Right now the channel countries have eager fetch configured, but it might change in #14286

[diimpp]

How about using criteria matching?

Following simpler is better paradigm, filter might be more readable and more common occurrence in the source codes, there is only single matching() in the whole codebase.

return $zoneMembers->matching(Criteria::create()->where(Criteria::expr()->eq('code', $code)))->first();

Right now the channel countries have eager fetch configured, but it might change in #14286

Should it be replaced with $countryRepository->findEnabledForChannel instead?

[christopherhero]

Should it be replaced with $countryRepository->findEnabledForChannel instead?

What you mean ?

[christopherhero]

Thanks for PR @vvasiloi !

For me the filter is more consistent with the rest of the code.

[diimpp]

@christopherhero in case of bigger collection relations, it's more optimal to make repository call instead of processing collection directly, but that's more of theoretical question what should be done for EAGER removal, if it will happen.

[diimpp]

I've reproduced the issue, it can be re-tagged as bug. /cc @jakubtobiasz

1. Select few countries for a channel
   
2. Start checkout addressing step and see only previously selected countries as available countries list as expected.
   
3. Disable one of the countries, repeat checkout addressing step, it continues to show both enabled and disabled countries list instead of showing only enabled countries.
   
   

[christopherhero]

Hey @christopherhero, could you please also cover this:

Sylius/src/Sylius/Bundle/ApiBundle/Doctrine/QueryCollectionExtension/CountryCollectionExtension.php

Lines 53 to 57 in baa1311

	if ($channel->getCountries()->count() > 0) {
	$rootAlias = $queryBuilder->getRootAliases()[0];
	$queryBuilder
	->andWhere(sprintf('%s.id in (:%s)', $rootAlias, $countriesParameterName))
	->setParameter($countriesParameterName, $channel->getCountries())

@vvasiloi I will prepare another PR with the fix for the API.
In a few more places we could have used getEnabledCountries instead of getCountries but in this PR I focused solely on fixing this case described by @diimpp

[diimpp]

PR looks good code-wise, but I wonder should there be a scenario to cover those cases?

On separate note,

• Administrator might wonder why channel has a country, but it's not showing up in checkout
• What about address book, if previously saved country is no longer available, but address is applied to checkout? (Does address book respect countries list per channel at all?)
• Should address book page hide address for disabled country or indicate unavailability somehow else?
• Is there a validation for applied countries in checkout?
• How zones treat disabled countries?
• Already mentioned api checkout

It sounds like too many places needs to be modified to handle disabled counties within relation.
As an idea, what about disallowing disabling countries, if country is in use? It's a simpler coding solution, but not a great administrator experience, when country might be in use by tens of zones and several channels, one might want to exclude country with single click. (And still will require a solution for address book)

[github-actions]

Bunnyshell Preview Environment deployed

It will be automatically stopped in 4 hours.

Use the command /bns:start to start it if it's stopped.

Component	Endpoints
mailhog	https://mailhog-5a6ytu.bunnyenv.com/
php	https://store-5a6ytu.bunnyenv.com/

Available commands:

• /bns:stop to stop the environment
• /bns:deploy to redeploy the environment
• /bns:delete to remove the environment

[diimpp]

Since psalm was dropped from the project

Suggested change

	* @return Collection|CountryInterface[]
	*
	* @psalm-return Collection<array-key, CountryInterface>
	* @return Collection<array-key, CountryInterface>