[D7] User Navigation

We 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.

      array(
        'link_title' => 'Dashboard',
        'link_path' => $base_url . '/admin/dashboard',
        'menu_name' => 'user_nav_menu',
        'weight' => 1,
        'expanded' => 0,
      ),

Using the base URL means you're saving all these links as external URLs. That's just weird.

  cache_clear_all('*', 'cache_menu', TRUE);

I suspect that all caches get cleared after hook_install() / hook_uninstall() -- but check.

description = Enables users to navigate easily
package = User Navigation
version = VERSION

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?

      'description' => t('Permission for those who can see the User Nav bar.'),

This doesn't add any thing! Also, it's redundant to say 'Permission for...'.

function user_nav_page_build(&$page) {
  $path = drupal_get_path('module', 'user_nav');
  drupal_add_js($path . '/user_nav.js');
  drupal_add_css($path . '/user_nav.css');
  if (user_access("view user nav bar") && !path_is_admin(current_path())) {
    $page['page_top']['user_nav'] = array(
      '#weight' => -1000,
      '#markup' => user_nav_build_html(),
    );
  }
}

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?

function user_nav_form_alter(&$form, &$form_state, $form_id) {
  if ($form_id == "search_block_form") {
    $form['search_block_form']['#attributes']['placeholder'] = 'Search';
  }
}

What's this for? Needs a comment perhaps.

Hey ameymudras,

1. It's highly recommended to use:

Drupal.behaviors

instead of:

