-
Notifications
You must be signed in to change notification settings - Fork 45
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 37 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, | ||
|
||
| ); | ||
|
|
||
| $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); | ||
|
||
|
|
||
| $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); | ||
|
||
|
|
||
| $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 | ||
|
Member
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
Author
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 -
Member
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
Author
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.
Member
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_reveal'); | ||
pspanja marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| 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 |
|---|---|---|
|
|
@@ -14,6 +14,10 @@ final class TagViewController extends Controller | |
| */ | ||
| public function viewAction(TagView $view): TagView | ||
| { | ||
| if ($view->getTag()->isInvisible) { | ||
|
Member
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 should this include the check for the |
||
| throw $this->createNotFoundException(); | ||
| } | ||
|
|
||
| return $view; | ||
| } | ||
| } | ||
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
$showHiddenhere, 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 = falseis 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
nullis interpretedtrue(orfalse), and then it's kind of OK :)