Submenu reorder module

Hi Varun,

There seems to be much of formatting work done on the module
Have a look at
http://drupal.org/coding-standards/

Make sure you have all of the things are as mentoined in
http://ventral.org/pachecklist

To start with there should be a README.txt and LICENSE.txt is not needed.

Prefix function names with module names

CVS $Id tags are not needed as well

Install coder module and check it for code formatting issues. There are many issues related to indentations, extra spaces etc

Hi Varun,

On coder:
sites/all/modules/pareview_temp/./test_candidate/submenu_reorder.pages.inc:
+74: [minor] Potential problem: use the Form API to prevent against CSRF attacks. If you need to use $_POST variables, ensure they are fully sanitized if displayed by using check_plain(), !f
ilter_xss() or similar.

Might be you had to select the level of check as "monir" in the colder settings to see the minor ones too.

Also check the Ventral Automated Review as well @
http://ventral.org/pareview/httpgitdrupalorgsandboxvarunmishra2006168750...

You could also configure your IDE to suit the alignment and other coding standards
Ex: http://drupal.org/node/75242

Hi Varun,

Code looks clean (reg formatting)

Here are a couple of more small things that you can add upon:
1) Message after module is installed, probably linking to the config/permissions page

2) I doubt if a separate function submenu_reorder_can_reorder_tab_access() is really needed. You can replace it with
'access arguments' => array('permission name here'), in your hook_menu i guess

3) Even after installing the module and configuring the permissions (infact i am logged in as user =1 ) , i still dont see any reorder tab. trying url like node/xx/reorder gives an access denied page.

Best
Tanay

Hi, I'm still pretty new in reviewing applications but I hope I will be able to help a little.

/**
 * Access callback function for reorder menus.
 */
function submenu_reorder_can_reorder_tab_access() {
  if (user_access('reorder pages')) {
    return TRUE;
  }
}

Why such code? You should only use access arguments in menu section. Access callback is when you have to check another conditions.
What's more I cannot find where are you using this function. It looks like abandoned one.

'#suffix' => '<span id="submit-throbber" >&nbsp;&nbsp;&nbsp;&nbsp;</span>',
It what css is for. You can put exact witdth you want to not using html entities.

$order = check_plain($_POST['menu_order']);Why are you not using Drupal FAPI? In your submit functions you should use FAPI instead of plain $_POST

background: transparent url(../../../../../misc/throbber.gif) no-repeat 0px -18px;
You know I do not have to put my module in /sites/all/modules?
I can put it in sites/all/modules/contrib/mystartingmoduleset/favorite
Your path will not work there, although for Drupal it is valid path (I really like to put my modules in subfolder of /sites/all/modules - all sandbox modules i put in /sites/all/modules/drupal.org - so it was a problem for me)

Also about .js file - try to use Drupal.behavior instead of document.ready. Check some core module (block module or profile module) how they are doing it.

Hi

I have installed and tested this module. It worked absolutely fine without trouble. I was looking for such functionality. I have also tested the coding standard. Everything seems to be fine.
Good work.

Added link to trello project review.

Hi,

The module seems to work well.

Please fix the remaining errors found by code_review:

critical errors :
in submenu_reorder.info files must now specify all loadable code files explicitly.

