[D7] Hotjar

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

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.

Manual review

1. Individual user account - Yes: Application issue contains a link to the project page and a working "git clone" command.
2. No duplication - Yes: Does not cause module duplication.
3. Master Branch - Yes: Master branch is set proper to 7.x-1.x
4. Licensing - Yes: Repository does not contain a ‘LICENSE.txt’ file.
5. 3rd party code/content - No: Repository does not contain any 3rd party code.
6. Project page docs- Yes: Project page contains enough information.
7. Repository contains a detailed README.txt- Yes: Repository contains a detailed README.txt.
8. Code contains a well-balanced amount of inline-comments - Yes Code contains inline comments and function comments.

Did a manual review and I agree with comment #4 from @andrdrx.

Suggestion(s):

• You have multiple entries in the variables table: hotjar_account, hotjar_pages, hotjar_roles,...
  Storing these as a 'hotjar' array is more clean and bundled. It will also reduce the amount of DB calls.

A small question, in the part of 'not tracked status codes' I see the 403, 404 codes. Is this something you don't want to track? And if so, should it be general or do you want the user to be able to select what he tracks?

Bottomline: great and to-the-point module. Can't wait to see it approved!

Thank you the manual review @Novitsh and @andrdrx

And thank you the suggestion about storing the variables. We will apply this in the future.

About the "not tracked status codes": For now, we simply do not want to track the error pages. I like the idea to let the administrators set what type of error pages should be tracked.

Hello,
In the hotjar.module file hook_help() is mentioned but the help link is not shown up in the module list page.
In Line no.23 case 'admin/config/system/hotjar': add the case 'admin/help#hotjar': also to provide help link

@vineethaw Thank you the review. I fixed this in 7.x-1.0-rc5.

Automated Review

There is still a master branch, make sure to set the correct default branch: https://www.drupal.org/node/1659588 . Then remove the master branch, see also step 6 and 7 in https://www.drupal.org/node/1127732

-- Make this 7.x-1.x branch as the default branch.

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

[No: Does not follow] 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.

Manual Reviews

1. In hotjar_process_html function is it really needed to break the if condition in multiple lines?

             if (
               !$id
               || !_hotjar_check_status()
               || !_hotjar_should_be_added()
               || !_hotjar_check_user()
             )
           

2. Is there a specific reason that you are using hook_preprocess_html to add the JavaScript? We can use hook_page_alter() and inside it can add a inline/file Javascript using #attached or drupal_add_js().
3. You have used a lot of variable_get() calls. So while installing the module you can initialize those in hook_install() and while uninstalling the module you can remove these as well (which is good) in hook_uninstall().
4. $account = $GLOBALS['user']; instead this, can't we use global $user;?

I haven't done any security review as of now. Later will do it.

This review uses the Project Application Review Template.

Suggestion: If it's possible then please review other modules as well. If you complete at-least 3 module reviews then add the review comments in this page and add the tag "PAReview: review bonus" to this issue. This will help admins notice this module and will be promoted very early.

@aneek Thank you the rewiew.

1. That condition is one of the most important part of the code. I think with breaking the condition into multiple lines improves the readability and it draws the attention.
2. I'd like the tracking code the very end off the page. I found this way the most efficient for this.
3. In 7.x-1.0-rc6 there is an update hook to merge these variables into one. Also we use this merged variable in settings form and hook_process_html too. We created an uninstall hook to remove hotjar variables on unistall too.
4. I just really don't like to use global.

Thank you for your reviews. When finishing your review comment also set the issue status either to "needs work" (you found some problems with the project) or "reviewed & tested by the community" (you found no major flaws).

manual review:

1. hotjar_uninstall(): doc block is wrong, this is hook_uninstall(). See https://www.drupal.org/coding-standards/docs#hooks
2. hotjar_uninstall(): you forgot to remove hotjar_settings? And the other variables don't exist anymore, so can be removed here?
3. hotjar_process_html(): why this hook and not hook_page_build()? And you should put the JS in a dedicated file and add that with #attached to the page render array.
4. _hotjar_check_roles(): why do you use array_sum() here? Did you mean empty()? Please add a comment.

But otherwise looks good to me. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

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

@klausi thank you the review.

1. Fixed the hotjar_uninstall() doc block comment.
2. Yes I forgot the hotjar_settings variable. Now the hook_uninstall removes it, and removes only that variable. The old variable deletion moved to 7001 update hook.
3. After Hotjar updated the tracking code placement instructions I change the hotjar_process_html() into hotjar_preprocess_page(). It adds the tracking code to the head tag as inline script. This is the way Hotjar tracking code has to be placed.
4. I explaind this in an inline comment: The hotjar_roles stores the selected roles as an array where the keys are the role IDs. When the role is not selected the value is 0. If a role is selected the value is the role ID. Checking if $roles is not suitable for us because the roles has keys. But if no role is selected the sum of the values is zero.

@tikaszvince

regarding point 4: you can simply use array_filter() to remove all unselected roles here and then check if $roles is empty.

@stBorchert @klausi

I changed that condition as you requested.

But let me ask you where I can find the instructions about using empty() in the Coding standards?

In the previous form of the condition (with using array_sum()) there was a positive condition check, achieving with a single function call. In the current code a new variable has to be created and need two function calls to get the same result. Over and above the condition is using a negative approach. I believe, at that point of the code, for the easiest understanding that condition should use a positive approach.

The use of empty() is not a coding standard, but using array_filter() is a best practice for this use case (processing the values of checkboxes).
See user_role_change_permissions() as an example.

One minor note (just a personal taste ;) ):

<?php
  $checked_roles = array_filter($roles);
  if (!empty($checked_roles)) {
    foreach (array_keys($account->roles) as $rid) {
      if (isset($roles[$rid]) && $rid == $roles[$rid]) {
        $enabled = !$visibility;
        break;
      }
    }
  }
  else {
    // No role is selected for tracking, therefore all roles should be tracked.
    $enabled = TRUE;
  }
  return $enabled;
?>

This could be rewritten as follows (as far as I understand your code):

<?php
  $checked_roles = array_filter($roles);
  if (empty($checked_roles)) {
    // No role is selected for tracking, therefore all roles should be tracked.
    return TRUE;
  }
  if (count(array_intersect_key($account->roles, $checked_roles))) {
    $enabled = !$visibility;
  }
  return $enabled;
?>

@stBorchert

Thank you the suggestion.

I am trying this module on my local host

I face the problem see the image

@prajaankit,

I don't think this exception caused by this module. The report says the system module cannot create a new record in system table.

From this error all I can think is previously you had a module called hotjar and you just replaced it with this one.

I tested the download, install, configure, uninstall process with Drupal 7.37 with minimal profile installation on PHP 5.2, PHP5.3 and PHP5.5 too and cannot reproduce this error.

Please try to clear the cache or just delete manually the hotjar record (but only that record) from system table. If the error don't disappear please create a new issue and describing your environment:

• Your web server type and version, installed modules/extensions
• PHP version and list of installed PHP modules
• Drupal version and installed modules with exact versions
• and the list of steps you can reproduce this error with that environment

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

Thanks for your contribution, tikaszvince!

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.