<code>The user is unable to add multiple content types to the shortcut bar by clicking the "add shortcut" star icon. This can be worked around by adding the shortcut manually via "Edit Shortcuts", but it is confusing as it stands. This does not seem to be a problem in D7. Here are a couple of animated gifs that demonstrate different behaviour between D7 and D8.

D8 - adding shortcuts via star icon

D7 - adding shortcuts via + icon

Steps to replicate.

1. Go to node/add/page
2. Click the star icon to add shortcut
3. Go to node/add/article
4. Star icon is already activated. Clicking it removes the page shortcut added earlier.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

kattekrab’s picture

Noticed the same problem when trying to add a short cut for multiple themes.

kattekrab’s picture

Does not seem to be a problem with People section. Was able to add Roles and Permissions as well as Top level People.

lahoosascoots’s picture

Looks like the issue in the last issue was that it was using the full parameters instead of the raw parameters.

As the patch removed the check for the full parameters it was only checking on route name. The route name for all content types is node.add they they are seen as the same page.

This patch should fix this issue and still work for admin/people.

lahoosascoots’s picture

Status: Active » Needs review
jibran’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

@lahoosascoots nice fix. Can we add quick tests for the issue described in IS? You can look at test added in #2478151: Shortcuts to pages generated by views are not recognized as added to the shortcutset and are being added multiple times for reference.

lahoosascoots’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
1.03 KB
1.96 KB

With tests. Test only patch should fail.

(Oops, put wrong comment #)

lahoosascoots’s picture

lahoosascoots’s picture

FileSize
2.07 KB

And fixing my indentation issue.

Will fix anything that pops up tomorrow.

lahoosascoots’s picture

Assigned: Unassigned » lahoosascoots
jibran’s picture

Issue summary: View changes
+++ b/core/modules/shortcut/src/Tests/ShortcutLinksTest.php
@@ -153,6 +153,16 @@ public function testShortcutQuickLink() {
+    //Test two pages which use the same route name but different route parameters
...
+    //Assure that Article does not have its shortcut indicated as set

Needs a space after // and first line is more then 80 chars. Please see https://www.drupal.org/node/1354#inline for reference.

kattekrab’s picture

I had manually tested patch from #7 on simplytest.me and it works!

Screenshot:
Only local images are allowed.

kattekrab’s picture

The last submitted patch, 8: Node-Add-Shortcut-Fix-2511024-8-Tests-Only.patch, failed testing.

lahoosascoots’s picture

The last submitted patch, 15: Node-Add-Shortcut-Fix-2511024-15-Tests-Only.patch, failed testing.

willzyx’s picture

oops sorry for the inconvenience..
Manually tested patch in #17 and it seems to solve the issue and it doesn't seems to generate problems with pages generated by views.
Patch looks good, just a nitpick

+++ b/core/modules/shortcut/src/Tests/ShortcutLinksTest.php
@@ -153,6 +153,16 @@ public function testShortcutQuickLink() {
+    // Test two pages which use same route name but different route parameters
...
+    // Add Shortcut for Basic Page
...
+    // Assure that Article does not have its shortcut indicated as set

Comments need a trailing period

lahoosascoots’s picture

How DARE you make me do 4 more seconds of work!? =P

Should be the final patch.

The last submitted patch, 18: Node-Add-Shortcut-Fix-2511024-18-Tests-Only.patch, failed testing.

willzyx’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/shortcut/src/Tests/ShortcutLinksTest.php
@@ -153,6 +153,16 @@ public function testShortcutQuickLink() {
+    $link = $this->xpath('//a[normalize-space()=:label]', array(':label' => 'Remove from Default shortcuts'));
+    $this->assertTrue(empty($link), 'Link Remove to Default shortcuts not found for Create Article page.');

This is a negative assertion - can we also click the link to add the shortcut for the article page and ensure it gets added.

jibran’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
762 bytes
2.22 KB

Great idea. Here we go. Setting it back to RTBC because I just added two asserts.

kattekrab’s picture

Just did another manual test. Working nicely. RTBC++

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 84713a3 and pushed to 8.0.x. Thanks!

  • alexpott committed 84713a3 on 8.0.x
    Issue #2511024 by lahoosascoots, jibran, kattekrab: Can't add multiple...
kattekrab’s picture

\o/

Thanks @alexpott @jibran @willzyx @lahoosascoots :-)

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.