-
Notifications
You must be signed in to change notification settings - Fork 16
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-6750/IBX-6754/IBX-6839: Implementation of smart/expert mode #1017
Changes from all commits
5f8574e
51d61f1
270c5a9
4f6abd6
e18e20e
cb29cf9
8a0c375
94a1edd
9be981e
267a7f6
c599129
dfbdf99
314bdca
963c43b
c88d0cf
c840121
d31a7f1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,82 @@ | ||
<?php | ||
|
||
/** | ||
* @copyright Copyright (C) Ibexa AS. All rights reserved. | ||
* @license For full copyright and license information view LICENSE file distributed with this source code. | ||
*/ | ||
declare(strict_types=1); | ||
|
||
namespace Ibexa\Bundle\AdminUi\Controller\User; | ||
|
||
use Ibexa\AdminUi\Form\Data\User\UserModeChangeData; | ||
use Ibexa\AdminUi\Form\Type\User\UserModeChangeType; | ||
use Ibexa\AdminUi\UserSetting\UserMode; | ||
use Ibexa\Contracts\AdminUi\Controller\Controller; | ||
use Ibexa\User\UserSetting\UserSettingService; | ||
use Symfony\Component\HttpFoundation\RedirectResponse; | ||
use Symfony\Component\HttpFoundation\Request; | ||
use Symfony\Component\HttpFoundation\Response; | ||
|
||
final class UserModeController extends Controller | ||
{ | ||
private const RETURN_URL_PARAM = 'returnUrl'; | ||
|
||
private UserSettingService $userSettingService; | ||
|
||
public function __construct(UserSettingService $userSettingService) | ||
{ | ||
$this->userSettingService = $userSettingService; | ||
} | ||
|
||
public function changeAction(Request $request, ?string $returnUrl): Response | ||
{ | ||
$data = new UserModeChangeData(); | ||
$data->setMode($this->userSettingService->getUserSetting(UserMode::IDENTIFIER)->value === UserMode::EXPERT); | ||
|
||
$form = $this->createForm( | ||
UserModeChangeType::class, | ||
$data, | ||
[ | ||
'action' => $this->generateUrl( | ||
'ibexa.user_mode.change', | ||
[ | ||
self::RETURN_URL_PARAM => $returnUrl, | ||
] | ||
), | ||
'method' => Request::METHOD_POST, | ||
] | ||
); | ||
|
||
$form->handleRequest($request); | ||
if ($form->isSubmitted() && $form->isValid()) { | ||
$this->userSettingService->setUserSetting( | ||
UserMode::IDENTIFIER, | ||
$data->getMode() ? UserMode::EXPERT : UserMode::SMART | ||
); | ||
|
||
return $this->createRedirectToReturnUrl($request); | ||
} | ||
|
||
return $this->render( | ||
'@ibexadesign/ui/user_mode_form.html.twig', | ||
[ | ||
'form' => $form->createView(), | ||
] | ||
); | ||
} | ||
|
||
private function createRedirectToReturnUrl(Request $request): RedirectResponse | ||
{ | ||
$url = $request->query->get(self::RETURN_URL_PARAM); | ||
if (is_string($url) && $this->isSafeUrl($url, $request->getBaseUrl())) { | ||
return new RedirectResponse($url); | ||
} | ||
|
||
throw $this->createAccessDeniedException('Malformed return URL'); | ||
} | ||
|
||
private function isSafeUrl(string $referer, string $baseUrl): bool | ||
{ | ||
return str_starts_with($referer, $baseUrl); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
(function (global, doc) { | ||
const FORM_SELECTOR = 'form[name=user_mode_change]'; | ||
const form = doc.querySelector(FORM_SELECTOR); | ||
|
||
if (form) { | ||
form.querySelectorAll('input[type=checkbox]').forEach((input) => { | ||
input.addEventListener('change', () => { | ||
form.submit(); | ||
}); | ||
}); | ||
} | ||
})(window, window.document); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -29,9 +29,14 @@ | |
|
||
{% set body_row_cols = body_row_cols|merge([ | ||
{ content: translation.name }, | ||
{ content: translation.languageCode }, | ||
]) %} | ||
|
||
{% if ibexa_is_expert_mode() %} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I recommend utilizing If we name the function with the "expert" keyword, this designation will remain with us for the upcoming years. If we decide to change the name or introduce additional user modes, we will need to implement extra logic There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or even, we can use wildcards in twig function names and do it like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One issue with wildcards is that you cannot use mode value as a variable. You have to use the full function name immediately. {% set expected_mode = 'expert' %}
{{ expected_mode is same as 'expert' ? ibexa_is_expert_mode() : ibexa_is_smart_mode() }} Using an argument does not have this limitation. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Wildcard function is a good idea, but we don't need it at the moment. |
||
{% set body_row_cols = body_row_cols|merge([ | ||
{ content: translation.languageCode } | ||
]) %} | ||
{% endif %} | ||
|
||
{% if main_translation_switch %} | ||
{% set col_raw %} | ||
<input | ||
|
@@ -55,8 +60,14 @@ | |
{% set head_cols = [ | ||
{ has_checkbox: true }, | ||
{ content: 'tab.translations.language_name'|trans|desc('Language name') }, | ||
{ content: 'tab.translations.language_code'|trans|desc('Language code') }, | ||
] %} | ||
|
||
{% if ibexa_is_expert_mode() %} | ||
{% set head_cols = head_cols|merge([ | ||
{ content: 'tab.translations.language_code'|trans|desc('Language code') } | ||
]) %} | ||
{% endif %} | ||
|
||
{% if main_translation_switch %} | ||
{% set head_cols = head_cols|merge([{ | ||
content: 'tab.translations.main_language'|trans|desc('Main language'), | ||
|
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.
Why aren't we using
UserMode::EXPERT
andUserMode::SMART
directly, instead of relying on a straight up boolean?For me myself, it would make sense to use the same value as user settings. It would be also a lot easier to work with in case a third setting is introduced.
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.
Ah, I commented on the same issue. Fully agree, user setting was prepared to accept more user modes in the future. We could maybe even make it extendable by developers, especially with suggestions like #1017 (comment) Data format should reflect values from the user setting.