-
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?
Conversation
… in both API and persistence layer
…isible' columns in 'eztags' table
…n DoctrineDatabase, TagsMapper and Mapper classes
…anslation files and display them in admin UI
…show when a tag is hidden by parent
… hidden tags to content
/** | ||
* Hides $tag. | ||
* | ||
* @throws \Ibexa\Contracts\Core\Repository\Exceptions\UnauthorizedException If the current user is not allowed to delete this 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
@@ -14,6 +14,14 @@ final class TagViewController extends Controller | |||
*/ | |||
public function viewAction(TagView $view): TagView | |||
{ | |||
if ($view->getTag()->isHidden) { | |||
throw $this->createNotFoundException('Tag is hidden.'); |
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.
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.
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.
Removed the message: c698f26
@@ -226,6 +226,16 @@ public function newTagUpdateStruct(): TagUpdateStruct | |||
return $this->service->newTagUpdateStruct(); | |||
} | |||
|
|||
public function hideTag(Tag $tag): void | |||
{ | |||
$this->service->hideTag($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.
No event dispatching here and in unhide tag 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.
Forgot about them... added them now: 69672a5
$query->expr()->like('path_string', ':path_string'), | ||
) | ||
->andWhere( | ||
$query->expr()->neq('id', ':tag_id'), |
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.
When hiding a tag, the hidden root tag should also be invisible, so we don't need this neq
condition.
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.
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.
Ok, didn't know that this was how Ibexa works.
Removed the neq
: f8bcf8d
$query->expr()->like('path_string', ':path_string'), | ||
) | ||
->andWhere( | ||
$query->expr()->neq('id', ':tag_id'), |
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.
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.
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.
@@ -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']); |
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.
We don't need double cast here. You probably copied the thing from (bool) ((int) $row['language_mask'] & 1)
, but that has double cast due to bitwise and operator.
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.
Removed the double cast, kept only the bool
one: e5b2f5f
@@ -9,3 +9,5 @@ tags: | |||
deletesynonym: ~ | |||
makesynonym: ~ | |||
merge: ~ | |||
hide: ~ |
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.
You're missing hidesysnonym
and unhidesynonym
policies here. Also do we really need policies for both hide and 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.
Added hidesynonym
and unhidesynonym
policies: 7e88bdd
I also thought that just one policy for hiding tags is enough, but I made two just for clarity.
If you want, I can only leave 'hide' and 'hidesynonym' policies, which will mean that the user can both hide and reveal tags.
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 any changes about this? Should I just make one policy (e.g. visibility) or leave it as it is now?
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 one is enough. I don't see a usecase where someone can only hide, but not reveal and vice versa.
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 also remove the "synonym" variant too, which would leave only one hide
policy for everything.
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.
Left only the 'hide' policy, removed the rest: 71a9102
> | ||
{{ tag.keyword }} | ||
{% if tag.isHidden %} | ||
(hidden) |
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.
Needs translations 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.
Added translations here for '(hidden)' and '(hidden by parent)': dece52c
…e schemas and fix MapperTest
…g shown to the user
…visible' fields in Mapper
…latable in 'eztags' content field
…sible when revealing a tag
…and descendant hidden tags
…nd remove double cast from the Mapper
@AntePrkacin you can add upgrade instructions in Use |
Updated the |
@@ -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 comment
The 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 moveTagsAction
method. Same goes for revealTagsAction
.
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 OK. Just to confirm that I understood correctly - I will remove the 'Hide selected tags' and 'Reveal selected tags' buttons below the children list - childrenAction()
method. Instead, I will put two new buttons (somewhere else) that tell 'Hide kids' or 'Reveal kids'. When either of them is pressed, a multiselect form will show up where the user will be able to select the desired tags they want to hide or reveal.
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.
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 MoveTagsType
, try to reuse it by renaming it to e.g. MultiselectTagsType
or similar.
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.
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.
@AntePrkacin Sounds good 👍
], | ||
], | ||
]; | ||
} | ||
|
||
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 comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't mix escaping and formating. That is, leave escape
method as is, and just reuse is in the new formatTagTreeText
method. After all, all of the text should be escaped AFTER formatting it, and not just the starting text.
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.
Separated the escape logic from the formatting logic: 4a174cc
@@ -154,6 +161,13 @@ public function getChildren(int $tagId, int $offset = 0, int $limit = -1, ?array | |||
} | |||
|
|||
$query = $this->createTagFindQuery($translations, $useAlwaysAvailable); | |||
|
|||
if ($showHidden !== null && $showHidden === false) { |
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.
If $showHidden
were not nullable, as mentioned before, you wouldn't need double check here and in other places.
@@ -155,6 +155,10 @@ public function getConfigTreeBuilder(): TreeBuilder | |||
->defaultValue(25) | |||
->end() | |||
->end() | |||
->end() | |||
->booleanNode('show_hidden') |
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.
We should be explicit and name this show_hidden_tags
.
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.
Renamed show_hidden
to show_hidden_tags
here in the ConfigTree as well as in Service, Handler and Gateway layers (renamed parameter $showHidden
to $showHiddenTags
): 6692248
@@ -9,6 +9,8 @@ CREATE TABLE `eztags` ( | |||
`remote_id` varchar(100) NOT NULL default '', | |||
`main_language_id` int(11) NOT NULL default '0', | |||
`language_mask` int(11) NOT NULL default '0', | |||
`is_hidden` tinyint NOT NULL DEFAULT '0', |
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.
We also need MySQL and PostgreSQL upgrade scripts (see bundle/Resources/sql/upgrade
folder).
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.
Added MySQL and PostgreSQL upgrade scripts: 203dcd6
doc/UPGRADE.md
Outdated
If you're using MySQL, run the commands below: | ||
|
||
```sql | ||
ALTER TABLE `eztags` ADD COLUMN `is_hidden` TINYINT NOT NULL DEFAULT 0; |
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.
These should go to an upgrade SQL script as mentioned before and just referenced here.
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.
Added MySQL and PostgreSQL upgrade scripts and referenced them here (doc/UPGRADE.md
): 203dcd6
… and revealing a tag with CSRF token check
…eyword in TreeController
…igTree, Service, Handler and Gateway layers
…and 'is_invisible' columns
…add 'show_parent_field' option to the form
…Controller to use MultiselectTagsType form and render new templates
…onfigTreeBuilder and contextualize it
…look at 'autocomplete_provide_hidden_tags' config parameter
@emodric depends a bit on how we solve some other open questions, but we should probably take care that non-visible tags are not indexed. |
…n searching to add a tag to some content
…Netgen Tags adminUI
… and add the ability to select hidden tags in those processes
…ds that have showHiddenTags as parameter in DoctrineDatabase
fd4e11f The same is valid when merging a tag - the user can select a hidden tag to which they want to merge some tag. |
Added
isHidden
andisInvisible
fields to Tag objects. Updated the database schema and updated the whole workflow for hiding and unhiding a tag (Service, Mapper, Handler, Gateway, ...).Added columns 'Hidden' and 'Invisible' to table that show children tags of some tag in admin interface.
In admin interface added button for hiding a tag (or if a tag is already hidden, then this button will unhide it).
Also added buttons 'Hide selected tags' and 'Unhide selected tags' for the checked children tags.
When a tag is hidden, the route
/tags/view/Topics/{tag_name}
will throw 404 and return information that 'Tag is hidden' or 'Tag is hidden by parent'.If a tag is hidden, then the user won't be able to add that tag to some 'eztags' content field for some content.
If some content already has a hidden tag, then this tag will have ' (hidden)' appended to it and it will be in gray color.