Allow redirection to vocabulary list terms page after creating a new term

Hi @jeroendegloire. Thank you for the issue and patch.

There are some coding standards errors on the patch that will need to be fixed. You can see these be clicking on the test results.

This may be deemed as a feature. The current workflow is useful in the case where a user will want to add multiple tags. But the button does state "Add term" and not "terms". Because of this I've added the "Needs framework manager review" to determine which workflow should be used.

The patch applies cleanly, and when a new term is submitted I am returned to the term list page.

Aside from the coding standards, the code is nice and concise.

I've also added the 'needs tests' tag. If this is the intended behaviour then we should ensure that it is not reverted in the future.

I think that patch from @jeroendegloire really useful.
So I added fix for coding standard's issue and added test.
Hope fix fix come to core soon.

This now has tests, and I guess it looks good, this still needs a review from a framework manager or a product manager (more likely the latter I think). Adding that needs review tag as well.

Rerolled this patch so it applies to the latest drupal 9.1.x version.

I'm not sure how we get this on the list of items to look at for the tags listed here. Added ux tag as well, because that might help? Can't set to rtbc because we still need product/framework review (per #3)

Fixed testing issue, Please review tha patch.

Updated the deprecated AssertLegacyTrait::assertUrl() with $this->assertSession()->addressEquals() .

I have applied the "3056258-13.patch" patch on drupal 9.1 dev version,the patch applied successfully and when we submit a new term, it will redirect to the term list page.

Re-tagging, would be good to get a UX review here.

I think this would be better done by setting destination on the add term link, but keeping the redirect to the term itself when navigating to term add directly.

I don't think this is a bug. I think this is "work as designed" and it is a task if a UX reviewer says that it should be changed.
Let's see what the UX reviewer will say.

This also needs an issue summary update. It would be good to use the template and it looks like #16 is suggesting an alternative solution.

Applied patch #13 .It works fine.Adding visual below:

Rerolled patch.

Needs work for #16 and #19

Patch in #22 no longer applies. Rerolled and fixed custom command errors in #22. Retaining status "Needs work" as per #23.

Thank you!

Updated issue summary.

Add related issue, if that is resolved we can use the "optional destination parameter" for the "add term" action.

Adding Patch and tests based on the inputs given by #16.

The patch adds a ?destination= query to the url when user clicks "Add term" link , and made a change

+if(\Drupal::request()->query->get('destination') == ''){
     $form_state->setRedirectUrl($term->vid->entity->toUrl('overview-form'));
+   }

that checks use default save redirect if no destination is available.

Fixed coding standards of patch uploaded in #27.

The patch works as intended. As the picture shows, now the user will be redirected to the correct page.

Submitting patch adressing spell check and PHPCs complains.

Hello, I have reviewed patch #29, it worked and changes seemed ok for me, but I found a small lint error with phpcs in one of the new tests. I'm sending a new patch with the fix.

Hello, just reviewed the patch on #32. I used phpcs and couldn't find any error on the changed lines, then I tested the redirect and it worked fine. I'm moving to RTBC.

Please do not ask the testbot to try again until #3207086: [HEAD BROKEN] Consistent failure in MonthDatePluginTest is fixed.

#3207086: [HEAD BROKEN] Consistent failure in MonthDatePluginTest is fixed. I have not reviewed the code.

Just moved this back to needs review so some maintainer can give a usability review.

Ops.

Manual testing was done in #20 and #34, it was successful but did not update the tags. Removing testing tag.

I started to review the patch and found some items, see below. Then I wondered what D7 does. So I stopped and tested this on D7. In D7 when adding a term one stays on the add term page. Presumably that is helpful when adding a lot of terms. I think that makes this issue a Feature Request, for now? I am not changing the category because I am not sure. Also, leaving at NR for the usability review.

I agree this really needs a usability review and work on the patch is postponed until that happens.

Here are some of things I found, posting in case they are helpful after the usability review.

1. +++ b/core/modules/taxonomy/tests/src/Functional/VocabularyPermissionsTest.php
   @@ -369,4 +372,30 @@ public function testVocabularyPermissionsTaxonomyTerm() {
   +   * Check if destination query present and redirection is happening
   +   * back to overview.
   

   This is hard to follow. Can it be simpler and one line? 'Tests the redirection after adding a term'?

2. +++ b/core/modules/taxonomy/tests/src/Functional/VocabularyPermissionsTest.php
   @@ -369,4 +372,30 @@ public function testVocabularyPermissionsTaxonomyTerm() {
   +  public function testVocabularyRedirectWithDestination() {
   

   This tests is in a file VocabularyPermissionsTest, which hides what it does. It also doesn't need to do all the setup that is done here. Lets put this in its own file.

3. +++ b/core/modules/taxonomy/tests/src/Functional/VocabularyPermissionsTest.php
   @@ -369,4 +372,30 @@ public function testVocabularyPermissionsTaxonomyTerm() {
   +    // Test as admin user.
   +    $user = $this->drupalCreateUser(['administer taxonomy']);
   +    $this->drupalLogin($user);
   

   Just use $this->drupalLogin($this->rootUser);

4. +++ b/core/modules/taxonomy/tests/src/Functional/VocabularyPermissionsTest.php
   @@ -369,4 +372,30 @@ public function testVocabularyPermissionsTaxonomyTerm() {
   +    $this->clickLink(t('Add term'));
   

   Tests do not use t(), unless it is a translation test.

Addressed #40.

Addressed #40.

@KapilV, did you read all of comment #40? Work on the patch is to continue only after the usability review and only if that review confirms that this should be implemented.

I asked for direction on this issue in #ux, jhodgon confirmed that the current behavior, staying on the add term page is what is done on D6 and D7. And both rkoller and Antoniya prefer the current behaviour. And Antoniya going further to also want the adding of menu links to behave in a similar way. This is starting to feel like a won't fix but that is not my decision.

Since this is changing a workflow that has existed since Drupal 6, changing to a Feature Request.

Since there is some value in staying on the edit page if adding a bunch of terms, then I think it's OK that you return to the "add term" page. Or that if this change is made that you could do what was suggested in comment #16 and set the destination.

Other ideas: An advanced administration could be created in contrib or custom (possibly by using editable views) to provide a table or list view of the terms. Or, the vocabulary page could allow in place editing of the term names. But keep in mind that often large lists of terms are not created in the admin interface anyway. Terms are often generated by code or created in a tagging field.

Usability review

We discussed this issue, and tested the patch in #42, at #3206957: Drupal Usability Meeting 2021-04-09.

First, the testing. I applied the patch and installed the Umami demo profile. When adding a term to the Recipe vocabulary, I see that destination=/admin/structure/taxonomy/manage/"recipe_category"/overview is added as the query string. Because of the quotation marks (") this leads to a Page Not Found (404 response) when I submit the form.

Rather than fix that, we suggest a different approach.

As @quietone pointed out in #44, the current behavior has been around for a long time. Not only that, but the current behavior is very convenient if you want to add several terms to a vocabulary. This is a common use case, and the proposed change would make it much less convenient.

An alternative is to add a secondary button. Giving it the label "Save and return to list" would be explicit but longer than I like. You can probably come up with something better. Have a look at the dialogue from the Media Library, which already offers two "Save" buttons.

We might even consider making the new button the primary one. That would be more disruptive.

When you decide on an approach, update the Proposed resolution in the issue summary. I am adding an issue tag for that.

Thanks, @benjifisher for the review. I've updated the following things:
- Update issue summary with a proposed solution. (Needs eyes on button label)
- Add a patch with the solution
- Add test case.

I haven't updated an interdiff as this implementation approach is completely different.
Let's see how it goes!!

Hello, just checked the new implementation, made by @mohit_aghera in #47. Things seems to be in order so I'm moving the issue to RTBC.

It will help the final review if you provide "before" and "after" screenshots in the "User interface changes" section of the issue summary. I am setting to NW and adding an issue tag for that.

In #46, I suggested,

Have a look at the dialogue from the Media Library, which already offers two "Save" buttons.

It will help to add a screenshot of that, too.

Manually tested and works as advertised, screenshots below. I quite like this implementation and although 'Save and return to list' is wordy, it is clear and I am not sure how to make it shorter without making it less clear. The fact that it is not the primary button means it's easy to ignore if you want.

Very big +1 to this as a major UX improvement on something I have long thought was a very strange behaviour (mainly in that it's not a pattern used anywhere else AFAIK) but never bothered to do anything about.

Before patch: 'Add term' form:

Before patch: Behaviour on 'Save':

After patch: 'Add term' form:

After patch: Behaviour on 'Save and return to list':

After patch: Behaviour on 'Save' (confirming no change to default behaviour):

I cannot reproduce the behavior where the Media Library offers a primary and secondary button. Maybe I am thinking of #3059955: It is possible to overflow the number of items allowed in Media Library. Here is one of the screenshots from that issue:

Thanks for the screenshots in #53. I will add two of them to the issue summary. I am also updating the release-notes snippet.

I have not reviewed the code. Back to RTBC based on #51 and the screenshots I requested in #52.

@benjifisher you have to enable the secondary button at /admin/config/media/media-library and then it comes up when adding a new media item via the media library widget:

@pameela: Thanks, I needed that reminder (obviously).

Sorry, I changed my mind.

First problem: suppose some module provides a link to the "Add term" page with a destination parameter: for example, /admin/structure/taxonomy/manage/tags/add?destination=/node/add. When you submit the form, you are redirected to the destination parameter, no matter which button you use.

Second problem: what if there is a link to the "Add term" page from somewhere else? Then, when you submit the form (with the new button) you are going to the List page, but you are not returning there. Then "return to list" is confusing.

Not a problem: I was worried that "list" could be confusing, since we often refer to the listing page as the "Vocabulary overview page" or similar. But the primary tab on that page is labeled "List", as shown in one of the screenshots in #53, so I think "list" is clear.

For the first problem, one idea is to override the destination parameter. I think a better idea is to check if there is a destination parameter. If there is, then do not add the secondary button. I have not tested, but something like

if (!$this->getRequest()->query->has('destination')) {
 // Add the secondary button.
}

should do it.

For the second problem, we could use "Save and go to list". That is a tiny bit shorter than the current "Save and return to list". Or use a comma: "Save, go to list".

NW for #57. I need to get some sleep.

I looked at the code:

  protected function actions(array $form, FormStateInterface $form_state) {
    $element = parent::actions($form, $form_state);
    $element['return'] = [
      // ...
    ];

    return $element;
  }

  /**
   * Form submission handler for the 'return' action.
   * ...
   */
  public function return(array $form, FormStateInterface $form_state) {
    $vocabulary = $this->entityTypeManager->getStorage('taxonomy_vocabulary')->load($form_state->getValue('vid'));
    $form_state->setRedirectUrl($vocabulary->toUrl('overview-form'));
  }

Follow-up to #57: use something other than "return" for the key in the render array and the name of the callback. One option is "overview".

Keep code lines under 80 characters when you can. You can break the long line before ->getStorage() and ->load(). Maybe just the second.

Addressing the following issues:
- Update the action form title.
- Change the label of the button.
- Added condition to add redirect button only when destination parameter is not present.
- Add and update test cases accordingly.

Thanks for the updates!

1. You missed one place to update "return":

    +++ b/core/modules/taxonomy/src/TermForm.php
    @@ -95,6 +95,37 @@ public function form(array $form, FormStateInterface $form_state) {
    ...
    +  /**
    +   * Form submission handler for the 'return' action.
    +   *

2. I think we prefer to use the optional parameter to drupalGet() instead of adding a query string to the URL:

    +++ b/core/modules/taxonomy/tests/src/Functional/TermTest.php
    @@ -398,6 +398,25 @@ public function testTermInterface() {
    ...
    +    // Validate that "Save and go to list" doesn't exist when destination
    +    // parameter is present.
    +    $this->drupalGet('admin/structure/taxonomy/manage/' . $this->vocabulary->id() . '/add?destination=node/add');
    +    $this->assertSession()->pageTextNotContains('Save and go to list');

   For example, in the same test class I see this:

        $this->drupalGet('admin/structure/taxonomy/manage/' . $this->vocabulary->id() . '/overview', ['query' => ['page' => 1]]);

3. Earlier in the same test method, I see this:

        // Create the term to edit.
        $this->drupalPostForm('admin/structure/taxonomy/manage/' . $this->vocabulary->id() . '/add', $edit, 'Save');
   
        $terms = taxonomy_term_load_multiple_by_name($edit['name[0][value]']);
        $term = reset($terms);
        $this->assertNotNull($term, 'Term found in database.');
   
        // Submitting a term takes us to the add page; we need the List page.
        $this->drupalGet('admin/structure/taxonomy/manage/' . $this->vocabulary->id() . '/overview');

   Maybe we can move the new assertions and have less duplication.

Fixing the first two comments mentioned in #63

@benjifisher For the third point, I've intentionally kept separate values in $edit array.
The reason is, I want to evaluate the taxonomy term is getting saved or not.

I feel it hampers the overall testing flow when I move additional test cases somewhere after below code block.

 // Create the term to edit.
     $this->drupalPostForm('admin/structure/taxonomy/manage/' . $this->vocabulary->id() . '/add', $edit, 'Save');

     $terms = taxonomy_term_load_multiple_by_name($edit['name[0][value]']);
     $term = reset($terms);
     $this->assertNotNull($term, 'Term found in database.');

     // Submitting a term takes us to the add page; we need the List page.
     $this->drupalGet('admin/structure/taxonomy/manage/' . $this->vocabulary->id() . '/overview');

Feel free to let me know your thoughts. I'll refactor it otherwise.

@mohit_aghera:

Thanks for the follow up!

I guess we do need to test both the "Save" and "Save and go to list" buttons, so you are right not to combine too much.

I would like to have more consistency between the start of the test and the end of the test. That will make it easier for someone reading the test: "OK, we did this before with one button and now we are doing the same thing with the other button." Unless there is a good reason for the differences, let's try to minimize them.

For example, the lines at the start of the test submit the form with drupalPostForm(). The new code uses a 2-step process, with drupalGet() and then submitForm(). Is there a reason for that difference?

The existing part of the test uses $this->assertRaw() and $this->assertText(). (I do not know why it uses both.) The new code uses $this->assertSession()->pageTextContains() and $this->assertSession()->pageTextNotContains(). Why not be more consistent?

@benjifisher

For example, the lines at the start of the test submit the form with drupalPostForm(). The new code uses a 2-step process, with drupalGet() and then submitForm(). Is there a reason for that difference?

drupalPostForm() is deprecated now. That's why I've used drupalGet() and submitForm() methods.

The existing part of the test uses $this->assertRaw() and $this->assertText(). (I do not know why it uses both.) The new code uses $this->assertSession()->pageTextContains() and $this->assertSession()->pageTextNotContains(). Why not be more consistent?

Similarly assertText() and assertRaw() are also deprecated.
I'm using appropriate methods $this->assertSession()->pageTextContains($edit['name[0][value]']); instead of deprecated one. That is the cause of little difference between methods.

@mohit_aghera:

Thanks for explaining, and for keeping up with the deprecated functions. Since those were my only questions, back to RTBC.

Updated the screenshot in the IS, the text of the button has changed.

What I hope is a more accurate title.

Restoring status after HEAD was broken.

Is there a meta-issue for unifying this across entity types, thinking of issues like #2974203: Redirect back to media list after creating a media entity but I think there was a very old one trying to unify the pattern.

This looks good, and seems like a safe UI addition to make. Thanks @benjifisher for the UX feedback.

Re: #73, I vaguely remember the issues you've mentioned, but we could file a new one? It probably doesn't need to block this issue; the change here has passed UX review and seems like a sensible pattern for something that is a chronic annoyance in the Taxonomy UI.

The test coverage is sufficient as well. Just a couple minor notes on typing in the added code:

1. +++ b/core/modules/taxonomy/src/TermForm.php
   @@ -95,6 +95,37 @@ public function form(array $form, FormStateInterface $form_state) {
   +  protected function actions(array $form, FormStateInterface $form_state) {
   +    $element = parent::actions($form, $form_state);
   +    if (!$this->getRequest()->query->has('destination')) {
   +      $element['overview'] = [
   +        '#type' => 'submit',
   +        '#value' => $this->t('Save and go to list'),
   +        '#weight' => 20,
   +        '#submit' => array_merge($element['submit']['#submit'], ['::overview']),
   +      ];
   +    }
   ...
   +  public function overview(array $form, FormStateInterface $form_state) {
   +    $vocabulary = $this->entityTypeManager->getStorage('taxonomy_vocabulary')
   +      ->load($form_state->getValue('vid'));
   +    $form_state->setRedirectUrl($vocabulary->toUrl('overview-form'));
   +  }
   

   Both methods always return null, so let's typehint them as such. It's safe to do so even before the parent has the typehint. References: #3050720: [Meta] Implement strict typing in existing code, #3154762: [policy, no patch] All new Drupal 10 code should use appropriate type hints wherever possible.

2. +++ b/core/modules/taxonomy/src/TermForm.php
   @@ -95,6 +95,37 @@ public function form(array $form, FormStateInterface $form_state) {
   +   * @param array $form
   

   The array here is always multidimensional, so array[] would be a better doc typehint.

I've removed credit for @KapilV since the proposed patches did not completely address the feedback they were intended to address. You can get credit next time by carefully reading the comment and implementing all of the feedback, or asking questions for any parts of it you don't understand.

Similarly, I've removed credit for @Stefdewa for the broken reroll of the previous patch., and from @pjbaert for an unneeded reroll. Instructions on rerolling patches: https://www.drupal.org/community/contributor-guide/task/reroll-a-patch (Hopefully, some day CI will do this for us automatically so it doesn't add noise to the issue.)

Thanks everyone for working on this!

+++ b/core/modules/taxonomy/src/TermForm.php
@@ -95,6 +95,37 @@ public function form(array $form, FormStateInterface $form_state) {
+  /**
+   * {@inheritdoc}
+   */
+  protected function actions(array $form, FormStateInterface $form_state) {
+    $element = parent::actions($form, $form_state);
+    if (!$this->getRequest()->query->has('destination')) {
+      $element['overview'] = [
+        '#type' => 'submit',
+        '#value' => $this->t('Save and go to list'),
+        '#weight' => 20,
+        '#submit' => array_merge($element['submit']['#submit'], ['::overview']),
+      ];
+    }
+
+    return $element;
+  }
+

@xjm I think `actions()` method returns an array. So I've updated the return type accordingly.
Other two changes are also addressed.

Thanks @mohit_aghera.

The test fails were known random failures, so Iʻve sent it for a retest.

I removed the typehint of the actions() method otherwise we will need to change the forum module to fix this issue.
Do you agree with me or do you prefer to typehint the actions() method in TermForm.php and also typehint the actions() method in ForumForm.php?

Do you agree with me or do you prefer to typehint the actions() method in TermForm.php and also typehint the actions() method in ForumForm.php?

Could go either way, but in the name of getting this irritating UI bug fixed, I'm thinking it's better to just leave it as-is for now and fix the type hints later, rather than introduce scope creep (even if it's minor). Therefore, marking this RTBC since I have confirmed that @mohit_aghera addressed the other two points of feedback.

The only issue I can think of that attempted to unify this behaviour was #2975407: After adding, deleting or editing a menu item, I am redirected to the Menus Management page rather than returned to the Menu Edit page. (@xjm also noted #1347448: [META] Taxonomy admin usability Improvements as a related issue grouping all taxonomy UX improvements together.) However, that issue spawned a number of "regression" issues like #2957953: Editing menus user-experience has regressed, so I think the lesson is: optimize each form flow for the task at hand. So this issue seems good within that framework.

Tested the patch and it works as-advertised. We might want to consider bringing this same pattern over to menu items too perhaps, since they have the same "Ok, added a bunch... wait, WHAT was I doing again?" thing that vocabs have. :)

Committed and pushed to 9.3.x. Thanks!

I disagree on removing me from credits. You can argue about the patch but I also updated the issue description from a blob of text to the described issue template. If anyone puts any personal time into pushing a issue forward and it contributes to the progress, credit should be given. It is a matter of principle.

I agree with the point @Stefdewa made. Every type of comment, patch or test which adds some value & keeps the ticket going, should receive the credit it deserves in the end. Those entries meant the person was at least evaluating & thinking about the proposed solution at that time.
If we decide this credit isn’t needed, we’re sending out the wrong message to the community. It looks as if there is some arbitrariness since we’re only crediting the selected people who’s work the creditor values.

This introduced an issue that in some cases the link is shown for users that don't have access to the vocabulary overview.