[D7] Views Usage Audit

I guess this needs review? See https://drupal.org/node/532400

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.

Minor coding standard errors are not application blockers, please do a real manual review.

hi alanmackenzie,

nice module!
In your .info file you don't have to use "" around text.
And I wonder if this should go to Views package rather than Development, thought the purpose is different.
And this module should have dependency on Views, right?

In your views_usage_audit_reports() function you're using count() two times to get the same result in a foreach loop and again every iteration. Would you consider changing it to a variable? like this may be:

    $i = 1;
    foreach ($data as $view => $displays) {
      $rows[][0] = $view;
      foreach ($displays as $display => $paths) {
        $i++;
        $rows[$i][1] = $display;
        $path_column = array();
        foreach ($paths as $path => $value) {
          $path_column[] = $path;
        }
        $rows[$i][2] = implode('<br/>', $path_column);
      }
    }

The comments are also great, everything is well commented.

In your .info file you don't have a dependency on the Views module which you probably should (considering you're using the administer views permission in your hook_menu).

Speaking of hook_menu, perhaps changing the path from admin/reports/views-usage-audit-reports to just admin/reports/views-usage-audit as having the word "reports" in there seems redundant. Not really a bug, just an observation.

In your hook_schema, perhaps add the foreign key markers for the Views table keys you're using. https://api.drupal.org/api/drupal/includes!database!schema.inc/group/sch...

Great module though, looking forward to using it.

Forgot to add a link.

Hi alanmackenzie, thank you for a very good module and nice code. Here is some suggestions:
1) For views block all info is shown, but we can hide admin paths like admin/structure/views/nojs/preview/%/%. If you think that we need them then please explain why;
2) Like in this comment https://drupal.org/comment/7990199#comment-7990199 you are still using count() twice;
3) Please read this wonderful article about drupal caching http://www.lullabot.com/blog/article/beginners-guide-caching-data-drupal-7. After reading you will agree with me that _views_usage_audit_fetch_audit_data() function could be written in easier way

function _views_usage_audit_fetch_audit_data() {
  $data = & drupal_static(__FUNCTION__);

  if (!isset($my_data)) {
    if ($cache = cache_get('views_usage_audit_data')) {
      $data = $cache->data;
    }
    else {
      $results = db_select('views_usage_audit', 'va')
        ->fields('va')
        ->execute();

      foreach ($results as $result) {
        $data[$result->view][$result->display][$result->path] = TRUE;
      }

      cache_set('views_usage_audit_data', $data);
    }
  }

  return $data;
}

Thank you, very nice module!

And please add git clone to description. git clone http://git.drupal.org/sandbox/alanmackenzie/2099953.git views_usage_audit

As per comment #7, you have not declared dependency on views module. If you access the reports page (admin/reports/views-usage-audit-reports) with root account and no views module, you get
Fatal error: Call to undefined function views_get_all_views() in ../views_usage_audit/views_usage_audit.module on line 176
This is the only critical problem I see.

But there are a few minor ones:
- In _views_usage_audit_fetch_audit_data() you may consider adjusting your caching logic as per #8. Some of the checks you are making are unnecessary. #8 is very similar to drupal core code - see core function menu_load_all(). So it must be good.
- in README.txt "displays" is misspelled "dispays".
- in views_usage_audit_reports() you are calling count twice per iteration, and you can make it more readable and call count 0 times if you use something like:

$row = array()
$row[0] = $view;
..
$rows[] = $row;

I know that this is not the scope of this project, but have you thought about making this a full-blown statistics module with information for when a view was last used, and how many times in total?

There is a small issue in views_usage_audit_reports(), where you should be building one row per display, but in fact, you are overwriting the same row for every display of a view.

  $rows = array();

  foreach ($data as $view => $displays) {
    $row[0] = theme('views_usage_audit_report_link', array('view' => $view));

    foreach ($displays as $display => $paths) {
      $row[1] = $display;

      $path_column = array();
      foreach ($paths as $path => $value) {
        $path_column[] = $path;
      }
      $row[2] = implode('<br/>', $path_column);
    }

    $rows[] = $row;
  }

Other than that, I see no other issues (except possibly performance). The drush commands, and the general usage both work.

One thing I can't really review with confidence is the performance implications of this module. I think the scenario for performance problem is when a view is deployed on a very busy site for the first time on the front page, and many hits are made all at once, write-locking the database. I am not sure how to test this at all. I think someone that understands databases better should review.

Thank you for the explanation, and for contributing this great module. I am marking this RTBC.
As for the performance/database concerns, the locking API is used to prevent db_merge from locking the database, and the potential writes/locking are rare edge-case thing, and less severe then what dblog can cause.

Do you need cache_clear_all('views_usage_audit_data', 'cache'); after every View is executed in the shutdown function? Seems like that would slow down the site slightly. Perhaps a better idea to clear cache when the report is generated? Or allowing the admin to configure cache lifetime.

Thanks for your contribution, alanmackenzie!

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.

----
Top Shelf Modules - Crafted, Curated, Contributed.