Add "edit shortcuts" contextual link on the shortcut block

We need a quick patch for this. It should behave just like the current edit shortcuts link on the toolbar display of shortcuts.

Quick patch (basically a copy of the way the configure link is added to the toolbar).

The link works for me. Do we need a second opinion here? I'm +1 for RTBC.

#2: 717812-shortcut-block-configure.patch queued for re-testing.

Assuming the patch still applies/passes... we've probably waited long enough for a second opinion?

#2: 717812-shortcut-block-configure.patch queued for re-testing.

I'm not sure I agree with this. The navigation block doesn't have an "edit navigation" link just sitting around in it at the bottom, yet this is what this patch looks like:

Ick! ;)

What would be cool though is sticking the edit link in the contextual links like menu does.

(And though this adjusts something in t(), it doesn't add a new string so doesn't impact string freeze.)

Do we have to rehash this again?

The problem is that there is no other link to edit shortcuts, anywhere in the UI (or at least, there wasn't when I filed this issue), if you are using the block and not the Toolbar module. There has to be a link somewhere in the UI to get to the edit page. Doesn't there?

The Edit Shortcuts link is not contextual in the Toolbar either.

Sigh.

I'm not sure why the observation that this breaks UX freeze warrants a "sigh". The toolbar is not a block, so sticking a link in the middle there is fine. But for blocks, the pattern we employ elsewhere for "edit this" is contextual links, not sticking an edit link in the block body.

However, it appears that there is a link to edit it now under admin/config/user-interface/shortcut/shortcut-set-1.

So is this "won't fix"?

OK.

Yeah... of the options contextual links is the only one that makes sense to me. Using a "More" link (while definitely nicer on the eyes) again breaks the UX because every other place we use it it's a list of those things, not an admin panel.

I thought there was a UI in the user account editing space that gave you a link to edit your shortcuts too? I worked on a patch a while back that was supposed to do that... If that link exists, then this can actually be set to "won't fix".

The problem I see with this version of the patch is that
a) Not all themes display contextual links (Garland, for example, doesn't, at least last time I checked, which admittedly was months ago)
b) Not everyone has permission to see contextual links

The "edit shortcuts" link that is on the Toolbar display of shortcuts does not suffer from this limitation.

I am not sure, if its a really valid usecase - because it exists for many other modules that now have functionality exposed through contextual links.

When I filed the original issue on this, there was no way to get to the Edit Shortcuts page if you didn't use the toolbar (no contextual links existed). Has that changed? If not, by all means add the link using contextual links, but there should be a link somewhere, right?

Oh, sorry my post was probably somewhat confusing - my argument was that supporting a usecase where there is neither access to the toolbar or contextual links, seemed a bit weird.

#14: 717812-14-shortcut-block-configure.patch queued for re-testing.

Working on this in the first timers contribution workshop.

And now.. The patch

Missing tests...

Patch and tests generally look good, thanks!

Marking needs work for the following points and also marking Novice to fix the review.

