EZP-30427: Added support to paginate a list of drafts #2753

mikadamczyk · 2019-08-28T13:43:26Z

Question	Answer
JIRA issue	EZP-30427
Bug/Improvement	yes
New feature	no
Target version	`7.5`
BC breaks	no
Tests pass	yes
Doc needed	yes

Added support to paginate a list of drafts. ~~Added additional arguments ($limit and $offset) to ContentService::loadContentDrafts~~
Added new method ContentService::countContentDrafts to allow paginate content drafts list. This method returns the number of all drafts for the user, including those to which the user has no access.

Instead of adding additional parameters to existing ContentService::loadContentDrafts this PR adds new ContentService::loadContentDraftList to allow paginate and filter content drafts list. New method return ContentDraftList object. It can contain objects which implements ContentDraftListItemInterface. Those can be UnauthorizedContentDraftListItem or ContentDraftListItem

Related PR: ezsystems/ezplatform-admin-ui#1071

TODO:

Implement feature / fix a bug.
Implement tests.
Fix new code according to Coding Standards ($ composer fix-cs).
Ask for Code Review.

eZ/Publish/API/Repository/ContentService.php

eZ/Publish/Core/Repository/ContentService.php

adamwojs · 2019-08-28T18:40:54Z

eZ/Publish/Core/Repository/ContentService.php

     *
-     * @return \eZ\Publish\API\Repository\Values\Content\VersionInfo the drafts ({@link VersionInfo}) owned by the given user
+     * @throws \eZ\Publish\API\Repository\Exceptions\BadStateException
+     * @throws \eZ\Publish\API\Repository\Exceptions\InvalidArgumentException


Suggested change

* @throws \eZ\Publish\API\Repository\Exceptions\InvalidArgumentException

eZ/Publish/API/Repository/ContentService.php

eZ/Publish/Core/Persistence/Legacy/Content/Gateway/ExceptionConversion.php

alongosz

Overall looks good.

I would do some API namespace cleanup, because eZ/Publish/API/Repository/Values/Content/ gets too crowded. Please change the API-related file and corresponding namespace structure as agreed during internal sync.

Other remarks:

eZ/Publish/API/Repository/ContentService.php

eZ/Publish/API/Repository/Values/Content/ContentDraftListItem.php

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

eZ/Publish/Core/SignalSlot/ContentService.php

eZ/Publish/Core/Repository/ContentService.php

eZ/Publish/Core/Persistence/Legacy/Content/Handler.php

…drafts

mikadamczyk · 2019-09-09T10:36:50Z

ping @alongosz @ViniTou

alongosz · 2019-09-09T12:14:34Z

eZ/Publish/API/Repository/Values/Content/DraftList/Item/ContentDraftListItem.php

