While removing a user from a shortcut set in a module I was making, I received an error about column UID not being found. Looking in the shortcut.module I saw that shortcut_set_unassign_user was using the wrong table to remove the user from the shortcut set.

Attached patch correctly unassigns a user from the shortcut_set.

This is the first time that I'm submitting a patch to drupal core so forgive my ignorance if I'm doing this wrong.

The patch is made against drupal 7.4

CommentFileSizeAuthor
#11 shortcut_unassign-1228888-4778138.patch1.39 KBJohnny vd Laar
#3 shortcut_unassign-1228888-4777606.patch512 bytesJohnny vd Laar
shortcut_unassign_bug.patch484 bytesJohnny vd Laar

Comments

Status: Needs review » Needs work

The last submitted patch, shortcut_unassign_bug.patch, failed testing.

Johnny vd Laar’s picture

Johnny vd Laar commented

ok guess it was meant to fail at first try. going to try to create a new patch hold on ;-)

Johnny vd Laar’s picture

Johnny vd Laar commented
Version: 7.4 » 7.x-dev
StatusFileSize
new shortcut_unassign-1228888-4777606.patch512 bytes

ok second attempt made this to latest 7.x-dev version from git and created the patch with git instead of svn

marcingy’s picture

marcingy commented
Version: 7.x-dev » 8.x-dev
Issue tags: +Needs tests, +Needs backport to D7

This needs a test. And also moving to d8.

marcingy’s picture

marcingy commented
Status: Needs work » Needs review

Setting to needs review for the bot.

Johnny vd Laar’s picture

Johnny vd Laar commented

ah thank you I already wondered why it didn't test anymore

Johnny vd Laar’s picture

Johnny vd Laar commented

Yay, so is there anything I'm supposed to do now?

marcingy’s picture

marcingy commented
Status: Needs review » Needs work

We need a simpletest to test the behaviour is correct when shortcuts are deleted.

Johnny vd Laar’s picture

Johnny vd Laar commented

Well the function is never used by drupal core itself:
http://api.drupal.org/api/drupal/modules--shortcut--shortcut.module/func...

So will it be sufficient to add this?


  /**
   * Tests unassigning a shortcut set.
   */
  function testShortcutSetUnassign() {
    $new_set = $this->generateShortcutSet($this->randomName(10));

    shortcut_set_assign_user($new_set, $this->shortcut_user);
    shortcut_set_unassign_user($this->shortcut_user);
    $current_set = shortcut_current_displayed_set($this->shortcut_user);
    $default_set = shortcut_default_set($this->shortcut_user);
    $this->assertTrue($current_set->set_name == $default_set->set_name, "Successfully unassigned another user's shortcut set.");
  }
marcingy’s picture

marcingy commented

From a quick scan that look like it does enough - if you roll a combined patch I'll review it later :)

Johnny vd Laar’s picture

Johnny vd Laar commented
Status: Needs work » Needs review
StatusFileSize
new shortcut_unassign-1228888-4778138.patch1.39 KB

ok great, attached a new patch that also includes the test case

Bojhan’s picture

Bojhan commented

Subscribe, pretty stupid heh.

marcingy’s picture

marcingy commented
Status: Needs review » Reviewed & tested by the community

Looks good.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed to 7.x and 8.x. Thanks!

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