Engagement

Hi Dan,

Sounds like quite an interesting module. I will give it a manual review in a moment but first off, there are a few (relatively minor) coding style issues.

> It seems like you´ve a misnamed branch/tag created in your repository, please have a look at the release naming conventions. Essentially, your dev branch should simply be called 7.x-1.x, not 7.x-1.x-dev as you have now. This is where you should do all development work. Your master branch should be empty, except for a README.txt saying something like "See major version branches".

> Documentation-wise, for a developer oriented module such as this it would be nice to see some more examples of how it could be used to track and react to custom events. Perhaps you could create an engagement_example sub module demonstrating it's use.

> An automated code review revealed a couple of very minor coding style issues. It should only be a couple of minutes to get a completely clean slate. You can view the results here:

http://ventral.org/pareview/httpgitdrupalorgsandboxdonutdan41141588634git

> I notice you have a testing menu callback. You could perhaps consider using Drupal's testing framework here to run automated tests.

> In your JavaScript, it might be nicer to attach your _event function to the Drupal object. e.g. Drupal.engagement.trackEvent or something similar. See line 98 of /misc/ajax.js for an example ( Drupal.ajax= function() { ... } )

> In engagement_page_alter() you are using $field as an array without declaring it. It's minor but it would be nicer to see $field = array(); before you use it.

On the whole though, it's all looking fairly clean and tidy, good job! I'm going to install it now and have a bash at tracking some events.

Cheers,

Andy

coder and coder_tough_love are flagging a few more errors that ventral didn't pick up. I've attached them to this issue as an image to preserve the formatting, but you can easily view and retest them yourself by installing http://drupal.org/project/coder and http://drupal.org/project/coder_tough_love.

Most of it is stylistic again, but it raises a potential problem with the unfiltered $_POST variables.

Also, I'm curious to know your motivation for this constant as you only appear to use it once.

define('ENGAGEMENT_NODE_VIEW', 'node_view');

Ok. Manual review now.

> It would be nice to have a page in the reporting section showing the tracked events, perhaps with some way of filtering it down, much like the "Recent log entries" reporting section.

> Could you move debug mode messages into a drupal_set_message() call, or perhaps watchdog() instead. Or perhaps test if the site has develinstalled and use dpm(). The reason I say this is that when I put engagement_event_track() inside my module's form submit function that is called via ajax, the var_dump() caused extra output which broke the ajax call. Switching it to dpm() allowed me to track the event without errors.

> Tracking my custom event was quite easy ultimately. In a form submit callback I tracked an event called postcode_check, and then elsewhere I tested that value and output something when it was greater than 5.

My main comment would be that since engagement_event_count() is generally meant to track the current user, it is slightly clumsy to *have* to pass the $uid every time. My function didn't have global $user; in it, I had to declare it just to use this function. Perhaps if you reworked this function something like this:

function engagement_event_track($event_name, $event_value = NULL, $event_data = NULL, $uid = FALSE) {

  if($uid === FALSE) {
    global $user;
    $uid = $user->uid;
  }

  ...

}

That way I could track the current user without having to pass it in manually, but it would still allow overrides if necessary.

> As a feature request, it might be nice to integrate with the Context module and provide a tracking action as well as a event count reaction. So for example, if my custom event had been triggered more than 10 times, I could use Context to display an extra block. I think Context would be a good partner for your module as it would open up your API to people who don't want to write code, at least on a basic level.

All in all, I think this module has quite a lot of potential. A few tweaks here and there to make the function calls a little friendlier to use (the $uid comments above) perhaps, and personally I would really like the Context integration although I would be happy to RTBC this without it.

Look forward to seeing how this one progresses.

Cheers,

Andy

Agreed, I would be happy to RTBC this without the feature requests. I've found a couple of issues though:

1> Whenever you use a node_load(), you need to check that you actually have a node returned correctly. For example, on line 214 you do this:

// Check if we should be tracking this type of page.
  if (arg(0) == 'node') {
    $n = node_load(arg(1));
    $track = engagement_get_tracked_content_types();
    if (!in_array($n->type, $track)) {
      return FALSE;
    }
  }

However, on a clean install of my site, using Drupal's default frontpage (which is "node"), this results in a PHP error. You should do something like this instead:

// Check if we should be tracking this type of page.
  if (arg(0) == 'node' && ($n = node_load(arg(1)))) {
    $track = engagement_get_tracked_content_types();
    if (!in_array($n->type, $track)) {
      return FALSE;
    }
  }

The node_load has gone into the if statement in an extra set of brackets. If node_load() returns FALSE here, this will not fall through to the code inside the if statement.

Another common pattern here is to say:

