-
Notifications
You must be signed in to change notification settings - Fork 15
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
IBX-7376: Fixed redirect to the not-existing menu after switching to focus mode #1065
Conversation
68a8f0f
to
2f28c74
Compare
5bcac46
to
56fabc2
Compare
return new RedirectResponse( | ||
$this->resolveReturnPath($path) | ||
); |
Check failure
Code scanning / SonarCloud
HTTP request redirections should not be open to forging attacks
56fabc2
to
99f6b7d
Compare
99f6b7d
to
17d3807
Compare
src/lib/Strategy/FocusMode/ContentStructureRedirectStrategy.php
Outdated
Show resolved
Hide resolved
600d59b
to
77dddcd
Compare
'ibexa.section.list', | ||
'ibexa.content_type_group.list', | ||
'ibexa.object_state.groups.list', | ||
'ibexa.content_type_group.view', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this list be configurable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To me - not necessarily. It might look like an extension point nobody would really use. The thing is, we are lacking a proper list of columns/paths unavailable in focus mode which can be referenced here. That's why I decided to hardcode them and give a way to circumvent the default behavior by registering another strategy at the same time. To me, Open-Closed principle holds it ground in this case. 😉
src/lib/Strategy/FocusMode/ContentStructureRedirectStrategy.php
Outdated
Show resolved
Hide resolved
tests/lib/Strategy/FocusMode/ContentStructureRedirectStrategyTest.php
Outdated
Show resolved
Hide resolved
a6f1e6a
to
fd64555
Compare
Quality Gate passedThe SonarCloud Quality Gate passed, but some issues were introduced. 2 New issues |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regression tests passed
After switching to "focus mode" there are some sections that are no longer visible in the menu. It makes sense to redirect to the "Content structure" by default (agreed upon with UX team) whenever that happens.
It's not really reliable to those sections automatically due to variety of urls being part of menu items. Therefore I registered two basic strategies:
OriginalUrlRedirectStrategy
which basically is a projection of the current behavior,ContentStructureRedirectStrategy
fixing the issue by detecting affected paths and redirecting to the default content structure menu.Both implement
Ibexa\Contracts\AdminUi\FocusMode\RedirectStrategyInterface
which is an extension point allowing to circumvent the default behavior:New strategy can be registered as follows:
Documentation:
It's worth mentioning the above extension point for adding custom focus mode redirect strategies.
Checklist:
$ composer fix-cs
)