-
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 11 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 |
|---|---|---|
|
|
@@ -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('UnhideTagsAction')) { | ||
| return $this->redirectToRoute( | ||
| 'netgen_tags_admin_tag_unhide_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); | ||
|
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. Do we need to check the submitted button and CSRF token here?
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. Yes, we do. There is an example in
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. Added CSRF token check to |
||
|
|
||
| $this->addFlashMessage('success', 'tag_hidden', ['%tagKeyword%' => $tag->keyword]); | ||
|
|
||
| return $this->redirectToTag($tag); | ||
| } | ||
|
|
||
| public function unhideAction(Request $request, Tag $tag): Response | ||
| { | ||
| $this->denyAccessUnlessGranted('ibexa:tags:unhide' . ($tag->isSynonym() ? 'synonym' : '')); | ||
|
|
||
| $this->tagsService->unhideTag($tag); | ||
|
|
||
| $this->addFlashMessage('success', 'tag_unhidden', ['%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 unhideTagsAction(Request $request, ?Tag $parentTag = null): Response | ||
| { | ||
| $this->denyAccessUnlessGranted('ibexa:tags:unhide'); | ||
|
|
||
| $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->unhideTag($tagObject); | ||
| } | ||
|
|
||
| $this->addFlashMessage('success', 'tags_unhidden'); | ||
|
|
||
| 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,14 @@ | |
| */ | ||
| public function viewAction(TagView $view): TagView | ||
| { | ||
| if ($view->getTag()->isHidden) { | ||
| throw $this->createNotFoundException('Tag is hidden.'); | ||
|
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. We don't need the message here. As this is the frontend, we do not want to communicate any revealing info. As far as the user is concerned, the tag does not exist and that's it. Also, here, you can only check for visible/invisible since when the tag is hidden, it should automatically be invisible too. That is, it is not possible for the tag to be hidden but not invisible too.
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. Removed the message: c698f26 |
||
| } | ||
|
|
||
| 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('Tag is hidden by parent.'); | ||
| } | ||
|
|
||
| return $view; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -226,6 +226,16 @@ public function newTagUpdateStruct(): TagUpdateStruct | |
| return $this->service->newTagUpdateStruct(); | ||
| } | ||
|
|
||
| public function hideTag(Tag $tag): void | ||
| { | ||
| $this->service->hideTag($tag); | ||
|
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 event dispatching here and in unhide tag method?
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. Forgot about them... added them now: 69672a5 |
||
| } | ||
|
|
||
| public function unhideTag(Tag $tag): void | ||
| { | ||
| $this->service->unhideTag($tag); | ||
| } | ||
|
|
||
| public function sudo(callable $callback, ?TagsServiceInterface $outerTagsService = null): mixed | ||
| { | ||
| return $this->service->sudo($callback, $outerTagsService ?? $this); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -844,6 +844,62 @@ | |
| $query->execute(); | ||
| } | ||
|
|
||
| public function hideTag(int $tagId): void | ||
| { | ||
| $query = $this->connection->createQueryBuilder(); | ||
| $query | ||
| ->update('eztags') | ||
| ->set('is_hidden', 1) | ||
| ->where( | ||
| $query->expr()->eq('id', ':tag_id'), | ||
| )->setParameter('tag_id', $tagId, Types::INTEGER); | ||
|
|
||
| $query->execute(); | ||
|
|
||
| $query = $this->connection->createQueryBuilder(); | ||
| $query | ||
| ->update('eztags') | ||
| ->set('is_invisible', 1) | ||
| ->where( | ||
| $query->expr()->like('path_string', ':path_string'), | ||
| ) | ||
| ->andWhere( | ||
| $query->expr()->neq('id', ':tag_id'), | ||
|
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. When hiding a tag, the hidden root tag should also be invisible, so we don't need this This is the way Ibexa works when hiding content, so there is no need to do this differently here. It will only cause confusion in the long run.
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. Ok, didn't know that this was how Ibexa works. |
||
| ) | ||
| ->setParameter('path_string', '%/' . $tagId . '/%', Types::STRING) | ||
| ->setParameter('tag_id', $tagId, Types::INTEGER); | ||
|
|
||
| $query->execute(); | ||
| } | ||
|
|
||
| public function unhideTag(int $tagId): void | ||
| { | ||
| $query = $this->connection->createQueryBuilder(); | ||
| $query | ||
| ->update('eztags') | ||
| ->set('is_hidden', 0) | ||
| ->where( | ||
| $query->expr()->eq('id', ':tag_id'), | ||
| )->setParameter('tag_id', $tagId, Types::INTEGER); | ||
|
|
||
| $query->execute(); | ||
|
|
||
| $query = $this->connection->createQueryBuilder(); | ||
| $query | ||
| ->update('eztags') | ||
| ->set('is_invisible', 0) | ||
| ->where( | ||
| $query->expr()->like('path_string', ':path_string'), | ||
| ) | ||
| ->andWhere( | ||
| $query->expr()->neq('id', ':tag_id'), | ||
|
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 remark about root tag invisibility goes here. Also, I think we need to consider here what happens when some tag is already hidden explicitly below the tag being unhidden here. In that case, we can't really make tags below the already hidden tag invisible to maintain data consistency. We should see how Ibexa does this and just copy the implementation from there.
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. |
||
| ) | ||
| ->setParameter('path_string', '%/' . $tagId . '/%', Types::STRING) | ||
| ->setParameter('tag_id', $tagId, Types::INTEGER); | ||
|
|
||
| $query->execute(); | ||
| } | ||
|
|
||
| private function createTagIdsQuery(?array $translations = null, bool $useAlwaysAvailable = true): QueryBuilder | ||
| { | ||
| $query = $this->connection->createQueryBuilder(); | ||
|
|
@@ -930,6 +986,8 @@ | |
| 'eztags.remote_id', | ||
| 'eztags.main_language_id', | ||
| 'eztags.language_mask', | ||
| 'eztags.is_hidden', | ||
| 'eztags.is_invisible', | ||
| // Tag keywords | ||
| 'eztags_keyword.keyword', | ||
| 'eztags_keyword.locale', | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -35,6 +35,8 @@ public function createTagInfoFromRow(array $row): TagInfo | |
| $tagInfo->alwaysAvailable = (bool) ((int) $row['language_mask'] & 1); | ||
| $tagInfo->mainLanguageCode = $this->languageHandler->load($row['main_language_id'])->languageCode; | ||
| $tagInfo->languageIds = $this->languageMaskGenerator->extractLanguageIdsFromMask((int) $row['language_mask']); | ||
| $tagInfo->isHidden = (bool) ((int) $row['is_hidden']); | ||
|
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. We don't need double cast here. You probably copied the thing from
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. Removed the double cast, kept only the |
||
| $tagInfo->isInvisible = (bool) ((int) $row['is_invisible']); | ||
|
|
||
| return $tagInfo; | ||
| } | ||
|
|
@@ -60,6 +62,8 @@ public function extractTagListFromRows(array $rows): array | |
| $tag->alwaysAvailable = (bool) ((int) $row['language_mask'] & 1); | ||
| $tag->mainLanguageCode = $this->languageHandler->load($row['main_language_id'])->languageCode; | ||
| $tag->languageIds = $this->languageMaskGenerator->extractLanguageIdsFromMask((int) $row['language_mask']); | ||
| $tag->isHidden = (bool) ((int) $row['is_hidden']); | ||
|
pspanja marked this conversation as resolved.
Outdated
|
||
| $tag->isInvisible = (bool) ((int) $row['is_invisible']); | ||
| $tagList[$tagId] = $tag; | ||
| } | ||
|
|
||
|
|
||
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.
Wrong exception text
If the current user is not allowed to delete this tag. Shoud be hide instead of delete.Same goes for unhide.
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.
Fixed PHPDocs: c698f26
Will probably rename 'unhide' to 'reveal', as Petar suggested below