if(arg(0) == 'node' && is_numeric(arg(1))

That stops it matching the frontpage with a path of just "node". At the moment, viewing Drupal's default frontpage is being counted as a node_view, not to mention that it throws an error.

2> I have been trying to call your JavaScript API via Firebug and it just comes back saying "Invalid token". It works fine when I am logged in as user 1, but not for anonymous users.

3> I haven't been able to confirm this with anonymous users yet due to the tokens issue, but when logged in as user 1 I can run the following piece of JavaScript:

$.ajax({
    url:Drupal.engagement.api_url + Drupal.engagement.token(),
    data:'action=event_delete&event_name=&event_value=&',
    type:"POST",
    async:false,
    success:function(val){
      result = val;
   }
});

And this will empty the entire engagement table. Some checking of parameters here would be good. Also, reading through the code I can't see any access checking occcuring on the API calls. So once the aforementioned token issue is fixed, I would be able to come along as an anonymous user and empty your engagement table. The solution would be to define some more permissions, perhaps one for each event type and test whether the current user is allowed to perform this action (see user_access())

As for the automated review. It's coming back nearly clean now:

http://ventral.org/pareview/httpgitdrupalorgsandboxdonutdan41141588634git

Hi,

Looking a bit better. Just want to run through the logic in this function though:

/**
 * Delete an event based on the event_name.
 *
 * @param string $event_name
 *   Event name.
 */
function engagement_event_delete($event_name = NULL, $event_value = NULL, $uid = FALSE) {
  if ($uid) {
    $user = user_load($uid);
  }
  else {
    global $user;
  }
  if (!user_access('delete events', $user)) {
    return FALSE;
  }
  module_invoke_all('delete_event', $event_name, $event_value);
  $result = db_delete('engagement');
  if ($event_name) {
    $result->condition('event_name', "%$event_name%", 'LIKE');
  }
  if ($event_value) {
    $result->condition('event_value', "%$event_value%", 'LIKE');
  }
  if ($uid) {
    $result->condition('uid', $uid);
  }
  return $result->execute();
}

You are now testing the permission which is great. However, let's say I am logged in as a user who doesn't have permission to delete events. If I call this function and pass the $uid of a user who does have permission to delete events, I can neatly sidestep this access check.

If I understand this correctly, you want to pass the $uid in this function into the query in order to only delete events for that $uid. However, I don't think you want to be performing the user_access() call using this user. I think the following snippet is probably what you want?

/**
 * Delete an event based on the event_name.
 *
 * @param string $event_name
 *   Event name.
 */
function engagement_event_delete($event_name = NULL, $event_value = NULL, $uid = FALSE) {
  if (!user_access('delete events')) {
    return FALSE;
  }
  module_invoke_all('delete_event', $event_name, $event_value);
  $result = db_delete('engagement');
  if ($event_name) {
    $result->condition('event_name', "%$event_name%", 'LIKE');
  }
  if ($event_value) {
    $result->condition('event_value', "%$event_value%", 'LIKE');
  }
  if ($uid) {
    $result->condition('uid', $uid);
  }
  return $result->execute();
}

On a related note, I think the "delete events" permission is slightly too generic and might collide with other modules. It should probably be something like "delete engagement events".

The documented parameters for this function are out of date. Might be worth just double checking they're all up to date.

Will continue this in a moment, have to go cook tea...!

Just tried to enable your module on a fresh Drupal install and got a fatal error caused by this line:

user_role_change_permissions(user_role_load_by_name('global administrator'), 'delete engagement events');

As per the docs for this function, the second argument needs to be an array (http://api.drupal.org/api/drupal/modules%21user%21user.module/function/u...).

user_role_change_permissions(user_role_load_by_name('global administrator'), array('delete engagement events'));

Ok, nearly there I think!

Looking at your menu hook, your API call should technically have another argument in the URL. So instead of this:

'engagement/api' => array(
  'type' => MENU_CALLBACK,
  'page callback' => 'engagement_api',
  'access arguments' => array('access content'),
),

You should have a wildcard in the URL as you are using this URL component on line 37: $token = arg(2);:

'engagement/api/%' => array(
  'type' => MENU_CALLBACK,
  'page callback' => 'engagement_api',
  'access arguments' => array('access content'),
),

There are a couple of outstanding issues on the automated code review:
http://ventral.org/pareview/httpgitdrupalorgsandboxdonutdan41141588634git

The first one about using drupal_ prefixed string functions isn't a major problem, but it's also easy to fix. E.g. drupal_strtoupper() might be used instead of the native PHP function. They all do the same thing but to varying degrees they cope with edge cases that the native functions don't.

The second one regarding the argument order is about this function here:

function engagement_event_date_range($uid = NULL, $event_name = NULL, $event_value = NULL, $start, $end = NULL) {
  ...
}

It's complaining because all of the arguments have default values apart from $start in the middle. All of your non optional arguments should come before your optional ones. If it was me I might use this order:

function engagement_event_date_range($start, $end = NULL, $event_name = NULL, $event_value = NULL, $uid = NULL) {
  ...
}

We're talking about returning events for a date range, so the most pertinent arguments should be the start and end date. Start date being required, but omitting the end date would return all events until the present. The function then optionally allows filtering by event name, value and user.

Cool. I will take another (hopefully final) look later this week, probably wednesday.

Cool, looking good now. I'm going to RTBC this but there's a couple of minor things I spotted since:

* in engagement.admin.inc you have "Implements hook_FORM_ID()" in the documentation but this isn't acually a hook, it's just a form callback function.
* You have this at the top of the form:

  $access = user_access('admin engagement module');
  if (!$access) {
    print "You do not have permission to access this page.";
    exit();
  }

But you are already testing the permissions in the hook_menu() under "access arguments" so you can just remove this line. On a side note, it's not good practice to print something and then exit() like that. You probably should have returned drupal_access_denied() instead

* Moving forward, I would really like to see an engagement_example.module with examples of how developers can use your module. This isn't a blocker but it would be extremely useful.

We have currently exceeded the proposed project application thresholds, so this is on hold for me for now. Get a review bonus and I will review/approve this right away.

• Add a screenshot to your project page
• More documentation and usage introduction would be nice
• Delete the master branch
  

  git checkout 7.x-1.x
  git branch -D master
  git push origin :master

• Never exit directly by exit(); use drupal_exit(); instead

• function engagement_api() {
    $token = arg(2);

  function engagement_api($token) { should do the same job, but easier

• $obj->... = check_plain($_POST['...']);The concept of drupal is to filter output - not input!
• Implements hook_page_alter(). * Prints variables to the page.
  why don't you make use of drupal_add_js's 'settings' function ?
• Never access $_GET['q'] directly, use current_path() instead
• if (arg(0) == 'node' && ($n = node_load(arg(1)))) {, why arent you using menu_get_object() here?

got to stop, will continue later

Note that issues found are possibly false positives and fixing all issues is not a requirement for getting through the application process. Automated reviews may point you to possible security issues - what does not mean they are really security issues - note that it's a common case that automated reviews can have false positives.

There's no convention about this as far I know, but any kind of picture that gives a non-programmer user an idea what it does would be nice.

Go to https://drupal.org/documentation/modules/contributions
and at the bottom you'll find "Add child page", name the child page like your module.

Maybe you got to switch the default branch first:
goto your project > edit > default branch > set 7.x-1.x as default branch

let's continue

1. your implementing hook_page_build() to actually track node views - why you'r not using hook_node_view() ?
2. Also I would rather track such page views by javascript, because if a site is using additional caches like varnish that also cache for logged-in users, you have to problem that none of these hooks will be executed.
3. module_invoke_all('track_event', $uid, $event_name, $event_value, $event_data);
   all hooks you invoke must be prefixed with your module's machine name! also all of them must be documented in a api.php file (https://drupal.org/node/161085#api_php)

4. $script = "_gaq.push(['_trackEvent', '$event_name', '$event_value', '$event_data']);";
       drupal_add_js($script, 'inline');

   can you always be sure that these variables contain secure content? if they can contain user content you have to make sure to check_plain() them also for drupal_add_js()!

5. t("Manage") . ' ' . l(t('Google Analytics'), 'admin/config/system/googleanalytics');
   if your always concatenating strings this way, translators have the problem of missing context because they only see "Manage" and "Google analytics" seperately - on the other side its not good to use html within translatable strings. But this is okay and also used in core:t('Manage <a href="@ga_url">Google Analytics</a>', array('ga_url' => url(admin/config/system/googleanalytics)));
6. NEVER EVER do this t($description), - ALWAYS translate static string directly in any other case use placeholders of t() (@ ! or %)! as the contents of $description are already translated some lines above just use $descriptions without t() here
7. $description = "(module not found)";1. this code will have no impact it's out of scope 2. Translate static strings with t()!
8. 'Separate by new lines, not commas.' Translate ALL static strings by t()! (except within hook_menu, hook_schema and watchdog-calls)
9. in drupal 7 admin will always have all permission by default
   

     // Give global admin permissions to configure.
     $u = user_load(1);
     $u->roles[] = "admin engagement";
     user_save($u);
   

   this is not necessary.

10. It's not necessary to set any variables on install, that's what the default parameter in variable_get() is made for
11. user_role_change_permissions(user_role_load_by_name('global administrator')->rid, array('delete engagement events' => TRUE));As far I know there is no global administrator role on a standart drupal installation - but even if it were - never rely on this, simply remove it and let the user decide.

Fix points 3, 6, 7, 8 and we're done here

leaving rtbc

10) It's not bad, but it's not the way it's designed for. Worst-case: some of the variables are deleted from the variables table manually and as the default values are not given they'll all return NULL. You had to reinstall the module for having the default values back.

7) not fixed

Thanks for your contribution and welcome to the community of project contributors on drupal.org!

I've granted you the git vetted user role which will let you promote this to a full project and also create new projects as either sandbox or "full" projects depending on which you feel is best.

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.

As you continue to work on your module, keep in mind: Commit messages - providing history and credit and Release naming conventions.

Thanks to the dedicated reviewer(s) as well.