As a user who has "access taxonomy overview" but cannot add/edit/delete terms in a vocabulary, the OverviewTerms page prints a table with a header with 2 columns (name & operations), but only has table rows with one column, name. The "Operations" column is empty, and you are left with invalid HTML table structure.

Here is the table without access to any "operations":
Table missing operations cells

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dpagini created an issue. See original summary.

dpagini’s picture

Status: Active » Needs review
FileSize
582 bytes
dpagini’s picture

Adding an image to the issue summary.

abramm’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
75.69 KB

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

xjm’s picture

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.

xjm’s picture

Title: OverviewTerms page invalid table HTML (operations) » OverviewTerms page has invalid table HTML when the user does not have access to some terms
abramm’s picture

Status: Needs work » Active

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.

dpagini’s picture

It feels a bit odd to me to view the weight for some terms, but not others. At first I was questioning how/why someone would have access to update one term, but not another... that functionality doesnt exist in core, does it? But regardless, it's probably best practice. Given that, if you DID have access to edit 5 terms, and 10 were listed on the page, what good does that weight column do you? I feel like the weight column should be there for every term, regardless of update rights, and somehow that widget should display differently based on your edit access. Just sort of spit-balling, maybe that's not how all this works...

Doesn't this functionality exist on some other pages in core (to draw from)?

idebr’s picture

Status: Active » Needs review
FileSize
80.35 KB
74.74 KB
3.01 KB
2.68 KB

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:

Status: Needs review » Needs work

The last submitted patch, 9: 2920672-9.patch, failed testing. View results

idebr’s picture

Status: Needs work » Needs review
FileSize
1.69 KB
4.37 KB

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

dpagini’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
104.49 KB
117.5 KB

I'm not sure if I can be the one to mark it RTBC, but I applied this patch to my site, and it definitely solves my initial bug. Here is a blank taxonomy, and also the view of a user w/o edit/reorder permission.

idebr’s picture

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

xjm’s picture

Status: Reviewed & tested by the community » Needs work

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!

idebr’s picture

Status: Needs work » Needs review
FileSize
2.69 KB
5.65 KB

@xjm That makes sense, thanks for the pointers.

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

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Manuel Garcia’s picture

Status: Needs review » Reviewed & tested by the community

#14 was addressed, back to RTBC

mohit_aghera’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
5.67 KB

Re-roll for 8.6.x branch.

idebr’s picture

Status: Needs review » Reviewed & tested by the community

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.

catch’s picture

Version: 8.6.x-dev » 8.5.x-dev
Status: Reviewed & tested by the community » Fixed

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

  • catch committed d27a372 on 8.6.x
    Issue #2920672 by idebr, dpagini, abramm, xjm: OverviewTerms page has...

  • catch committed 920a8fa on 8.5.x
    Issue #2920672 by idebr, dpagini, abramm, xjm: OverviewTerms page has...
idebr’s picture

Status: Reviewed & tested by the community » Fixed

Updating issue status to ‘Fixed’

Status: Fixed » Closed (fixed)

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