heydemo [heydemo]

Hello, and thanks for applying for a CVS account. I am adding the review tags, and some volunteers will review your code, pointing out what needs to be changed.

Thought bases on just a code review:

* Your README.txt links to a D5 page: http://drupal.org/getting-started/5/install-contrib/modules
* Your install file docblock says: "Installs the Recurring Fee module."
* Not all functions have docblocks.
* Keep lines of comments to 80 characters wide.
* On line 218, you are not using table prefixing: {table_name}
* What are the licensing on the images included in this module?
* In theme_uc_viral_offer_page(), you use a lot of break tags, when you should use div's and CSS, and the b tag is not semantically correct, you should use the strong tag
* For classes in themes, use your full module name like uc-viral-*
* In this, you are not actually using @title:
$title = t("Refer a friend and save $@amount off the price of your order.", array('@title' => $product->title, '@amount' => $discount));

Overall, seems to be a solid module. Good work.

There have not been replies in the past week; I am marking this application as won't fix.

1. You have a README.txt~ in there.
2. You CSS file does not have $Id$ or a docblock.

3. 
     $items['uc_viral/link'] = array(
       'title' => 'Tell a friend and get $10 off your order.',
       'page callback' => 'uc_viral_link_page',
       'page arguments' => array(2),
       'access callback' => TRUE,
       'type' => MENU_CALLBACK,
     );
   
     $items['uc_viral/offer'] = array(
       'page callback' => 'uc_viral_offer_page',
       'access callback' => TRUE,
       'type' => MENU_CALLBACK,
     );
   

   Your first item uses a page argument that is not defined. You should be using page loaders or a wildcard. Your second item sets access callback to TRUE which is badly formed and not secure.

4.     $link = $base_url .'/uc_viral/link/'. $order_product_id;
   

   You should be using url().

5.     'uc_viral_offer_page' => array('product_title, link, discount'),
       'fb_share_button' => array('link'),
       'twitter_share_button' => array('tweet'),
   

   Your argument definitions are wrong. See documentation on hook_theme. You need to use your modules prefix for these theme functions.

6.                                     JOIN {uc_orders} USING(order_id)
                                       JOIN {uc_viral_products} USING(nid)
   

   I am not 100% sure, but this is not the standard SQL that Drupal uses and is probably not supported by all databases. Please use JOIN ON and specify the type of JOIN. You use the right syntax in other places.

7. /*
    * Retrieves the order_product_id linked to the referrer of the current
    * customer
    */
   

   Please note the correct syntax for all your docblocks:

   /**
    * Retrieves the order_product_id linked to the referrer of the current
    * customer
    */
   

8.   $content .= '<div id="uc-viral-refferal"><strong>Your referral link:</strong>&nbsp;'. $link .'</div>';
   

   You are not using t() when you need to.

Important:

1.       '#field_prefix' => '$',
   

   Is there some way to use the symbol of currency based on the language of the site or some other settings? You have just hard-coded US into your module.

2.       db_query("UPDATE {uc_viral_products} SET enabled = %d, discount = %f, redirect = '%s' WHERE nid = %d", $enabled, $discount, $redirect, $node->nid);
         if (!db_affected_rows()) {
           db_query("INSERT INTO {uc_viral_products} (nid, enabled, discount, redirect) VALUES(%d, %d, %f, '%s')", $node->nid, $enabled, $discount, $redirect);
   

   Utilize drupal_write_record()

3.   module_load_include('inc', 'uc_credit', 'uc_credit.admin');
   

   Your module depends on uc_credit but this is not a dependency in the .info. Maybe this is a dependency of uc_order?

Minor:

1. Yes, though core may do that, it is still good practice, IMO, to ensure all menu items have access control. For instance, what if you build a site where only a certain role can access the store functionality.

2. 
         'discount' => array(
           'description'     => 'The discount offered for each successful referral purchase of this product.',
           'type' => 'numeric',
           'precision' => 16,
           'scale' => 5,
           'not null' => TRUE,
           'default' => 0.0,
         ),
         'redirect' => array(
           'type' => 'varchar',
           'length' => 255,
           'not null' => TRUE,
           'default' => '',
           'description' => 'Path to which referred users are redirected',
         ),
         'enabled'           => array(
           'description'     => 'Determines whether or not discount is actively offered to new purchasers',
           'type'            => 'int',
           'size'            => 'tiny',
           'not null'        => TRUE,
           'default'         => 0,
   

   You should pick a consistent way of representing arrays as well as spacing though out the module. Consistency promotes better readability.

Looks good. I think there is still a fair amount of room to improve coding standards and make the code more readable and add comments, but this good enough to get in. Please keep up with the coding standards and use Coder.

I appreciate your patience in this and hope that you have benefited from this process. I definitely encourage you to get involved with other aspects of the Drupal community.

As far as using CVS on Drupal.org, please read the following.
http://drupal.org/handbook/cvs/quickstart
http://drupal.org/node/10259

I have approved your application. Welcome, and please don't hesitate to ask questions in the correct channels!