-
Notifications
You must be signed in to change notification settings - Fork 44
NGSTACK-977 tags visibility #169
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
base: master
Are you sure you want to change the base?
Changes from 41 commits
0c21b61
71034c0
981a985
d3dc59e
62ba8ad
0ee8a42
033de91
b47e3a5
302837d
fcabf76
402ca4c
f12c262
9813914
9987286
c698f26
f8bcf8d
e5b2f5f
7e88bdd
dece52c
543afc4
8b20a72
54ec1e9
a61028b
45b2f03
e50dfdb
69672a5
086e954
4af3ecd
0f8c7dd
75858b8
d99a9d4
d09d660
a6d7415
eed05e2
37da894
e69b243
514e3d0
81d4007
a720391
a69c38c
08ef43d
550f0e3
4a174cc
6692248
203dcd6
ec62925
c05ab16
07866bc
535890e
96279be
9a00e4e
cf15dfc
fd4e11f
c81ccef
45cf8e6
50a4c29
40605b9
88fc80e
71a9102
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,18 @@ | ||
<?php | ||
|
||
declare(strict_types=1); | ||
|
||
namespace Netgen\TagsBundle\API\Repository\Events\Tags; | ||
|
||
use Ibexa\Contracts\Core\Repository\Event\BeforeEvent; | ||
use Netgen\TagsBundle\API\Repository\Values\Tags\Tag; | ||
|
||
class BeforeHideTagEvent extends BeforeEvent | ||
{ | ||
public function __construct(private readonly Tag $tag) {} | ||
|
||
public function getTag(): Tag | ||
{ | ||
return $this->tag; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
<?php | ||
|
||
declare(strict_types=1); | ||
|
||
namespace Netgen\TagsBundle\API\Repository\Events\Tags; | ||
|
||
use Ibexa\Contracts\Core\Repository\Event\BeforeEvent; | ||
use Netgen\TagsBundle\API\Repository\Values\Tags\Tag; | ||
|
||
class BeforeRevealTagEvent extends BeforeEvent | ||
{ | ||
public function __construct(private readonly Tag $tag) {} | ||
|
||
public function getTag(): Tag | ||
{ | ||
return $this->tag; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
<?php | ||
|
||
declare(strict_types=1); | ||
|
||
namespace Netgen\TagsBundle\API\Repository\Events\Tags; | ||
|
||
use Ibexa\Contracts\Core\Repository\Event\AfterEvent; | ||
use Netgen\TagsBundle\API\Repository\Values\Tags\Tag; | ||
|
||
class HideTagEvent extends AfterEvent | ||
{ | ||
public function __construct(private readonly Tag $tag) {} | ||
|
||
public function getTag(): Tag | ||
{ | ||
return $this->tag; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
<?php | ||
|
||
declare(strict_types=1); | ||
|
||
namespace Netgen\TagsBundle\API\Repository\Events\Tags; | ||
|
||
use Ibexa\Contracts\Core\Repository\Event\AfterEvent; | ||
use Netgen\TagsBundle\API\Repository\Values\Tags\Tag; | ||
|
||
class RevealTagEvent extends AfterEvent | ||
{ | ||
public function __construct(private readonly Tag $tag) {} | ||
|
||
public function getTag(): Tag | ||
{ | ||
return $this->tag; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -37,6 +37,7 @@ public function autoCompleteAction(Request $request): JsonResponse | |
$searchResult = $this->tagsService->searchTags( | ||
$request->query->get('searchString') ?? '', | ||
$request->query->get('locale') ?? '', | ||
showHidden: false, | ||
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. Should this be controlled through the configuration as well? 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 hardcoded here false because the 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. 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 would like to agree with @AntePrkacin on this one that we should always avoid outputting hidden tags in the autocomplete actions like searching for tags in tag fields, but there can be some quite silly edge-cases where editor would hide the tag so tags/view page is not visible yet, but they would like to start filling in the content with those hidden tags before revealing the tag all-together. For sure this configuration parameter must be set to false as default, but for those pesky edge-cases, a parameter is needed. 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. Yes, parameter is definitely needed. Also, please do not use named parameters, at least not mixed like this, it just looks wrong :D Either change all of them to use named parameters, or none. 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. Added config parameter called Also, just to follow up on this discussion - if some tag is hidden by parent (so it is invisible, but not hidden), should the autocomplete method show this tag or not? Probably not, right? 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. Hidden and invisible is, for all intents and purposes, basically the same. The difference comes from hiding/unhiding procedure, since you need to care about whether one tag was hidden automatically or manually. 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. Ok, thanks! |
||
); | ||
|
||
$data = $data = $this->filterTags($searchResult->tags, $subTreeLimit, $hideRootTag); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -500,9 +500,49 @@ public function childrenAction(Request $request, ?Tag $tag = null): Response | |
); | ||
} | ||
|
||
if ($request->request->has('HideTagsAction')) { | ||
return $this->redirectToRoute( | ||
'netgen_tags_admin_tag_hide_tags', | ||
[ | ||
'parentId' => $tag?->id ?? 0, | ||
], | ||
); | ||
} | ||
|
||
if ($request->request->has('RevealTagsAction')) { | ||
return $this->redirectToRoute( | ||
'netgen_tags_admin_tag_reveal_tags', | ||
[ | ||
'parentId' => $tag?->id ?? 0, | ||
], | ||
); | ||
} | ||
|
||
return $this->redirect($request->getPathInfo()); | ||
} | ||
|
||
public function hideAction(Request $request, Tag $tag): Response | ||
{ | ||
$this->denyAccessUnlessGranted('ibexa:tags:hide' . ($tag->isSynonym() ? 'synonym' : '')); | ||
|
||
$this->tagsService->hideTag($tag); | ||
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. Do we need to check the submitted button and CSRF token here? 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. Yes, we do. There is an example in 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. Added CSRF token check to |
||
|
||
$this->addFlashMessage('success', 'tag_hidden', ['%tagKeyword%' => $tag->keyword]); | ||
|
||
return $this->redirectToTag($tag); | ||
} | ||
|
||
public function revealAction(Request $request, Tag $tag): Response | ||
{ | ||
$this->denyAccessUnlessGranted('ibexa:tags:reveal' . ($tag->isSynonym() ? 'synonym' : '')); | ||
|
||
$this->tagsService->revealTag($tag); | ||
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. Do we need to check the submitted button and CSRF token here? 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. Yes, we do. There is an example in 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. Added CSRF token check to |
||
|
||
$this->addFlashMessage('success', 'tag_reveal', ['%tagKeyword%' => $tag->keyword]); | ||
|
||
return $this->redirectToTag($tag); | ||
} | ||
|
||
/** | ||
* This method is called from a form containing all children tags of a tag. | ||
* It shows a confirmation view. | ||
|
@@ -666,6 +706,60 @@ public function deleteTagsAction(Request $request, ?Tag $parentTag = null): Resp | |
); | ||
} | ||
|
||
public function hideTagsAction(Request $request, ?Tag $parentTag = null): Response | ||
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. Same here with CSRF. What we need to do here is create a Symfony multiselect form for selecting the tags in lieu of 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. @emodric OK. Just to confirm that I understood correctly - I will remove the 'Hide selected tags' and 'Reveal selected tags' buttons below the children list - 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. No no, do it in the same way as it is now, but with Symfony form in lieu of "move tags". Try and see if you can reuse the existing form type. It's now called 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. 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. @AntePrkacin Sounds good 👍 |
||
{ | ||
$this->denyAccessUnlessGranted('ibexa:tags:hide'); | ||
|
||
$tagIds = (array) $request->request->get( | ||
'Tags', | ||
$request->hasSession() ? $request->getSession()->get('ngtags_tag_ids') : [], | ||
); | ||
|
||
if (count($tagIds) === 0) { | ||
return $this->redirectToTag($parentTag); | ||
} | ||
|
||
$tags = []; | ||
foreach ($tagIds as $tagId) { | ||
$tags[] = $this->tagsService->loadTag((int) $tagId); | ||
} | ||
|
||
foreach ($tags as $tagObject) { | ||
$this->tagsService->hideTag($tagObject); | ||
} | ||
|
||
$this->addFlashMessage('success', 'tags_hidden'); | ||
|
||
return $this->redirectToTag($parentTag); | ||
} | ||
|
||
public function revealTagsAction(Request $request, ?Tag $parentTag = null): Response | ||
{ | ||
$this->denyAccessUnlessGranted('ibexa:tags:reveal'); | ||
|
||
$tagIds = (array) $request->request->get( | ||
'Tags', | ||
$request->hasSession() ? $request->getSession()->get('ngtags_tag_ids') : [], | ||
); | ||
|
||
if (count($tagIds) === 0) { | ||
return $this->redirectToTag($parentTag); | ||
} | ||
|
||
$tags = []; | ||
foreach ($tagIds as $tagId) { | ||
$tags[] = $this->tagsService->loadTag((int) $tagId); | ||
} | ||
|
||
foreach ($tags as $tagObject) { | ||
$this->tagsService->revealTag($tagObject); | ||
} | ||
|
||
$this->addFlashMessage('success', 'tags_revealed'); | ||
|
||
return $this->redirectToTag($parentTag); | ||
} | ||
|
||
public function searchTagsAction(Request $request): Response | ||
{ | ||
$this->denyAccessUnlessGranted('ibexa:tags:read'); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,6 +12,7 @@ | |
use Symfony\Contracts\Translation\TranslatorInterface; | ||
|
||
use function htmlspecialchars; | ||
use function mb_strtolower; | ||
use function str_replace; | ||
|
||
use const ENT_HTML401; | ||
|
@@ -44,6 +45,8 @@ public function __construct( | |
'merge_tag' => $this->translator->trans('tag.tree.merge_tag', [], 'netgen_tags_admin'), | ||
'convert_tag' => $this->translator->trans('tag.tree.convert_tag', [], 'netgen_tags_admin'), | ||
'add_synonym' => $this->translator->trans('tag.tree.add_synonym', [], 'netgen_tags_admin'), | ||
'hide_tag' => $this->translator->trans('tag.tree.hide_tag', [], 'netgen_tags_admin'), | ||
'reveal_tag' => $this->translator->trans('tag.tree.reveal_tag', [], 'netgen_tags_admin'), | ||
]; | ||
|
||
$this->treeLinks = [ | ||
|
@@ -55,6 +58,8 @@ public function __construct( | |
'merge_tag' => $this->router->generate('netgen_tags_admin_tag_merge', ['tagId' => ':tagId']), | ||
'convert_tag' => $this->router->generate('netgen_tags_admin_tag_convert', ['tagId' => ':tagId']), | ||
'add_synonym' => $this->router->generate('netgen_tags_admin_synonym_add_select', ['mainTagId' => ':mainTagId']), | ||
'hide_tag' => $this->router->generate('netgen_tags_admin_tag_hide', ['tagId' => ':tagId']), | ||
'reveal_tag' => $this->router->generate('netgen_tags_admin_tag_reveal', ['tagId' => ':tagId']), | ||
]; | ||
} | ||
|
||
|
@@ -119,13 +124,13 @@ private function getRootTreeData(): array | |
*/ | ||
private function getTagTreeData(Tag $tag, bool $isRoot = false): array | ||
{ | ||
$synonymCount = $this->tagsService->getTagSynonymCount($tag); | ||
|
||
return [ | ||
'id' => $tag->id, | ||
'parent' => $isRoot ? '#' : $tag->parentTagId, | ||
'text' => $synonymCount > 0 ? $this->escape($tag->keyword) . ' (+' . $synonymCount . ')' : $this->escape($tag->keyword), | ||
'text' => $this->formatTagTreeText($tag), | ||
'children' => $this->tagsService->getTagChildrenCount($tag) > 0, | ||
'hidden' => $tag->isHidden, | ||
'invisible' => $tag->isInvisible, | ||
'a_attr' => [ | ||
'href' => str_replace(':tagId', (string) $tag->id, $this->treeLinks['show_tag']), | ||
'rel' => $tag->id, | ||
|
@@ -165,13 +170,37 @@ private function getTagTreeData(Tag $tag, bool $isRoot = false): array | |
'url' => str_replace(':tagId', (string) $tag->id, $this->treeLinks['convert_tag']), | ||
'text' => $this->treeLabels['convert_tag'], | ||
], | ||
[ | ||
'name' => 'hide_tag', | ||
'url' => str_replace(':tagId', (string) $tag->id, $this->treeLinks['hide_tag']), | ||
'text' => $this->treeLabels['hide_tag'], | ||
], | ||
[ | ||
'name' => 'reveal_tag', | ||
'url' => str_replace(':tagId', (string) $tag->id, $this->treeLinks['reveal_tag']), | ||
'text' => $this->treeLabels['reveal_tag'], | ||
], | ||
], | ||
], | ||
]; | ||
} | ||
|
||
private function escape(string $string): string | ||
private function formatTagTreeText(Tag $tag): string | ||
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. We shouldn't mix escaping and formating. That is, leave 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. Separated the escape logic from the formatting logic: 4a174cc |
||
{ | ||
return htmlspecialchars($string, ENT_QUOTES | ENT_SUBSTITUTE | ENT_HTML401, 'UTF-8'); | ||
$synonymCount = $this->tagsService->getTagSynonymCount($tag); | ||
|
||
$text = htmlspecialchars($tag->keyword, ENT_QUOTES | ENT_SUBSTITUTE | ENT_HTML401, 'UTF-8'); | ||
|
||
if ($tag->isHidden) { | ||
$text .= ' (' . mb_strtolower($this->translator->trans('tag.hidden', [], 'netgen_tags_admin')) . ')'; | ||
} elseif ($tag->isInvisible) { | ||
$text .= ' (' . $this->translator->trans('tag.hidden_by_parent', [], 'netgen_tags_admin') . ')'; | ||
} | ||
|
||
if ($synonymCount > 0) { | ||
$text .= ' (+' . $synonymCount . ')'; | ||
} | ||
|
||
return $text; | ||
} | ||
} |
Uh oh!
There was an error while loading. Please reload this page.
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.
Possibly we should have a default value for
$showHidden
here, instead of resolving it in the Persistence implementation. @emodric I'll leave that up to you :)(Then the same applies for the similar cases below)
Edit: ignore the above, I think the parameter should not be optional in the Persistence layer, we probably need it to be optional here so we can properly handle it in the siteaccess-aware implementation.
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.
I agree with @pspanja , however, nullable bool seems weird. Either we show or hide hidden tags, there's no third option for
null
. Sobool $showHidden = false
is probably good enough.Uh oh!
There was an error while loading. Please reload this page.
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.
@emodric one question though, if here the default is not
null
, how do we detect explicitly passed value in the siteaccess-aware layer?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.
Hm. Good point. Although, do we even have the need for explicit API layer control of this flag? Can we rely only on siteaccess aware config?
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.
I would say we do, this is not only for rendering, it might be used in import for example.
Uh oh!
There was an error while loading. Please reload this page.
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.
Do I then leave it to be null by default or should I change it?
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.
I think this is okay, but let @pspanja confirm too.
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.
In general I'm OK with it, but I'm not clear about this:
That should definitely not be the case, if the value was passed to the API method call, it must not be overridden somewhere below. That's why in the original comment I suggested making the parameter mandatory in the Persistence layer. It should be controlled from API layer only - either through siteaccess aware layer or by being explicitly given when calling the method.
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.
What I meant was, it looks like API interface is assuming the implementation, or at least one of them.
In terms of API, we either show hidden tags, or don't show hidden tags, there's no third option. And then along comes the third option, that says: "let the implementation decide", which is kinda ambiguous since the user does not know what the end result will be. That's why I'm saying that this API interface is looking weird.
But no matter, we already decided that this is relatively fine, so lets just move on :)
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.
As mentioned f2f, we need to document that
null
is interpretedtrue
(orfalse
), and then it's kind of OK :)