In submenu_reorder.module;
Line 68: hook_access removed in favor of hook_node_access.
Check function submenu_reorder_tab_access($node) {

Remove also trailing spaces on
Line 12 and 13.

Hi,

I have installed and tested this module. Everything seems to be fine.

@varunmishra2006,

Could you fix #16 please?

Setting status to needs work.

/**
  38  * Implements hook_menu().
  39  */
  40 function submenu_reorder_permission() {

Wrong comment.

/**
  66  * Access callback function for reorder tab.
  67  */
  68 function submenu_reorder_tab_access($node) {

Should make this function private via name "_submenu_reorder_tab_access" or do you want others to call this and expose as public API of your module?

Drupal.behaviors.submenu_reorder = {
   4     attach: function (context, settings) {
   5       $("#reorder_menu_box ul", context).sortable({

You want to add .once() here before the .sortable(). And the same is true for the #edit-submit down below. Else this "might" break when ajax requests come in. It is good practice at least to do so.

That was not a full review, but just what I saw quickly as I was curious about the module.

function submenu_reorder_enable() {
  12   drupal_set_message(t('The Submenu reorder module has been successfully installed.

I totally love that part :). That is really nice.

+      $('#edit-submit').once('edit-submit').click(function(){

Nice, but what happens if I use this in my project as well? I think the 'edit-submit' should be 'submenu-reorder' instead. The same for the other .once.

See also here:

http://drupal.org/update/modules/6/7#jquery_once

name = Submenu Reorder
   2 description = Reorders node sub menus
   3 core = 7.x

That feels a little naked indeed. What about comparing to node.info:

name = Node
description = Allows content to be submitted to the site and displayed on pages.
package = Core
version = VERSION
core = 7.x
files[] = node.module
files[] = node.test
required = TRUE
configure = admin/structure/types
stylesheets[all][] = node.css

I think it would be good to specify a version=7.x-1.x-dev and the other items as long as they apply. Choosing a package category (don't invent some) might also be good, though I can't find the page where those are currently.

configure key might apply as well here.

stylesheets you probably don't want as you are adding them on the fly.

Best,

Fabian

@varunmishra2006,

Sorry for point 1) of #19. I was to fast. So now i did a quick manual review.

I still find trailing spaces at lines 12 & 13 of submenu_reorder.install. Either by Code_review or by manual review, while PAReview (online or local) don't find them.

May be you could check it?

Thks.

Module seems clean enough to go to RTBC.

Just waiting for @Fabianx advice on its comments.

Yup, I was mistaken here. According to http://drupal.org/node/542202 all required fields are in. Sorry for the noise.

@chipway-drupal: From me all is clear of what I have tested, if you have done a full functionality test, etc. this RTBC then IMHO.

@chipway-drupal or any other person that has completely tested and reviewed the code will need to pull that trigger based on their review.

@varunmishra2006,

I focused previously too much on the quality of your code that seems ok now, but I am annoyed. :(

I did further testing of the module on a functional point of view; the one of a webmaster downloading your module ant trying to use it after reading README.txt. And I have some difficulties to make it work:

For example, when I read README.txt, I should get a reorder tab on node edit page (so I understand node/xxx/edit). I don't get this tab. I did everything found in the README.

Note: I was using node_hierarchy aside of your module (which was misleading for the use case). I thought it was conflicting and disabled node_hierarchy without no positive result.

Could you explain how to do and what could be wrong please?

Do we need some other module for yours to work?

Thks

Create a node and create three child node of that node.
If you have enabled secondary menus , whenever you go to any of the secondary menu item of that parent node , you will see a reorder tab.

Could you make this more in a step-by-step fashion. I know it seems apparent to you, but it unfortunately isn't for us:

What do you mean with "child node"? What do you mean with "secondary menu"?

Step by step instruction example would be:

* Create a node and in the menu configuration add a menu item to "main menu" with title "parent menu item 1".
* Create another 3 nodes and in the menu configuration ... make them subitems of "parent menu item 1".
* Then ...
* Afterwards configure ...

etc.

That would help. An example whose instructions you can follow step-by-step and have success in the end.

Make that detailed. Your users and reviewers will thank you for it :).

I am logged in as user 1.
I have enabled secondary menus, but I see no reorder tab.

The missing link may be in the sentence "Create a node and create three child node of that node." What do you mean exactly? Suppose I am a 6 years old child ;)

Hi,

Still the code have some issue

Review of the 7.x-1.x branch:

Drupal Code Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.
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.

FILE: ...al-7-pareview/sites/all/modules/pareview_temp/test_candidate/README.txt
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 2 WARNING(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
53 | WARNING | Line exceeds 80 characters; contains 86 characters
54 | WARNING | Line exceeds 80 characters; contains 84 characters
--------------------------------------------------------------------------------

Regards
Alok

@chipway-drupal and @Fabianx

I think the previous instructions were clear for me as a drupal developer. But the new instructions are more clear for a laymen also.
I had given this module to one of my junior drupal developer and he is able to use that module without any trouble.
I found this module quite useful for the websites which contains lots of secondary menus for each of primary menu item. It gives flexibility to change the order of secondary menu items easily.

I have tested this module completely and it worked without any trouble.
@Fabianx if you have also tested it , then I would like to change its status to RTBC.

@varunmishra2006,

I installed a brand new d7 and tested the module thanks to your enhanced README.txt. Now my 6 years old mind understood what to do. And it works as designed.

@Fabianx,
Tks for your help.

I am setting the module to RTBC.

Hope it will pass next step.

manual review:

1. project page is too short, see http://drupal.org/node/997024
2. README.txt.bak? Accidentially committed?
3. submenu_reorder_node_page_reorder(): this is vulnerable to XSS exploits. If I have a menu item with title <script>alert('XSS');</script> there is a nasty javascript popup on the reorder page. You need to sanitize user provided text before printing, make sure to read http://drupal.org/node/28984 again.
4. submenu_reorder_submenu_form_submit(): Do not use the raw $form_state['input'], always use $form_state['values'] where possible.
5. submenu_reorder_submenu_form_submit(): why are you using check_plain() here? You are not printing the variable to the user?

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

I had downloaded the latest code and reviewed it manually. This time the functionality in module is implemented in drupal fashion similar to the other drupal core drag and drop pages works.
The code quality seems fine to me.
I installed the module and the module worked as expected.

@ klausi
I checked your point 3 for XSS vulnerability. I didn't find any issue. The text is sanitized now.
Also, the project page is more detailed.

I am setting the module to RTBC.
I think it should move to next step.

Please don't remove the security tag, we keep that for statistics and to show examples of security problems.

Thanks for your contribution, varunmishra2006!

I updated your account to let you 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 get 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.

You have to create an additional 6.x-1.x branch and create releases accordingly.