Media author views filter should use the user_name plugin

Thanks for the patch.

1) Patch applies cleanly to 8.9, 9.0, and 9.1.

[mac:kristen:drupal-8.9.x-dev]$ patch -p1 < 3113989-2.patch 
patching file core/modules/media/src/MediaViewsData.php

[mac:kristen:drupal-9.0.x-dev]$ patch -p1 < 3113989-2.patch 
patching file core/modules/media/src/MediaViewsData.php

[mac:kristen:drupal-9.1.x-dev]$ patch -p1 < 3113989-2.patch 
patching file core/modules/media/src/MediaViewsData.php

2) Searched the codebase for something similar to:

$data['media_field_data']['x']['filter']

but didn't see anything.

3) Looked at NodeViewsData based on issue summary note and see:

$data['node_field_data']['uid']['filter']['id'] = 'user_name';

which is the same pattern as the patch so this looks ok.

4) I'm not sure this would be considered a "Bug report" but I'll leave that as is.

5) What's the best way to manually test this?

Also, this might need tests.

Doh! Sorry for the noise. :) Working on too little sleep and too many issues.

@acbramley Do you have any thoughts on best way to manual test this?

Awesome, I added this to the issue summary. I'll try to test soon.

Tested manually and it works great!

Without patch:

With patch:

Moving back to "Needs work" for the tests.

Looked around for tests for a similar filter and had a hard time. I found testExposedFilter in:

core/modules/user/tests/src/Functional/Views/HandlerFilterUserNameTest.php

but that's for the user entity and name property. Also found FilterUidRevisionTest in:

core/modules/node/tests/src/Kernel/Views/FilterUidRevisionTest.php

but that's for node author / node revision author check.

Trying to get some guidance from the Bug Smash Initiative on where the test(s) should go.

How about adding another test method in core/modules/media/tests/src/Functional/MediaOverviewPageTest.php to add the media author as an exposed filter to the default media overview page? Then assert it's an autocomplete text field, not a simple integer UID field? I don't think we need to / want to actually use a JS test to verify the autocomplete works as autocomplete. That's already tested elsewhere. We just need to make sure that this filter is using the right plugin.

@dww Nice. That seems like a great approach, thanks!

Thanks for the patch. Not sure I follow it completely :) but I noticed a couple things.

