EZP-31650: Added Criterions handling for search in trash #80

michal-myszka · 2020-06-23T07:33:02Z

Question	Answer
JIRA issue	EZP-31650
Type	feature
Target eZ Platform version	`v3.1`
BC breaks	no
Tests pass	yes
Doc needed	no

Added handling criterias for trash list

Checklist:

PR description is updated.
Old tests update
Tests are implemented.
Added code follows Coding Standards (use $ composer fix-cs).
PR is ready for a review.

eZ/Publish/Core/Base/Container/Compiler/Search/Legacy/CriteriaConverterPass.php

eZ/Publish/Core/Search/Legacy/Content/Trash/Gateway/CriteriaConverter.php

adamwojs · 2020-06-23T11:15:09Z

Would be great to have some integration tests 😉

eZ/Publish/Core/Persistence/Legacy/Content/Location/Gateway/DoctrineDatabase.php

alongosz

There are several architecture-related, but naming-wise and DX issues:

While PMs define this feature as "Search in trash", conceptually this is not LSE, nor any search engine. It's very misleading to declare it like that and use Search* namespace for Legacy Storage implementations (e.g. eZ/Publish/Core/Search/Legacy/Content/Common/Gateway/SortClauseHandler/Trash/*). The parts, e.g. Criteria, re-used from LSE could stay as-is. Not happy about mixing things, but better to keep it DRY probably.
The set of supported Criteria and Sort Clauses is limited. It's going to be very difficult for Developers to understand which ones are supported. It's fine to refer them do the doc as long as they know they should. However since findTrashItems accepts Query, it gives an impression that all of them are supported. Not sure if anything can be done for the method signature due to BC[1], but each SortClause and Criterion should implement marker interface to indicate that it supports trash filtering (I'm really not fine with calling it search). E.g.: eZ\Publish\SPI\Repository\Values\Trash\Criterion and eZ\Publish\SPI\Repository\Values\Trash\SortClause respectively - the similar concept exists for EZP-31569: Implemented Repository filtering #54
ezpublish from new container definitions/parameters needs to disappear regardless of 1. & 2.

[1] Maybe PHPDoc improvements only? Or maybe, since those criteria never worked anyway, it would be fine to make signature more strict? Or maybe to be implemented as a separate method.

alongosz · 2020-06-26T12:09:43Z

eZ/Publish/API/Repository/Values/Content/Query/Criterion/DateMetadata.php

     * @param string $operator One of the Operator constants
     * @param mixed $value The match value, either as an array of as a single value, depending on the operator
     */
    public function __construct(string $target, string $operator, $value)
    {
-        if ($target != self::MODIFIED && $target != self::CREATED) {
+        if (!in_array($target, [self::MODIFIED, self::CREATED, self::TRASHED])) {


Could you extract [self::MODIFIED, self::CREATED, self::TRASHED] as a private constant? e.g.:

Suggested change

if (!in_array($target, [self::MODIFIED, self::CREATED, self::TRASHED])) {

if (!in_array($target, self::TARGETS)) {

alongosz · 2020-06-26T12:12:35Z

eZ/Publish/API/Repository/Values/Content/Query/Criterion/DateMetadata.php

    /**
     * Creates a new DateMetadata criterion on $metadata.
     *
     * @throws \InvalidArgumentException If target is unknown
     *
-     * @param string $target One of DateMetadata::CREATED or DateMetadata::MODIFIED
+     * @param string $target One of DateMetadata::CREATED, DateMetadata::MODIFIED or DateMetadata::TRASHED (works only with LSE)


This is not true, LSE does not cover searching in Trash at all as no Search engine does. Conceptually search in trash is not a part of LSE.
What could be safer is:

Suggested change

* @param string $target One of DateMetadata::CREATED, DateMetadata::MODIFIED or DateMetadata::TRASHED (works only with LSE)

* @param string $target One of DateMetadata::CREATED, DateMetadata::MODIFIED or DateMetadata::TRASHED (applies to TrashService::findTrashItems only).

*

* @see \eZ\Publish\API\Repository\TrashService::findTrashItems

alongosz · 2020-06-26T12:16:40Z

eZ/Publish/Core/settings/search_engines/legacy/criterion_handlers_common.yml

@@ -36,6 +36,10 @@ parameters:
            - "tab_search_normalize"

 services:
+    ezplatform.search.legacy.gateway.criteria_converter.trash:


again, this is not search in terms of our domain definition, though indeed feature is called search in trash (which should be trash filtering, but what can I do...).

Suggested change

ezplatform.search.legacy.gateway.criteria_converter.trash:

ezplatform.trash.search.legacy.gateway.criteria_converter:

alongosz · 2020-06-26T12:17:10Z

eZ/Publish/Core/settings/search_engines/legacy/criterion_handlers_common.yml

@@ -86,6 +90,7 @@ services:
        tags:
            - {name: ezpublish.search.legacy.gateway.criterion_handler.content}
            - {name: ezpublish.search.legacy.gateway.criterion_handler.location}
+            - {name: ezplatform.search.legacy.gateway.criterion_handler.trash}


Suggested change

- {name: ezplatform.search.legacy.gateway.criterion_handler.trash}

- {name: ezplatform.trash.search.legacy.gateway.criterion_handler.trash}

alongosz · 2020-06-26T12:19:43Z

eZ/Publish/Core/settings/search_engines/legacy/sort_clause_handlers_common.yml

@@ -4,6 +4,10 @@ services:
        arguments:
            $connection: '@ezpublish.persistence.connection'

+    ezpublish.search.legacy.gateway.sort_clause_converter.trash:


Suggested change

ezpublish.search.legacy.gateway.sort_clause_converter.trash:

ezplatform.trash.search.legacy.gateway.sort_clause_converter:

alongosz · 2020-06-26T12:23:04Z

...ublish/Core/Search/Legacy/Content/Common/Gateway/SortClauseHandler/Trash/ContentTypeName.php

+        array $languageSettings
+    ): void {
+        $query->innerJoin(
+            'c', ContentTypeGateway::CONTENT_TYPE_TABLE, 'cc', 'c.contentclass_id = cc.id'


why cc for Content Type? ct, content_type, type I would understand

Suggested change

'c', ContentTypeGateway::CONTENT_TYPE_TABLE, 'cc', 'c.contentclass_id = cc.id'

'c', ContentTypeGateway::CONTENT_TYPE_TABLE, 'ct', 'c.contentclass_id = ct.id'

alongosz · 2020-06-26T12:28:13Z

eZ/Publish/Core/Persistence/Legacy/User/Gateway/DoctrineDatabase.php

@@ -25,6 +25,8 @@
 */
 final class DoctrineDatabase extends Gateway
 {
+    public const USER_TABLE = 'ezuser';


Those should be defined on the Gateway, not implementation because Gateway itself applies to Legacy Storage only. Past implementations are a bit inconsistent, but the most recent one, regarding switch from eZc Database Handler to Doctrine shows the correct path.

alongosz · 2020-06-26T12:30:08Z

eZ/Publish/Core/Persistence/Legacy/Content/Location/Gateway/DoctrineDatabase.php

-        'location_path' => 'path_string',
-        'trashed' => 'trashed',
-    ];
+    public const TRASH_TABLE = 'ezcontentobject_trash';


same comment as before - belongs to Gateway.

alongosz · 2020-06-26T12:38:41Z

eZ/Publish/API/Repository/Tests/TrashServiceTest.php

@@ -669,7 +669,7 @@ public function testFindTrashItems()
        $query = new Query();
        $query->filter = new Criterion\LogicalAnd(
            [
-                new Criterion\Field('title', Criterion\Operator::LIKE, '*'),
+                new Criterion\ContentTypeId(1),


There are more Criteria supported by Trash filtering, but the coverage is missing.

michal-myszka · 2020-06-26T13:17:51Z

There are several architecture-related, but naming-wise and DX issues:

While PMs define this feature as "Search in trash", conceptually this is not LSE, nor any search engine. It's very misleading to declare it like that and use Search* namespace for Legacy Storage implementations (e.g. eZ/Publish/Core/Search/Legacy/Content/Common/Gateway/SortClauseHandler/Trash/*). The parts, e.g. Criteria, re-used from LSE could stay as-is. Not happy about mixing things, but better to keep it DRY probably.

The set of supported Criteria and Sort Clauses is limited. It's going to be very difficult for Developers to understand which ones are supported. It's fine to refer them do the doc as long as they know they should. However since findTrashItems accepts Query, it gives an impression that all of them are supported. Not sure if anything can be done for the method signature due to BC[1], but each SortClause and Criterion should implement marker interface to indicate that it supports trash filtering (I'm really not fine with calling it search). E.g.: eZ\Publish\SPI\Repository\Values\Trash\Criterion and eZ\Publish\SPI\Repository\Values\Trash\SortClause respectively - the similar concept exists for #54

ezpublish from new container definitions/parameters needs to disappear regardless of 1. & 2.

Yes, that's right, this is not strict "search" feature in our project mind. I tried to avoid the situations you are writing about, I even made two conceptual approaches, but finally without BC breaks they proved impossible. Let's talk on Zoom first.

alongosz · 2020-06-26T14:21:31Z

Note: agreed on MVP during internal sync.

alongosz

+1
Some minor nitpicks:

alongosz · 2020-06-29T08:48:23Z

eZ/Publish/API/Repository/Tests/TrashServiceTest.php

@@ -1121,4 +1283,41 @@ private function getTrashItemDouble(int $trashId, int $contentId = 44, int $pare
            'contentInfo' => new ContentInfo(['id' => $contentId]),
        ]);
    }
+
+    private function trashDifferentContents(): void


plural of a content is content Items ;)

Suggested change

private function trashDifferentContents(): void

private function trashDifferentContentItems(): void

alongosz · 2020-06-29T09:01:03Z

eZ/Publish/Core/Search/Legacy/Content/Common/Gateway/SortClauseHandler/Trash/UserName.php

+use eZ\Publish\Core\Search\Legacy\Content\Common\Gateway\SortClauseHandler;
+use eZ\Publish\API\Repository\Values\Content\Query\SortClause;
+
+class UserName extends SortClauseHandler


Nitpick: all handlers should be final and @internal (I mean new ones).

alongosz · 2020-06-29T09:02:52Z

eZ/Publish/Core/Search/Legacy/Content/Common/Gateway/CriterionHandler/DateMetadata.php

+            default:
+                return 'c.' . Criterion\DateMetadata::PUBLISHED;


Kinda risky. What if Developer passes something completely different?
We tend to do throw new InvalidArgumentException or RuntimeException on default.

That's for save old logic (before refactor)

adamwojs · 2020-06-29T08:42:59Z

eZ/Publish/API/Repository/Tests/TrashServiceTest.php

+
+        $ascQuery = new Query();
+        foreach ($sortClausesClasses as $sortClauseClass) {
+            $ascQuery->sortClauses[] = new $sortClauseClass(Query::SORT_ASC);


Pass \eZ\Publish\API\Repository\Values\Content\Query\SortClause instance instead of dynamically create instances from class string

It's intended because I need to pass different arguments to sortClause instance

adamwojs · 2020-06-29T08:53:59Z

eZ/Publish/API/Repository/Tests/TrashServiceTest.php

+            $ascResultsIds[] = $result->contentInfo->id;
+        }
+
+        $this->assertGreaterThanOrEqual(2, count($ascResultsIds));


This assert is not guarantee that trash find results are in correct order.

$this->assertEquals([1, 2, 3], $ascResultsIds);

This assert doesn't check the order, but this is for requiring 2 items in result. Checking sort is in the next steps below

adamwojs · 2020-06-29T08:54:59Z

eZ/Publish/API/Repository/Tests/TrashServiceTest.php

+        krsort($descResultsIds);
+        $descResultsIds = array_values($descResultsIds);
+
+        $this->assertSame($descResultsIds, $ascResultsIds);


This assert will fail in case of more the 1 page of results so make sure this assumption is fulfilled by specifying limit on query.

There is expecting count = 2, and the limit is 25 (as default), but I can also add the fit query limit.

adamwojs · 2020-06-29T09:03:06Z

...ublish/Core/Search/Legacy/Content/Common/Gateway/SortClauseHandler/Trash/ContentTypeName.php

+        $query
+            ->addSelect(
+                sprintf(
+                    'ct.identifier AS %s',


Implementation does not fit to class name: results will be sorted by content type identifier instead of content type name.

adamwojs · 2020-06-29T09:04:41Z

...ublish/Core/Search/Legacy/Content/Common/Gateway/SortClauseHandler/Trash/ContentTypeName.php

+                )
+            );
+
+        return (array)$column;


Suggested change

return (array)$column;

return [$column];

adamwojs · 2020-06-29T09:48:45Z

eZ/Publish/Core/Search/Legacy/Content/Common/Gateway/SortClauseHandler/Trash/UserName.php

+use eZ\Publish\Core\Search\Legacy\Content\Common\Gateway\SortClauseHandler;
+use eZ\Publish\API\Repository\Values\Content\Query\SortClause;
+
+class UserName extends SortClauseHandler


IMHO `UserLogin` is better naming choice for this sort clause

Delete unnecessary criterion converter Handled Criterion on count trash items Added sort clauses handling in trash search Fixed tests After CR After CR Fixes Added tests for search in trash filtering Added tests for search in trash sorting Improved tests CR suggestions CR suggestions CR suggestions

tomaszszopinski

QA approved on eZPlatform ee 3.1dev with diff.

michal-myszka self-assigned this Jun 23, 2020

michal-myszka changed the title ~~WIP: EZP-31650 - Search in trash~~ EZP-31650: Search in trash Jun 23, 2020

michal-myszka changed the title ~~EZP-31650: Search in trash~~ EZP-31650: Added Criterions handling for search in trash Jun 23, 2020

michal-myszka force-pushed the EZP-31650-search-in-trash branch from f147337 to eb276ba Compare June 23, 2020 10:54

michal-myszka marked this pull request as ready for review June 23, 2020 10:54

michal-myszka force-pushed the EZP-31650-search-in-trash branch from eb276ba to eb1e35c Compare June 23, 2020 11:02

michal-myszka requested review from alongosz, adamwojs, ViniTou and a team June 23, 2020 11:02

adamwojs reviewed Jun 23, 2020

View reviewed changes

eZ/Publish/Core/Base/Container/Compiler/Search/Legacy/CriteriaConverterPass.php Outdated Show resolved Hide resolved

eZ/Publish/Core/Search/Legacy/Content/Trash/Gateway/CriteriaConverter.php Outdated Show resolved Hide resolved

adamwojs reviewed Jun 24, 2020

View reviewed changes

eZ/Publish/Core/Persistence/Legacy/Content/Location/Gateway/DoctrineDatabase.php Outdated Show resolved Hide resolved

michal-myszka force-pushed the EZP-31650-search-in-trash branch from 2358978 to ea66cb8 Compare June 26, 2020 10:23

alongosz requested changes Jun 26, 2020

View reviewed changes

alongosz approved these changes Jun 29, 2020

View reviewed changes

adamwojs reviewed Jun 29, 2020

View reviewed changes

Michał Myszka added 2 commits June 29, 2020 12:44

CR suggestions

8345ff0

michal-myszka requested a review from adamwojs June 29, 2020 11:31

tomaszszopinski self-assigned this Jun 29, 2020

tomaszszopinski added the QA approved label Jun 29, 2020

tomaszszopinski approved these changes Jun 29, 2020

View reviewed changes

lserwatka merged commit 071369e into master Jun 29, 2020

lserwatka deleted the EZP-31650-search-in-trash branch June 29, 2020 15:10

michal-myszka mentioned this pull request Jun 30, 2020

EZP-31650: Aligned tag names of Trash filtering Sort Clauses #85

Merged

adamwojs mentioned this pull request Jul 7, 2020

Allowed LogicalOr/LogicalNot/MatchAll/MatchNone criteria be used as part of the Trash query #87

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EZP-31650: Added Criterions handling for search in trash #80

EZP-31650: Added Criterions handling for search in trash #80

michal-myszka commented Jun 23, 2020 •

edited

Loading

adamwojs commented Jun 23, 2020

alongosz left a comment

alongosz Jun 26, 2020

alongosz Jun 26, 2020

alongosz Jun 26, 2020

alongosz Jun 26, 2020

alongosz Jun 26, 2020

alongosz Jun 26, 2020

alongosz Jun 26, 2020

alongosz Jun 26, 2020

alongosz Jun 26, 2020

michal-myszka commented Jun 26, 2020

alongosz commented Jun 26, 2020

alongosz left a comment

alongosz Jun 29, 2020

alongosz Jun 29, 2020

alongosz Jun 29, 2020

michal-myszka Jun 29, 2020

adamwojs Jun 29, 2020

michal-myszka Jun 29, 2020

adamwojs Jun 29, 2020

michal-myszka Jun 29, 2020

adamwojs Jun 29, 2020

michal-myszka Jun 29, 2020

adamwojs Jun 29, 2020

adamwojs Jun 29, 2020

adamwojs Jun 29, 2020

tomaszszopinski left a comment

	if (!in_array($target, [self::MODIFIED, self::CREATED, self::TRASHED])) {
	if (!in_array($target, self::TARGETS)) {

-     * @param string $target One of DateMetadata::CREATED, DateMetadata::MODIFIED or DateMetadata::TRASHED (works only with LSE)
+     * @param string $target One of DateMetadata::CREATED, DateMetadata::MODIFIED or DateMetadata::TRASHED (applies to TrashService::findTrashItems only).
+     *
+     * @see \eZ\Publish\API\Repository\TrashService::findTrashItems

	ezplatform.search.legacy.gateway.criteria_converter.trash:
	ezplatform.trash.search.legacy.gateway.criteria_converter:

	- {name: ezplatform.search.legacy.gateway.criterion_handler.trash}
	- {name: ezplatform.trash.search.legacy.gateway.criterion_handler.trash}

	ezpublish.search.legacy.gateway.sort_clause_converter.trash:
	ezplatform.trash.search.legacy.gateway.sort_clause_converter:

	'c', ContentTypeGateway::CONTENT_TYPE_TABLE, 'cc', 'c.contentclass_id = cc.id'
	'c', ContentTypeGateway::CONTENT_TYPE_TABLE, 'ct', 'c.contentclass_id = ct.id'

	private function trashDifferentContents(): void
	private function trashDifferentContentItems(): void

EZP-31650: Added Criterions handling for search in trash #80

EZP-31650: Added Criterions handling for search in trash #80

Conversation

michal-myszka commented Jun 23, 2020 • edited Loading

Checklist:

adamwojs commented Jun 23, 2020

alongosz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michal-myszka commented Jun 26, 2020

alongosz commented Jun 26, 2020

alongosz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomaszszopinski left a comment

Choose a reason for hiding this comment

michal-myszka commented Jun 23, 2020 •

edited

Loading