DDEV is the official local development tool of Drupal. And like Drupal, DDEV depends on the support of the open source community.Problem/Motivation
A child taxonomy term is automatically deleted when it only has one parent term and that parent is deleted. In one delete submission, multiple terms can be deleted, but the user is never fully informed of the deleted terms. Neither before or after confirmation of the deletion. In this situation, users might not realize which terms are actually deleted.
After completion of the operation, the produced log notice only include the root deleted term. Leaving no trace to help a confused user who don;t understand why a term he did not delete disappeared.
When the user is asked to confirm the deletion of a taxonomy term, a confusing "Deleting a term will delete all its children if there are any. This action cannot be undone." is displayed. This message is confusing because:
- On the confirmation page, the user has no way to known if the term as any children.
- The deletion is recursive (all descendants are affected, not just direct children). But the message usage of "children" fail to acknowledge it.
- Any child of a deleted descendants that has a non-deleted parents will not be deleted. But the message fail to inform the user that some descendants may bot be deleted.
What behavior were you expecting?
Before confirmation of the operation, the user should be presented with a list of all taxonomy terms that will be deleted.
On completion of the delete operation, a confirmation message including the list of all the deleted taxonomy terms, and a matching log notice.
What happened instead?
Before confirmation of the operation, a confusing "Deleting a term will delete all its children if there are any. This action cannot be undone." message.
One confirmation message reading "Deleted term Term1" with a matching log notice.
Steps to reproduce
- Create a taxonomy term, Term1
- Create another taxonomy term, Term2, whose parent is Term1
- Delete Term1 (Term2 will then be deleted)
Proposed resolution
Override the methods used for confirmation messages and log to include the complete list of deleted terms.
Remaining tasks
- Please review the attached patch and make suggestions as necessary.
- Tests are needed.
Add links to before and after screenshots in the next section so the reviewer can find the latest ones.
User interface changes
Before
TBA
After
TBA
API changes
Data model changes
Release notes snippet
Note: Original reporter, @guschilds, came across this issue when debugging #726490: Taxonomy term record for term with multiple parents is not deleted when parent is deleted.
| Comment | File | Size | Author |
|---|---|---|---|
| #28 | after patch.png | 85.23 KB | dishakatariya |
| #28 | beforepatch.png | 82.48 KB | dishakatariya |
| #27 | Tags-DrupalPod (1).png | 89 KB | dishakatariya |
| #27 | Tags-DrupalPod (2).png | 93.18 KB | dishakatariya |
| #25 | confirmation_message-with-taxonomy_list.png | 64.46 KB | shalini_jha |
Issue fork drupal-1766486
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #1
guschilds CreditAttribution: guschilds commentedAttaching the proposed solution.
Comment #2
guschilds CreditAttribution: guschilds commentedActually, this patch moves the alerts to a more intelligent place. They now live in the same loop of postDelete as the check for orphans.
A message will be output for every term deleted, rather than only the single submitted deletion.
Comment #5
shipra.wasson CreditAttribution: shipra.wasson commentedThe delete confirmation message alreay says " Deleting a term will delete all its children if there are any. This action cannot be undone. ". Attaching a screenshot .
Comment #6
David Hernández CreditAttribution: David Hernández commentedHello!
Thank you for working on this issue!
We should all try and use the same sprint tag. According to https://groups.drupal.org/node/447258 it should be SprintWeekend2015 with no #.
Comment #7
mgiffordNot sure why this is Needs review.
Comment #8
pbuyle CreditAttribution: pbuyle as a volunteer and at Floe design + technologies commentedI think @shipra.wasson used the review status because the current behavior is acceptable. Prior to deletion, the user is warned the children (if any) will be deleted. I guess Shipra didn't want to close the issue as "work as designed" but wait for some else confirmation that the current behavior was enough.
I tend to disagree, IMHO providing complete feedback on the deleted terms is indeed needed.
The message (and watchdog) logic are in
ContentEntityDeleteForm::submitForm, but the logic to delete orphans terms is inTerm::postDelete. The message and watchdog should be one inTermDeleteForm::submitForm, but there is no way to get the list of orphans after the call toparent::submitForm. In order to display the list of deleted term, the form would need its orphans detection logic prior to callingparent::submitFormto be able to display the list of deleted terms.Comment #9
pbuyle CreditAttribution: pbuyle as a volunteer and at Floe design + technologies commentedThe attached patch provides more complete message and log.
However, I'm not satisfied with the complicated solution I ended up to nicely display a list of entity labels. Is there a better way to display a list of strings, each escaped as a emphasized placeholder?
Also, the added
TermDeleteForm::getDeletedTerms()method is non-trivial and should be covered by some form of tests. Same for the list formatting logic. I've limited experience of testing in Drupal 8. How are this kind of stuffs usually tested? It could be tested via a web test, but that seems unnecessary complex. A unit test of the class seems easier to do.Comment #18
pameeela CreditAttribution: pameeela commentedStill valid in 9.3.x
Comment #23
shalini_jha CreditAttribution: shalini_jha at QED42 for QED42 commentedI have able to reproduce the issues by following the steps in 11.x-dev.
Comment #25
shalini_jha CreditAttribution: shalini_jha at QED42 for QED42 commentedHi everyone,
I have taken reference from #9 and added all the required changes and added a Mr with test .
added screen shot for your reference.
please review.
Comment #26
dishakatariya CreditAttribution: dishakatariya as a volunteer and at QED42 for Drupal India Association commentedComment #27
dishakatariya CreditAttribution: dishakatariya as a volunteer and at QED42 for Drupal India Association commentedHi, I have verified and tested MR- merge_requests/6306 file on the 11.x version with.It is working as expected now.
Testing steps-
Install the Drupal 11.x version
Now go to the /structure/taxonomy and click on list terms
Now click to Add term
Add themultiple terms, set as one parent term and in that drag the other as child terms
Now click to save
and then click to the Delete operation
and now see on the popup which displays the parent term name with the child name as well with the notification.
Testing Result:
Orphan taxonomy terms are deleted with the notification.
Attaching screenshot for reference.
Thanks!
Comment #28
dishakatariya CreditAttribution: dishakatariya as a volunteer and at QED42 for Drupal India Association commentedComment #29
quietone CreditAttribution: quietone at PreviousNext commentedOh, I do love older issues getting fixed. Thanks!
I'm triaging RTBC issues.
I checked the history and the issue summary was last updated in 2016, so I suspect it is out of date. Yes, it is it states that tests are needed but I some in the MR. It is a well written issue summary though. Easy to know what is being fixed here. I am tagging for an Issue Summary update. This is changing the UI so I am tagging for Usability. Since it does change the UI before and after screenshots should be available to the reviewer from the Issue Summary. I have updated the issue summary to the current standard template to help with the update.
I then read the comments. #9 states "I'm not satisfied with the complicated solution I ended up to nicely display a list of entity labels". That patch was converted to an MR and that code looks to be the same. That still needs to be addressed. And further, I do not a comment that anyone has done a code review.
There is a contributor task for https://www.drupal.org/community/contributor-guide/task/review-a-patch-or-merge-request that you may find helpful.
I have not reviewed the MR nor tested it.