Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug][Channel] The hostname resolver does not check for the channel status #13697

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions UPGRADE-1.10.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,10 @@ between guests and logged in customers.
2. Not passing `createdByGuestFlagResolver` through constructor in `Sylius\Component\Core\Cart\Context\ShopBasedCartContext`
is deprecated in Sylius 1.10.9 and it will be prohibited in Sylius 2.0.

# UPGRADE FROM `v1.10.9` TO `v1.10.10`

1. The ChannelRepository findOneByHostname has been deprecated, consider replace it with the new findOneEnabledByHostname method.

# UPGRADE FROM `v1.10.x` TO `v1.10.8`

1. Update `payum/payum` to `^1.7` and execute Doctrine Migrations
Expand Down
15 changes: 15 additions & 0 deletions features/channel/browsing_shop_on_right_channel.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
@channels
Feature: Browsing shop on right channel
In order to shop
As a Customer
I want to be redirected to the right channel
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should not be able to browse disabled shop?


Background:
Given the store operates on a channel named "Default" in "USD" currency and with hostname "127.0.0.1"
And the store also operates on a channel named "Alternative" in "USD" currency and with hostname "127.0.0.1"
And the channel "Default" is disabled

@ui
Scenario: Browsing shop in active channel
When I visit the homepage
Then I should be on channel "Alternative"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should brows "Alternative" shop? "Channel" is not a concept Customer should be aware of 🚀

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right!

16 changes: 16 additions & 0 deletions src/Sylius/Behat/Context/Ui/ChannelContext.php
Original file line number Diff line number Diff line change
Expand Up @@ -96,4 +96,20 @@ public function shouldSeePluginMainPageWithContent(string $content): void
{
Assert::same($this->pluginMainPage->getContent(), $content);
}

/**
* @When I visit the homepage
*/
public function iVisitTheHomepage(): void
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably use Sylius\Behat\Context\Ui\Shop\HomepageContext::iCheckLatestProducts() method, just add @When I visit the homepage in steps definition 🖖

{
$this->homePage->open();
}

/**
* @Then I should be on channel :channel
*/
public function iShouldBeOnChannel(ChannelInterface $channel): void
{
Assert::eq($this->homePage->getMetaTitle(), $channel->getName());
}
}
6 changes: 6 additions & 0 deletions src/Sylius/Behat/Page/Shop/HomePage.php
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,11 @@ function (NodeElement $element) {
);
}

public function getMetaTitle(): string
{
return $this->getElement('meta_title')->getText();
}

protected function getDefinedElements(): array
{
return array_merge(parent::getDefinedElements(), [
Expand All @@ -111,6 +116,7 @@ protected function getDefinedElements(): array
'latest_products' => '[data-test-latest-products]',
'locale_selector' => '[data-test-locale-selector]',
'logout_button' => '[data-test-logout-button]',
'meta_title' => 'title',
]);
}
}
2 changes: 2 additions & 0 deletions src/Sylius/Behat/Page/Shop/HomePageInterface.php
Original file line number Diff line number Diff line change
Expand Up @@ -38,4 +38,6 @@ public function getAvailableLocales(): array;
public function switchLocale(string $localeCode): void;

public function getLatestProductsNames(): array;

public function getMetaTitle(): string;
}
1 change: 1 addition & 0 deletions src/Sylius/Behat/Resources/config/suites.yml
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ imports:
- suites/ui/admin/locale.yml
- suites/ui/admin/login.yml
- suites/ui/cart/shopping_cart.yml
- suites/ui/channel/channels.yml
- suites/ui/channel/managing_channels.yml
- suites/ui/channel/products_accessibility_in_multiple_channels.yml
- suites/ui/channel/theming.yml
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
# This file is part of the Sylius package.
# (c) Paweł Jędrzejewski

default:
suites:
ui_channels:
contexts:
- sylius.behat.context.hook.doctrine_orm

- sylius.behat.context.transform.channel

- sylius.behat.context.setup.channel

- sylius.behat.context.ui.channel
filters:
tags: "@channels&&@ui"
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,11 @@ public function findOneByHostname(string $hostname): ?ChannelInterface
return $this->findOneBy(['hostname' => $hostname]);
}

public function findOneEnabledByHostname(string $hostname): ?ChannelInterface
{
return $this->findOneBy(['hostname' => $hostname, 'enabled' => true]);
}

public function findOneByCode(string $code): ?ChannelInterface
{
return $this->findOneBy(['code' => $code]);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,6 @@ public function __construct(ChannelRepositoryInterface $channelRepository)

public function findChannel(Request $request): ?ChannelInterface
{
return $this->channelRepository->findOneByHostname($request->getHost());
return $this->channelRepository->findOneEnabledByHostname($request->getHost());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have mixed feelings... because it's a good change, but also kind of a BC Break 💃 Even though the previous implementation was faulty, one could depend on it and we change the behaviour of the class and its findChannel method. Maybe we could inject a parameter in the constructor (sth like bool $onlyEnabledChannels = true), choose a proper repository method based on it and inform about this change in the UPGRADE file? This way it would be easy to return the previous behaviour by overriding the service definition 🖖

cc @lchrusciel

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe you are right, but the current implementation hides a relevant bug. We have an eCommerce with two channels and with two different hosts. After a while, we added another channel with the same hostname as the first but disabled, because that channel should only contain some feed products. After creating the channel the first shop started to show the default Sylius theme and only the products available in the disabled channel. We have deleted the disabled channel, fixed the bug, and then recreated the channel to fix that problem.

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,11 @@

interface ChannelRepositoryInterface extends RepositoryInterface
{
/** @deprecated No longer used by internal code and not recommended. */
public function findOneByHostname(string $hostname): ?ChannelInterface;

public function findOneEnabledByHostname(string $hostname): ?ChannelInterface;

public function findOneByCode(string $code): ?ChannelInterface;

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ function it_finds_the_channel_by_request_hostname(
): void {
$request->getHost()->willReturn('example.org');

$channelRepository->findOneByHostname('example.org')->willReturn($channel);
$channelRepository->findOneEnabledByHostname('example.org')->willReturn($channel);

$this->findChannel($request)->shouldReturn($channel);
}
Expand All @@ -49,7 +49,7 @@ function it_returns_null_if_channel_was_not_found(
): void {
$request->getHost()->willReturn('example.org');

$channelRepository->findOneByHostname('example.org')->willReturn(null);
$channelRepository->findOneEnabledByHostname('example.org')->willReturn(null);

$this->findChannel($request)->shouldReturn(null);
}
Expand Down