- Issue created by @dalin
- First commit to issue fork.
- 🇮🇳India utkarsh_33
I have added some logic that tests whether the child links are present for the link that we are deleting. If we have child elements then it shows a message(may not be correctly defined now) that this link contains child links which will be effected due to the deletion of the parent link.I am attaching the screenshots as a kickstart to this.
We can similarly do this after the successful deletion of the link as well as described in the issue. - last update
over 1 year ago Custom Commands Failed - @utkarsh_33 opened merge request.
- last update
over 1 year ago 30,241 pass, 66 fail - 🇲🇽Mexico dalin 🇨🇦, 🇲🇽, 🌍
@Utkarsh_33
This isn't about when menu items are deleted. It's about when nodes (or I guess other entities too) within the menu are deleted. - 🇧🇷Brazil joaopauloc.dev
joaopauloc.dev → made their first commit to this issue’s fork.
- Merge request !5807Adding warning message when node delete is referenced by a menu link → (Closed) created by joaopauloc.dev
- 🇧🇷Brazil joaopauloc.dev
Hi folks, I have two questions.
First, the warning message will be displayed only for a node that has a menu item as a parent? or should we warn users since the content has any kind of reference in menus? If yes I need to update the implementation.
In Drupal 11 the menu link is also deleted if the content is deleted. We need to update the message, I thought something like
This page has 3 menu children. These menu links will be deleted too if you continue.
Thanks.
- 🇲🇽Mexico dalin 🇨🇦, 🇲🇽, 🌍
@joaopauloc.dev
I like how you moved to ContentEntityDeleteForm.php so that this will apply to all content, not just nodes. But I'm not sure that getQuestion() is the right place for this. There's several returns in that function, and you've just added one more. The original MR used buildForm(), and that might be a better choice. Especially since we want to also use a list.First, the warning message will be displayed only for a node that has a menu item as a parent? or should we warn users since the content has any kind of reference in menus?
Only warn when an entity is being deleted, and that entity has a menu item, and that menu item has child menu items.
In Drupal 11 the menu link is also deleted if the content is deleted. We need to update the message, I thought something like: This page has 3 menu children. These menu links will be deleted too if you continue.
Yes, the menu item being deleted is what causes the problem that this ticket is trying to solve. But are you saying that in D11 the _children_ menu items are also blindly deleted??? I can't get https://simplytest.me/ to build D11 so that I can confirm. But if that's the case, then that's a separate critical issue of content destruction. I'm guessing that's _not_ happening.
I've updated the ticket description to have clearer messaging. Can you take a look?
- 🇧🇷Brazil joaopauloc.dev
Thank you @dalin for the feedback.
I'll adjust the menu to warn only on the criteria that you mentioned. Also, fix the getQuestion things that you mentioned.
Regarding the menu item also been deleted I'll confirm one more time but I'm almost sure that was deleted too.
I'll work on this issue until this weekend and let you know.
thanks. - Assigned to joaopauloc.dev
- 🇧🇷Brazil joaopauloc.dev
Hey @dalin, I could confirm that only the menu item of the content was deleted.
Working on the issue... - Issue was unassigned.
- 🇧🇷Brazil joaopauloc.dev
Updated the implementation to show the warning message before and after the content is deleted.
Missing unit tests and fixing probably unit test issues. - Status changed to Needs review
about 1 year ago 1:24pm 19 January 2024 - Status changed to RTBC
about 1 year ago 2:20pm 19 January 2024 - 🇮🇳India arunkumark Coimbatore
The MR 5807 applied cleanly. The patch working as expected. Attached screenshot of manual testing.
- Status changed to Needs work
about 1 year ago 9:08pm 3 February 2024 - 🇸🇰Slovakia poker10
What will happe, if there will be hundreds of child menu items? I suppose this will not look good. Do we need to print the child menu items?
I think we need to add a test for this new feature, so that we can confirm that the extra message is here, when the menu item has children and that the message is not there, when deleting a menu item without child items. Thanks!
- Assigned to dpi
- Merge request !6479Warn if an entity delete will cause menu item re-parenting → (Open) created by dpi
- Issue was unassigned.
- Status changed to Needs review
about 1 year ago 7:53am 8 February 2024 - 🇦🇺Australia dpi Perth, Australia
I took a look at the existing MR5807, and found some issues in my review:
- The moderation state change looks irrelevant.
- The changes here should work with any entity, not just nodes.
- Shouldnt get menu items by menu link content entity to get descendants, via entity queries.
- The menu renders lists with HTML in t-strings, Markup elements, etc.
Rather than add feedback to the MR, I've started from scratch as I dont think the code is usable.
Notes on the new implementation at MR6479:
- Uses a similar method to `menu_ui_get_menu_link_defaults()` to determine the menu link of the entity in context.
- Renders with the standard menu builder. Which uses a proper list element instead of markup
- Reworded warning message, use singular labels instead of _content_
- Used formatplural, depending on number of child menu items.
- Created a internal class service, without service id. Instead of using the old classResolver pattern.
- In line with hooks, the new form alter is not overridable:
- the class been finalised.
- no service ID.
- This was chosen as it's possible to easily unfinalise or add a service ID in the future (new issue) if we decide thats needed. Doing the reverse is not possible without dealing with backwards compatibility and deprecations.
- update the used `getTranslationFromContext` method with a generic, so PHPStan knows its @return-ing the same entity type as is passed into the parameter.
- Test coverage.
Screenshots
Single menu item child
Multiple menu item children
Message after delete:
Re: @poker10 feedback in #17,
> What will happen, if there will be hundreds of child menu items?
The message will only return the direct children. Not multiple levels down.
> I think we need to add a test for this new feature
Added. I think the tests are comprehensive.
- 🇦🇺Australia dpi Perth, Australia
Added notes to review. Remains _Needs review_.
- 🇦🇺Australia dpi Perth, Australia
Removed "node" references from issue summary.
- Status changed to Needs work
about 1 year ago 12:07am 10 February 2024 - 🇲🇽Mexico dalin 🇨🇦, 🇲🇽, 🌍
I've discovered that this also includes a menu entity delete. Updating the description accordingly. I updated the remaining tasks.
IMO we don't need to do anything special if there are lots (a hundred?) direct children. The list will be long. So be it. But this will almost never happen in real life.
- 🇲🇽Mexico dalin 🇨🇦, 🇲🇽, 🌍
IMO we should still show something for menu items that the user doesn't have access to. They should be included in the total count, but in the list we should show "access denied" or something similar.
- Status changed to Needs review
about 1 year ago 8:01am 10 February 2024 - 🇦🇺Australia dpi Perth, Australia
I've discovered that this also includes a menu entity delete.
Agree this is a case worth covering, updated the MR and tests to acommodate Menu Link Content entities.
we don't need to do anything special if there are lots (a hundred?) direct children. The list will be long. So be it. But this will almost never happen in real life.
I think it'd be more common than "almost never", but I think since the menu system is already capable of rendering the menu items to the front end in runtime, then it will also be fine for backend tasks.
Nothing further required for the item.
IMO we should still show something for menu items that the user doesn't have access to. They should be included in the total count, but in the list we should show "access denied" or something similar.
I disagree, I dont think theres a case in Drupal where we inform the user there is some secret data the user doesnt know about but is affected by an operation, except when permission to do the operation straight up denies the user.
To me, only exposing data relevant to the user is sufficient.
----
Changes in MR.
- 🇺🇸United States smustgrave
Seems next remaining task is to agree on wording. UX team should have a say in that.
- Status changed to Needs work
about 1 year ago 11:01am 27 February 2024 The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.
- 🇮🇳India samit.310@gmail.com
samit.310@gmail.com → made their first commit to this issue’s fork.
- Status changed to Needs review
8 months ago 10:52am 16 August 2024 - 🇮🇳India samit.310@gmail.com
Fix conflicts and chnages in test file with D11 compatibility.
Thanks
Samit K. - Status changed to Needs work
8 months ago 2:38pm 16 August 2024 - First commit to issue fork.
- 🇮🇳India shalini_jha
After rebasing merge request (MR), it seems that PHPStan is flagging issues in StringTranslationTrait.php .
- 🇮🇳India shalini_jha
AS bot reported issue is fixed , and rebased the MR, so Moving this Back to NR. Kindly review.
- 🇺🇸United States benjifisher Boston area
We discussed this issue at ✨ Warn user when entity delete will cause menu item re-parenting Needs review . That issue will have a link to a recording of the meeting.
Thanks for working on this issue. At least one attendee has run into exactly this problem, and there were a lot of child menu items involved.
For the record, the attendees at the usability meeting were @benjifisher, @rkoller, @simohell, and @worldlinemine.
When you tag an issue for usability review, please make it easy for the usability team to review the issue. Update the issue summary:
- The "Proposed resolution" section should describe all the changes made in the issue.
- The "User interface changes" should show the existing UI and the proposed UI.
- The "Remaining tasks" should include one explaining the usability issue(s).
Most of the time, I prefer to have plain text in the "Proposed resolution" section and screenshots in the "User interface changes" section.
You can also attend the weekly usability meeting to present an issue.
Usability review
We have a few recommendations:
- Shorten the text in the confirmation form.
- Suggest changing the parent of the menu item before deleting it.
- Special treatment when deleting custom menu links.
- Follow-up issue: provide an option to delete the child menu items.
Shorten the text
There is room for improvement, but this is the best we could do during the meeting. After the standard "This action cannot be undone.", either
Deleting this [content item] will make these [count] child menu items top level:
...or
Deleting this [content item] will move these [count] child menu items under [parent link]:
...Make adjustments for singular or plural, as the current MR already does.
Suggest changing the parent
A good status message starts by describing the situation, but then it suggests what to do about it. In this case, a common action will be to change the parent of the menu item that is going to be deleted: then, after deletion, all the child menu items will be children of the new parent.
Changing the parent item can be done from the menu page (where all the items in the menu are listed, with a click-and-drag interface), from the edit page for the individual menu link (where the parent can be selected from a list) or from the edit page of the content item. We feel that the click-and-drag interface is problematic. There might be permission issues with editing the menu item, but that gives the most flexibility.
If the user is deleting a content entity (not a custom menu link ... see the next point) and the suggestion is to edit the related menu link, then it would help to give a link to that edit page.
We do not have suggested text, but this information can go after the list of affected child items.
Special treatment when deleting custom menu links
Custom menu links are content items, so the warning gets added to the confirmation form when deleting a custom menu link. So far, so good! But the text on the current MR, when deleting a custom menu link, includes "The menu items for this custom menu link", which does not make sense. Pay attention to the case where the content item being deleted is a custom menu link.
With the text proposed earlier, that entire sentence is removed, so maybe we do not have to worry about it. But keep this case in mind while continuing to work on this issue.
Follow-up issue
Sometimes, the user will want to delete the child menu items instead of move them to a different parent. There are a lot of things to be considered here, so we recommend adding a follow-up issue to discuss how to implement this. Maybe the right thing to do is turn the menu page into a form that allows bulk operations. Maybe there should be an option on the confirmation form, or the edit form for a custom menu link, to delete children. Should that include all descendants? How can we make it clear whether we are deleting content entities or their related custom menu links?
If you want more feedback from the usability team, a good way to reach out is in the #ux channel in Slack.
- 🇩🇪Germany rkoller Nürnberg, Germany
Usability review
We discussed this issue at đź“Ś Drupal Usability Meeting 2024-02-16 Active . The link to the recording of the meeting is: https://youtu.be/GeLGUAqrMTQ
For the record, the attendees at the usability meeting were @AaronMcHale, @benjifisher, @rkoller, @simohell, and @worldlinemine.
During followup discussions for the usability meeting on friday, we’ve realized that we had already discussed this issue a few months ago in february; unfortunately, it slipped through, and we haven’t had posted any summary yet. Apologies. :(
But the two reviews complement each other quite well, because back then we haven’t had the time to get into the whole wordsmithing part. And as a disclaimer, I haven’t rewatched the entire recording, but just focused on close to the end, where we usually summarize our conclusions. So in addition to the points @benjifisher summarized in #3387665-37: Warn user when entity delete will cause menu item re-parenting → the following aspects should be considered:
In the examples and screenshots shared on this issue, the number of listed children on the confirmation dialoge modal and the warning message is rather small. But one of the attendees raised the concern, based on his own experiences, that the odds are high to have a very large set of menu items with many children and grandchildren. By providing a list of menu items, that list will become unwieldy fast, if all affected menu items are being listed. Providing some logic that displays a list with equal or less than for example three items, while hiding the list with more than three items might overcomplicate things. So the recommended approach we had a consensus on was to only to provide a summary on the confirmation dialog modal and the warning message (micro copy wise the proposal from the “shorten the text” section in #3387665-37: Warn user when entity delete will cause menu item re-parenting → would work), the URLs should be moved to a log message. That log message should be created containing the list of affected URLs, so the information about the made change is available persistently for easier recovery, and the retracing of the applied steps, if necessary.
And it was noted that the following scenario is currently no covered by the MR yet - if a node is being deleted and that node is contained in more than one menu, the user is currently not being informed about that fact. Should that detail be moved to a followup issue or already be worked on within the scope of this issue?
If you want more feedback from the usability team, a good way to reach out is in the #ux channel in Slack.
- 🇧🇷Brazil joaopauloc.dev
Hi folks.
I updated the issue summary adding a screenshot with the User interface changes as recommended in comment #37.
Also, when I tested the MR I found the following issue.
Finally, update the modal message with the comments suggested in comment #37. Regarding this message, I would like to suggest another one when there is only one item, for the singular message we could use something like:
Deleting this @singular_label will move its child menu item to the top level:
What do you guys think? - 🇧🇷Brazil joaopauloc.dev
Hi @benjifisher and @rkoller, thanks for reviewing this issue.
These are my suggestions for the points Suggest changing the parent, Special treatment when deleting custom menu links.
For the Suggest changing the parent
The link will be opened in another tab. What do you guys think?Regarding the Special treatment when deleting custom menu links
I changed the text message as you guys suggested. - 🇺🇸United States smustgrave
So I went into test
Drupal 11.x
Added an article under Main navigation (Article 1)
Added an article under the one I just created. (Article 1.1)
Went to delete Article 1
I never received a warning or anything.