Should not be able to re-add same shortcut to same set twice
webchick - October 20, 2009 - 03:13
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | shortcut.module |
| Category: | bug report |
| Priority: | critical |
| Assigned: | bleen18 |
| Status: | closed |
Description
If I click "Dashboard" in the shortcut menu, I get the link to add the Dashboard page to my current shortcut set. And if I click it, I will end up with two of them:
This should be fixed. :)
#1
Incidentally, IMO the way this should be fixed is for that add button to change to a delete button if I'm viewing a page that's already in my currently active shortcut set.
#2
subscribe
#3
Been working on webchick's suggestion and I'm about 75% there ... stay tuned
#4
Ok ... so I took webchick's suggestion and checked if the current page is already a shortcut in the current shortcut set. If it is, a "remove shortcut" link is displayed instead of an "add shortcut" link.
When reviewing this patch, please note that I had to change
/modules/toolbar/toolbar.pngin order to add a "minus" button to compliment the "plus" button. That PNG is also attachedPlease review and let me know
#5
The last submitted patch failed testing.
#6
resubmitting the same patch because it passed here without my changing anything: http://drupal.org/node/612208
Bad testbot. BAD!
Please don't forget that I had to change /modules/toolbar/toolbar.png in order to add a "minus" button to compliment the "plus" button. I'm attaching that PNG again
#7
The last submitted patch failed testing.
#8
So this patch passed locally and passed here: http://drupal.org/node/612208
Chatter on IRC (boombatower & ilo---) suggests that testbot is very unhappy right now. So, I'm not sure exactly what to do now ... I guess, please review this patch. Would love for it to get into D7
#9
yay testbot ... finally passed
#10
Functionality-wise, this looks great, and is exactly what I had in mind. Screenshots:
Unbookmarked page:
Unbookmarked page (hover over add icon):
Bookmarked page:
Bookmarked page (hover over remove icon):
However, if I go to edit shortcuts, and create a new toolbar set and start playing with it, I start getting errors, like:
# Notice: Undefined property: stdClass::$links in shortcut_page_build() (line 535 of /Users/webchick/Sites/core/modules/shortcut/shortcut.module).
# Warning: Invalid argument supplied for foreach() in shortcut_page_build() (line 535 of /Users/webchick/Sites/core/modules/shortcut/shortcut.module).
These errors no longer occur when I revert the patch, so this still needs some work.
Implementation-wise, hard-coding the visual styling of the shortcut bar in toolbar.png seems a bit odd. First, because shortcut is its own module, but secondly, I'm sure these icons look assy in something like Garland. However, this isn't really for this patch to solve, since you're just hinging off the existing implementation. Would be good to have a follow-up issue to discuss this though. It seems like shortcut module ought to only offer text links and for themes to style these however they'd like.
The shortcut_link_remove_inline() function makes me a bit nervous. Normally, we would stick confirm forms around any kind of deletion actions, so some "clever" dipstick on your website can't make an image in their comment pointing to admin/config/system/shortcut/1/remove-link-inline and start tricking an admin into deleting their links by looking at it (CSRF vulnerability). It's unclear to me whether all that various token-checking stuff solves this issue. Is this copy/pasted from the delete link on the, or..?
I need to spend some more time studying the changes to shortcut_page_build(), which I don't really have time to do now. It'd be great instead to pull in Jacob Singh or David Rothstein or one of the others who worked on the initial patch to get their thoughts. But as a general statement, watch your coding standards compliance... comments should end in a period and begin with a capital letter, there should be spaces around function calls, else goes on a separate line, etc.
#11
Also, I ran into #613662: Shortcut item mis-alignment while testing this patch. I'm not sure if it's caused by this patch or not, so I made it a separate issue. Could you check?
#12
I'll start with this and then take a look at addressing your thoughts about
shortcut_link_remove_inline:There is already a set of confirmations in place for when you use the edit shortcuts links
I agree with your comment about the images living in toolbar.png but I figured I'd go with the flow on that one for now.
re: coding standards... I'm still trying to break some old habits :) I'm getting there - slowly
#13
This patch fixes the errors that webchick was seeing when she added another shortcut set. This patch also takes care of the misalignment of the "+" links (#613662: Shortcut item mis-alignment). I'm still thinking about the best way to tackle the suggestion of utilizing confirmation form(s) when deleting a shortcut.
#14
Waaaay easier than I thought to utilize the confirmation forms for deleting a shortcut. This patch should be real close... please to review, comment, etc..
#15
I haven't actually tried out the patch, just reviewed the code - but overall this looks pretty good! Here are some things I noticed, though:
tcut_link = $form_state['values']['shortcut_link'];
- menu_link_delete($shortcut_link['mlid']);
+ _shortcut_link_delete($shortcut_link);
$form_state['redirect'] = 'admin/config/system/shortcut/' . $shortcut_link['menu_name'];
- drupal_set_message(t('The shortcut %title has been deleted.', array('%title' => $shortcut_link['link_title'])));
}
/**
+ * Helper function for deleting shortcuts.
+ */
+function _shortcut_link_delete($link){
+ menu_link_delete($link['mlid']);
+ drupal_set_message(t('The shortcut %title has been deleted.', array('%title' => $link['link_title'])));
+}
Breaking this out into a separate function doesn't seem that useful since it's only used once. I think maybe it is a relic from an earlier version of the patch that can now be removed?
-div.add-to-shortcuts a span.icon {+div.add-remove-shortcuts {
I think something like "add-or-remove-shortcut" might be better (here and elsewhere)?
+div.remove-shortcut a span.icon{+ background-position: -62px -60px;
+}
Code style (here and elsewhere): Space before the
{, and an indentation of two spaces (rather than a tab) on the next line.- // Otherwise, use the default set.- if (!$shortcut_set) {
+
+ if ($shortcut_set) {
+ $shortcut_set = shortcut_set_load($shortcut_set->set_name);
+ }
+ else { // Otherwise, use the default set.
$shortcut_set = shortcut_default_set($account);
}
This is a nice little bugfix! - I wonder if in addition to the fact that it's needed here, it might solve some other bugs too. (But note code style issue - the code comment should be on its own line.)
As part of fixing this, it looks to me like we can also modify the database query that runs right above it:
<?php$query = db_select('shortcut_set', 's');
$query->fields('s');
$query->join('shortcut_set_users', 'u', 's.set_name = u.set_name');
$query->condition('u.uid', $account->uid);
$shortcut_set = $query->execute()->fetchObject();
?>
With your fix, it is now pulling more information than we need from the database - since most of that information gets duplicated again in shortcut_set_load(). Actually, the best way to fix this might possibly be to add an optional $account parameter to shortcut_set_load() itself and have that function join against the {shortcut_set_users} table in the case where the $account parameter is provided. This is not necessary for this issue and it could be a followup, but worth trying if you're interesting in tackling the database API a bit.
- if (shortcut_set_edit_access()) {+ if (shortcut_set_edit_access()) {
Here and in a few other places you are adding extra space at the end of a line, which adds a code style violation even while you are not changing the code itself :) There should be no whitespace at the end of a line of code, and blank lines should have no whitespace at all. I'm guessing these came in by accident while you were working on the patch, but it's best to try to remove them because it also makes the patch bigger and a bit harder to review.
+ else { // $mlid is set, so remove the link.+ $query['token'] = drupal_get_token('shortcut-remove-link');
Since we're redirecting to a confirmation form in this case, I don't think we need to add a token (Drupal's form API already provides tokens automatically). We only need it for the "add shortcut" link because in that case it goes to a URL that executes the action directly.
Also, in this part of the code, there seems to be a fair amount of duplication between the "add" and "remove" cases. Consider whether that could be consolidated... although the existing code that was already there is, I must admit, pretty ugly anyway, so maybe not worth doing too much as part of this patch.
RCS file: /cvs/drupal/drupal/modules/toolbar/toolbar.png,vretrieving revision 1.4
diff -u -p -r1.4 toolbar.png
Binary files /tmp/cvsiucope and toolbar.png differ
Index: themes/seven/page.tpl.php
Yeah, as mentioned above, the fact that this is in toolbar.png is pretty messed up - that was an oversight that we missed in the rush to the original patch. We should perhaps make a separate issue for decoupling the images so that shortcut.module has its own normal images for the plus and minus icons - that'll be a prerequisite to fulfill some of the followup cleanup work at #511286: D7UX: Implement customizable shortcuts anyway.
Overall: Despite all the minor nitpicks above, I think this patch is really good. Reusing the confirmation form certainly makes sense since it's already there. In theory, I think this would be a great place in Drupal to not use a confirmation form for deletion but rather to just do the action automatically (which should be safe as long as there is a token in the URL) and then provide an "undo" link - it's a case where an undo link would actually not be that hard to implement. However, that's more than we need to do for this patch, and maybe it's not a great idea to introduce the "undo" pattern here when it's not elsewhere in Drupal (even though overall it is a better pattern). So I think the confirmation form is certainly fine for now.
#16
OK, what I wrote there makes absolutely zero sense - I have no idea what I thought I was trying to suggest. Ignore, please :)
We do, however, want to change this query:
<?php$query = db_select('shortcut_set', 's');
$query->fields('s');
$query->join('shortcut_set_users', 'u', 's.set_name = u.set_name');
$query->condition('u.uid', $account->uid);
$shortcut_set = $query->execute()->fetchObject();
?>
so that it no longer pulls everything from the table but instead just loads the set name -- since that is the only thing that needs to get passed in to shortcut_set_load() as a result of this patch.
#17
@David_Rothstein
Thanks for the feedback... here are a couple of notes before I get cracking on some revisions:
Actually I broke this function out because it was being used in two places in my previous patch and I forgot to remove it
sold!
remnants from my previous patch when deleting happened without a confirm form
I know, I know ... I can't seem to get this right. Are there any good tools out there that just check this for me and then smack me when I get it wrong? Thanks for keeping on top of it though - only way Ill ever learn it
#18
Great, looking forward to the next version!
For code style stuff, you might try the Coder module - I know it checks some of that for you, although I'm not sure exactly how much.
#19
Lets give this patch a whirl ... once we get this patch settled I'll open a new ticket and breakout the shortcut images from toolbar.png
#20
Ok ... it seemed a little silly to open a whole new issue just for the toolbar.png issue, so I have included a fix for that in this patch. Please note the atached shortcut.png and the toolbar.png files which belong in the root of their respective module folders. (this toolbar.png is different than the one I attached above).
The CSS changes have also been made to accommodate...
Functionally though, this patch is identical to "remove_shortcut-609152_4.patch"
#21
bump ... would love to get this into d7 before the doors shut
#22
No worries, the doors never shut on critical bugs. This one is going to get solved one way or the other :)
I just took a quick look at the patch. The new images look nice! (Although when I copy them into my installation I'm not seeing the shortcut +/- icons for some reason.)
Anyway, only minor issues with the code that I can see right now:
t($link_pretext.' shortcuts')This kind of thing doesn't work well for translators - as I understand it, it makes it difficult to correctly translate the text. I think you want to fully define all the translatable string separately - for example,
t('Add to shortcuts')andt('Remove from shortcuts')- even if that means a tiny bit of code duplication between the "add" and "remove" sections.Also, there are still some spacing issues in the CSS file (each style declaration should have two spaces before it - looks like you have some tabs and such in the patch) and there are blank lines introduced by this patch that consist of spaces or tabs, whereas it really should be just a blank line with nothing on it.
Those are overall pretty minor. I'd want to look at this one more time more carefully, but it looks pretty close to me. Thanks!
#23
OK... I feel really good about this one :)
#24
bump :)
#25
The last submitted patch failed testing.
#26
gonna try and retest this. I dont understand why the patch changed to "fail" 5 days later
#27
The last submitted patch failed testing.
#28
Rerolled to current head and manually tested, behaves as expected aside from one weirdness: when I click "remove shortcut..." it brings me to a confirmation page, but at this confirmation page I seem to be able to add a shortcut (that would theoretically point to this confirmation page), but when doing so, nothing changes even though I get the following notice:
Added a shortcut for Are you sure you want to delete the shortcut <em>Dashboard</em>?.so maybe we should define patterns for pages that should never be added as a shortcut
this patch assumes that shortcut.png and toolbar.png was replaced by the ones in #20
#29
Thanks for rerolling
#30
Looks pretty good to me. In the attached, I rerolled the patch to fix a few remaining whitespace issues and also to update the database query to only fetch the one column we need (as discussed above).
I'd say it's RTBC now except for one thing... I noticed that there is still code in themes/seven/style.css that uses the old CSS ID:
<?php/* Shortcut theming */
div.add-to-shortcuts {
float: left;
margin-left: 6px;
margin-top: 6px;
}
?>
It seems that probably just needs to be updated to use add-or-remove-shortcuts instead, but then I wasn't sure because I also noticed there's some other minor theming differences here. Compare the two attached screenshots. The (minor) differences in the shortcut bar and in the placement of the + and - icons actually might look better with the patch as is (meaning maybe we just want to delete the above stuff from the Seven theme altogether), but it also seems that the CSS changes here caused the tabs ("List" and "Configure") to dip down a bit too low so that the words there are almost getting cut off? So overall, this needs one more pass through the CSS to make sure it is correctly doing what it wants to be doing :)
After that, I think this is RTBC. The issue discussed in #28 is probably a tricky one to solve (and also not introduced by this patch - the confirmation form was there before and had the same problem then).
#31
ups, did I forget seven's style.css? :x
updated that and changed it to padding so it'll override shortcut.css's padding-top (btw, should we move seven's in there or stick with the minimalistic padding-top?)
and I added the padding values that got lost on div#toolbar div.toolbar-shortcuts ul li a
- padding: 5px 10px 5px 5px;+ padding: 5px 10px;
this makes before & after look the same
#32
Ok ... I just double checked the last patch that seutje provided everything looks good. The old CSS that David_Rothstein mentioned in #30 isn't there and the shortcut '+' & '-' line up correctly.
I think we're good to go
#33
Agreed.
The patch in #31 combined with modules/shortcut/shortcut.png and modules/toolbar/toolbar.png in #20 are ready to go from my perspective. Nice work!
#34
Ahhhh. Now that is much better! :D
Great work, folks! Committed to HEAD.
#35
woo hooo... my first real patch :)
#36
Oh, yay! Congratulations, bleen!
Now, on a completely unrelated note, I must go get some jello and feathers... *whistles innocently*
#37
Automatically closed -- issue fixed for 2 weeks with no activity.