EZP-28493: Interface improvements when assigning roles to user / groups #170

webhdx · 2017-12-13T13:47:26Z

Question	Answer
Tickets	https://jira.ez.no/browse/EZP-28493
Bug fix?	no
New feature?	no
BC breaks?	no
Tests pass?	yes
Doc needed?	no
License	GPL-2.0

Screenshot:

Checklist:

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

lserwatka · 2017-12-13T13:48:21Z

@micszo we need very careful QA on this one, including DB entires.

lserwatka · 2017-12-13T13:51:15Z

@webhdx could you attach screenshot?

sunpietro · 2017-12-13T13:55:39Z

src/bundle/Resources/public/js/scripts/admin.role_assignment.add.js

            restInfo: {token, siteaccess}
        }), udwContainer);
    };
+    const toggleDisableState = () => {


toggleDisabledState

sunpietro · 2017-12-13T13:56:27Z

src/bundle/Resources/public/js/scripts/admin.role_assignment.add.js

            restInfo: {token, siteaccess}
        }), udwContainer);
    };
+    const toggleDisableState = () => {
+        limitationsRadio.forEach(radio => {
+            const disableNode = doc.querySelector(radio.dataset.disableSelector);


Do you have to query the whole document?

sunpietro · 2017-12-13T14:00:27Z

src/bundle/Resources/public/js/scripts/admin.role_assignment.add.js

-    btns.forEach(btn => btn.addEventListener('click', openUDW, false));
-})();
+    doc.querySelector('.ez-btn--select-subtree').addEventListener('click', selectSubtree, false);
+    limitationsRadio.forEach(radio => radio.addEventListener('change', toggleDisableState, false));


I'm not sure the use case here. If you're clicking on a specific radio button then I would expect to change the state of a clicked button. The toggleDisabledState function tries to toggle the state of all radio buttons. It looks quite suboptimal.

The event change is fired only on radio which gets the checked=true. I have to loop over every to set disable state on other buttons

sunpietro · 2017-12-13T14:01:36Z

src/bundle/Resources/views/admin/role_assignment/create.html.twig

-                    {{ form_widget(form.locations.select_content, {'attr': {'class': 'btn btn-secondary btn--open-udw'}}) }}
-                    {% do form.locations.setRendered %}
+                <div class="card-body ez-limitations">
+                    <div>


Do you need the div wrapper here?

sunpietro

+1 for the frontend part

ezrobot · 2017-12-13T17:18:54Z

Tool version : PHP CS Fixer 2.7.1 Sandy Pool by Fabien Potencier and Dariusz Ruminski
Command executed php-cs-fixer --dry-run -v fix
This Pull Request does not respect PSR-2 Coding Standards, please, see the suggested diff below:

diff --git a/src/lib/Form/Type/Role/RoleAssignmentCreateType.php b/src/lib/Form/Type/Role/RoleAssignmentCreateType.php
index 2fe85b7..44cf5b2 100644
--- a/src/lib/Form/Type/Role/RoleAssignmentCreateType.php
+++ b/src/lib/Form/Type/Role/RoleAssignmentCreateType.php
@@ -61,12 +61,9 @@ class RoleAssignmentCreateType extends AbstractType
                 'multiple' => false,
                 'expanded' => true,
                 'choices' => [
-                    RoleAssignmentCreateData::LIMITATION_TYPE_NONE
-                        => RoleAssignmentCreateData::LIMITATION_TYPE_NONE,
-                    RoleAssignmentCreateData::LIMITATION_TYPE_SECTION
-                        => RoleAssignmentCreateData::LIMITATION_TYPE_SECTION,
-                    RoleAssignmentCreateData::LIMITATION_TYPE_LOCATION
-                        => RoleAssignmentCreateData::LIMITATION_TYPE_LOCATION,
+                    RoleAssignmentCreateData::LIMITATION_TYPE_NONE => RoleAssignmentCreateData::LIMITATION_TYPE_NONE,
+                    RoleAssignmentCreateData::LIMITATION_TYPE_SECTION => RoleAssignmentCreateData::LIMITATION_TYPE_SECTION,
+                    RoleAssignmentCreateData::LIMITATION_TYPE_LOCATION => RoleAssignmentCreateData::LIMITATION_TYPE_LOCATION,
                 ],
                 'choice_name' => function ($value) {
                     return $value;

ezrobot · 2017-12-13T17:43:46Z

Tool version : PHP CS Fixer 2.7.1 Sandy Pool by Fabien Potencier and Dariusz Ruminski
Command executed php-cs-fixer --dry-run -v fix
This Pull Request does not respect PSR-2 Coding Standards, please, see the suggested diff below:

diff --git a/src/lib/Form/Type/Role/RoleAssignmentCreateType.php b/src/lib/Form/Type/Role/RoleAssignmentCreateType.php
index 2fe85b7..44cf5b2 100644
--- a/src/lib/Form/Type/Role/RoleAssignmentCreateType.php
+++ b/src/lib/Form/Type/Role/RoleAssignmentCreateType.php
@@ -61,12 +61,9 @@ class RoleAssignmentCreateType extends AbstractType
                 'multiple' => false,
                 'expanded' => true,
                 'choices' => [
-                    RoleAssignmentCreateData::LIMITATION_TYPE_NONE
-                        => RoleAssignmentCreateData::LIMITATION_TYPE_NONE,
-                    RoleAssignmentCreateData::LIMITATION_TYPE_SECTION
-                        => RoleAssignmentCreateData::LIMITATION_TYPE_SECTION,
-                    RoleAssignmentCreateData::LIMITATION_TYPE_LOCATION
-                        => RoleAssignmentCreateData::LIMITATION_TYPE_LOCATION,
+                    RoleAssignmentCreateData::LIMITATION_TYPE_NONE => RoleAssignmentCreateData::LIMITATION_TYPE_NONE,
+                    RoleAssignmentCreateData::LIMITATION_TYPE_SECTION => RoleAssignmentCreateData::LIMITATION_TYPE_SECTION,
+                    RoleAssignmentCreateData::LIMITATION_TYPE_LOCATION => RoleAssignmentCreateData::LIMITATION_TYPE_LOCATION,
                 ],
                 'choice_name' => function ($value) {
                     return $value;

lserwatka · 2017-12-14T10:41:38Z

@webhdx it looks like you need to run cs-fixer here

ezrobot · 2017-12-14T13:45:27Z

Tool version : PHP CS Fixer 2.7.1 Sandy Pool by Fabien Potencier and Dariusz Ruminski
Command executed php-cs-fixer --dry-run -v fix
This Pull Request does not respect PSR-2 Coding Standards, please, see the suggested diff below:

diff --git a/src/lib/Form/Type/Role/RoleAssignmentCreateType.php b/src/lib/Form/Type/Role/RoleAssignmentCreateType.php
index 2fe85b7..44cf5b2 100644
--- a/src/lib/Form/Type/Role/RoleAssignmentCreateType.php
+++ b/src/lib/Form/Type/Role/RoleAssignmentCreateType.php
@@ -61,12 +61,9 @@ class RoleAssignmentCreateType extends AbstractType
                 'multiple' => false,
                 'expanded' => true,
                 'choices' => [
-                    RoleAssignmentCreateData::LIMITATION_TYPE_NONE
-                        => RoleAssignmentCreateData::LIMITATION_TYPE_NONE,
-                    RoleAssignmentCreateData::LIMITATION_TYPE_SECTION
-                        => RoleAssignmentCreateData::LIMITATION_TYPE_SECTION,
-                    RoleAssignmentCreateData::LIMITATION_TYPE_LOCATION
-                        => RoleAssignmentCreateData::LIMITATION_TYPE_LOCATION,
+                    RoleAssignmentCreateData::LIMITATION_TYPE_NONE => RoleAssignmentCreateData::LIMITATION_TYPE_NONE,
+                    RoleAssignmentCreateData::LIMITATION_TYPE_SECTION => RoleAssignmentCreateData::LIMITATION_TYPE_SECTION,
+                    RoleAssignmentCreateData::LIMITATION_TYPE_LOCATION => RoleAssignmentCreateData::LIMITATION_TYPE_LOCATION,
                 ],
                 'choice_name' => function ($value) {
                     return $value;

mnocon

Looks good! UI looks good, database entries look the same as in v1.

I've added 3 tickets:
https://jira.ez.no/browse/EZP-28534 (behaviour already present in v1)
https://jira.ez.no/browse/EZP-28533 (possible improvement)
https://jira.ez.no/browse/EZP-28524 (not related only to this ticket)

…es (#170)

webhdx added Improvement Ready for QA Ready for review labels Dec 13, 2017

webhdx assigned webhdx and dew326 Dec 13, 2017

webhdx requested review from lserwatka, mikadamczyk, sunpietro, Nattfarinn and micszo December 13, 2017 13:47

barbaragr self-assigned this Dec 13, 2017

barbaragr removed their assignment Dec 13, 2017

micszo requested a review from mnocon December 13, 2017 13:57

mnocon self-assigned this Dec 13, 2017

mnocon removed the Ready for QA label Dec 13, 2017

sunpietro suggested changes Dec 13, 2017

View reviewed changes

sunpietro approved these changes Dec 13, 2017

View reviewed changes

webhdx force-pushed the update-roles branch from b1bfbba to 4532c71 Compare December 14, 2017 13:39

EZP-28493: Interface improvements when assigning roles to user / groups

9ab9f5f

webhdx force-pushed the update-roles branch from 4532c71 to 9ab9f5f Compare December 14, 2017 13:50

mnocon approved these changes Dec 14, 2017

View reviewed changes

mnocon added the QA approved label Dec 14, 2017

lserwatka merged commit 275f083 into ezsystems:master Dec 14, 2017

webhdx mentioned this pull request Dec 15, 2017

Fixes unit tests (LocationsTransformer Test) #192

Merged

webhdx deleted the update-roles branch March 7, 2018 13:53

mateuszdebinski pushed a commit that referenced this pull request Oct 25, 2022

IBX-1526: change edit fields width to full column; Page Builder updat…

e176e6a

…es (#170)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EZP-28493: Interface improvements when assigning roles to user / groups #170

EZP-28493: Interface improvements when assigning roles to user / groups #170

webhdx commented Dec 13, 2017 •

edited by dew326

Loading

lserwatka commented Dec 13, 2017

lserwatka commented Dec 13, 2017

sunpietro Dec 13, 2017

sunpietro Dec 13, 2017

sunpietro Dec 13, 2017

dew326 Dec 13, 2017

sunpietro Dec 13, 2017

sunpietro left a comment

ezrobot commented Dec 13, 2017

ezrobot commented Dec 13, 2017

lserwatka commented Dec 14, 2017 •

edited

Loading

ezrobot commented Dec 14, 2017

mnocon left a comment

EZP-28493: Interface improvements when assigning roles to user / groups #170

EZP-28493: Interface improvements when assigning roles to user / groups #170

Conversation

webhdx commented Dec 13, 2017 • edited by dew326 Loading

Screenshot:

Checklist:

lserwatka commented Dec 13, 2017

lserwatka commented Dec 13, 2017

sunpietro Dec 13, 2017

Choose a reason for hiding this comment

sunpietro Dec 13, 2017

Choose a reason for hiding this comment

sunpietro Dec 13, 2017

Choose a reason for hiding this comment

dew326 Dec 13, 2017

Choose a reason for hiding this comment

sunpietro Dec 13, 2017

Choose a reason for hiding this comment

sunpietro left a comment

Choose a reason for hiding this comment

ezrobot commented Dec 13, 2017

ezrobot commented Dec 13, 2017

lserwatka commented Dec 14, 2017 • edited Loading

ezrobot commented Dec 14, 2017

mnocon left a comment

Choose a reason for hiding this comment

webhdx commented Dec 13, 2017 •

edited by dew326

Loading

lserwatka commented Dec 14, 2017 •

edited

Loading