EZP-29179: Implement permissions for "Content/Hide" #538

mikadamczyk · 2018-06-26T11:23:22Z

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

In addition we added ajax request to change location visibility

Checklist:

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

lserwatka · 2018-06-26T13:11:36Z

@sunpietro @dew326 we need frontend code review here.

sunpietro · 2018-06-26T13:39:05Z

src/bundle/Resources/public/js/scripts/admin.location.visibility.js

-        doc.querySelector('#location_update_visibility_data_hidden').checked = !event.target.checked;
+    const onVisibilityUpdated = (event) => {
+        const { target } = event;
+        const { checked: isHidden } = target;


Do you want set a default value if none exists?

Should I? I think that there is no default value in this case.

sunpietro · 2018-06-26T13:39:28Z

src/bundle/Resources/public/js/scripts/admin.location.visibility.js


-        doc.querySelector('#location_update_visibility_data_location').value = event.target.value;
-        doc.querySelector('#location_update_visibility_data_hidden').checked = !event.target.checked;
+    const onVisibilityUpdated = (event) => {


({ target }) => {

Please, remove empty line above the const onVisibilityUpdated

sunpietro · 2018-06-26T13:40:51Z

src/bundle/Resources/public/js/scripts/admin.location.visibility.js

+        target.closest('.ez-checkbox-icon').classList.toggle('is-checked', isHidden);
+    };
+    const handleUpdateError = global.eZ.services.notification.showErrorNotification;
+    const handleUpdateResponse = (event) => (response) => {


Is it intentional? You created a higher order function. Function that returns function.

Yes, it's intentional.

Please, use binding instead.

OK, I saw comment below.

dew326 · 2018-06-26T13:18:13Z

src/bundle/Resources/public/js/scripts/services/notification.service.js

+        showDangerNotification(error.message);
+    };
+
+    eZ.services = eZ.services || {};


I don't know if services is an accurate name. Maybe helpers?

I was thinking about this and I actually searched for an answer on the Internet and I encountered this thread:
https://softwareengineering.stackexchange.com/questions/132067/difference-between-a-service-class-and-a-helper-class

The top answer says:

A Service class/interface provides a way of a client to interact with some functionality in the application.

and for me this code, which is sending notifications does that, "provides a way of a client to interact with some functionality in the application".
If you have better definition etc. I would gladly see it because I really want to name it correctly. :)

A helper for me would be e.g. a set of functions generating CustomEvents but not dispatching it to the DOM.

IMO, a service is meant to manage data.

@sunpietro , can you provide some reference?

dew326 · 2018-06-26T13:19:50Z

src/bundle/Resources/public/js/scripts/admin.location.visibility.js

+            .catch(handleUpdateError);
+    };
+
+    for (const checkbox of visibilityCheckboxes) {


Why not old fashion forEach? :)

It's all about expressing ideas through code. forEach exists probably because there was no for ( . of . ) construct before in language and I think that for ( . of . ) was added partly because it is more clear. It also works with any iterable.
Moreover, it think it is good practice to avoid functions with side effects like forEach.

As discussed in private. There is no point to avoid side effects in this case and with forEach the code would be simpler.

dew326 · 2018-06-26T13:32:36Z

src/bundle/Resources/public/js/scripts/admin.location.visibility.js


-    [...doc.querySelectorAll('.ez-checkbox-icon__checkbox')].forEach(checkbox => checkbox.addEventListener('change', updateVisibility, false));
+        fetch(changeVisibilityLink, fetchInit)
+            .then(handleUpdateResponse(event))


Why not just handleUpdateResponse.bind(null, event)?

I think that this: handleUpdateResponse(event) is more clear than this: handleUpdateResponse.bind(null, event).
In my opinion bind don't express well intention of applying one argument to the function. Higher order functions are better IMO.

But bind is for applying arguments to function.

It's not a common approach in our codebase to use HoF. I'd stick with current approach and use .bind() function instead.

OK, this is some argument.
I accept it, consistency is important, but do you have any other arguments against using HoF in such cases? I am just asking from pure curiosity about good practices. :)

There is no point to create some workarounds if something is native in JS.
This is about using .bind() vs using imitation of bind.

dew326 · 2018-06-26T13:36:27Z

src/bundle/Resources/config/routing.yml

@@ -479,6 +479,16 @@ ezplatform.location.update_visibility:
    defaults:
        _controller: 'EzPlatformAdminUiBundle:Location:updateVisibility'

+ezplatform.location.hide:


I think hide is not an accurate name.

I would stick with visibility considering action is named updateVisibility

dew326 · 2018-06-26T13:41:43Z

src/bundle/Resources/public/js/scripts/admin.location.visibility.js

