purchase counter

This needs work and I would like to resort to someone else to decide if this shouldn't be a code snippet instead of module...

- You are using variables in your SQL query. In this case it's fine but potentially, you are exposing your module to SQL Injection. Not good, please look at http://drupal.org/writing-secure-code
- In the part where you display $nb... 'total', you are not using t() function and your code is not translatable. In this case, you need both format_plural() and t().

There are lots of coding/documentation standards problems. Start by removing all the $Id$ and all the tabs, then adding doxygen comment blocks to all your functions. Read http://drupal.org/coding-standards and apply all those rules to your module. Run the Coder module to look for any remaining coding standards issues.

Also, your repository is structured wrong. You should not have all the code in a "uc_purchase_counter" directory - it should all be top-level.

All your functions have indentation issues.

Make sure each new level has 2 spaces indented from the parent indentation, ie the following:

  $pdtId = drupal_substr($form_id, 0, 27);
  if ($pdtId == 'uc_product_add_to_cart_form') {
  //custom visibility settings evaluation
  $pc_rule = uc_purchase_counter_evaluate_rule();
  if ($pc_rule) {
    $form['purchase_counter'] = array('#value' => $nbp);
    }
  }

should be:

  $pdtId = drupal_substr($form_id, 0, 27);
  if ($pdtId == 'uc_product_add_to_cart_form') {
    //custom visibility settings evaluation
    $pc_rule = uc_purchase_counter_evaluate_rule();
    if ($pc_rule) {
      $form['purchase_counter'] = array('#value' => $nbp);
    }
  }

Inline comments should use //, not /**/ and span multiple lines. If it spans multiple lines, just use // on each successive line (see http://drupal.org/node/1354#inline)

The following code makes no sense, as you initially set $nbp with format_plural, but then overwrite it.

$nbp = format_plural($total, 'Number of purchase %sngl', 'Number of purchases @count', array('%sngl' => $dbrslt));
$nbp =  t("Number of purchases");
if ($total != 0) {
  $nbp =  $nbp . $total;
}
else {
  $nbp = $nbp ." 0";
}

If you're wanting something basic to take advantage of format_plural, I would suggest the following:
$nbp = format_plural($total, '1 purchase', '@count purchases');

Other than those, I don't see anything else at the moment.

Setting to review. Will get back to this when I get a chance.

Hi!

Automated review (Please keep in mind that this is primarily a high level check that does not replace but, after all, eases the review process. There is no guarantee that no other issues could show up in a more in-depth manual follow-up review.)

It appears you are working in the "master" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.
Review of the master branch:

• Run coder to check your style, some issues were found (please check the Drupal coding standards):
  

  Severity minor, Drupal Commenting Standards, Internationalization, Drupal Security Checks, Drupal SQL Standards, Drupal Coding Standards
  
  sites/all/modules/pareview_temp/test_candidate/uc_purchase_counter.module:
   +7: [minor] indent secondary line of comment one space 
   +7: [minor] Comment should be read "Implements hook_foo()."
   +8: [minor] indent secondary line of comment one space 
   +18: [minor] There should be no trailing spaces
   +33: [minor] indent secondary line of comment one space 
   +33: [minor] put a space between the asterisk and the comment text
   +33: [minor] Comment should be read "Implements hook_foo()."
   +34: [minor] indent secondary line of comment one space 
   +39: [minor] There should be no trailing spaces
   +46: [minor] indent secondary line of comment one space 
   +46: [minor] Comment should be read "Implements hook_foo()."
   +47: [minor] indent secondary line of comment one space 
   +69: [minor] There should be no trailing spaces
   +70: [minor] There should be no trailing spaces
   +71: [minor] There should be no trailing spaces
   +113: [minor] Use an indent of 2 spaces, with no tabs
   +114: [minor] Use an indent of 2 spaces, with no tabs
   +115: [critical] Using eval() or drupal_eval() in your module's code could have a security risk if the PHP input provided to the function contains malicious code.
   +115: [minor] Use an indent of 2 spaces, with no tabs
   +116: [minor] Use an indent of 2 spaces, with no tabs
  
  Status Messages:
   Coder found 1 projects, 1 files, 1 critical warnings, 19 minor warnings, 0 warnings were flagged to be ignored
  

