Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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":
Comment | File | Size | Author |
---|---|---|---|
#18 | 2920672-18.patch | 5.67 KB | mohit_aghera |
#15 | 2920672-15.patch | 5.65 KB | idebr |
#15 | interdiff-11-15.txt | 2.69 KB | idebr |
#9 | 2920672-9-after.png | 74.74 KB | idebr |
#9 | 2920672-9-before.png | 80.35 KB | idebr |
Comments
Comment #2
dpagini CreditAttribution: dpagini as a volunteer commentedComment #3
dpagini CreditAttribution: dpagini as a volunteer commentedAdding an image to the issue summary.
Comment #4
abrammWorks for me, screenshot comparing admin and test user (having just 'access taxonomy overview' permission) below.
Comment #5
xjmHuh, 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:But then further down the form the weight header is only added conditionally:
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:
So I think we might need to think a bit more about the fix:
The first option is probably simpler and has less risk of introducing other bugs, so I'd suggest starting with that.
Comment #6
xjmComment #7
abrammHi 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.
Comment #8
dpagini CreditAttribution: dpagini as a volunteer commentedIt 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)?
Comment #9
idebr CreditAttribution: idebr at ezCompany commentedAttached patch implements the suggestions by xjm in #5:
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:
Comment #11
idebr CreditAttribution: idebr at ezCompany commentedUpdated the VocabularyPermissionsTest to reflect that Weight is in fact visible on the page.
Comment #12
dpagini CreditAttribution: dpagini as a volunteer commentedI'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.
Comment #13
idebr CreditAttribution: idebr at ezCompany commentedI 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
Comment #14
xjmGood 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.
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!
Comment #15
idebr CreditAttribution: idebr at ezCompany commented@xjm That makes sense, thanks for the pointers.
I have updated the test coverage to include checks for the actual form field for Weight.
Comment #17
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commented#14 was addressed, back to RTBC
Comment #18
mohit_aghera CreditAttribution: mohit_aghera as a volunteer and at Axelerant commentedRe-roll for 8.6.x branch.
Comment #19
idebr CreditAttribution: idebr at ezCompany commentedRe #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.
Comment #20
catchCommitted/pushed to 8.6.x and cherry-picked to 8.5.x. Thanks!
Comment #23
idebr CreditAttribution: idebr at ezCompany commentedUpdating issue status to ‘Fixed’