1. +++ b/core/modules/media/tests/src/Functional/MediaOverviewPageTest.php
   @@ -171,4 +172,72 @@ public function testMediaOverviewPage() {
    
   +  public function testMediaOverviewAuthorFilter() {
   

   Missing doc block.

2. +++ b/core/modules/media/tests/src/Functional/MediaOverviewPageTest.php
   @@ -171,4 +172,72 @@ public function testMediaOverviewPage() {
   +    $view = View::load('media');
   +    $display =& $view->getDisplay('default');
   

   Nitpick: Although I see a small number of tests use the =& $view formatting like:

   $display =& $view->getDisplay('default');
   

   most of the core code uses = &$view like:

   $display = &$view->getDisplay('default');
   

3. +++ b/core/modules/media/tests/src/Functional/MediaOverviewPageTest.php
   @@ -171,4 +172,72 @@ public function testMediaOverviewPage() {
   +    $this->drupalGet('/admin/content/media');
   +    $role = Role::load(RoleInterface::AUTHENTICATED_ID);
   +    $this->grantPermissions($role, ['access media overview']);
   +    $this->drupalGet('/admin/content/media');
   

   I'm unclear why $this->drupalGet('/admin/content/media'); is done twice here.

Ah, we crossposted. Ignore #23.1.

Thanks for the updates. I reviewed again and feel like I understand the test now. Not sure why the timestamp is adjusted for $media2 and $media3 but it doesn't hurt anything. I see that the text being checked is the correct string so, if the tests pass, this is looking good to me.

+++ b/core/modules/media/tests/src/Functional/MediaOverviewPageTest.php
@@ -171,4 +172,64 @@ public function testMediaOverviewPage() {
+    $this->assertSession()->pageTextContains('Enter a comma separated list of user names.');
...
 

[mac:kristen:drupal-9.1.x-dev]$ grep -r "Enter a comma separated list of user names" .
./core/modules/user/src/Plugin/views/filter/Name.php:      '#description' => $this->t('Enter a comma separated list of user names.'),

Woot! Looks like the test is passing fine and the test-only patch fails as expected. If you want to adjust it to remove those couple timestamps, that's fine. I'll wait a bit before moving this forward. Thanks.

Nice! Ok, crossing fingers there is no glitch in the testbot matrix.

#30 looks great.
Actual fix is tiny and correct.
Test coverage looks reasonable.
No nits found.
No CS violations.

Therefore... RTBC!

Thanks,
-Derek

This needs an update path for existing view. If I apply this patch and have a view created as suggested in the issue summary I get an error - The operator is invalid on filter: Media: Authored by.

- Added a post_update hook to change the filter and related attributes.
- Add test case to evaluate the post_update hook logic.

- Added a post_update hook to change the filter and related attributes.
- Add test case to evaluate the post_update hook logic. Update testcase is passing on local

Ignore the patch from comment #36
Accidentally, I created diff from older 9.2.x head.
I've hidden patch and interdiff.

Nice, like @Kristen Pol not convinced it's a bug (can you not just workaround it by adding a relationship to the author?), but don't care too much.

Some things I see:

1. +++ b/core/modules/media/src/MediaViewsData.php
   @@ -15,6 +15,7 @@ class MediaViewsData extends EntityViewsData {
   +    $data['media_field_data']['uid']['filter']['id'] = 'user_name';
   
   +++ b/core/modules/media/tests/src/Functional/MediaOverviewPageTest.php
   @@ -171,4 +172,62 @@ public function testMediaOverviewPage() {
   +    $display['display_options']['filters']['uid'] = [
   +      'id' => 'uid',
   +      'table' => 'media_field_data',
   +      'field' => 'uid',
   +      'operator' => 'in',
   +      'exposed' => TRUE,
   +      'expose' => [
   +        'label' => 'Authored by',
   +        'identifier' => 'uid',
   +      ],
   +      'entity_type' => 'media',
   +      'entity_field' => 'uid',
   +      'plugin_id' => 'user_name',
   +    ];
   

   Since we are hardcoding the plugin ID in the test when we add the filter, this would pass even without the change in MediaViewsData, so that makes the test a little suspect I think. If we really want to test that setting, we would need to add the filter through the UI.

2. +++ b/core/modules/views/views.post_update.php
   @@ -68,3 +68,41 @@ function views_post_update_rename_default_display_setting() {
   +function views_post_update_media_author_views_filter_update() {
   

   Ideally the update would use \Drupal\views\ViewsConfigUpdater so we also update any config provided by contrib and the like

Updating test cases and post_update hook as mentioned in #39

Thanks @mohit_aghera, looks great already.

One last thing I see:

+++ b/core/modules/views/src/ViewsConfigUpdater.php
@@ -477,4 +491,45 @@ protected function mapOperatorFromSingleToMultiple($single_operator) {
+  protected function processMediaAuthorFilterPlugins(array &$handler, string $handler_type): bool {

We should also add a call to this in \Drupal\views\ViewsConfigUpdater::updateAll so that Views getting saved are also updated, see the other calls in updateAll

Thanks, @Lendude for correcting me.
I've updated test cases accordingly.

Thanks for the update.

1) I confirmed the interdiff addresses #43.

2) Since @Lendude already approved other than that, I only scanned the code quickly. I found one minor nitpick:

+++ b/core/modules/views/src/ViewsConfigUpdater.php
@@ -71,6 +72,14 @@ class ViewsConfigUpdater implements ContainerInjectionInterface {
   protected $triggeredDeprecations = [];
 
+
+  /**

Extra empty line.

Removed the additional empty space issue mentioned in #46

Please do not ask the testbot to try again until #3207086: [HEAD BROKEN] Consistent failure in MonthDatePluginTest is fixed.

#3207086: [HEAD BROKEN] Consistent failure in MonthDatePluginTest is fixed. I have not reviewed the code.

While it should have been moved back to needs review in #51 rather than RTBC, I've confirmed that the interdiff in #45 does fix the nitpick in #45 so RTBC is good :)

Restoring status after HEAD was broken.

Looking great, couple of questions

1. +++ b/core/modules/media/tests/src/Functional/MediaOverviewPageTest.php
   @@ -171,4 +171,63 @@ public function testMediaOverviewPage() {
   +    $this->submitForm($edit, 'Add and configure filter criteria');
   +
   +    $edit = [
   +      'options[expose_button][checkbox][checkbox]' => 1,
   +    ];
   +    $this->submitForm($edit, 'Expose filter');
   +    $edit = [
   +      'options[expose_button][checkbox][checkbox]' => 1,
   +      'options[group_button][radios][radios]' => 0,
   +    ];
   +    $this->submitForm($edit, 'Apply');
   
   +++ b/core/modules/views/tests/src/Functional/Update/MediaExposedFilterPluginTest.php
   @@ -0,0 +1,39 @@
   +    $view = View::load('test_media_view_exposed_filter');
   

   if we already have a test view with the media author exposed, shouldn't we just use that in test instead of going to the trouble of editing the default media view?

2. +++ b/core/modules/views/tests/fixtures/update/drupal-9.0.0.filled.standard.php.gz
   @@ -0,0 +1,651 @@
   +�Cne`drupal-9.0.0.filled.standard.php�k�㸙.8��W(*&��'�ٸ���=v�>�8m���=��
   

   We already have a d9 filled standard in core/modules/system/tests/fixtures/update/drupal-9.0.0.filled.standard.php.gz

   Any reason to add a new one? Can we just use the existing one?

3. +++ b/core/modules/views/views.post_update.php
   @@ -68,3 +68,18 @@ function views_post_update_rename_default_display_setting() {
   +function views_post_update_media_author_views_filter_update18(&$sandbox = NULL) {
   

   is the 18 in the function name significant here, this is the first instance of it I can see for the views post update file.

Can we do a fixture that adds to the existing dump, like e.g. core/modules/system/tests/fixtures/update/drupal-8.update-test-semver-update-n-enabled.php

All points of @larowlan have been addressed.
I have just one question:

+++ b/core/modules/views/src/ViewsConfigUpdater.php
@@ -477,4 +493,45 @@ protected function mapOperatorFromSingleToMultiple($single_operator) {
+      // Remove additional attributes set for numeric filter.
+      unset($handler['expose']['placeholder']);
+      unset($handler['expose']['min_placeholder']);
+      unset($handler['expose']['max_placeholder']);

What happens when somebody has a custom project that uses the view with the min/max placeholder values?

The patch works fine for me. after applying the same it is generating prediction auto-completion successfully.

Before Patch

After Patch

Saving issue credit

Can we get an answer for #66 and a change notice here.

With both of those, I think this is good to go.

For #72

Re: #66, since we are updating the plugin, those values wouldn't do much anymore anyway, so I don't see any harm in removing them with the current fix, but it does raise a very good point because the same goes for projects that have a filter and they are using it with a 'between' operator (or anything not '='), we are destroying that too ('between' will become 'in', so does something completely different after the update). I think it is fairly unlikely that anybody would have that, but we do have to ask the question if we are ok with that.

Found some other things that probably need addressing.

1. +++ b/core/modules/views/src/ViewsConfigUpdater.php
   @@ -138,6 +151,9 @@ public function updateAll(ViewEntityInterface $view) {
   +      if ($this->processMediaAuthorFilterPlugins($handler, $handler_type)) {
   +        $changed = TRUE;
   +      }
   

   Calling this without the media_field_data check in that method will have the potential to update WAY more then the field we are trying to target (see point 4 below)

2. +++ b/core/modules/views/src/ViewsConfigUpdater.php
   @@ -477,4 +493,45 @@ protected function mapOperatorFromSingleToMultiple($single_operator) {
   +    if ($handler_type === 'filter' && isset($handler['id']) && $handler['field'] === 'uid') {
   

   we are checking $handler['id'] in the isset() but checking $handler['field'] after that, should they both check $handler['field']?

3. +++ b/core/modules/views/tests/fixtures/update/drupal-9.add-media-uid-exposed-filter-view.php
   @@ -0,0 +1,36 @@
   + * @file
   + * Database to mimic the installation of the update_test_schema module.
   

   This comment needs an update

4. +++ b/core/modules/views/views.post_update.php
   @@ -68,3 +68,18 @@ function views_post_update_rename_default_display_setting() {
   +    if ($view->get('base_table') !== 'media_field_data') {
   +      return;
   +    }
   

   This should probably be checked in needsMediaAuthorFilterPluginId() or probably processMediaAuthorFilterPlugins()

+++ b/core/modules/views/src/ViewsConfigUpdater.php
@@ -138,6 +151,9 @@ public function updateAll(ViewEntityInterface $view) {
+      if ($this->processMediaAuthorFilterPlugins($handler, $handler_type)) {
+        $changed = TRUE;
+      }

@@ -477,4 +493,48 @@ protected function mapOperatorFromSingleToMultiple($single_operator) {
+  public function needsMediaAuthorFilterPluginId(ViewEntityInterface $view): bool {
+    if ($view->get('base_table') !== 'media_field_data') {
+      return FALSE;
+    }
...
+  protected function processMediaAuthorFilterPlugins(array &$handler, string $handler_type): bool {
+    if ($handler_type === 'filter' && isset($handler['field']) && $handler['field'] === 'uid') {

Hmm the check on base table is still circumvented in the updateAll call like this. We probably need to pass the View along to the process method and check it there.

The update still feels potentially destructive though. An alternative would be to do this without an update and provide the user name filter as a separate filter so the original can stay as it is. Too extreme BC? ¯\_(ツ)_/¯

Something like this would also work, without the BC implications. We should probably add the test coverage, but this is just an idea.

When I add the 'Authored by' filter without the patch, I see all the options in the screenshot. In the update we are mapping all of these to the 'in' operator. Again, I doubt anybody is using those operators, but we can't be sure.

Or are your seeing something else?

i manually tested the patch in #79 on 9.4.x, standard install. Both before and after the patch the uid was used for the filter not the name. I did not experience any difference before and after the patch.

Oh, I see the problem. I don't think the steps in the IS are correct. The last step is to apply the filter again. To me that means the same filter used in the previous step when instead it is to use the new filter with the title 'Author name'. When I use that filter it works as expected.

Tagging for issue summary update for proposed resolution and possibly testing steps (per #85).

Re-rolled #79 for 9.4

@ranjith_kumar_k_u Thanks for the reroll. Tagging for tests.

Compared patch in #88 with previous one in #79 and the changes are the same.

Patch in #88 applies cleanly to versions 9.4 and 10 and with offsets for 9.3.

[drupal-9.3.x-dev/9.3.x] [drupal-9.3.x-dev]$ patch -p1 < 3113989-88.patch 
patching file core/modules/media/media.post_update.php
patching file core/modules/media/src/MediaViewsData.php
Hunk #1 succeeded at 25 with fuzz 2 (offset 7 lines).

Moving back to needs work.

As @acbramley points out in #90, tests were in patch #76. These need to be added back.

Adding tests again from #76 as suggested in #91.

Updated patch with interdiff.

Removing tests related to upgrade path and fixtures. Also updated the testMediaOverviewAuthorFilter inline comment.

Thank you for the patch in #99. It still works in Drupal core 10.2.3.

I opened a MR (in case that helps move this along) which contains the changes from #99, plus hopefully fixes the tests, although I can already see they've failed...

Made the requested changed to the MR. Unfortunately I can't do anything more to make the tests pass because the error appears to have nothing to do with the test being added here.

Just came across this and fell into the same trap as #85.
Updating IS for clarity.

- All three points mentioned by @acbramley is fixed by @maskedjellybean
- Issue summary is updated and PR is rebased with latest 11.x.
- Merge conflict is resolved.
- Hiding older patches in favour of PR based approach.
- I think it is ready for moving to needs review.

Ran test-only feature

1) Drupal\Tests\media\Functional\MediaOverviewPageTest::testMediaOverviewAuthorFilter
Behat\Mink\Exception\ElementNotFoundException: Form field with id|name|label|value "name[media_field_data.user_name]" not found.
/builds/issue/drupal-3113989/vendor/behat/mink/src/WebAssert.php:731
/builds/issue/drupal-3113989/core/tests/Drupal/Tests/UiHelperTrait.php:85
/builds/issue/drupal-3113989/core/modules/media/tests/src/Functional/MediaOverviewPageTest.php:219
FAILURES!
Tests: 2, Assertions: 58, Failures: 1.

Manually testing editing the view I'm seeing the new filters.

Believe this is good

There are no unanswered questions here.

After reading the IS I expected to see a new plugin of some type, but there isn't one. The IS says there are no UI changes but there are so there should be screenshots available from the Issue summary.

I then read the CR and saw that the screenshots are out of date. For that I applied the diff to make new ones. I then saw that the description for the new filter, 'Author name' is that it is the user ID. That is not correct, it is the user name. Should this be doing the same as the Content view and use a title of "Authored by" and a description of "The username of the content author."?

At the lest the description should change. Setting to NW.

1. Updated label and description to match /admin/content (agree with #112)
2. Updated issue summary
3. Updated screenshots
4. Updated change record

Updated 'After' screenshot to reflect change to default label

Appears to have a relevant test failure

Have not reviewed yet.

Test coverage passing now; applies a filter on username (rather than uid) and returns expected media entities as results.

Ran test-only feature

1) Drupal\Tests\media\Functional\MediaOverviewPageTest::testMediaOverviewAuthorFilter
Behat\Mink\Exception\ElementNotFoundException: Form field with id|name|label|value "name[media_field_data.user_name]" not found.
/builds/issue/drupal-3113989/vendor/behat/mink/src/WebAssert.php:731
/builds/issue/drupal-3113989/core/tests/Drupal/Tests/UiHelperTrait.php:90
/builds/issue/drupal-3113989/core/modules/media/tests/src/Functional/MediaOverviewPageTest.php:270
FAILURES!
Tests: 3, Assertions: 65, Failures: 1.
Exiting with EXIT_CODE=1

Resolved the open threads as they appears to be addressed

Applied the MR before doing an install
Followed the steps in the summary and am seeing the new filter "The username of the content author."
Applied it to the Media Library view I can filter by usernames

LGTM

Committed and pushed 68013115a83 to 11.x and c3a9311a660 to 11.1.x. Thanks!