• Comments: there should be a space after "//", see http://drupal.org/node/1354#inline
  

  uc_purchase_counter.module:24:    //custom visibility settings evaluation
  

• Comments should be on a separate line before the code line, see http://drupal.org/node/1354#inline
  

  56:    'access callback' => 'user_access', // Check that the current user
  

• Remove all old CVS $Id tags, they are not needed anymore.
  

  README.txt:1:# $Id: README.txt,v 1.0.0.1 2011/03/21  $
  

• All text files should end in a single newline (\n). See http://drupal.org/node/318#indenting
  

  ./README.txt ./uc_purchase_counter.module
  

This automated report was generated with PAReview.sh, your friendly project application review script. Go and review some other project applications, so we can get back to yours sooner.

HTH, dave

There are still files other than README.txt in the master branch, make sure to remove them. See also step 5 in http://drupal.org/node/1127732
There is a git tag that has the same name as the branch 6.x-1.x. Make sure to remove this tag to avoid confusion.

@ronline has been contacted to ask if the application is abandoned.

After ten weeks with a status of needs work: the applicant may be contacted by a reviewer to determine whether the application was indeed abandoned. The action taken by the reviewer should be documented in the project application issue.

http://drupal.org/node/894256

@ronline: If you ask in the #drupal-gitsupport IRC channel, I'm sure someone will be able to help you resolve your git problem.

I see you have had this under review for over 12 months, so I'll do my bit to help push this through.

You have a few coding standards errors.. This is the easiest thing to overcome in the review process, keeping on top of it will stop delays in the review by easily avoiding things that all the reviews check first:

see http://ventral.org/pareview/httpgitdrupalorgsandboxronline1095418git

There has been alot of work done to drupalcs of late, so it's good to keep your copy updated because new errors often get found that weren't being checked for previously. This is likely especially annoying for you over a 12 month review process

Manual Review

1. You have minor grammatical issue in on line 2 of uc_purchase_counter.info "Shows number of purchase per product" should be "Shows number of purchases per product" It may be a good idea to work on the wording of this a little.

2. function uc_purchase_counter_form_alter does not appear to be checking the $form_id and and will be modifying all forms; consider user hook_form_FORM_ID_alter instead.

3. Consider using single quotes instead of double quotes, or perhaps a heredoc or nowdoc syntax to assign the long help string to the $output variable in function uc_puchase_counter_help, for readability, it also offers an better code efficiency however very minor.
Also on this string the concatenation of . "

" seems surpurfluous and can be included in the previous string; Yeah I can see that it's efficient because the rest is pass through t() but you already have html in there anyway, what is 3 chars compared to readability?

4. Add a uc_purchase_counter.install file with a hook_unistall with code to delete your rules_page and rules_options drupal variables.

Cheers

Hi ronline
You still have some code format errors:
http://ventral.org/pareview/httpgitdrupalorgsandboxronline1095418git
I will look into the code in order to give a manual review.
You still have the master branch. Please delete it to avoid confusion among reviewers about where the "real" version of code is.
Here is some guidelines to delete master branch and set the default branch:
Set default branch to 6.x-1.x: http://drupal.org/node/1659588.
Remove the master branch, see also step 6 and 7 in http://drupal.org/node/1127732

Minor coding standard issues are not application blockers.

When I install the module I get following warning:

user warning: Table 'do_review_6_dev.uc_order_products' doesn't exist query: SELECT SUM(uc.qty) FROM node n, uc_order_products uc, uc_orders o WHERE n.type = 'product' AND status = 1 AND uc.order_id = o.order_id AND o.order_status = 'completed' AND n.nid = '0' AND n.nid = uc.nid in /home/mikkel/workspace/do-review-6.dev/htdocs/sites/all/modules/purchase_counter/uc_purchase_counter.module on line 19.

The table uc_order_products does not exist because the module: "uc_order" has not been installed.

I guess it would make sense to add another line to the .info file:

