EZP-30139: As an editor I want to hide and reveal a content item #865

pawbuj · 2019-02-28T20:27:30Z

Question	Answer
Tickets	EZP-30139
Bug fix?	no
New feature?	yes
BC breaks?	no
Tests pass?	yes
Doc needed?	yes
License	GPL-2.0

Admin UI for hide/reveal content

Checklist:

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

lserwatka · 2019-02-28T20:30:08Z

@pawbuj rebase is needed.

dew326 · 2019-03-01T07:22:56Z

You don't have to compile AlloyEditor.

pawbuj · 2019-03-01T07:43:38Z

@dew326 done

pawbuj · 2019-03-01T07:52:15Z

@lserwatka done

src/bundle/Resources/public/scss/_icons.scss

src/bundle/Resources/public/scss/_mixins.scss

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

src/bundle/Resources/views/content/tab/locations/tab.html.twig

ViniTou · 2019-03-01T08:26:09Z

src/bundle/Controller/ContentController.php

+        $contentInfo = $contentData->getContentInfo();
+        $location = $contentData->getLocation();
+
+        if ($contentInfo->isHidden) {


Should this not be handled inside data object?

ViniTou · 2019-03-01T08:26:42Z

src/bundle/Controller/ContentController.php

+        $form = $this->createForm(ContentType::class);
+        $form->handleRequest($request);
+
+        if (!$form->isSubmitted() || !$form->isValid()) {


submit handler for those?

src/bundle/Controller/ContentViewController.php

src/lib/Form/Data/Content/ContentData.php

mikadamczyk · 2019-03-01T08:12:17Z

src/bundle/Controller/ContentController.php

+        $form->handleRequest($request);
+
+        if (!$form->isSubmitted() || !$form->isValid()) {
+            throw new BadRequestHttpException();


I think we used to return information with errors

Because we can not find place where similar notification are displayed do not take this into account

mikadamczyk · 2019-03-01T08:17:51Z

src/bundle/Controller/ContentController.php

+        /** @var \EzSystems\EzPlatformAdminUi\Form\Data\Content\ContentData $contentData */
+        $contentData = $form->getData();
+        $contentInfo = $contentData->getContentInfo();
+        $location = $contentData->getLocation();


in EzSystems\EzPlatformAdminUi\Form\Data\Content\ContentData getLocation() can return null and below we have redirectToLocation(). Will not it be a problem?

mikadamczyk · 2019-03-01T08:22:48Z

src/bundle/Controller/ContentViewController.php

+    /**
+     * @param \eZ\Publish\Core\MVC\Symfony\View\ContentView $view
+     */
+    private function supplyContentReverseRelations(ContentView $view)


Suggested change

private function supplyContentReverseRelations(ContentView $view)

private function supplyContentReverseRelations(ContentView $view): void

src/bundle/Resources/public/img/ez-icons.svg

mikadamczyk · 2019-03-01T08:25:36Z

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

+                                <svg class="ez-icon ez-alert--icon">
+                                    <use xlink:href="/bundles/ezplatformadminui/img/ez-icons.svg#hide"></use>
+                                </svg>
+                                {{ 'content.hidden.message'|trans()|desc('This content item is hidden and is not publicly available') }}</div>


</div> at the end should be in the new line

mikadamczyk · 2019-03-01T08:26:36Z

src/bundle/Resources/views/content/tab/locations/tab.html.twig

-        </table>
+    <table class="ez-table table">
+        <thead>
+        <tr>


please, fix indentation

mikadamczyk · 2019-03-01T08:29:57Z

src/bundle/Resources/views/content/tab/locations/tab.html.twig

+                                    type="checkbox"
+                                    name="updateVisibility"
+                                    {% if not can_hide[location.id] %} disabled="disabled"{% endif %}
+                                    value="{{ location.id }}">


Suggested change

value="{{ location.id }}">

value="{{ location.id }}" />

mikadamczyk · 2019-03-01T08:30:07Z

src/bundle/Resources/views/content/tab/locations/tab.html.twig

+                            {{ location.main ? 'checked' }}
+                            {{ not location.canEdit ? 'disabled' }}
+                            name="updateMainLocation"
+                            value="{{ location.id }}">


Suggested change

value="{{ location.id }}">

value="{{ location.id }}" />

mikadamczyk · 2019-03-01T08:30:27Z

src/lib/Form/Data/Content/ContentData.php

+    private $location;
+
+    /**
+     * ContentData constructor.


this comment is not needed

webhdx

I don't think ContentType and ContentData is a good approach. First, ContentType is very confusing name in our domain. ContentData should not have getLocation() method inside. We tend to have specific Type+Data class for every action. I'd go for ContentVisibilityUpdateType and matching Data class.

webhdx · 2019-03-01T08:28:53Z

src/bundle/Controller/ContentViewController.php

+        $contentInfo = $view->getLocation()->getContentInfo();
+        $relations = $this->contentService->loadReverseRelations($contentInfo);
+
+        $view->addParameters(['contentHasReverseRelations' => (bool)count($relations)]);


We use underscore names in Twig. Also small nitpick: count(...) > 0 feels more logical and is easier to understand.

webhdx · 2019-03-01T08:33:43Z

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

+                <div class="ez-modal-body__main">
+                    <p>
+                        {{ 'content.hide.modal.message1'|trans()|desc('This item is being used by other content item(s). Please remove all instance(s) to avoid a 404 error.') }}<br/>
+                        {{ 'content.hide.modal.message2'|trans()|desc('If you wish to continue, please click on Confirm.') }}


Please use more meaningful translation keys.

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

alongosz · 2019-03-01T08:43:25Z

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

+            </div>
+        </div>
+    </div>
+</div>


Missing line feed at the end of this file

ezrobot · 2019-03-04T09:44:31Z

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/bundle/Controller/ContentController.php b/src/bundle/Controller/ContentController.php
index eea0e78..ac9fa7d 100644
--- a/src/bundle/Controller/ContentController.php
+++ b/src/bundle/Controller/ContentController.php
@@ -4,7 +4,6 @@
  * @copyright Copyright (C) eZ Systems AS. All rights reserved.
  * @license For full copyright and license information view LICENSE file distributed with this source code.
  */
-
 namespace EzSystems\EzPlatformAdminUiBundle\Controller;
 
 use eZ\Publish\API\Repository\ContentService;
@@ -34,7 +33,6 @@ use EzSystems\EzPlatformAdminUi\Specification\ContentType\ContentTypeIsUser;
 use Symfony\Component\HttpFoundation\RedirectResponse;
 use Symfony\Component\HttpFoundation\Request;
 use Symfony\Component\HttpFoundation\Response;
-use Symfony\Component\HttpKernel\Exception\BadRequestHttpException;
 use Symfony\Component\OptionsResolver\Exception\InvalidOptionsException;
 use Symfony\Component\Translation\Exception\InvalidArgumentException;
 use Symfony\Component\Translation\TranslatorInterface;
@@ -196,7 +194,7 @@ class ContentController extends Controller
 
                     $this->notificationHandler->success(
                         $this->translator->trans(
-                        /** @Desc("New Version Draft for '%name%' created.") */
+                            /** @Desc("New Version Draft for '%name%' created.") */
                             'content.create_draft.success',
                             ['%name%' => $contentInfo->name],
                             'content'
@@ -259,7 +257,7 @@ class ContentController extends Controller
 
                 $this->notificationHandler->success(
                     $this->translator->trans(
-                    /** @Desc("Main location for '%name%' updated.") */
+                        /** @Desc("Main location for '%name%' updated.") */
                         'content.main_location_update.success',
                         ['%name%' => $contentInfo->name],
                         'content'
@@ -357,7 +355,7 @@ class ContentController extends Controller
                 $this->contentService->updateContentMetadata($contentInfo, $contentMetadataUpdateStruct);
                 $this->notificationHandler->success(
                     $this->translator->trans(
-                    /** @Desc("Main language for '%name%' updated.") */
+                        /** @Desc("Main language for '%name%' updated.") */
                         'content.main_language_update.success',
                         ['%name%' => $contentInfo->name],
                         'content'
@@ -398,14 +396,13 @@ class ContentController extends Controller
 
         if ($form->isSubmitted()) {
             $this->submitHandler->handle($form, function (ContentVisibilityUpdateData $data) {
-
                 $contentInfo = $data->getContentInfo();
                 $newValue = $data->getVisible();
 
                 if ($contentInfo->isHidden && $newValue === false) {
                     return $this->notificationHandler->success(
                         $this->translator->trans(
-                        /** @Desc("Content '%name%' was already hidden.") */
+                            /** @Desc("Content '%name%' was already hidden.") */
                             'content.hide.already_hidden',
                             ['%name%' => $contentInfo->name],
                             'content'
@@ -416,7 +413,7 @@ class ContentController extends Controller
                 if (!$contentInfo->isHidden && $newValue === true) {
                     return $this->notificationHandler->success(
                         $this->translator->trans(
-                        /** @Desc("Content '%name%' was already visible.") */
+                            /** @Desc("Content '%name%' was already visible.") */
                             'content.reveal.already_visible',
                             ['%name%' => $contentInfo->name],
                             'content'
@@ -426,9 +423,10 @@ class ContentController extends Controller
 
                 if ($newValue === false) {
                     $this->contentService->hideContent($contentInfo);
+
                     return $this->notificationHandler->success(
                         $this->translator->trans(
-                        /** @Desc("Content '%name%' has been hidden.") */
+                            /** @Desc("Content '%name%' has been hidden.") */
                             'content.hide.success',
                             ['%name%' => $contentInfo->name],
                             'content'
@@ -438,16 +436,16 @@ class ContentController extends Controller
 
                 if ($newValue === true) {
                     $this->contentService->revealContent($contentInfo);
+
                     return $this->noti(...)

NOTICE : Report truncated to 5Kb

ezrobot · 2019-03-04T09:48:49Z

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/bundle/Controller/ContentController.php b/src/bundle/Controller/ContentController.php
index eea0e78..ac9fa7d 100644
--- a/src/bundle/Controller/ContentController.php
+++ b/src/bundle/Controller/ContentController.php
@@ -4,7 +4,6 @@
  * @copyright Copyright (C) eZ Systems AS. All rights reserved.
  * @license For full copyright and license information view LICENSE file distributed with this source code.
  */
-
 namespace EzSystems\EzPlatformAdminUiBundle\Controller;
 
 use eZ\Publish\API\Repository\ContentService;
@@ -34,7 +33,6 @@ use EzSystems\EzPlatformAdminUi\Specification\ContentType\ContentTypeIsUser;
 use Symfony\Component\HttpFoundation\RedirectResponse;
 use Symfony\Component\HttpFoundation\Request;
 use Symfony\Component\HttpFoundation\Response;
-use Symfony\Component\HttpKernel\Exception\BadRequestHttpException;
 use Symfony\Component\OptionsResolver\Exception\InvalidOptionsException;
 use Symfony\Component\Translation\Exception\InvalidArgumentException;
 use Symfony\Component\Translation\TranslatorInterface;
@@ -196,7 +194,7 @@ class ContentController extends Controller
 
                     $this->notificationHandler->success(
                         $this->translator->trans(
-                        /** @Desc("New Version Draft for '%name%' created.") */
+                            /** @Desc("New Version Draft for '%name%' created.") */
                             'content.create_draft.success',
                             ['%name%' => $contentInfo->name],
                             'content'
@@ -259,7 +257,7 @@ class ContentController extends Controller
 
                 $this->notificationHandler->success(
                     $this->translator->trans(
-                    /** @Desc("Main location for '%name%' updated.") */
+                        /** @Desc("Main location for '%name%' updated.") */
                         'content.main_location_update.success',
                         ['%name%' => $contentInfo->name],
                         'content'
@@ -357,7 +355,7 @@ class ContentController extends Controller
                 $this->contentService->updateContentMetadata($contentInfo, $contentMetadataUpdateStruct);
                 $this->notificationHandler->success(
                     $this->translator->trans(
-                    /** @Desc("Main language for '%name%' updated.") */
+                        /** @Desc("Main language for '%name%' updated.") */
                         'content.main_language_update.success',
                         ['%name%' => $contentInfo->name],
                         'content'
@@ -398,14 +396,13 @@ class ContentController extends Controller
 
         if ($form->isSubmitted()) {
             $this->submitHandler->handle($form, function (ContentVisibilityUpdateData $data) {
-
                 $contentInfo = $data->getContentInfo();
                 $newValue = $data->getVisible();
 
                 if ($contentInfo->isHidden && $newValue === false) {
                     return $this->notificationHandler->success(
                         $this->translator->trans(
-                        /** @Desc("Content '%name%' was already hidden.") */
+                            /** @Desc("Content '%name%' was already hidden.") */
                             'content.hide.already_hidden',
                             ['%name%' => $contentInfo->name],
                             'content'
@@ -416,7 +413,7 @@ class ContentController extends Controller
                 if (!$contentInfo->isHidden && $newValue === true) {
                     return $this->notificationHandler->success(
                         $this->translator->trans(
-                        /** @Desc("Content '%name%' was already visible.") */
+                            /** @Desc("Content '%name%' was already visible.") */
                             'content.reveal.already_visible',
                             ['%name%' => $contentInfo->name],
                             'content'
@@ -426,9 +423,10 @@ class ContentController extends Controller
 
                 if ($newValue === false) {
                     $this->contentService->hideContent($contentInfo);
+
                     return $this->notificationHandler->success(
                         $this->translator->trans(
-                        /** @Desc("Content '%name%' has been hidden.") */
+                            /** @Desc("Content '%name%' has been hidden.") */
                             'content.hide.success',
                             ['%name%' => $contentInfo->name],
                             'content'
@@ -438,16 +436,16 @@ class ContentController extends Controller
 
                 if ($newValue === true) {
                     $this->contentService->revealContent($contentInfo);
+
                     return $this->noti(...)

NOTICE : Report truncated to 5Kb

adamwojs · 2019-03-04T10:34:45Z

src/bundle/Controller/Controller.php

+    /**
+     * Redirects to the previous URL or to the dashboard if referer is different host.
+     *
+     * @param Request $request


Nitpick: Please use FQCN (also applicable in other places).

adamwojs · 2019-03-04T10:38:28Z

src/bundle/Controller/ContentController.php

@@ -381,4 +383,72 @@ public function updateMainTranslationAction(Request $request): Response

        return $this->redirectToRoute('ezplatform.dashboard');
    }
+
+    /**
+     * @param Request $request


Nitpick: Please use FQCN.

adamwojs · 2019-03-04T10:38:35Z

src/bundle/Controller/ContentController.php

+    /**
+     * @param Request $request
+     *
+     * @return Response


Nitpick: Please use FQCN.

src/bundle/Resources/public/scss/_modals.scss

webhdx · 2019-03-04T09:52:22Z

src/bundle/Controller/Controller.php

+     */
+    public function redirectBack(Request $request): RedirectResponse
+    {
+        $referer = $request->headers->get('referer');


I'm not convinced with this method. You don't know what's inside referrer. Even though it won't redirect you to different domain it's still not predictable.

In your case you can simply redirect user back to Content View.

webhdx · 2019-03-04T09:52:30Z

src/bundle/Controller/Controller.php

+    /**
+     * Redirects to the previous URL or to the dashboard if referer is different host
+     *
+     * @param Request $request


webhdx · 2019-03-04T10:58:30Z

src/bundle/Controller/ContentController.php

+            });
+        }
+
+        return $this->redirectBack($request);


See my other comment. You can use https://github.com/ezsystems/ezplatform-admin-ui/blob/master/src/bundle/Controller/Location/TrashLocationWithAssetController.php#L67 as a reference.

Also it will disable possibility to override redirection by EventListeners: https://github.com/ezsystems/ezplatform-admin-ui/blob/master/src/lib/Form/SubmitHandler.php#L77

ViniTou · 2019-03-04T11:30:34Z

src/bundle/Controller/ContentController.php

+                $contentInfo = $data->getContentInfo();
+                $newValue = $data->getVisible();
+
+                if ($contentInfo->isHidden && $newValue === false) {


Could those two ifs be handled inside DataForm validation?

then, the behaviour will change, I do not want to show an error to the user if everything is ok.

…ystems#865) * EZP-30139: As an editor I want to hide and reveal a content item * cs fixes and refactoring * fixes js cs * cs fixes * more cs fixes * cs fixes * fix * fixes

…tion (#865)

pawbuj added Ready for review Feature labels Feb 28, 2019

pawbuj self-assigned this Feb 28, 2019

pawbuj requested review from adamwojs, mikadamczyk, sunpietro, alongosz, webhdx, tischsoic, dew326 and ViniTou February 28, 2019 20:27

pawbuj force-pushed the EZP-30139 branch from 01d1f1a to 9d22d46 Compare March 1, 2019 07:42

pawbuj force-pushed the EZP-30139 branch from 9d22d46 to 66203e0 Compare March 1, 2019 07:51

dew326 requested changes Mar 1, 2019

View reviewed changes

ViniTou reviewed Mar 1, 2019

View reviewed changes

mikadamczyk requested changes Mar 1, 2019

View reviewed changes

webhdx suggested changes Mar 1, 2019

View reviewed changes

alongosz reviewed Mar 1, 2019

View reviewed changes

pawbuj force-pushed the EZP-30139 branch from 64da081 to 135328e Compare March 4, 2019 09:45

pawbuj requested review from mikadamczyk, webhdx, alongosz, ViniTou and dew326 March 4, 2019 09:46

pawbuj requested review from dew326 and mikadamczyk March 4, 2019 10:34

dew326 approved these changes Mar 4, 2019

View reviewed changes

adamwojs reviewed Mar 4, 2019

View reviewed changes

pawbuj requested a review from adamwojs March 4, 2019 10:58

webhdx suggested changes Mar 4, 2019

View reviewed changes

ViniTou reviewed Mar 4, 2019

View reviewed changes

pawbuj requested review from webhdx and ViniTou and removed request for webhdx March 4, 2019 13:23

webhdx approved these changes Mar 4, 2019

View reviewed changes

pawbuj force-pushed the EZP-30139 branch from c2730a3 to f116102 Compare March 4, 2019 13:39

adamwojs approved these changes Mar 4, 2019

View reviewed changes

mikadamczyk approved these changes Mar 4, 2019

View reviewed changes

pawbuj added 8 commits March 4, 2019 14:48

EZP-30139: As an editor I want to hide and reveal a content item

738f24c

cs fixes and refactoring

50db204

fixes js cs

c01e3e1

cs fixes

906c285

more cs fixes

bed8310

cs fixes

b45daa5

fix

2b4f837

fixes

c157e28

pawbuj force-pushed the EZP-30139 branch from f116102 to c157e28 Compare March 4, 2019 13:49

katarzynazawada approved these changes Mar 4, 2019

View reviewed changes

katarzynazawada added the QA approved label Mar 4, 2019

lserwatka merged commit b3a97e5 into master Mar 4, 2019

lserwatka deleted the EZP-30139 branch March 4, 2019 15:30

mateuszdebinski pushed a commit that referenced this pull request Feb 13, 2024

IBX-6260: Fixed preview voter taking language priority into considera…

4523e6b

…tion (#865)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EZP-30139: As an editor I want to hide and reveal a content item #865

EZP-30139: As an editor I want to hide and reveal a content item #865

pawbuj commented Feb 28, 2019

lserwatka commented Feb 28, 2019

dew326 commented Mar 1, 2019

pawbuj commented Mar 1, 2019

pawbuj commented Mar 1, 2019

ViniTou Mar 1, 2019

ViniTou Mar 1, 2019

mikadamczyk Mar 1, 2019

mikadamczyk Mar 1, 2019

mikadamczyk Mar 1, 2019

mikadamczyk Mar 1, 2019

mikadamczyk Mar 1, 2019

mikadamczyk Mar 1, 2019

mikadamczyk Mar 1, 2019

mikadamczyk Mar 1, 2019

mikadamczyk Mar 1, 2019

webhdx left a comment

webhdx Mar 1, 2019

webhdx Mar 1, 2019

alongosz Mar 1, 2019

ezrobot commented Mar 4, 2019

ezrobot commented Mar 4, 2019

adamwojs Mar 4, 2019

adamwojs Mar 4, 2019

adamwojs Mar 4, 2019

webhdx Mar 4, 2019

webhdx Mar 4, 2019

webhdx Mar 4, 2019 •

edited

Loading

ViniTou Mar 4, 2019

pawbuj Mar 4, 2019

	private function supplyContentReverseRelations(ContentView $view)
	private function supplyContentReverseRelations(ContentView $view): void

EZP-30139: As an editor I want to hide and reveal a content item #865

EZP-30139: As an editor I want to hide and reveal a content item #865

Conversation

pawbuj commented Feb 28, 2019

Checklist:

lserwatka commented Feb 28, 2019

dew326 commented Mar 1, 2019

pawbuj commented Mar 1, 2019

pawbuj commented Mar 1, 2019

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

webhdx 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

ezrobot commented Mar 4, 2019

ezrobot commented Mar 4, 2019

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 Mar 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webhdx Mar 4, 2019 •

edited

Loading