IBX-6750/IBX-6754/IBX-6839: Implementation of smart/expert mode #1017

adamwojs · 2023-11-30T10:03:52Z

Question	Answer
Tickets	IBX-6750/IBX-6754/IBX-6839
Bug fix?	no
New feature?	yes
BC breaks?	no
Tests pass?	yes
Doc needed?	yes
License	GPL-2.0

Changes summary:

New APIs to interact with smart/expert mode
- Twig functions: ibexa_is_smart_mode, ibexa_is_expert_mode
- Ibexa\AdminUi\Specification\UserMode\IsUserModeEnabled specification (internal)
Added mode switcher to user context menu
Applied user mode to main menu item & tabs visibility
Renamed "Quick preview" tab to "Data"
Hidden "Language Code" column in "Translations tab" when smart mode is active
Fixed sub-items list initialization which rely on forms from "Technical Details" tab
Added missing "Modified" and "Published" field to the "Authors" tab

Smart / Expert mode comparison

Smart mode

Expert mode

Checklist:

Coding standards ($ composer fix-cs)
Ready for Code Review

src/bundle/Controller/User/UserModeController.php

kisztof · 2023-11-30T11:41:19Z

src/bundle/Resources/views/themes/admin/content/tab/translations/tab.html.twig

        ]) %}

+        {% if ibexa_is_expert_mode() %}


I recommend utilizing ibexa_user_settings['user_mode'] instead of a dedicated Twig function.

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

Or ibexa_mode(...) function if we cannot rely on user settings being present in the template.

Or even, we can use wildcards in twig function names and do it like ibexa_is_*_mode and handle it that way.

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.

ibexa_user_settings['user_mode'] or ibexa_mode(...) is a no go for me. It's simply bad DX:

{% if ibexa_user_settings['user_mode'] === constant('\\Ibexa\\AdminUi\\UserSetting\\UserMode::SMART') %} Do something {% endif %}

Wildcard function is a good idea, but we don't need it at the moment.

Steveb-p · 2023-12-01T09:28:45Z

src/bundle/Controller/User/UserModeController.php

+        if ($form->isSubmitted() && $form->isValid()) {
+            $this->userSettingService->setUserSetting(
+                UserMode::IDENTIFIER,
+                $data->getMode() ? UserMode::EXPERT : UserMode::SMART


Why aren't we using UserMode::EXPERT and UserMode::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.

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.

Steveb-p · 2023-12-01T09:33:41Z

src/bundle/Resources/views/themes/admin/content/tab/translations/tab.html.twig

        ]) %}

+        {% if ibexa_is_expert_mode() %}


Or ibexa_mode(...) function if we cannot rely on user settings being present in the template.

webhdx · 2023-12-01T10:16:19Z

src/lib/Form/Data/User/UserModeChangeData.php

