-
Notifications
You must be signed in to change notification settings - Fork 44
NGSTACK-906 eztags priority sorting #170
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
…greSQL database schemas
…r and update adapter implementation
…bottom of children list
* | ||
* @throws \Ibexa\Contracts\Core\Repository\Exceptions\UnauthorizedException If the current user is not allowed to read tags | ||
* | ||
* @return \Netgen\TagsBundle\API\Repository\Values\Tags\TagList | ||
*/ | ||
public function loadTagChildren(?Tag $tag = null, int $offset = 0, int $limit = -1, ?array $languages = null, bool $useAlwaysAvailable = true): TagList; | ||
public function loadTagChildren(?Tag $tag = null, int $offset = 0, int $limit = -1, ?array $languages = null, bool $useAlwaysAvailable = true, ?string $sortBy = null, ?string $sortOrder = null): TagList; |
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 we need this level of control, in contrast to just using whatever info is storred on parent 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.
I think this level of control is necessary because in the Gateway layer (getChildren()
method), the fetched data from the database needs to first be sorted and then sliced using $limit and $offset values.
Which info stored on parent tag do you mean? Perhaps another approach for sorting would be if each Tag object would have information attached to it about the sortBy and sortOrder, something like the Location object in Ibexa has (if I'm not mistaken).
EDIT: read the comment below this one. OK, I will implement the sorting mechanism similar to the Ibexa one
@@ -45,6 +45,12 @@ public function showTagAction(Request $request, ?Tag $tag = null): Response | |||
if (!$tag instanceof Tag || !$tag->isSynonym()) { | |||
$configResolver = $this->getConfigResolver(); | |||
|
|||
$sortBy = $request->query->get('sort_by'); |
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 was under the impression that the sorting mechanism will be stored on parent tag, similar to how Ibexa does it in their locations. That way, you don't need to set the sorting for every children fetch, it will automatically fetch tags with the defined sort.
Default sort can then be set in configuration.
->setMaxResults($limit > 0 ? $limit : PHP_INT_MAX); | ||
)->setParameter('parent_id', $tagId, Types::INTEGER); | ||
|
||
if ($sortBy !== null && $sortOrder !== null) { |
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 have an enum with allowed sorting property and sorting directions, so we don't need to pass direct values. This way, we would only allow sorting by predefined columns, and not besically anything the user sends down the method call chain.
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 enums for sortBy and sortOrder: 4224d33
Also used enum cases to iterate over different options when selecting the property by which the user wants to sort the tags.
$tagIdsQuery->orderBy('eztags.keyword', 'ASC'); | ||
} | ||
|
||
$tagIdsQuery |
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 don't need to separate setFirstResult
and setMaxResults
method calls from the main query builder calls.
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 put it back together, after refactoring the whole process of sorting children tags
…en() method in Gateway layer
…ren() method in Handler layer
…ildren() method in Service layer
…nd move it out of tabs folder
…d TagStruct classes
…d and sortOrder data for some tag
… DoctrineDatabase class
$sortOrder = $request->request->get('sort_order'); | ||
|
||
$tagUpdateStruct = new TagUpdateStruct(); | ||
$tagUpdateStruct->sortField = TagSortField::from((string) $sortBy); |
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 can use ::tryFrom
here, catch the exception and throw BadRequestHttpException
. Otherwise, transfering an invalid value would get you error 500.
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.
Did so here: e16f73f
@@ -128,6 +128,10 @@ public function getFullTagDataByKeywordAndParentId(string $keyword, int $parentI | |||
|
|||
public function getChildren(int $tagId, int $offset = 0, int $limit = -1, ?array $translations = null, bool $useAlwaysAvailable = true): array | |||
{ | |||
$tagData = $tagId !== 0 ? $this->getBasicTagData($tagId) : []; | |||
$sortBy = $tagData['sort_by'] ?? '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.
We should add configuration for the default sort.
We also need configuration for the default sort for root tag (which has ID = 0), since we can't store this in the database.
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 can also try to instantiate the enum to double check that the value stored in the database is an allowed stored type and direction.
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.
First four commits made on 25 June 2025 are regarding the config parameters for default sorting.
There are two new config parameters added:
- one for default sorting of all child tags (
sort.by
andsort.order
) - one for default sorting of root tag children (
sort.root.by
andsort.root.order
)
These config parameters are enums (enumNode), so their values will always match the cases of enum. There is even an option to put an enum class in the node, but that is only for Symfony 7.3 (I don't know which Symfony versions you want this bundle to support).
Also, in the database, the 'sort_by' and 'sort_order' column types were changed from enums to just varchar(100). By default, when a new tag is added, it will have empty string ('') NULL set at 'sort_by' and 'sort_order' columns - in this case, these two new config parameters will be fetched for sorting.
Only when the user sorts the children tags of some tag, then will the 'sort_by' and 'sort_order' columns be set for that (parent) tag.
This was done because the <someEnum>::tryFrom()
method returns null if the corresponding (given) string has no matching case. In that case, the configResolver is used to get a parameter. Example:
$tag->sortBy = TagSortBy::tryFrom($row['sort_by'])
?? TagSortBy::from($this->configResolver->getParameter('sort.by', 'netgen_tags'));
…enum cases in sort_children_tags template and add translation for enums
…ms don't fit the enums when updating children sorting for some tag
…varchar in database
…ing specifically root tag children
… determining sortBy and sortOrder properties
Should the dropdowns for sorting by and sorting order be added/shown to the 'Top level tags'? Or is it good enough to just be able to set the |
…tag and also show tag priority below the tag title
148a3c3 |
We don't have anywhere to store this info, so for now, the config is good enough for top level tags. |
…Tag objects and in the database
…base to properly handle the case when sortBy or sortOrder is null
…sortOrder is null and extracted that logic in a separate function
…sortOrder is null and fix TagsIntegrationTest
59beb45 84d9758 fd507dd a81d3f6 I wanted to avoid this because then, if someone changes the config params for default sorting, the sorting will not change for this one tag because (unintentionally) editing the tag already set the sorting. With the commits I pasted above, this problem is handled by making the |
…'sort_by' and 'sort_order' columns and updated the UPGRADE doc
0ce8c25 3e9628a 031c1aa |
…ty' field when adding or editing a synonym
Two main things:
priority
property to the Tag objectThe sorting functionality is added below the children list for some tag. If tag has no children, then the sorting functionality is not rendered at all.
The sorting works by modifying the
getChildren()
method in the Gateway layer. It first sorts and orders the fetched data and then slices it by using offset and limit values.