+    };
+    const handleUpdateError = global.eZ.services.notification.showErrorNotification;
+    const handleUpdateResponse = (event) => (response) => {
+        if (response.status === 200) {


I would do it in opposite way in the if checked if there is an error and did return.

Nattfarinn · 2018-06-26T13:50:48Z

src/bundle/Controller/LocationController.php

+            return new JsonResponse(['error' => $e->getMessage()], Response::HTTP_UNAUTHORIZED);
+        }
+
+        try {


Why two try/catch blocks if you catch the same Exception in both?

Nattfarinn · 2018-06-26T13:52:15Z

src/bundle/Resources/config/routing.yml

@@ -479,6 +479,16 @@ ezplatform.location.update_visibility:
    defaults:
        _controller: 'EzPlatformAdminUiBundle:Location:updateVisibility'

+ezplatform.location.hide:
+    path: /location/hide/{locationId}/{hidden}
+    methods: ['GET']


Why do you used GET instead of POST for an update?

Nattfarinn · 2018-06-26T13:54:31Z

src/bundle/Controller/LocationController.php

+     *
+     * @return \Symfony\Component\HttpFoundation\JsonResponse
+     */
+    public function updateVisibilityAjaxAction(int $locationId, int $hidden): JsonResponse


Remove Ajax part of name.

Nattfarinn · 2018-06-26T13:55:48Z

src/bundle/Resources/config/routing.yml

@@ -479,6 +479,16 @@ ezplatform.location.update_visibility:
    defaults:
        _controller: 'EzPlatformAdminUiBundle:Location:updateVisibility'

+ezplatform.location.hide:


I would stick with visibility considering action is named updateVisibility

Nattfarinn · 2018-06-26T14:07:07Z

src/bundle/Controller/LocationController.php

@@ -485,6 +483,9 @@ public function updateVisibilityAction(Request $request): Response
        if ($form->isSubmitted() && $form->isValid()) {
            $result = $this->submitHandler->handle($form, function (LocationUpdateVisibilityData $data) use ($contentInfo) {
                $location = $data->getLocation();
+
+                $this->canUserHideLocation($location);


Don't use "breakpoint"-like methods (no return value with exception in case of failure). There is need to wrap this simple if here.

EZP-29179: Implement permissions for "Content/Hide"

sunpietro · 2018-06-27T08:48:49Z

src/bundle/Resources/public/js/scripts/admin.location.visibility.js


-        doc.querySelector('#location_update_visibility_data_location').value = event.target.value;
-        doc.querySelector('#location_update_visibility_data_hidden').checked = !event.target.checked;
+    const onVisibilityUpdated = (event) => {


Please, remove empty line above the const onVisibilityUpdated

sunpietro · 2018-06-27T08:49:07Z

src/bundle/Resources/public/js/scripts/admin.location.visibility.js

+        target.closest('.ez-checkbox-icon').classList.toggle('is-checked', isHidden);
+    };
+    const handleUpdateError = global.eZ.services.notification.showErrorNotification;
+    const handleUpdateResponse = (event) => (response) => {


Please, use binding instead.

sunpietro · 2018-06-27T08:49:50Z

src/bundle/Resources/public/js/scripts/admin.location.visibility.js

+
+        throw new Error(response.statusText);
+    };
+    const updateVisibility = (event) => {


const updateVisibility = ({ value: locationId, checked: isVisible }) => {

The event is passed to handleUpdateResponse so it is rather impossible to write it like that.

sunpietro · 2018-06-27T08:52:16Z

src/bundle/Resources/public/js/scripts/admin.location.visibility.js


-    [...doc.querySelectorAll('.ez-checkbox-icon__checkbox')].forEach(checkbox => checkbox.addEventListener('change', updateVisibility, false));
+        fetch(changeVisibilityLink, fetchInit)
+            .then(handleUpdateResponse(event))


It's not a common approach in our codebase to use HoF. I'd stick with current approach and use .bind() function instead.

sunpietro · 2018-06-27T08:52:53Z

src/bundle/Resources/public/js/scripts/services/notification.service.js

+     */
+    const showNotification = (detail) => {
+        const event = new CustomEvent('ez-notify', {
+            detail,


It can be in the same line with the event variable declaration

What's more, you don't even need this variable and make the whole const showNotification function declaration in one line.

sunpietro · 2018-06-27T08:54:59Z

src/bundle/Resources/public/js/scripts/services/notification.service.js

+        showDangerNotification(error.message);
+    };
+
+    eZ.services = eZ.services || {};


IMO, a service is meant to manage data.

lserwatka · 2018-06-27T09:00:55Z

@tischsoic @sunpietro please reach the conclusion on the implementation details in private. We MUST merge this PR today!

dew326 · 2018-06-27T12:08:14Z

src/bundle/Controller/LocationController.php

-            'locationId' => $contentInfo->mainLocationId,
-            '_fragment' => LocationsTab::URI_FRAGMENT,
-        ]));
+        return new JsonResponse();


I think this should return some success and ideally some text for the success notification.

dew326 · 2018-06-27T12:13:04Z

src/bundle/Resources/views/content/locationview.html.twig

@@ -110,6 +110,7 @@

 {% block javascripts %}
    {%  javascripts
+        '@EzPlatformAdminUiBundle/Resources/public/js/scripts/services/notification.service.js'


I think this should be in layout.html.twig to make this global not only for content structure.

…sage

dew326 · 2018-06-27T12:46:20Z

src/bundle/Resources/public/js/scripts/admin.location.visibility.js

        doc.querySelector('#location_update_visibility_data_location').value = event.target.value;
        doc.querySelector('#location_update_visibility_data_hidden').checked = !event.target.checked;

-        doc.querySelector('form[name="location_update_visibility_data"]').submit();
-    }
+        $.ajax({


lserwatka · 2018-06-27T13:41:53Z

@dew326 @sunpietro can we merge it now?

dew326 · 2018-06-27T13:43:16Z

Not with jQuery used for ajax request.

…orm on the site.

tischsoic · 2018-06-27T14:10:08Z

I removed jQuery in last commit. @dew326

sunpietro

Actually, there is a couple of things to improve.

sunpietro · 2018-06-28T06:15:25Z

src/bundle/Resources/public/js/scripts/admin.location.visibility.js

+        }
+
+        onVisibilityUpdated(event);
+        response.json().then(handleUpdateSuccess);


You should return response.json() and then in the next then() you should proccess this data.

sunpietro · 2018-06-28T06:16:05Z

src/bundle/Resources/public/js/scripts/admin.location.visibility.js

+            throw new Error(response.statusText);
+        }
+
+        onVisibilityUpdated(event);


If you handle response here, then you should call this method in the next then()

dew326 · 2018-06-28T06:17:33Z

src/bundle/Resources/public/js/scripts/admin.location.visibility.js

+        onVisibilityUpdated(event);
+        response.json().then(handleUpdateSuccess);
+    };
+    const updateVisibility = (event) => {
        doc.querySelector('#location_update_visibility_data_location').value = event.target.value;
        doc.querySelector('#location_update_visibility_data_hidden').checked = !event.target.checked;


This can be changed to not search in whole doc but only on form.

Co-authored-by: Michał Grabowski <michal.grabowski@ibexa.co>

mikadamczyk added the Ready for review label Jun 26, 2018

mikadamczyk self-assigned this Jun 26, 2018

mikadamczyk requested review from adamwojs, sunpietro, Nattfarinn, webhdx, dew326 and ViniTou June 26, 2018 11:23

ViniTou approved these changes Jun 26, 2018

View reviewed changes

sunpietro suggested changes Jun 26, 2018

View reviewed changes

dew326 requested changes Jun 26, 2018

View reviewed changes

Nattfarinn requested changes Jun 26, 2018

View reviewed changes

mikadamczyk and others added 2 commits June 27, 2018 10:44

EZP-29179: Implement permissions for "Content/Hide"

c915454

EZP-29179: Implement permissions for "Content/Hide"

EZP-29179: Enable changing content visibility by Ajax request.

d43936f

mikadamczyk force-pushed the EZP-29179 branch from b8ac801 to d43936f Compare June 27, 2018 08:45

sunpietro suggested changes Jun 27, 2018

View reviewed changes

Nattfarinn approved these changes Jun 27, 2018

View reviewed changes

dew326 reviewed Jun 27, 2018

View reviewed changes

EZP-29179: Implement permissions for "Content/Hide" - add success mes…

a5ca200

…sage

dew326 requested changes Jun 27, 2018

View reviewed changes

tischsoic force-pushed the EZP-29179 branch from b606ae6 to 15dbe56 Compare June 27, 2018 13:19

EZP-29179: Enable changing content visibility by Ajax request using f…

2b44d18

…orm on the site.

tischsoic force-pushed the EZP-29179 branch from 15dbe56 to 2b44d18 Compare June 27, 2018 13:48

sunpietro approved these changes Jun 28, 2018

View reviewed changes

sunpietro suggested changes Jun 28, 2018

View reviewed changes

dew326 requested changes Jun 28, 2018

View reviewed changes

EZP-29179: Add small improvements to the code

f7c6c2b

sunpietro approved these changes Jun 28, 2018

View reviewed changes

dew326 approved these changes Jun 28, 2018

View reviewed changes

lserwatka merged commit 144397d into ezsystems:master Jun 28, 2018

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

Fixed fallback HTML structure for content edit sections (#538)

c415c2a

Co-authored-by: Michał Grabowski <michal.grabowski@ibexa.co>

EZP-29179: Implement permissions for "Content/Hide" #538

EZP-29179: Implement permissions for "Content/Hide" #538

Conversation

mikadamczyk commented Jun 26, 2018 • edited

Checklist:

lserwatka commented Jun 26, 2018

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

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

tischsoic Jun 27, 2018 • edited

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

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

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

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

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

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

lserwatka commented Jun 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lserwatka commented Jun 27, 2018

dew326 commented Jun 27, 2018

tischsoic commented Jun 27, 2018

sunpietro left a 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

mikadamczyk commented Jun 26, 2018 •

edited

tischsoic Jun 27, 2018 •

edited