TechNikh [nikhildubbaka]

I work for a university and we will use this and have been waiting
For something like this to help. I can't wait to use it!

I'd like to put some comments:

- In relay_form() function, there are plenty of hidden form fields which have title passed to t(). Since this is a hidden field, so there is no UI for these t() passed words. Using t() will put un-translated sentences into {locals_source} database table, which is unused. We should remove t() or even remove #title in this case.

- You should require_once() only when needed, for example require_once("relay_func_file.inc") only in functions relay_cron() and relay_file_move(). Doing require_once() in the global of header file will cause these files loaded everytime the boottrap loads, even when relay module is not in use.

- You should replace strlen($dir) by drupal_strlen($dir) for more string handling.

- There are some unused code commented out like in relay_perm() function and more. We could get rid of them without trouble.

- Capitalize this word "implementation".

Setting to 'needs work' based on comments by trungonly

Just a note: the function to use to load a PHP file is module_load_include().

Some remarks while scanning though the code and UI:

• In relay_form(), the 'hidden' form elements are using properties that are not used for this type (e.g. #title, #maxlength, #required, ...) since there is no user input - See http://api.drupal.org/api/drupal/developer--topics--forms_api_reference.... for properties to use
• On admin/settings/relay/configure you might want to provide default values instead of "". You should also add a validation handler to relay_admin_settings() to make sure these settings make sense (e.g. max limit should be numeric, path and default user should exist, etc...).
• You might want to add filter_form() in the body_filter form definition at relay_form(), so users are able to select an appropriate input format for the body. Unless you intentionally want to keep this field plain text.
• Line 325: 'file' property is not needed, since the theming system will look into the relay.module file anyway...
• The 'admin/settings/relay/configure' menu item could be removed, since you have only 1 settings page anyway
• You should probably use the menu wildcard loader arguments functionality. See http://drupal.org/node/224170. This means you should use $items['node/%node/meta'] in hook_menu(), so you get passed the $node object as argument to relay_meta_info() and relay_meta_access(). This avoids needing to call node_load in both of these functions. Same for node/%node/download.
• Please use a blank CVS header in relay.views.inc (// $Id$)
• Not sure what the echo statements in relay_func_node.inc are for on line 111 and 224, but it's normally best practice to put those through a theme() function, and return them with a return statement instead of printing or echoing.

Oh... and don't use t() in relay_schema(). See http://drupal.org/node/322731

If you like to put "#title" in hidden form field, for a place holder for later use, you may put them into a comment segment next to it ?

Bump. Good luck.

HI,

I"ve been following this module. I'd like to use it soon. When will this be approved for download / collaboration.

based on trungonly comments from #14 I am changing the status to reviewed & tested by the community."
and change status to reviewed & tested by the community.

Can someone review and approve this module?

What seems to be the hold up?

I really would like to use it and I think many other people would as well, its fairly useful for educational institutions that have Camtasia Relay servers.

Can this be approved yet? I really want to use this module, but it needs approved for me to do so. Thank You,

Keith

My review of this module as requested by TechNikh: (reviewing off of file in #12)

Need fixed before approval

1. All variables should be in the modules namespace. "dir_search_style" should be corrected to "modulename_dir_search_style".
2. The ending comments for functions are not part of the Drupal standard and should be removed. ex: //function relay_node_info()
3. Node name should be sentence case. "Camtasia relay"
4. The module should not be put into a package of its own in the .info file. Refer to http://drupal.org/node/231036#package
5. Doc block comments should follow the Drupal Doxygen format not the PHPdoc format. http://drupal.org/node/1354
6. In "relay_meta_access()" and "relay_download_access" the else clause needs braces around it.
7. Since simplexml is being used and it is only available in PHP5, a requirement should be listed in the .info file. ex: php = 5.x
8. The functions in relay_func_file.inc and relay_func_node.inc should be prefixed with the module name to prevent namespace collisions.

Non-blocking suggestions

1. The INSTALL.txt file says in step #3 to give the "upload files" permission to the anonymous role. This raises huge security issues that I do not think most sites will be willing to accept. While this is not a complete show stopper I do not see the module being of much use if this is a requirement.
2. The module name I think could stand to be more specific. While "relay" is short it is also very generic. I would like to see this namespace saved for a more generic relay module not one specific on a service.
3. A README.txt file describing what this module is and any other important info would be great. This is usually great place to start writing what will be on the project page.
4. Permissions are confusing. Node is called "Camtasia Relay" while all the permissions refer to a presentation. It would be best to choose 1 term and stick with it.
5. Comments should begin with a capital letter.
6. Arrays in "relay_view()" should follow the standard and place the ending ); on a line by itself.
7. The path option in "relay_views_api()" is redundant as it is by default the module's path.
8. Use a more specific path for "node/%node/meta". That adds a meta path to every node page and it is only specific to your camtasia relay meta information. A solution might be "node/%node/relay-meta"
9. Same for the "node/%node/download" path.
10. relay.views.inc has some spacing issues with some $data lines not properly aligned.

All in all, this is good work. I have no doubt you are well on you well to being a great Drupal developer. Please take these an all other comments as information to help you grow in a positive way.

Forgot the status change.