1. +++ b/core/modules/shortcut/shortcut.module
   @@ -11,6 +11,7 @@
   +use Drupal\shortcut\Plugin\Block\ShortcutsBlock;
   
   @@ -443,3 +444,19 @@ function shortcut_themes_installed($theme_list) {
   +function shortcut_block_view_shortcuts_alter(array &$build, ShortcutsBlock $block) {
   

   The type-hint should be BlockPluginInterface.

2. +++ b/core/modules/shortcut/shortcut.module
   @@ -443,3 +444,19 @@ function shortcut_themes_installed($theme_list) {
   +    $shortcut_set = shortcut_default_set();
   

   I think we should use shortcut_current_displayed_set() instead of shortcut_default_set() to match what the block is doing.

3. +++ b/core/modules/shortcut/shortcut.module
   @@ -443,3 +444,19 @@ function shortcut_themes_installed($theme_list) {
   +    $shorcut_id = $shortcut_set->id();
   ...
   +        'route_parameters' => ['shortcut_set' => $shorcut_id],
   

   There's a typo in the variable name: "shorcut" instead of "shortcut"

4. +++ b/core/modules/shortcut/shortcut.module
   @@ -443,3 +444,19 @@ function shortcut_themes_installed($theme_list) {
   +    if(!empty($shorcut_id)) {
   

   I don't think we need this check, the shortcut set will always have an ID.

5. +++ b/core/modules/shortcut/shortcut.module
   @@ -443,3 +444,19 @@ function shortcut_themes_installed($theme_list) {
   +}
   \ No newline at end of file
   

   Let's add a newline at the end.

6. +++ b/core/modules/shortcut/tests/src/Functional/ShortcutLinksTest.php
   @@ -438,6 +438,24 @@ public function testShortcutBlockAccess() {
   +    // Creates a block instance and place in a region through api.
   

   I don't think this comment adds much value, let's just remove it.

Ok, i've made some modification.

Wow @Cybercraft2003, I think you have nominated yourself for the coveted Fastest-Patch-of-the-Day award. ;-) Thanks a lot!

I found only some minor issue, but I hope it's OK to kick it back to you one more time. I promise it will be the last round ;-)

1. +++ b/core/modules/shortcut/shortcut.module
   @@ -443,3 +444,20 @@ function shortcut_themes_installed($theme_list) {
   + *
   + * @param array $build
   + * @param BlockPluginInterface $block
   

   Let's remove the parameter documentation. Our documentation standard does not allow it unfortunately. (Yes, it looks nicer in PhpStorm that way, but we cannot have nice things ;-))

2. +++ b/core/modules/shortcut/shortcut.module
   @@ -443,3 +444,20 @@ function shortcut_themes_installed($theme_list) {
   +    $shortcut_id = $shortcut_set->id();
   ...
   +      'route_parameters' => ['shortcut_set' => $shortcut_id],
   

   Instead of putting $shortcut_id into a separate variable let's just use ['shortcut_set' => $shortcut_set->id()] directly.

3. +++ b/core/modules/shortcut/tests/src/Functional/ShortcutLinksTest.php
   @@ -438,6 +438,25 @@ public function testShortcutBlockAccess() {
   +   * Check if the granted user access to contextual "Edit shortcut" link
   

   This should be "Checks" instead of "Check" (i.e. with a trailing "s".

4. +++ b/core/modules/shortcut/tests/src/Functional/ShortcutLinksTest.php
   @@ -438,6 +438,25 @@ public function testShortcutBlockAccess() {
   +    // Verify that anonymous users don't see the
   +    // shortcut edit link.
   

   "don't" should be "do not". Also this should not break into a new line. It should only break if it is longer than 80 characters.

The last from the car ;)

1. +++ b/core/modules/shortcut/shortcut.module
   @@ -443,3 +444,20 @@ function shortcut_themes_installed($theme_list) {
   +    $user = \Drupal::currentUser();
   ...
   +    if (in_array('authenticated', $user->getRoles())) {
   

   better to call `\Drupal::currentUser()->isAuthenticated()`

2. +++ b/core/modules/shortcut/shortcut.module
   @@ -443,3 +444,20 @@ function shortcut_themes_installed($theme_list) {
   +    var_dump($user->getRoles());
   

   should be removed

Oups... I will remove the var dump...

Ok, this my final patch (i hope).

The test condtion is more relevant with "hasPermission" method.
I remove the unnecessary test, fix code commentary, etc...

The previous patch has not the yml file... I've fixed that too.

EDIT : My patch created with phpstorm seems to not working... I will reupload new patch file. Sorry

It's not possible to test this way contextual links. It should be requested from contextual.render.
We need to test it, like in Contextual module, I guess. Something like:

    $account = $this->drupalCreateUser(['access shortcuts', 'customize shortcut links', 'access contextual links']);
    $this->drupalLogin($account);
    $this->drupalGet('');
    $ids[] = 'block:block=shortcuts:langcode=en|shortcuts:shortcut_set=default:langcode=en';
    $response = $this->renderContextualLinks($ids, '');
    $this->assertSame(200, $response->getStatusCode());
    $json = Json::decode((string) $response->getBody());
    $this->assertContains('Edit shortcuts', $json[$ids[0]]);

Don't know, what to do with renderContextualLinks. Should we copy the function or re-use from Contextual module tests?

Yep, the patch seems to work as designed but test crash the rebuild because it's not relevant.

I will try new test logic... It's my first contribution and I don't know how to drive unit test with the ddev container based quick sprint environment. I will try to execute phpunit test on my container based environment. Or if you know how to do that?

I will try with this logic

Reroll #56 patch for 8.7.x

Updating patch #58

Tagging for upcoming contribution days.

Updated the route name in shortcut.links.contextual.yml to use 'entity.shortcut_set.customize_form'. Feel like it makes more sense to go to the list link page instead of the edit set name.

Updated patch to remove extra space causing issue in test.

Looks like the test is still failing, based on @LeStat comment above in #51 it might be a good idea to try re-using the contextual module tests instead of reinventing the wheel.

Queued patch for retesting PHP 7.1 & MySQL 5.7

I am working on recreating this patch for 8.8.x. thanks

I tested manually the functionality and it works.
However the patch was not working because it needed to be migrated to WebDriverTest since it relies on JavaScript actions. Also I moved the test inside the the shortcuts module.

Attaching a new patch which is addressing the coding style issues reported by the test bot.

tagging for DrupalNorth 2019

Tested missing-shortcuts-link-717812-77.patch from #77 on local 8.8.x-dev and it worked fine. Added a shortcut link and saved, all good. At #D4DBoston

Based on #79

+++ b/core/modules/shortcut/shortcut.module
@@ -443,3 +444,19 @@ function shortcut_themes_installed($theme_list) {
+    if ($user->hasPermission('customize shortcut links')) {

should we be adding #cache context here to indicate this varies by access now?

#83: Thanks for calling that out!

+++ b/core/modules/shortcut/shortcut.module
@@ -443,3 +444,19 @@ function shortcut_themes_installed($theme_list) {
+    if ($user->hasPermission('customize shortcut links')) {
+      $shortcut_set = shortcut_current_displayed_set();

Woah… 😨

Even varying by user.permissions is not enough — shortcut_current_displayed_set() can even vary per user (user cache context)!

This again goes back to #2339903: Shortcut module's access checking is insane, #2083123: Shortcut cleanup: Remove duplicated code and deprecate legacy functions and more. That's why this particular contextual links item needs this pretty crazy work-around.

IMHO we should stop building more on top of the excessive complexity of the Shortcut module and first simplify it, first make it sane. Only then can we truly make the UX better.

currently i am working on it .....

Tested missing-shortcuts-link-717812-77.patch from #77 on local 8.9.x-dev and it worked fine. Added a shortcut link and saved, all good. Please find the attached screenshot ....

Thank you for the screenshot, @Rithesh BK. It usually helps to also add this to the issue summary. At. this point, I think that the issue is still at work based on @larowlan's question and @Wim Leer's comment. We should leave the issue at Needs work.

I added the Needs issue summary update and removed the Novice tag because I'm not sure what the next step is based on that review. Should the issue be postponed on #2083123: Shortcut cleanup: Remove duplicated code and deprecate legacy functions?

rerolled for 9.5 but have not addressed #83 and #84. And based on #84 believe this should be postponed until https://www.drupal.org/project/drupal/issues/2083123 is committed

Triaged for bugsmash, this feels like a feature request to me