dependencies[] = uc_order

Another choice (which make less sense) would be to make a check in uc_purchase_counter_form_alter (line 10) if the tables needed exist.
Like:

if (!db_table_exists('uc_order') || !db_table_exists('uc_order_products')) {
  return;
}

function uc_purchase_counter_form_alter:
Instead of checking the form id in the end of the function and thereby running code and db call on every single form on the site I would move the check to the beginning.
Through the function you have variables that are only used once. You can minimize the code by not declaring the variables first.
You declare the variable $total twice and that is in general not good practice.

Here are my suggestions:

function uc_purchase_counter_form_alter(&$form, &$form_state, $form_id) {
  if (
    drupal_substr($form_id, 0, 27) != 'uc_product_add_to_cart_form'
    && !uc_purchase_counter_evaluate_rule()
  ) {
    return;
  }
  $sql = "SELECT SUM(uc.qty) FROM {node} n,  {uc_order_products} uc, {uc_orders} o
    WHERE n.type = 'product'
    AND status = 1
    AND uc.order_id = o.order_id
    AND o.order_status = 'completed'
    AND n.nid = '%d'
    AND n.nid = uc.nid";
  $result = db_result(db_query($sql, $form['nid']['#value']));
  $form['purchase_counter'] = array(
    '#value' => format_plural(
      $result > 1 ? $result : 0,
      '1 purchase', '@count purchases'
    ),
  );
}

You are using "user access" as Access Check in hook_menu(). This means anyone can access the settings = insecure, especially since you are allowing PHP code evaluation in the settings. Why don't you make the whole thing a block and use core visibility settings for blocks?

Adding security tag.

  if (user_access('counter visibility settings')) {
    // Settings page.
    $items['admin/settings/uc_purchase_counter'] = array(
      'title' => 'Purchase Counter settings',
      'description' => 'Custom visibility settings',
      'page callback' => 'drupal_get_form',
      'page arguments' => array('uc_purchase_counter_admin_settings'),
      'type' => MENU_NORMAL_ITEM,
    );
  }

I'm afraid that this is a totally wrong usage of the menu API, please re-read the documentation about hook_menu() and how to properly use the 'access callback' functionality.
http://api.drupal.org/api/drupal/developer!hooks!core.php/function/hook_...

Hi,

Ronline, sorry but I don't understand what is this module for or how to make it work and the description isn't helping much...
The only place where you seem to output content is making a hook_form_alter of uc_product_add_to_cart_form. Where is this form ? How to display it ?

Without knowing what this module does I can only see an evil drupal_eval applied to the data entered by an user into a textarea without any kind of parsing...

Review

.module file line 11:

if (drupal_substr($form_id, 0, 27) != 'uc_product_add_to_cart_form') {

This works but it's a bit complicated when you could simply use:

if ($form_id === 'uc_product_add_to_cart_form') {

.module line 126:

I don't get why you make a drupal_eval() using as param the content of that textarea...
this textarea is supossed to contain a list of paths but NO PHP code.
You should really remove that function to get this approved or explain well why you want to use it.

@logicdesign: When reviewing modules like this which are add-ons to large packages (Ubercart in this case), it's necessary to have at least some familiarity with the large package involved.

Specifically, your advice about the uc_product_add_to_cart_form is wrong and misguided. Every product node has it's own add_to_cart_form, with a name uc_product_add_to_cart_form_{nid} where {nid} is the node id for that product. So the applicant's code is not only correct but it's also optimal. @logicdesign: What you suggest is just wrong in this case.

Likewise, the purpose of this module is immediately clear to anyone with some Ubercart experience.

I acknowledge that the applicant is not interested in pursuing the issue.

I'm only bringing this up @logicdesign because of your expressed desire to become a project application administrator. I want YOU to be aware that administration and review of project applications such as this require specialized knowledge and should not be undertaken superficially.

I'm reading it again and I see this review was wrong.
Thank you to point it out, I will be much more careful in the future.

Closing due to lack of activity. Feel free to reopen if you are still working on this application (see also the project application workflow).

I'm a robot and this is an automated message from Project Applications Scraper.