[D7] User Recording

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxMarcus_Johansson23708...

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.

Automated Review

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

• Bad line endings were found, always use unix style terminators. See https://www.drupal.org/coding-standards#indenting
  

  ./user_recording.min.js:        ASCII text, with very long lines, with no line terminators
  
  

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards).
  

  
  FILE: /var/www/drupal-7-pareview/pareview_temp/user_recording.min.js
  --------------------------------------------------------------------------------
  FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
  --------------------------------------------------------------------------------
   1 | WARNING | File appears to be minified and cannot be processed
  --------------------------------------------------------------------------------
  
  Time: 398ms; Memory: 9.75Mb
  

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

Source: http://pareview.sh/ - PAReview.sh online service

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and/or fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements.

3rd party assets/code

Yes: Follows the guidelines for 3rd party assets/code.

README.txt/README.md

Yes: Follows the guidelines for in-project documentation and/or the README Template.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes: Meets the security requirements.

Coding style & Drupal API usage

1. Use Drupal.t() function in Javascript to support i18n.
2. There are code duplicates in user_recording_get() function.
3. Use Drupal.behaviors.user_recording (or similar) instead of using $(document).ready().
4. * Drupal 7 has built-in jQuery.cookie. It's better to use it instead of custom urGetCookie().
5. You have a lot of hardcoded integers in Javascript. Imho, it's better to move all of this to drupal_settings_form().
6. There is a function drupal_get_query_arguments(). It's better to use this instead of directly fetching data from $_REQUEST.
7. * It's better to use drupal_json_output() command to return JSON or delivery_callback => 'ajax_delivery' in hook_menu.
8. * There is a function https://api.drupal.org/api/drupal/includes!locale.inc/function/country_g... to return a list of countries.
9. * It's more correct to add JS via #attached in hook_page_built() instead of preprocess_html().
10. You can use db_merge() instead of db_update and db_insert if entry is not exist.
11. There is a function ip_address() to properly determine IP address of a visitor.

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.

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

This review uses the Project Application Review Template.

user_recording.track.inc:13

For better readability, i would suggest replace

$var = array();
  $var[':user_id'] = isset($params['ur_track_id']) ? $params['ur_track_id'] : '';
  $var[':navigator'] = isset($params['ur_navigator']) ? $params['ur_navigator'] : '';
  ...

with

$var = array(
    ':user_id' => isset($params['ur_track_id']) ? $params['ur_track_id'] : '',
    ':navigator' => isset($params['ur_navigator']) ? $params['ur_navigator'] : '',
    ...
);

user_recording.track.inc:35

drupal_exit() should be used instead of exit, more information is here https://api.drupal.org/api/drupal/includes%21common.inc/function/drupal_...

user_recording.languages.inc

Duplicate array key (at line 32),
Duplicate array key (at line 139)
Duplicate array key (at line 157)

user_recording.js

Unterminated statement at line 103

user_recording_player.js

Constructor call without new (at line 70)

I would suggest rewrite this line in this way:

var re = new RegExp('[?&]' + name + '=([^&]*)');
var match = re.exec(window.location.search);
...

Thanks for your contribution.
The module looks interesting and it does an awesome job.

Please address the issues mentioned by Eugene Fidelin.

user_recording.track.inc:35

echo drupal_json_output(array('status' => 'ok'));
Why are you using echo before the drupal_json_output which returns the data in JSON format.

Review of the 7.x-1.x branch (commit 75aa3bc):

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.
• DrupalPractice has found some issues with your code, but could be false positives.
  

  
  FILE: /home/klausi/pareview_temp/user_recording.module
  ---------------------------------------------------------------------------
  FOUND 0 ERRORS AND 5 WARNINGS AFFECTING 5 LINES
  ---------------------------------------------------------------------------
   17 | WARNING | Do not use drupal_add_js() in hook_page_build(), use
      |         | #attached on the $page render array instead
   18 | WARNING | Do not use drupal_add_js() in hook_page_build(), use
      |         | #attached on the $page render array instead
   19 | WARNING | Do not use drupal_add_css() in hook_page_build(), use
      |         | #attached on the $page render array instead
   56 | WARNING | Do not use drupal_add_js() in hook_page_build(), use
      |         | #attached on the $page render array instead
   75 | WARNING | Do not use drupal_add_js() in hook_page_build(), use
      |         | #attached on the $page render array instead
  ---------------------------------------------------------------------------
  

• 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. user_recording_uninstall(): do not call drupal_uninstall_schema() here, this will be done automatically by Drupal core for you.
2. user_recording_admin(): to access this form only the 'setup user recording" permission is required. But on that page you can configure arbitrary PHP code, meaning anyone with that permission can take over your site. So user_recording_page_build() should only call php_eval() if module_exists('php'), same as block_block_list_alter() for example does. And user_recording_admin() should check for the "use PHP for settings permission" same as for example block_admin_configure() does. This is a security blocker right now. And please don't remove the security tag, we keep that for statistics and to show examples of security problems.

The review bonus tag is already removed, you can add it again if you did another 3 reviews of other projects.

manual review continued:

1. user_recording_list(): all user facing text such as "Play page" needs to run through t() for translation.
2. Settings: Cookie time says "s" at the end, should this be "days" instead?
3. user_recording_list(): this is vulnerable to XSS exploits. If an attacker send <script><lert('XSS');</script> as use agent then you will get a nasty javascript popup on this page. You need to sanitize all user provided text before printing, make sure to read https://www.drupal.org/node/28984 again.
4. "l(user_load($row->uid)->name, 'user/' . $row->uid)": use theme('username', ...) instead of l() here.
5. user_recording_searchform_submit(): do not use drupal_goto() in form submit handler since it stops execution and exits. Use $form_state['redirect'] instead.
6. Why do have geoip as hard dependency? I think you should just use it if it is there, but the module could work fine without it?

Congrats! You are near RTBC :) . Just few nitpicks
Manual Review :

1. user_recording.install : Add the description to all fields defined in user_recording_schema that will make the other developers to easily understand the usage of this field and they will easily contribute to the module.
2. user_recording.js : Define all the variables in single line that will helps in reducing the compressed assets(js) size, when js compression will be enabled.
   

         var urMouseMove = {};
         var urScroll = {};
         var urClicks = {};
         var urKeypress = {};
   

3. (+)user_recording.js : Move these functions(ur_send_data , ur_make_id) under Drupal object because it might create some conflict with some other custom module as well.So I think this should be something like
   

   Drupal.user_recording = {};
   Drupal.user_recording.ur_send_data = function......
   Drupal.user_recording.ur_make_id = function......
   

   you can also add more add on functions under it.and please also change where are u calling these.After these you should call them like Drupal.user_recording.ur_make_id(); instead of ur_make_id();

4. user_recording.js : function ur_send_data : Are we also doing check_plain on the parameter "ur_track_id" because cookies are also user supplied inputs as per the OWASP.
5. user_recording.js : function ur_send_data : Are we also doing check_plain on the parameter "ur_track_id" because cookies are also user supplied inputs as per the OWASP.Would you please specify its usage.
6. user_recording.admin.inc : user_recording_admin :
   

   $form['savability']['countries']['user_recording_countries'] = array(
         '#markup' => t('You must install the !geoip module for this to work', array('!geoip' => l(t('GeoIP API'), 'https://www.drupal.org/project/geoip'))),
       );

   As the GeoIP module has soft dependency.So I would suggest you to update this message a little.

7. user_recording.get.inc : user_recording_get : Better to use drupal_substr instead of substr
8. user_recording.module : user_recording_page_build : Using #attached is pefer over drupal_add_js , drupal_add_css, and its friends.
9. user_recording.module : user_recording_menu :
   

    $items['admin/reports/user_recording'] = array(
       'page callback' => 'user_recording_list',
       'title' => 'User Recording List',
       'description' => 'See all sessions that has been recorded.',
       'access arguments' => array('view user recording'),
       'file' => 'user_recording.list.inc',
       'type' => MENU_NORMAL_ITEM,
     );
   
     $items['admin/config/system/user_recording'] = array(
       'page arguments' => array('user_recording_admin'),
       'page callback' => 'drupal_get_form',
       'title' => 'User Recording Settings',
       'access arguments' => array('setup user recording'),
       'file' => 'user_recording.admin.inc',
       'description' => 'Manage when the user should be recorded.',
       'type' => MENU_NORMAL_ITEM,
     );

   The hook_menu type "MENU_NORMAL_ITEM" is default so we can easily remove it.

10. $items['user_recording_track'] = array(
        'page callback' => 'user_recording_track',
        'access callback' => TRUE,
        'file' => 'user_recording.track.inc',
        'type' => MENU_CALLBACK,
      );

    This menu path has direct access callback to true.The operation in function user_recording_track is not too much expensive that will leads to DOS.So I think it needs more experianced reviewers eyes.What they reckon on this ?

11. user_recording.list.inc : user_recording_list : line 108 : check_plain($row->id), This is not a user provided input.check_plain is not needed here.
12. user_recording.list.inc : user_recording_searchform_submit :
    

    function user_recording_searchform_submit($form, &$form_state) {
      $values = $form_state['values'];
      unset($values['submit']);
      unset($values['form_build_id']);
      unset($values['form_token']);
      unset($values['form_id']);
      unset($values['op']);
      $form_state['redirect'] = array('admin/reports/user_recording', array('query' => $values));
    }

    Only pass those parameters that are needed in query.

@naveenvalecha:
2) Writing verbose Javascript with multiple assignment lines is fine - JS minifiers will do the hard work for you anyway and more verbose code is easier to maintain.
9) including the type explicitly does not hurt and makes it more clear what the code is doing, so this is fine as well.
10) Well that page callback needs to accessible by any user, so that the recorded data can be tracked, right?

manual review:

1. user_recording_page_build(): Doc block should state that this is a hook implementation, see https://www.drupal.org/coding-standards/docs#hooks
2. user_recording_list(): do not call drupal_render() and theme() here, just return a full nested render array, Drupal core will render it later for you. See https://www.drupal.org/node/930760

But otherwise looks RTBC to me.

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

no objections for more than a week, so ...

Thanks for your contribution, Marcus_Johansson!

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.