Content is assigned to wrong menu level if user does not have access to the current level

We're also having problems with pages jumping to wrong places because the correct menu parent isn't visible to the editors due to workbench access permissions. I created a patch partly reverting the changes done in #2988119 and building on top of it to always show the current menu parent in the options regardless of the permission definitions. This likely isn't ideal, though – maybe the menu element should be disabled altogether if the parent doesn't match? Anyway, it seems that the attached patch solves the problem for us for now.

Patches must be set to Needs Review to trigger testing.

The initial change (mentioned in #8) comes with a test. The new patch breaks tests.

What we need is a bullet list of steps to reproduce the error.

I don't know if this will help but I applied the patch and it solved my problem.

This is what I observed before applying the patch...

If a page has "" as its "Parent Link" and the person editing that page does not have access to "Main navigation" in "Workbench Access", that option will not appear in the "Parent Link" field. Instead, the parent of that page will be set to the first item in the dropdown of the "Parent Link" field.

I've confirmed this by giving new roles to that same person (I only use roles in "Workbench Access"). If a new role has access to a new menu item and that item appears above what was previously the top one (remember that "" does not show), than this new item becomes that page's parent menu item once it is saved.

Ex (roles in "Workbench Access" in parenthesis):
(role = admin)
- About (role = support)
- How to buy (role = seller)
- Consumers (role = seller)

ALL scenarios below pressupose that originally, "Parent link" is set to ""

Scenario 1: "admin" edits the "Consumers" page => everything is fine.
Scenario 2: "seller" edits the "Consumers" page => the parent link changes to to "How to buy" (I believe it's because "How to buy" becomes the first available item in the "Parent Link" menu. is not present anymore)
Scenario 3: I give "seller" a second role of "support". "seller" edits the "Consumers" page => the parent link changes to "About".

+1 for the patch fixing the issue. I don't have time today to look into the failing tests but I'll add it to my list if I get any in the next couple of days.

Weirdly our user had NO access to the menu at all, but the menu entry was still being changed every time they saved the node in question. As an experiement I gave the user the accesses needed to edit the menu and it still changed the parent menu item. I could load the node up with User 1 and with the restricted user in two separate browsers and User 1's edit view had the correct parent menu item on load and the restricted user did not.

@drakythe Our issue is similar that some user roles don't have access to edit menu items and that causes the menu level to jump to wrong position.

I am really tempted to attribute these problems to the menu system generally and say that you shouldn't use the menu scheme if you don't let users access the menu.

However, that seems counter-productive, so we should look at a test and a patch.

I also wonder if https://www.drupal.org/project/workbench_menu_access → solves this problem by allowing menu access but restricting its use.

Wait a second....

If a page has "" as its "Parent Link" and the person editing that page does not have access to "Main navigation" in "Workbench Access", that option will not appear in the "Parent Link" field. Instead, the parent of that page will be set to the first item in the dropdown of the "Parent Link" field.

Why does this person have access to edit the node at all?

The attached patch does indeed fix the issue -- but it breaks default value handling, causing a new problem.

The question becomes: if the default menu item for a content type is not in the author's sections, what do we do with it?

See the test added in #2988119: Setting a default menu value →

Updated with a merge request that does the following:

* Ensures we do not remove the top-level parent if no other options exist (which should solve the initial report)
* Removes any options -- including the default_value -- if not accessible by the user.
* Updates the existing test to remove the explicit "selected" check

See https://git.drupalcode.org/project/workbench_access/-/merge_requests/15

Attached is also a patch version for manual / composer review.

#9 solved the problem for me. Unfortunately #20 did not.

We still have problems because our users are not always assigned to a top-level page section. For a real world example, we have a "Student Life" section with a "Student Handbook" child page, and numerous subsections. Like such:

• Student Life
• Orientation
• Student Services
• Student Handbook
• Section 1
• Section 2
• ...

Somebody has been assigned editing privileges to the entire Student Handbook section, but NOT the rest of Student Life.

While creating new pages, she can only insert nodes to the menu as descendants of Student Handbook. This is surely correct behavior.

While editing existing pages, she still can only insert nodes as descendants of Student Handbook. This is mostly fine, but the problem arises when she tries to edit the Student Handbook page itself. Since it is not a descendant of itself, she cannot keep it in place, which is pretty nonsensical.

I believe #9 is on the right track in stipulating that authors should never be forced to relocate existing nodes as a consequence of updating content.

It's even more problematic with Content Moderation enabled. Relocating a page in the menu is not permitted while creating or editing a draft (because menu links aren't revisioned at all). In this case, when Workbench Access forces the user to relocate the page it means she cannot create a draft at all!

#9 is also similar to the way Drupal Core handles text formats. While editing a node with a text field that already has an un-permitted text format assigned it's OK to leave the format in place but we simply cannot edit the text in that case. We aren't forced to remove the formatted that a higher privileged user has already written.

I rarely +1 here but the use case of @gslexie is 100% what we have been struggling with and completely agree with his comment.

#9 does not address all use-cases and has no tests, so this needs a refactor.

I do see that we need this: "authors should never be forced to relocate existing nodes as a consequence of updating content."

That in itself needs a test.

I wonder if relocation is not permitted, should we just hide the field? #20 does that if the user is not assigned to any sections.

This came up again on a new project and I finally found some time to take another look. I've added a new MR that passed all tests locally, with a combination of changes from the patches in #9 and in #20, and my own adjustments:

NodeFormMenuTest:

• Added a test that previously assigned menu links won't be forcibly removed even if the editor doesn't have direct access to the parent section.
• Fixed a prior test that looks to be intended to create a new menu link, but doesn't (I need this link for my test).

Menu::alterForm:

• A little bit of refactoring to (hopefully) simplify the main foreach loop
• Removed the separate check for root menu item. As far as I can tell, this check is necessarily covered by the Remove unusable elements, except the existing parent. section already and probably exists for historical reasons. The tests pass without it.
• Included from #20: hiding the menu UI where the user has no sections. BUT skip this if there is already a menu link previously assigned by another user (i.e., we are editing a previously created node).
• Included from #9: Don't remove the default value, BUT only if it was already assigned by another user
• Included from #20: the final fallback behavior

MR !19 fixed the bug in my case, thank you gslexie!

I don't change issue status as the review should probably be more than that.

Also, it seems it's more a bug than a feature. Pages get moved in the menu without anyone wanting that.