There are two ways to reproduce this bug

1. Go to user/1/shortcuts, choose new set and hit the "change set" button without giving any input in text field. This will create new shortcut set without any label.
2. Go to add shortcut admin page admin/config/user-interface/shortcut/add-set, hit "create new set" button without giving any input in text field. This will create new shortcut set without any label.

I guess the expected behavior is to report form error, attached patch will fix this issue by adding title and required attribute to form array.

CommentFileSizeAuthor
#13 800366_shortcut_set_name_13.patch7.13 KBDavid_Rothstein
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 800366_shortcut_set_name_13.patch. View
#10 800366_shortcut_set_name_10.patch3.22 KBSivaji
PASSED: [[SimpleTest]]: [MySQL] 20,429 pass(es). View
#8 800366_shortcut_set_name_8.patch1.51 KBSivaji
PASSED: [[SimpleTest]]: [MySQL] 20,421 pass(es). View
#5 800366_shortcut_set_name_5.patch1.2 KBSivaji
PASSED: [[SimpleTest]]: [MySQL] 20,361 pass(es). View
shortcut_empty_label.patch1.09 KBSivaji
FAILED: [[SimpleTest]]: [MySQL] 20,339 pass(es), 1 fail(s), and 0 exception(es). View
shortcut_set_admin.png23.01 KBSivaji
shortcut_set_user_1.png21 KBSivaji
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

Sivaji’s picture

Status: Active » Needs review

Possible related issue is #647084: Missing shortcut edit and delete operations on shortcuts admin, i didn't had time to go through the discussion when creating this issue.

aspilicious’s picture

Nice catch and good patch.

Status: Needs review » Needs work

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

David_Rothstein’s picture

Looks like the test failure is because on the user account page, the textfield is being made always required, even when the corresponding radio button is not selected. To work correctly, it seems like it's unfortunately going to require a custom validation handler (which would check which radio button was chosen before throwing the error).

Why are you adding '#title' => t('Label') to the form? Doesn't seem related to this issue...? This might be required for screen-readers (I'm not 100% sure it is in this particular case, due to the order of the HTML), but for non-screenreaders the "New set" already serves as the joint label. So, if we need to add this at all, I think it should be with #title_display set to 'invisible' (see http://api.drupal.org/api/function/theme_form_element/7). It should also probably say 'Set name' rather than 'Label' to be consistent with the terminology used elsewhere.

Sivaji’s picture

Status: Needs work » Needs review
FileSize
1.2 KB
PASSED: [[SimpleTest]]: [MySQL] 20,361 pass(es). View

Attached patch adds custom check to validation handler.

Why are you adding '#title' => t('Label') to the form?

To prevent FormAPI throwing error like '' field is required. with custom check #title and #required properties is needless.

aspilicious’s picture

Hmmm wouldn't it be nice if we had tests for this?

Cibes’s picture

Patch does the job nicely.

But it is possible to create shortcut sets with the name of an already existing set. This makes working with those sets rather difficult. Can somebody also add a routine to check for existing names (or should I file a new issue - but this seems "almost related")?

Creating a new shortcut set in user/1/shortcuts is not so obvious I think. I was actually wondering what this textbox is supposed to do. Maybe the textbox could be put directly after the radiobutton and instead have "New set" as a default value? Again this is probably the wrong place to mention this ...

Sivaji’s picture

Title: Prevent creating shortcut set with empty label » Prevent creating shortcut set with empty or duplicate label
FileSize
1.51 KB
PASSED: [[SimpleTest]]: [MySQL] 20,421 pass(es). View

Attached patch add duplicate label check to the original patch.

Cibes’s picture

Status: Needs review » Needs work

Thanks for including the duplicates in the issue.

Patch works fine if you try to create a duplicate under user/1/shortcuts but you can still create duplicates as an admin in admin/config/user-interface/shortcut/add-set - looks like it is not checked on that form at all.

Sivaji’s picture

Status: Needs work » Needs review
FileSize
3.22 KB
PASSED: [[SimpleTest]]: [MySQL] 20,429 pass(es). View

Nice that you noticed this. I had same thought today morning, I even wrote a patch to fix this.

asrob’s picture

Status: Needs review » Reviewed & tested by the community

Relates to sivaji's description I reproduced this bug and works for me.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs work

Oops, I had really meant to review this earlier :)

  1. This seems to mostly work well; however, it introduces a bug where editing an existing shortcut set does not allow you to submit the form without also renaming the set (it checks the name against itself).
  2. There are some minor code style issues that need to be fixed (spelling errors, code comments not starting with a capital letter, missing @see blocks for the new validate functions)...
  3. The shortcut_set_name_is_exists() function has a grammatical error in the function name (should get rid of the "is") and as an API function, belongs in the shortcut.module file rather than in the shortcut.admin.inc file.
  4. We might consider making the wording of the error messages consistent with other places in Drupal that check for uniqueness (e.g., filter module, menu module), at least if it's the case that those are already consistent with each other?
  5. As aspilicious suggests, it would be nice to have tests for this (although I don't think it's a critical thing to write a test for).

But the overall approach of the patch definitely looks solid to me. I'll see if I can reroll this a bit later with those fixes so we can get this in. Thanks!

David_Rothstein’s picture

Status: Needs work » Needs review
FileSize
7.13 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 800366_shortcut_set_name_13.patch. View

Revised patch with the above issues fixed, plus some tests.

I also changed the internals of the API function to use the db_query_range() method for determining if a database record exists, since that is the recommended method used throughout core (although I'm noticing that in this case the database column does not have an index...)

Look OK?

asrob’s picture

Status: Needs review » Reviewed & tested by the community

It is OK. I reproduced again.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Reviewed this patch, and ran the tests locally. Code looks good and everything works as expected. Committed to CVS HEAD. Thanks!

Status: Fixed » Closed (fixed)
Issue tags: -Quick fix, -Novice

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