Problem/Motivation

The workspace manage page can not handle the case when thousands of entities are changed in a workspace.

Steps to reproduce

Change 2000 or more entities inside a workspace.

Proposed resolution

Add pagination to the workspace manage page.

Remaining tasks

Make the pager limit configurable, like the taxonomy one?

User interface changes

Nope.

API changes

API addition: three new optional parameters are added to \Drupal\workspaces\WorkspaceAssociationInterface::getTrackedEntities().

Data model changes

Nope.

Release notes snippet

Nope.

CommentFileSizeAuthor
#22 Screenshot 2024-04-26 at 2.17.00 PM.png100.15 KBsmustgrave
#19 3273461-nr-bot.txt2.14 KBneeds-review-queue-bot
#4 interdiff-4.txt733 bytesamateescu
#4 3273461-4.patch6.74 KBamateescu
#2 3273461.patch6.79 KBamateescu

Issue fork drupal-3273461

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

amateescu created an issue. See original summary.

amateescu’s picture

amateescu commented
Status: Active » Needs review
StatusFileSize
new 3273461.patch6.79 KB

This should do it.

Status: Needs review » Needs work

The last submitted patch, 2: 3273461.patch, failed testing. View results

amateescu’s picture

amateescu commented
Status: Needs work » Needs review
StatusFileSize
new 3273461-4.patch6.74 KB
new interdiff-4.txt733 bytes

Let's keep the previous sort order for now. It might make sense to try and change that in a different issue, since we want to show the latest revisions first in the UI.

Status: Needs review » Needs work

The last submitted patch, 4: 3273461-4.patch, failed testing. View results

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

amateescu opened merge request !7194

amateescu’s picture

amateescu commented
Issue summary: View changes
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative 
1) Drupal\Tests\workspaces\Functional\WorkspaceTest::testWorkspaceManagePage
Behat\Mink\Exception\ExpectationException: Link with label Term 1 found.
/builds/issue/drupal-3273461/core/tests/Drupal/Tests/WebAssert.php:580
/builds/issue/drupal-3273461/core/tests/Drupal/Tests/WebAssert.php:350
/builds/issue/drupal-3273461/core/modules/workspaces/tests/src/Functional/WorkspaceTest.php:224
/builds/issue/drupal-3273461/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
ERRORS!
Tests: 9, Assertions: 211, Errors: 1.

Test-only feature shows test coverage

I don't have 50 workspaces :) but based on how I've seen other parts of core do this this appears correct.

amateescu’s picture

amateescu commented

@smustgrave, you don't need 50 workspaces, just to edit >50 entities in a workspace :P

One thing I don't really like about this MR is the large number of arguments of WorkspaceAssociation::getTrackedEntities(), but I didn't have any better idea. Maybe a new method is needed for getting a "paginated" list of tracked entities, without the ability to filter by entity type ID and entity ID... not sure.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Is there any reason why we're not using \Drupal\Core\Database\Query\PagerSelectExtender and \Drupal\Core\Database\Query\TableSortExtender?

amateescu’s picture

amateescu commented

@alexpott because WorkspaceAssociation::getTrackedEntities() is a "low level" API function that shouldn't be influenced by any global pager. Maybe this is another point in favor of adding a separate method for this listing :)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Maybe we could add getTrackedEntitiesAsQuery() and return the query object for us to do stuff with - or just not use it... but given \Drupal\workspaces\WorkspaceAssociation::TABLE - maybe this returning a query object you to do stuff to would work nicely.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs review

Gonna put this back to needs review while @amateescu has a think.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Per slack from @amateescu

amateescu’s picture

amateescu commented
Status: Needs work » Needs review

Implemented the separate method as discussed above, and it looks much better!

Regarding \Drupal\Core\Database\Query\TableSortExtender, we can't use it because the data displayed in the table is coming from the individual revision objects, not from the `workspace_association` table.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3273461-nr-bot.txt2.14 KB

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

amateescu’s picture

amateescu commented
Status: Needs work » Needs review
amateescu’s picture

amateescu commented

Btw, a quick`n`dirty way to test this MR is to edit a couple of entities in a workspace and manually change WorkspaceViewBuilder.php::$limit to 1 :)

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new Screenshot 2024-04-26 at 2.17.00 PM.png100.15 KB

after

I did just that haha thanks for the pointer

I applied some minor change to the MR for the return type but overall rest of the changes appear good

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Version: 11.x-dev » 10.3.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed f7950eb418 to 11.x and fdac75b8b0 to 10.3.x. Thanks!

  • alexpott committed fdac75b8 on 10.3.x 
    Issue #3273461 by amateescu, smustgrave, alexpott: Add pagination to the...

  • alexpott committed f7950eb4 on 11.x 
    Issue #3273461 by amateescu, smustgrave, alexpott: Add pagination to the...

  • alexpott committed f7950eb4 on 11.0.x 
    Issue #3273461 by amateescu, smustgrave, alexpott: Add pagination to the...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

amateescu closed merge request !7194