+
+final class UserModeChangeData
+{
+    private ?bool $mode;


Why don't you use UserMode class constants for the value? It makes more sense since the setting is prepared to handle more user modes if there is ever a need. If your data class could reflect it, introducing more user modes will be easier as you would only need to change the form type.

At the moment is checkbox (rendered as switcher) on the user menu (expert mode is enabled or no). I could add data transformer if need.

You don't even need a Data Transformer. You can use arbitrary string values in data class and do some magic with false_values and value on CheckboxType (ref. https://symfony.com/doc/5.4/reference/forms/types/checkbox.html).

mnocon · 2023-12-04T08:20:23Z

58c890d contains the Behat changes (together with https://github.com/ibexa/version-comparison/pull/62 and https://github.com/ibexa/product-catalog/pull/1103 it's enough to make tests pass)

webhdx · 2023-12-04T08:48:07Z

@mnocon There are some left over var_dumps in the Behat code.

webhdx · 2023-12-04T08:49:42Z

tests/bundle/Templating/Twig/_fixtures/location/ibexa_location_sort_order_as_rest_value.test

+{{ ibexa_location_sort_order_as_rest_value(location_asc) }}
+{{ ibexa_location_sort_order_as_rest_value(location_desc) }}
+--DATA--
+use \Ibexa\Contracts\Core\Repository\Values\Content\Location;


Is the import required?

I think it does, SORT_ORDER_ASC and SORT_ORDER_DESC constants reference Location.

mnocon · 2023-12-04T08:52:06Z

@mnocon There are some left over var_dumps in the Behat code.

Thanks, already removed 😅

sonarqubecloud · 2023-12-04T08:52:27Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
3 Code Smells

No Coverage information
1.7% Duplication

konradoboza · 2023-12-04T09:13:26Z

tests/bundle/Templating/Twig/_fixtures/location/ibexa_location_sort_order_as_rest_value.test

+{{ ibexa_location_sort_order_as_rest_value(location_asc) }}
+{{ ibexa_location_sort_order_as_rest_value(location_desc) }}
+--DATA--
+use \Ibexa\Contracts\Core\Repository\Values\Content\Location;


I think it does, SORT_ORDER_ASC and SORT_ORDER_DESC constants reference Location.

konradoboza · 2023-12-04T09:13:38Z

...bundle/Templating/Twig/_fixtures/location/ibexa_location_sort_field_as_rest_sort_clause.test

+{{ ibexa_location_sort_field_as_rest_sort_clause(location) }}
+{% endfor %}
+--DATA--
+use \Ibexa\Contracts\Core\Repository\Values\Content\Location;


Is this import needed?

adamwojs added 8 commits November 29, 2023 22:44

Added APIs to interact with user mode

5f8574e

Renamed "Quick Preview" to "Data"

51d61f1

Applied smart/expert mode to tabs

270c5a9

Fixed sub-items tab not being properly initialized in smart mode

4f6abd6

Added "Published" and "Modified" fields to "Authors" tab

e18e20e

Applied smart / expert mode to main menu

cb29cf9

Added form to switch between smart/expert mode to user menu

8a0c375

Conditional visibility of language code in Translations tab

94a1edd

github-actions bot assigned dew326, ciastektk, mikadamczyk, Nattfarinn and ViniTou Nov 30, 2023

adamwojs added 2 commits November 30, 2023 11:10

Reverted flatpickr.js changes

9be981e

Fixed code style issues

267a7f6

github-advanced-security bot found potential problems Nov 30, 2023

View reviewed changes

src/bundle/Controller/User/UserModeController.php Fixed Show resolved Hide resolved

kisztof approved these changes Nov 30, 2023

View reviewed changes

Fixed translation extraction

c599129

adamwojs changed the title ~~Implemntation of smart/expert mode~~ Implementation of smart/expert mode Nov 30, 2023

adamwojs added 3 commits December 1, 2023 07:04

fixup! Added form to switch between smart/expert mode to user menu

dfbdf99

Fixed redirection after switching mode

314bdca

Fixed MainMenuBuilerTest test

963c43b

Steveb-p reviewed Dec 1, 2023

View reviewed changes

fixup! Fixed MainMenuBuilerTest test

c88d0cf

webhdx reviewed Dec 1, 2023

View reviewed changes

Added missing unit tests

c840121

mnocon force-pushed the smart_expert_mode branch 3 times, most recently from b3623d7 to 58c890d Compare December 1, 2023 23:57

mnocon mentioned this pull request Dec 4, 2023

Run regression for Smart/Expert mode ibexa/commerce#493

Closed

webhdx approved these changes Dec 4, 2023

View reviewed changes

webhdx requested review from ViniTou, Steveb-p and a team December 4, 2023 08:50

[Behat] Adapted tests

d31a7f1

mnocon force-pushed the smart_expert_mode branch from 58c890d to d31a7f1 Compare December 4, 2023 08:51

konradoboza approved these changes Dec 4, 2023

View reviewed changes

adamwojs merged commit 438ba7c into main Dec 4, 2023
22 of 23 checks passed

adamwojs deleted the smart_expert_mode branch December 4, 2023 09:41

adamwojs changed the title ~~Implementation of smart/expert mode~~ IBX-6750/IBX-6754/IBX-6839: Implementation of smart/expert mode Dec 4, 2023

webhdx added the Ready for QA label Dec 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IBX-6750/IBX-6754/IBX-6839: Implementation of smart/expert mode #1017

IBX-6750/IBX-6754/IBX-6839: Implementation of smart/expert mode #1017

adamwojs commented Nov 30, 2023 •

edited

Loading

kisztof Nov 30, 2023

Steveb-p Dec 1, 2023

ViniTou Dec 1, 2023

Steveb-p Dec 1, 2023 •

edited

Loading

adamwojs Dec 1, 2023

Steveb-p Dec 1, 2023

webhdx Dec 1, 2023

Steveb-p Dec 1, 2023

webhdx Dec 1, 2023 •

edited

Loading

adamwojs Dec 1, 2023 •

edited

Loading

webhdx Dec 1, 2023

mnocon commented Dec 4, 2023

webhdx commented Dec 4, 2023

webhdx Dec 4, 2023

konradoboza Dec 4, 2023

mnocon commented Dec 4, 2023

sonarqubecloud bot commented Dec 4, 2023

konradoboza Dec 4, 2023

konradoboza Dec 4, 2023

IBX-6750/IBX-6754/IBX-6839: Implementation of smart/expert mode #1017

IBX-6750/IBX-6754/IBX-6839: Implementation of smart/expert mode #1017

Conversation

adamwojs commented Nov 30, 2023 • edited Loading

Changes summary:

Smart / Expert mode comparison

Smart mode

Expert mode

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Steveb-p Dec 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webhdx Dec 1, 2023 • edited Loading

Choose a reason for hiding this comment

adamwojs Dec 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mnocon commented Dec 4, 2023

webhdx commented Dec 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mnocon commented Dec 4, 2023

sonarqubecloud bot commented Dec 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamwojs commented Nov 30, 2023 •

edited

Loading

Steveb-p Dec 1, 2023 •

edited

Loading

webhdx Dec 1, 2023 •

edited

Loading

adamwojs Dec 1, 2023 •

edited

Loading