jQuery(document).ready(function() {

Additional info you can reach here Managing JavaScript in Drupal.

2. Another thing is that you are using HTML directly:

$output = "<div class='user-menu-container'><div class='user-menu-left-container'>";

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.

Hi 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.

There 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.

Doesn'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

if (empty($user_object->picture->uri)) {
      $profile_pic = "<img src='$base_url/$path/images/default_profile.png' height='65' width ='65'>";
    }
    else {
      $profile_pic = theme('image', array(
        'path' => $user_object->picture->uri,
        'width' => 65,
        'height' => 60,
        'alt' => t("Profile pic"),
      ));
    }

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?

1, 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?

Hello Willietse,

Thanks for reviewing our module. I have fixed the issues as mentioned by you.

Hi,

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.

Hey Saggy,

Thanks for your valuable review. I have fixed all the issues mention by you.

Hi 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 some issues with your code (please check the Drupal coding standards).
  

  
  FILE: /var/www/drupal-7-pareview/pareview_temp/user_nav.js
  --------------------------------------------------------------------------------
  FOUND 1 ERROR AFFECTING 1 LINE
  --------------------------------------------------------------------------------
   1 | ERROR | Missing file doc comment
  --------------------------------------------------------------------------------
  
  <strong>Manual Review</strong>:
  
  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.
  
  <code>$menu = array(
      'menu_name' => 'user-nav-menu',
      'title' => 'User Navigation Menu',
      'description' => 'User Navigation Menu',
    );

  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.

  $menu = array(
      'menu_name' => 'user-nav-menu',
    );

  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!!!

Hi 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 some issues with your code (please check the Drupal coding standards).
  

  
  FILE: /var/www/drupal-7-pareview/pareview_temp/user_nav.js
  --------------------------------------------------------------------------------
  FOUND 1 ERROR AFFECTING 1 LINE
  --------------------------------------------------------------------------------
   1 | ERROR | Missing file doc comment
  --------------------------------------------------------------------------------
  

  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.

  $menu = array(
      'menu_name' => 'user-nav-menu',
      'title' => 'User Navigation Menu',
      'description' => 'User Navigation Menu',
    );

  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.

  $menu = array(
      'menu_name' => 'user-nav-menu',
    );

  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!!!

Hi Irfworld,

Thanks for your valuable review and suggestions. I have done all fixes mention by you.

Any specific reason that you used 'Plugins' instead of 'Modules' in Menu.Plugin is not Drupal specific term.

Hi 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.

sites/all/modules/2211983/js/user_nav.js
user_nav.js

    severity: normalreview: comment_docblock_fileFile: @file block missing (Drupal Docs) [comment_docblock_file]

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.

Hi 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.

Manual 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

Assigning to myself for next review, which will hopefully be tonight.

Automated 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.

FILE: /var/www/drupal-7-pareview/pareview_temp/user_nav.install
--------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
--------------------------------------------------------------------------------
85 | WARNING | Unused variable $key.
--------------------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/js/user_nav.js
--------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------------
25 | ERROR | [x] Whitespace found at end of line
--------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/user_nav.install
--------------------------------------------------------------------------------
FOUND 1 ERROR AND 1 WARNING AFFECTING 1 LINE
--------------------------------------------------------------------------------
89 | ERROR | Do not use t() or st() in installation phase hooks, use $t =
| | get_t() to retrieve the appropriate localization function name
89 | WARNING | Only string literals should be passed to t() where possible
--------------------------------------------------------------------------------

Manual 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

(*) No: 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

Yes. If "no", list security issues identified.

Coding style & Drupal API usage

1. (*) Leading slash missing in following code that make wrong URL, if user visit any menu links from inner pages. Better to use l() 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>.
2. hook_help() is missing in your module.
3. (+) user_nav.js: Closing <div> tag is missing in following code.
   var dt = "<div class='clock-date'>" + month[now.getMonth()] + " " + (now.getDay() + 1) + ", " + now.getFullYear() + "<div>";
4. Why you rename 'Module' link label to 'Plugins', keep it consistent. Same with 'Content' link renamed to 'Posts' and other links too. Is there any specific reason to do this, need your comment?
5. user_nav_page_build() : 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.

6. (*) Disabled functionality is not working, if user disabled any menu link from this menu, it still appear that looks weird. This is a basic need of any menu that need to be fix on priority.
   
   
   
7. You are only fetching first level menu items, if user add any sub links these can't be visible.
8. (+) Project Page: Extend this little bit more, better to go through Tips for a great project page again.

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

Hi er.pushpinderrana,

As said by you we have fixed all the issues mentioned by you.

Review of the 7.x-1.x branch (commit 79006a9):

• No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

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:

1. project page: what are the differences to existing projects such as https://www.drupal.org/project/navbar ? Please add that to your project page, see also https://www.drupal.org/node/997024
2. user_nav_page_build(): looks like you have a dependency on the core search module, why is that not listed in the info file?
3. user_nav.tpl.php: "Please click on the image to edit your profile": all user facing text must run through t() for translation.
4. user_nav.tpl.php: this is vulnerable to XSS exploits. If I enter 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.

Hi 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).

Review of the 7.x-1.x branch (commit 8c83cfb):

• No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

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:

1. <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.
2. user_nav.tpl.php: now you ruin you title with drupal_html_class(). That should only be applied to the class name used in the div tag. The actual title for the link can be left as is because l() will sanitize it for you.
3. user_nav.tpl.php: this looks still vulnerable to XSS exploits. $user_object->name is printed to HTML without sanitization, or did I miss something? While usually users cannot create user names with malicious script tags in them there are use cases when user names are imported from third party applications, so this is considered a security issue by the Drupal Security team. Make sure to read https://www.drupal.org/node/28984 again. You can either use check_plain() or theme('username', ...) here.

Hi Klausi,

Thanks for your review,
I have fixed all the issues mentioned by you.

Thanks

manual review:

• user_nav_form_alter(): if you are only targeting one form you should use hook_form_FORM_ID_alter() instead. See https://api.drupal.org/api/drupal/modules!system!system.api.php/function...

But otherwise looks RTBC to me.

Assigning to dman as he might have time to take a final look at this.

Hi Klausi,

Thanks for giving RTBC,
I have changed hook_form_alter with hook_form_FORM_ID_alter.

Thanks

no 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:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

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.