[D7] Commerce Cart Stats

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

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.

Visually inspected the code. It looks good. Well structured and makes good use of Drupal and Drupal Commerce API functions.

Installed Module and tested manually. No installation or uninstall errors or issues.

Ran coder with PHP_CodeSniffer, received the following messages (minor or non-issues):

.module

Line 12: Array initialization may be unnecessary if the array is assigned in all code paths. [druplart_array_init]

$items = array();

Line 80: Consider $GLOBALS['user'] instead of global $user. [druplart_user]

global $user;

.display.inc

Line 146: Comments containing code should be removed for releases. [production_code]

// $variables['attributes'] = array(array('width' => '25%'),);

Line 179: Comments containing code should be removed for releases. [production_code]

// $query->condition('co.status', 'completed', '!=');

Line 218: Consider using ++$foo instead of $foo++ when simply incrementing or decrementing. [druplart_unary]

$product_count++;

--
Module does what it says it does.

Please include git clone --branch 7.x-1.0-alpha1 http://git.drupal.org/sandbox/dasfuller/2263275.git commerce_cart_stats in your application, makes it easy for reviewers.

Line #18 in commerce_cart_stats.display.inc

$more_qres = db_query("SELECT COUNT(DISTINCT(hostname)) FROM {commerce_order} WHERE mail=''")->fetch();

Change this using a placeholder instead

$more_qres = db_query("SELECT COUNT(DISTINCT(hostname)) FROM {commerce_order} WHERE mail = :mail", array(':mail' => ' '))->fetch();

Wouldn't be better and consistent if cart statistics will be on commerce/reports/cart url? Reports page is defined in Commerce Reporting module. Maybe it will be better to join forces with Commerce Reporting maintainers and create new submodule.

Visual review, as I don't have a Commerce install to test against.

1. Your branch should be named 7.x-1.x. You can assign tags to specific releases at a later date. See https://drupal.org/node/1015226

2. In commerce_cart_stats_schema(), I don't think you need to specify your PK as also being unique. I don't recall ever seeing that in core.

3. commerce_cart_stats_display() has untranslated strings.

4. In the queries in commerce_cart_stats.display.inc, you could save yourself some grief by assigning the COUNT() results to a name.

5. I think in commerce_cart_stats_adds_removes() the $title is going to come right from the database to the output w/o proper handling (needs to be wrapped in a check_plain()). Adding PAReview: security, but I think someone should double check this.

6. There is a lot of hardcoded markup in commerce_cart_stats.display.inc, and commerce_cart_stats_display() doesn't use a render array. This basically means that this page is unalterable. At a minimum, you need to return a render array. The best thing to do, though, is to implement a hook_theme(), set up a commerce_cart_stats_adds.tpl.php, and build up a render array using #theme. This way people can alter it as needed.

Long term, I would see if you can implement a hook_views_data() to get this data into views. If this is missing from Commerce and friends, then I think that people will like that in addition to the default page you are providing.

Setting this back to Needs Work, as #6 is a pretty big item even if I am wrong about #5.

dasfuller, you should literally name the branch 7.x-1.x. The explicit version numbers are tags that you assign later.

OK, looking better, but it looks like on line 244 you have an unfiltered $title that needs a check_plain(). Double check everything else. If the source data came from user input, then you need to filter it on output.

The themed output is better, but should really be a render array that built up from hook_theme() and a template that you define. This isn't a show stopper, though. Just a strong suggestion. :)

I'm setting this back to Needs Work until you get the render array thing done; it's pretty important and when you set it back to Needs Review, someone will see it. Otherwise, I don't see any problems.

For the render array, try to find a page or module that does something similar. You basically need to

1. Implement a hook_theme(), set up your theme machine name, defined the variables that you pass in, and set it up to use a file and template function.

2. Stub out your template_preprocess_commerce_cart_stats function. It can be empty.

3. Then in commerce_cart_stats_display(), you build up a array from all of the parts. Each of the four string concatenations would be an entry. In the end it would look something like

<?php
function commerce_cart_stats_display() {
  $build = array();

  $build['#theme'] = 'commerce_cart_stats';

  $build['#count_uniques'] = array(
    '#theme' => 'table',
    '#header' => $header,
    '#rows' => $rows;
  );

  $build['#adds_removes] = array(
    '#theme' => 'table',
    '#header' => $header,
    '#rows' => $rows;
  );

  ...

  $return $build;
}

In other words, you never call theme directly.

4. Make your commerce_cart_stats.tpl.php, rough out some basic markup, and then print out four variables for the four tables.

It's actually pretty easy.

Thanks for doing this. After some feedback from the project administrators, the render array wasn't blocking issue, so this was RBTC at #24. However, I think your module is in better shape.

The issues I mention look like they were addressed, but your render array is still a bit off. See the patch that I attached. The idea is that you just build up the table data, and stuff it into the render array. When the whole page gets rendered out, Drupal will take care of calling theme_table() for the data you made. Your theme function assembles the four tables in $build, and then wraps things with some generic markup. I'm not sure if that will work as is, but that is the Drupal way.

I am no seeing any third party code, security issues, project duplication issues, or major API issues, so I am setting this as RBTC.

Yeah, back that out and make sure everything is OK. Sorry, the patch was an idea, not totally working code. Set back to RBTC once the repo is back to working order. It's not a show stopper; just an idea.

Automated Review

Review of the 7.x-1.x branch:

• 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, and may be duplicate results from Coder Sniffer. See attachment.

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.

Fix the files[] problems when you get a chance.

Manual Review

See if you can get a full render array working in commerce_cart_stats_display() instead of directly calling theme().

No major issues, not seeing any security issues.

This has been RBTC status for about two weeks now without objections, so...

Thanks for your contribution, dasfuller!

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.