OverviewTerms page has invalid table HTML when the user does not have access to some terms

Works for me, screenshot comparing admin and test user (having just 'access taxonomy overview' permission) below.

Huh, interesting. Good catch! Thanks also for the screenshots illustrating the issue. It looks like this was introduced in #1848686: Add a dedicated permission to access the term overview page (without 'administer taxonomy' permission).

I read over OverviewTerms::buildForm() in 8.5.x and it looks like the weight column has a similar issue:

      $update_access = $term->access('update', NULL, TRUE);
      $change_weight_access = $change_weight_access->andIf($update_access);

      if ($update_access->isAllowed()) {
        $form['terms'][$key]['weight'] = [
          '#type' => 'weight',
          '#delta' => $delta,
          '#title' => $this->t('Weight for added term'),
          '#title_display' => 'invisible',
          '#default_value' => $term->getWeight(),
          '#attributes' => ['class' => ['term-weight']],
        ];
      }

      if ($operations = $this->termListBuilder->getOperations($term)) {
        $form['terms'][$key]['operations'] = [
          '#type' => 'operations',
          '#links' => $operations,
        ];
      }

But then further down the form the weight header is only added conditionally:

    if ($change_weight_access->isAllowed()) {
      $form['terms']['#header'][] = $this->t('Weight');

Note that $change_weight_access is only supposed to be true if the user has access to update all the terms in the list, whereas the weight cells for each term are being added conditionally depending on whether the user has access to the term. That might mean that the weight header does not get added, but a cell does get added for the weight under the next header over if the user has access to update one term!

The operations header has no conditions on whether it's added:

    $form['terms']['#header'][] = $this->t('Operations');

So I think we might need to think a bit more about the fix:

• One possibility is that we always initialize all the headers for Name, Weight, and Operations (together in one place, so that it's readable) and also always initialize an empty array corresponding to those columns for the whole term row at the top of the foreach.
• The other possibility is to loop through all the terms and then only add the respective columns for all of them if at least one term has something in that column.

The first option is probably simpler and has less risk of introducing other bugs, so I'd suggest starting with that.

Hi xjm,

I thought about that, however, that means the Weight and Operations columns would be never shown for an empty table which is a bad UX.

We could always display them for an empty table but that means these columns could disappear once some term is added. Also the behavior might be inconsistent across multiple pages.

Attached patch implements the suggestions by xjm in #5:

One possibility is that we always initialize all the headers for Name, Weight, and Operations (together in one place, so that it's readable) and also always initialize an empty array corresponding to those columns for the whole term row at the top of the foreach.

In addition, the patch fixes the vocabulary overview when only a single vocabulary was available and the current user has the permission 'administer vocabularies'.

Before:

After:

Updated the VocabularyPermissionsTest to reflect that Weight is in fact visible on the page.

I created a followup issue to display the Weight and Operations column only when applicable, so the visual regression can be fixed first and the UX improved at #2921700: Conditionally show Operations, Weight column in Vocabulary, Terms overview

Good idea on scoping the followup #2921700: Conditionally show Operations, Weight column in Vocabulary, Terms overview and just fixing the bug in a reliable way here.

+++ b/core/modules/taxonomy/tests/src/Functional/VocabularyPermissionsTest.php
@@ -108,7 +108,7 @@ public function testTaxonomyVocabularyOverviewPermissions() {
-    $assert_session->pageTextNotContains('Weight');
+    $assert_session->pageTextContains('Weight');

@@ -154,7 +154,7 @@ public function testTaxonomyVocabularyOverviewPermissions() {
-    $assert_session->pageTextNotContains('Weight');
+    $assert_session->pageTextContains('Weight');

@@ -201,7 +201,7 @@ public function testTaxonomyVocabularyOverviewPermissions() {
-    $assert_session->pageTextNotContains('Weight');
+    $assert_session->pageTextContains('Weight');

For these updated assertions, I think it shows that the test in HEAD is not testing exactly what it should. What the test actually should be testing is that there is no editable weight field.

So in order to ensure we have test coverage for our fix, I would update the assertions as we have here, but also add an additional assertion that asserts the weight field is not there (probably using its markup and the fieldExists() method or something, instead of the text of the header column).

Thanks!

@xjm That makes sense, thanks for the pointers.

I have updated the test coverage to include checks for the actual form field for Weight.

#14 was addressed, back to RTBC

Re-roll for 8.6.x branch.

Re #18

Issues with the status 'Reviewed & tested by the community' are retested every 24 hours. The patch in #15 still applies cleanly to 8.6.x, so there was no need for a reroll.

Either way, the patch in #18 still solves the problem.

Committed/pushed to 8.6.x and cherry-picked to 8.5.x. Thanks!

Updating issue status to ‘Fixed’