Views' User Name exposed group filter validation

Can you specify how to replicate this in the issue summary?

Looking at the issue description and the published patch, I can confirm that this issue is a duplicate of this one: #2576927: Grouped exposed taxonomy filter fails validation for autocomplete widget. Let's continue there. I'll ask someone to make sure we transfer issue credits.

Thanks for your contributions!

After giving it a second thought and discussing it with @lendude in Slack, it was decided to keep those issues separate.

I'm updating the issue summary and moving to NR for an open merge request. Hopefully, tests will pass.

Hiding initial patch because we're using a merge request flow.

I realized that target branch was 8.9.x, and my rebase/merge attempts failed... That's why there is a new merge request against 9.4.x, where I tried to address @lendude's feedback that was added in https://git.drupalcode.org/project/drupal/-/merge_requests/1445

I also added a helper method and changed the way the view results are verified in the test. I think that current approach is not reliable. It's looking for user ID in the whole page markup. If the user id (e.g. 2) is available somewhere else, then test is not quite correct.

The method is almost identical as in #3251394: Introduce ViewTestBase::assertIds to simplify view results testing, so maybe it's worth pushing the issue with the helper method first.

Yeh the refactoring of the test makes sense to me, let's keep that.

Went through this again and this look good.

Sorry, This will need to be on 10.0.x I edited the MR to be on 10.0.x but was informed it needed a rebase. It will still need an MR for 9.5.x

Uploading a patch to be able to run tests for all active branches.

Uploading a patch with a small difference in buildExposedFiltersGroupForm() method.

I was just thinking that changing values in the plugin object might be not a good idea because it transforms the list of IDs to a single string.
Instead, we should format the string only for the form element. It was initially changed in #2576927: Grouped exposed taxonomy filter fails validation for autocomplete widget, so I'm just trying to keep the consistency.

I'm leaving it as NR because of these changes. The interdiff is the last commit in the merge request.

I just discovered something else, which is described in another issue: #3280477: Name and TaxonomyIndexTid filter plugins have incorrect default values in grouped filters.

The tests were failing because the form element already had a default value from the previously configured "Value", when the filter type was "single". It's a pretty edge case because you'd need to create a single exposed filter first, add some values, and then switch to grouped filter.

To move forward, I just added an extra condition to avoid type errors and suggest fixing this in a follow-up that I created.

Looks really good, just some nitpicks really.

1. +++ b/core/modules/user/src/Plugin/views/filter/Name.php
   @@ -39,14 +44,31 @@ protected function valueForm(&$form, FormStateInterface $form_state) {
   +    // Autocomplete puts the values in target_id. Move the values to the
   +    // expected depth.
   

   Might be good to specify what is 'expecting' this?

2. +++ b/core/modules/user/src/Plugin/views/filter/Name.php
   @@ -124,4 +146,19 @@ public function adminSummary() {
   +        $users = User::loadMultiple($item['value']['#default_value']);
   

   Static :(

3. +++ b/core/modules/user/tests/src/Functional/Views/HandlerFilterUserNameTest.php
   @@ -127,6 +127,38 @@ public function testAdminUserInterface() {
   @@ -137,6 +169,10 @@ public function testExposedFilter() {
   

   The changes in this test method are unrelated right? If so, lets not do that here, it makes it more difficult to review

@lendude thanks for the review!

The first 2 items were addressed. The 3rd one is not done yet, because I think they should remain.
In this test we introduced a new helper method - HandlerFilterUserNameTest::assertIds(). Therefore, we use a new method for other assertions to make it more bullet-proof. For example, the previous assertion is:
$this->assertSession()->pageTextContains($account->id());
If there is a text on the page that matches user ID, it'll be a false positive. With a new approach, we're looking only into view results.

A similar change was done in the last commit. After injecting the entity type manager, I changed this line, although it is out of scope: https://git.drupalcode.org/project/drupal/-/merge_requests/1473/diffs#62...

P.S. I encourage you to use MR to add comments. The patch is just a copy, that allows me to run tests against multiple Drupal versions.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

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