+class ContentDraftListItem implements ContentDraftListItemInterface
+{
+    /**
+     * @var \eZ\Publish\API\Repository\Values\Content\VersionInfo


I thought you wanted one line class member docs...

It is not about what I want :)

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

eZ/Publish/API/Repository/ContentService.php

eZ/Publish/Core/Persistence/Legacy/Content/Handler.php

eZ/Publish/API/Repository/Values/Content/DraftList/Item/ContentDraftListItem.php

eZ/Publish/Core/Repository/ContentService.php

eZ/Publish/Core/SignalSlot/ContentService.php

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

eZ/Publish/Core/Repository/ContentService.php

Co-Authored-By: Adam Wójs <adam.wojs@ez.no>

andrerom · 2019-09-18T19:12:27Z

eZ/Publish/API/Repository/Values/Content/DraftList/Item/UnauthorizedContentDraftListItem.php

+/**
+ * Item of content drafts list which represents draft to which user has no access for.
+ */
+class UnauthorizedContentDraftListItem implements ContentDraftListItemInterface


Why do we need to push this logic to API consumer?

As in:

Drafts are personal so at least in the past you usually had access to YOUR drafts.

Even if there are cases where permissions won't let you, then I would expect it to behave like other API methods, aka filter out what you don't have access to. Otherwise paging in UI will have gaps, right?

Ad 1 - You can pass user as an argument to loadContentDrafts(User $user = null) so we have a case with a user's drafts which not belongs to you.
Ad 2 - To create "classic" pagination we need a total number of items. We can not use the search query to get it, because we have zeta components. This is a 3rd line issue and we do not have time to get rid of them. Also, it would be a BC break I think. This is why I used UnauthorizedContentDraftListItem here and then in Admin UI.

Also, it would be a BC break I think

This is for a new new API method, so there is no BC on it before it's released, that is when it will become harder to change all this ;)

You can pass user as an argument to loadContentDrafts(User $user = null) so we have a case with a user's drafts which not belongs to you.

Good point 👍

But........ 😏 Then maybe you just made a very good argument for dropping this argument on the new API method in the first place. If people want to get drafts for other users they can either still use the old one or set current user to the user they want to get drafts for.

Dropping that argument seems like it will avoid a lot of the complexity introduced here.
And it will greatly improve DX for the primary use case: Getting drafts of current user.

The goal was to keep all functionalities including the possibility to fetch drafts of another user even if it is a big number of them. Also, it will still be the issue when a user loses access to some of their drafts. I am against developing a feature which is buggy by design, even if it looks like a corner case.

We have a similar problem in content reverse relations where it is a common situation when a user has no access to some contents. And we have the plan to solve this issue in the same way (return the list with two kinds of items)

andrerom · 2019-09-18T19:14:41Z

eZ/Publish/API/Repository/Values/Content/ContentDraftList.php

+    public $totalCount = 0;
+
+    /**
+     * @var \eZ\Publish\API\Repository\Values\Content\DraftList\ContentDraftListItemInterface[]


Depending on the other question this can directly be VersionInfo instead perhaps.

andrerom · 2019-09-18T19:52:37Z

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

+     *
+     * @return \Doctrine\DBAL\Query\QueryBuilder
+     */
+    private function createVersionInfoFindQueryBuilder(): DoctrineQueryBuilder


You can (by now) instead reuse ->queryBuilder-> createVersionInfoQueryBuilder() which was just added as part of #2739

EDIT: Partly ignore this, unless we refactor it to work for your use case it won't atm (it expects to give current version or a provided version number only atm, and this one instead uses version table in from which is better in this case)

micszo

QA Approved with 6k drafts on eZ Platform EE v2.5.5 with both branches.
For the record, on Versions tab effect is not as amazingly smooth as on the Dashboard but also significant.
User does not have access message OK.

mikadamczyk added the Bug label Aug 28, 2019

mikadamczyk self-assigned this Aug 28, 2019

mikadamczyk force-pushed the EZP-30427 branch from 34539c4 to 8c17575 Compare August 28, 2019 13:46

ViniTou reviewed Aug 28, 2019

View reviewed changes

eZ/Publish/API/Repository/ContentService.php Show resolved Hide resolved

adamwojs requested changes Aug 28, 2019

View reviewed changes

mikadamczyk force-pushed the EZP-30427 branch from 93ffe24 to f2742c3 Compare September 3, 2019 12:32

alongosz requested changes Sep 3, 2019

View reviewed changes

mikadamczyk mentioned this pull request Sep 4, 2019

Refactored integration test for ContentService #2757

Merged

3 tasks

mikadamczyk force-pushed the EZP-30427 branch from 7c8d0e2 to 70aaadb Compare September 5, 2019 07:23

mikadamczyk added 4 commits September 5, 2019 12:46

EZP-30427: Added support to paginate a list of drafts

203b503

EZP-30427: Added test to pagination on list of drafts

bb0bc34

EZP-30427: Refactored integration test for ContentService

c27588d

EZP-30427: Added new method to allow fetching paginated content drafts

adf1471

mikadamczyk force-pushed the EZP-30427 branch 2 times, most recently from 6c786c3 to 3713898 Compare September 9, 2019 09:16

EZP-30427: [CR] Added new method to allow fetching paginated content …

d7269c5

…drafts

mikadamczyk force-pushed the EZP-30427 branch from 3713898 to d7269c5 Compare September 9, 2019 09:27

mikadamczyk requested review from ViniTou, alongosz and adamwojs September 9, 2019 10:36

ViniTou approved these changes Sep 9, 2019

View reviewed changes

alongosz approved these changes Sep 9, 2019

View reviewed changes

adamwojs requested changes Sep 16, 2019

View reviewed changes

Apply suggestions from code review

9ad846a

Co-Authored-By: Adam Wójs <adam.wojs@ez.no>

mikadamczyk force-pushed the EZP-30427 branch from 7404d5a to 9ad846a Compare September 17, 2019 09:36

mikadamczyk requested a review from adamwojs September 17, 2019 09:37

mikadamczyk mentioned this pull request Sep 17, 2019

EZP-30427: Changed drafts pagination to use limit and offset ezsystems/ezplatform-admin-ui#1071

Merged

2 tasks

adamwojs approved these changes Sep 17, 2019

View reviewed changes

mikadamczyk added the Ready for QA label Sep 17, 2019

mikadamczyk added the Ready for review label Sep 17, 2019

andrerom reviewed Sep 18, 2019

View reviewed changes

micszo self-assigned this Sep 25, 2019

micszo approved these changes Sep 27, 2019

View reviewed changes

micszo added QA approved and removed Ready for QA Ready for review labels Sep 27, 2019

micszo removed their assignment Sep 27, 2019

lserwatka merged commit 2e584a0 into 7.5 Sep 27, 2019

lserwatka deleted the EZP-30427 branch September 27, 2019 09:18

mikadamczyk mentioned this pull request Sep 30, 2019

Merged PR with changed drafts pagination to use limit and offset ezsystems/ezplatform-admin-ui#1096

Merged

2 tasks

mikadamczyk mentioned this pull request Oct 17, 2019

EZP-30988: Implemented Section filtering URL Query Criterion #2783

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EZP-30427: Added support to paginate a list of drafts #2753

EZP-30427: Added support to paginate a list of drafts #2753

mikadamczyk commented Aug 28, 2019 •

edited

Loading

adamwojs Aug 28, 2019

alongosz left a comment •

edited

Loading

mikadamczyk commented Sep 9, 2019

alongosz Sep 9, 2019

mikadamczyk Sep 9, 2019

andrerom Sep 18, 2019 •

edited

Loading

mikadamczyk Sep 20, 2019

andrerom Sep 21, 2019 •

edited

Loading

andrerom Sep 21, 2019 •

edited

Loading

mikadamczyk Sep 24, 2019

andrerom Sep 18, 2019

andrerom Sep 18, 2019 •

edited

Loading

andrerom Sep 19, 2019

micszo left a comment

EZP-30427: Added support to paginate a list of drafts #2753

EZP-30427: Added support to paginate a list of drafts #2753

Conversation

mikadamczyk commented Aug 28, 2019 • edited Loading

adamwojs Aug 28, 2019

Choose a reason for hiding this comment

alongosz left a comment • edited Loading

Choose a reason for hiding this comment

mikadamczyk commented Sep 9, 2019

alongosz Sep 9, 2019

Choose a reason for hiding this comment

mikadamczyk Sep 9, 2019

Choose a reason for hiding this comment

andrerom Sep 18, 2019 • edited Loading

Choose a reason for hiding this comment

mikadamczyk Sep 20, 2019

Choose a reason for hiding this comment

andrerom Sep 21, 2019 • edited Loading

Choose a reason for hiding this comment

andrerom Sep 21, 2019 • edited Loading

Choose a reason for hiding this comment

mikadamczyk Sep 24, 2019

Choose a reason for hiding this comment

andrerom Sep 18, 2019

Choose a reason for hiding this comment

andrerom Sep 18, 2019 • edited Loading

Choose a reason for hiding this comment

andrerom Sep 19, 2019

Choose a reason for hiding this comment

micszo left a comment

Choose a reason for hiding this comment

mikadamczyk commented Aug 28, 2019 •

edited

Loading

alongosz left a comment •

edited

Loading

andrerom Sep 18, 2019 •

edited

Loading

andrerom Sep 21, 2019 •

edited

Loading

andrerom Sep 21, 2019 •

edited

Loading

andrerom Sep 18, 2019 •

edited

Loading