This is a module that creates a simple navigation menu for all the users. It creates a new menu called "User Navigation Menu" where in the admin can add or remove the links to be shown.
Project page:
https://drupal.org/sandbox/mudrasamey/2211983
Git link:
git clone --branch 7.x-1.x http://git.drupal.org/sandbox/mudrasamey/2211983.git user_navigation
Manual reviews of other projects:
https://drupal.org/node/2275959#comment-8839801
https://drupal.org/node/2272503#comment-8842081
https://drupal.org/node/2278611#comment-8839753
https://drupal.org/node/2378895#comment-9360627
https://drupal.org/node/2381597#comment-9380271
https://drupal.org/node/2373551#comment-9380375
https://drupal.org/node/2391091#comment-9434003
Comment | File | Size | Author |
---|---|---|---|
#34 | navigation_menu_issue.png | 46.48 KB | er.pushpinderrana |
#27 | user-navigation.png | 12.89 KB | gaurav.pahuja |
Comments
Comment #1
PA robot CreditAttribution: PA robot commentedWe are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)
Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).
I'm a robot and this is an automated message from Project Applications Scraper.
Comment #2
joachim CreditAttribution: joachim commentedUsing the base URL means you're saving all these links as external URLs. That's just weird.
I suspect that all caches get cleared after hook_install() / hook_uninstall() -- but check.
Don't include the version -- it's added automatically by d.org packaging.
The description should be a bit clearer, eg, 'Provides a ... '
Not sure you need a package for a single module -- unless this is a package name already in use by other modules?
This doesn't add any thing! Also, it's redundant to say 'Permission for...'.
Don't include the CSS and JS if you're not going to show the menu!
Also, this seems rather heavy-handed. Shouldn't this be better done as a block that the admin can place themselves?
What's this for? Needs a comment perhaps.
Comment #3
ameymudras CreditAttribution: ameymudras commentedHi Joachim,
Thanks for reviewing my module.
I did implement the changes as suggested by you.
I did not make it a block because, i wanted it to be added at the beginning of the page. The way admin menu adds its menu.This is a similar module which to admin menu but has a different user interface.
Comment #4
ameymudras CreditAttribution: ameymudras commentedComment #5
ameymudras CreditAttribution: ameymudras commentedComment #6
ameymudras CreditAttribution: ameymudras commentedComment #7
ameymudras CreditAttribution: ameymudras commentedComment #8
Smartling CreditAttribution: Smartling commentedHey ameymudras,
1. It's highly recommended to use:
Drupal.behaviors
instead of:
Additional info you can reach here Managing JavaScript in Drupal.
2. Another thing is that you are using HTML directly:
and that does not provide users to manage theming customly. It's much better to use hook_theme and register on templates to provide this capability to user.
Thank you.
Comment #9
Anthony Goode CreditAttribution: Anthony Goode commentedHi ameymudras,
I installed your module in a drupal 7.28 clean installation with no theme or other module but User Navigation, and this message error was shown:
Notice: Trying to get property of non-object in user_nav_build_html() (line 45 of /var/www/sandboxtests/sites/all/modules/user_nav/user_nav.module).
Otherwise apart from the suggestions of the previous comments, I like the idea of the module because I think that it is easy for a themer to add more menu items, the corresponding icon images and style them in the CSS.
Comment #10
PA robot CreditAttribution: PA robot commentedThere are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxmudrasamey2211983git
I'm a robot and this is an automated message from Project Applications Scraper.
Comment #11
ameymudras CreditAttribution: ameymudras commentedHi Smartling & Anthony,
Firstly thanks for reviewing our module.
@Smartling As suggested by you we have updated our module to use
Drupal.behaviors
. Regarding hook_theme we have also incorporated it.@Anthony its good to hear that you liked the concept of this module.We have fixed the notice issue that you used to get when you installed this module
Comment #12
ameymudras CreditAttribution: ameymudras commentedComment #13
Nitesh Pawar CreditAttribution: Nitesh Pawar commentedComment #14
gwprod CreditAttribution: gwprod commentedDoesn't pass automated testing:
FILE: /var/www/drupal-7-pareview/pareview_temp/user_nav.tpl.php
--------------------------------------------------------------------------------
FOUND 4 ERRORS AFFECTING 4 LINES
--------------------------------------------------------------------------------
5 | ERROR | [x] Whitespace found at end of line
11 | ERROR | [x] Whitespace found at end of line
28 | ERROR | [ ] The control statement should use the ":" alternative syntax
| | instead of curly braces in template files
42 | ERROR | [ ] Files must end in a single new line character
--------------------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------
FILE: /var/www/drupal-7-pareview/pareview_temp/user_nav.module
--------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
--------------------------------------------------------------------------------
73 | WARNING | A comma should follow the last multiline array item. Found: )
--------------------------------------------------------------------------------
in user_nav.module:
on line 38, can
be changed to use theme_user_picture?
On line 46:
'alt' => t("Profile pic"),
should be
'alt' => t('Profile pic'),
In user_nav.js, can this be adapted to attach to a jquery object using context?
Comment #15
ameymudras CreditAttribution: ameymudras commentedHello gwprod
Thanks for reviewing my module. I have fixed the issues as mentioned by you
Comment #16
willietse CreditAttribution: willietse commented1, There is no dependencies in the info file, if the menu is disabled, The error is "Fatal error: Call to undefined function menu_save() ..."
2, In the hook_init(), Not all pages need to load user_nav.js, user_nav.css, i thinks need to control is displayed in what page.
3, line 39, the img tags, user theme('image').
4, In the user_nav.js, month string you can user like Drupal.t('January'), We can translate it.
5, The code is too short, add some functionality, settings, or code comments?
Comment #17
willietse CreditAttribution: willietse commentedComment #18
ameymudras CreditAttribution: ameymudras commentedhello willietse,
Thanks for reviewing the module. Will fix the issues pointed out by you asap
Comment #19
Nitesh Pawar CreditAttribution: Nitesh Pawar commentedHello Willietse,
Thanks for reviewing our module. I have fixed the issues as mentioned by you.
Comment #20
Nitesh Pawar CreditAttribution: Nitesh Pawar commentedComment #21
Sagar Ramgade CreditAttribution: Sagar Ramgade commentedHi,
Please find my reviews :
a. Do not use hook_init() for adding css and js file, use #attached in hook_page_build instead.
b. Use t() function for "Search" text in hook_form_alter.
c. Do not use base_url for referencing images from your module folder (line no 48), use file_create_url.
Comment #22
Nitesh Pawar CreditAttribution: Nitesh Pawar commentedHey Saggy,
Thanks for your valuable review. I have fixed all the issues mention by you.
Comment #23
irfworld CreditAttribution: irfworld commentedHi Ameymudras and Nitesh,
Thanks for your contributions to this community.
Automatic Review
Found following issue:
Review of the 7.x-1.x branch (commit 6574d12):
So kindly remove the unnecessary things (title, description) from $menu array, As they would not be always the same as provided, they can be modified in "edit menu" section.
All the other things seem to be fine to me. But for better file structure you can put your ".js, .css" in their appropriate folders.
All the best!!!
Comment #24
irfworld CreditAttribution: irfworld commentedHi Ameymudras and Nitesh,
Thanks for your contributions to this community.
Automatic Review
Found following issue:
Review of the 7.x-1.x branch (commit 6574d12):
Manual Review:
In the install file, The menu name "user_nav_menu" is illegal, as you can only use "lowercase letters, numbers, and hyphens" but under scores are found in the name, which is creating issue while i am trying to modify the menu description (Please refer the attached screenshot). I will suggest you to modify the name to "user-nav-menu".
One more thing, only the menu_name in $menu is enough for menu_delete() function in your .install file.
So kindly remove the unnecessary things (title, description) from $menu array, As they would not be always the same as provided, they can be modified in "edit menu" section.
All the other things seem to be fine to me. But for better file structure you can put your ".js, .css" in their appropriate folders.
All the best!!!
Comment #25
Nitesh Pawar CreditAttribution: Nitesh Pawar commentedHi Irfworld,
Thanks for your valuable review and suggestions. I have done all fixes mention by you.
Comment #26
Nitesh Pawar CreditAttribution: Nitesh Pawar commentedComment #27
gaurav.pahuja CreditAttribution: gaurav.pahuja commentedAny specific reason that you used 'Plugins' instead of 'Modules' in Menu.Plugin is not Drupal specific term.
Comment #28
samir_mankar CreditAttribution: samir_mankar commentedHi Ameymudras and Nitesh,
Thanks for your contributions to this community.
Automatic Review
Found following issue:
Review of the 7.x-1.x branch (commit 6574d12):
Coder Sniffer has found a very minor issue with your code.
Manual Review
This is just a suggestion, as most of the modules settings are in configuration , it would be better if the Settings menu link points to config instead of structure.
i have install the module and tested it locally. It looks very promising.
Comment #29
Nitesh Pawar CreditAttribution: Nitesh Pawar commentedHi gaurav and samir,
Thanks to reviewed our module.
@gaurav: There is no specific reason to gave name like 'plugins', but its configurable from menu settings. Anyway I renamed it from Plugins to Modules.
@samir : The @file doc is already present in .js file. It might be issue with coder review module https://www.drupal.org/node/1834598 . I tried to solve it with solution provided in #10 of given link but in pareview.sh it throws some warnings. And thanks for your suggestion I replaced Settings link path from structure to config.
Comment #30
pkamerakodi CreditAttribution: pkamerakodi commentedManual Review
Individual user account
Yes: Follows the guidelines for individual user accounts.
No duplication
Yes: Does not cause module duplication and fragmentation.
Master Branch
Yes: Follows the guidelines for master branch.
Licensing
Yes: Follows the licensing requirements
3rd party code
Yes: Follows the guidelines for 3rd party code.
README.txt/README.md
Yes: Follows the guidelines for in-project documentation and the README Template.
Code long/complex enough for review
Yes: Follows the guidelines for project length and complexity.
Secure code
No. If "no", list security issues identified.
Coding style & Drupal API usage
1) var month = new Array(7); not sure if this is right of passing in 7.
2) I think it is better to attach click events only once rather attached it everytime, since u have included drupal behaviour it will get attached for every ajax that happens in the page. I think you try using drupal.once
3) Below code good be improved
if (empty($user_object->picture->uri)) {
$variables['path'] = $path . '/images/default_profile.png';
$profile_pic = theme('image', $variables);
}
else {
$variables['path'] = $user_object->picture->uri;
$profile_pic = theme('image', $variables);
}
to
if () {
$variables['path'] = $path . '/images/default_profile.png';
$profile_pic = theme('image', $variables);
}
else {
$variables['path'] = $user_object->picture->uri;
}
$variables['path'] = empty($user_object->picture->uri) ? $path . '/images/default_profile.png' : $user_object->picture->uri;
$profile_pic = theme('image', $variables);
4) Not sure what is the purpose of
$links = array(
array(
array(
),
array(
),
));
we could have kept it
$links = array(
array(
),
array(
),
);
5) May be it is better to localize the menu names
$title = t($menu['link']['link_title']);
Prajwal
Comment #31
ameymudras CreditAttribution: ameymudras commentedHello Prajwal,
Thanks for your review. we have fixed all the issues mentioned by you.
Comment #32
ameymudras CreditAttribution: ameymudras commentedComment #33
er.pushpinderrana CreditAttribution: er.pushpinderrana commentedAssigning to myself for next review, which will hopefully be tonight.
Comment #34
er.pushpinderrana CreditAttribution: er.pushpinderrana commentedAutomated Review
(+) Best practice issues identified by pareview.sh / drupalcs / coder. Yes, http://pareview.sh/pareview/httpgitdrupalorgsandboxmudrasamey2211983git reported number of issues that need to be address.
Manual Review
print l($profile_pic, 'user/'.$user_object->uid, array('html' => TRUE));
instead of<a href = 'user/<?php echo $user_object->uid; ?>'><?php echo $profile_pic; ?></a>
.<div>
tag is missing in following code.var dt = "<div class='clock-date'>" + month[now.getMonth()] + " " + (now.getDay() + 1) + ", " + now.getFullYear() + "<div>";
if (user_access("view user nav bar") && !path_is_admin(current_path())) {
Why you have restricted (
!path_is_admin(current_path())
) this navigation for admin pages, need comment? Can't you manage this as a block that help admin to manage visibility of this? Currently if user want to restrict this block for some pages can't be done without touching your code.The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.
Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.
Thanks
Comment #35
Nitesh Pawar CreditAttribution: Nitesh Pawar commentedHi er.pushpinderrana,
As said by you we have fixed all the issues mentioned by you.
Comment #36
ameymudras CreditAttribution: ameymudras commentedComment #37
ameymudras CreditAttribution: ameymudras commentedComment #38
ameymudras CreditAttribution: ameymudras commentedComment #39
Nitesh Pawar CreditAttribution: Nitesh Pawar commentedComment #40
klausiReview of the 7.x-1.x branch (commit 79006a9):
This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.
manual review:
f'><script>alert('XSS');</script>
as link title in the menu I will get a nasty javascript popup. You need to sanitize user provided text before printing it directly into HTML as class. Use drupal_html_class() to sanitize it. Also make sure to read https://www.drupal.org/node/28984 again. And please don't remove the security tag, we keep that for statistics and to show examples of security problems.Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.
Comment #41
ameymudras CreditAttribution: ameymudras commentedHi Klausi,
Thanks for your review,
We have fixed all the issues mentioned by you. We have also updated our project page stating the differences to the nav bar module
Thanks
Comment #42
ameymudras CreditAttribution: ameymudras commentedComment #43
Sagar Ramgade CreditAttribution: Sagar Ramgade commentedHi Amey,
Thank you for your contribution, My two cents. The spaces in the first level menu items are converted to hypens because of drupal_html_class in user-nav.tpl.php and changing the menu items hampers the design. We can create css on the fly using themable function which users can override using tpl.php overrides as per their need. The users should have options to assign images to the menu items (think of integration with menu icons or others).
Comment #44
ameymudras CreditAttribution: ameymudras commentedComment #45
ameymudras CreditAttribution: ameymudras commentedComment #46
ameymudras CreditAttribution: ameymudras commentedComment #47
klausiReview of the 7.x-1.x branch (commit 8c83cfb):
This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.
manual review:
<p>t(Please click on the image to edit your profile)</p>
: this will not work, you will have to use PHP tags around it so that the PHP function t() is executed.Comment #48
Nitesh Pawar CreditAttribution: Nitesh Pawar commentedHi Klausi,
Thanks for your review,
I have fixed all the issues mentioned by you.
Thanks
Comment #49
klausimanual review:
But otherwise looks RTBC to me.
Assigning to dman as he might have time to take a final look at this.
Comment #50
Nitesh Pawar CreditAttribution: Nitesh Pawar commentedHi Klausi,
Thanks for giving RTBC,
I have changed hook_form_alter with hook_form_FORM_ID_alter.
Thanks
Comment #51
klausino objections for more than a week, so ...
Thanks for your contribution, ameymudras!
I updated your account so you can promote this to a full project and also create new projects as either a sandbox or a "full" project.
Here are some recommended readings to help with excellent maintainership:
You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and stay involved!
Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.
Thanks to the dedicated reviewer(s) as well.
Comment #52
ameymudras CreditAttribution: ameymudras commentedThanks a lot Klausi