[Bug][Channel] The hostname resolver does not check for the channel status #13697
Conversation
lruozzi9
commented
Feb 24, 2022
lruozzi9
commented
| Q | A |
|---|---|
| Branch? | 1.10 |
| Bug fix? | yes |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | yes |
| Related tickets | fixes #13677 |
| License | MIT |
probot-autolabeler
bot
added
the
Maintenance
|
Hi @lruozzi9 . Could it be possible to come up with a testing scenario on that please ? |
lruozzi9
force-pushed
the
fix-hostname-channel-resolver
branch
from
8cb3051
to
3e14f90
Compare
|
Done! |
lruozzi9
force-pushed
the
fix-hostname-channel-resolver
branch
from
3e14f90
to
b3da30b
Compare
| /** | ||
| * @When I visit the homepage | ||
| */ | ||
| public function iVisitTheHomepage(): void |
There was a problem hiding this comment.
We should probably use Sylius\Behat\Context\Ui\Shop\HomepageContext::iCheckLatestProducts() method, just add @When I visit the homepage in steps definition 🖖
| @ui | ||
| Scenario: Browsing shop in active channel | ||
| When I visit the homepage | ||
| Then I should be on channel "Alternative" |
There was a problem hiding this comment.
I should brows "Alternative" shop? "Channel" is not a concept Customer should be aware of 🚀
| Feature: Browsing shop on right channel | ||
| In order to shop | ||
| As a Customer | ||
| I want to be redirected to the right channel |
There was a problem hiding this comment.
I should not be able to browse disabled shop?
| @@ -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()); | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
It was when I opened the PR 🤣. |
… status (lruozzi9, jakubtobiasz) This PR was merged into the 1.11 branch. Discussion ---------- | Q | A | |-----------------|--------------------------------------------------------------| | Branch? | 1.11 <!-- see the comment below --> | | Bug fix? | yes | | New feature? | no | | BC breaks? | no | | Deprecations? | yes <!-- don't forget to update the UPGRADE-*.md file --> | | Related tickets | continuation of #13697 | | License | MIT | <!-- - Bug fixes must be submitted against the 1.10 or 1.11 branch(the lowest possible) - Features and deprecations must be submitted against the master branch - Make sure that the correct base branch is set To be sure you are not breaking any Backward Compatibilities, check the documentation: https://docs.sylius.com/en/latest/book/organization/backward-compatibility-promise.html --> What has been done: - Updated behats (In my opinion now they're now a little bit more natural to read) - I rebased the PR on top 1.11 and updated the `UPGRADE-1.11.md` and the `UPGRADE-1.10.md` files We've had a discussion about parametrizing the `HostnameBasedRequestResolver` (what I've done btw 😂) but we decided to drop that idea and keep the original concept. Thank you @lruozzi9 for your PR we could base on :). Commits ------- 28e39e2 [Channel] Do not consider disabled channel on hostname based request resolver 27406cd Adjust behat scenarios 066cf33 Update UPGRADE files 351425d Update deprecation message 55edb2d Remove redundant method 673eb91 Make filtering out scenario available in both ui and api contexts
|
Closing in favour of #14155 |
