jm.federico [jmfederico]

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.

This is indeed a requested module, and I'm not aware of any duplication.
The code looks really tidy, and the docblocks are all accurate.

Mostly good use of t() and other Drupal API.
I'm not in favor of using serialized blobs in the database to store your array data, but that's your choice.
Curious indenting on some nested arrays, but I don't think it's wrong.

1

Unfortunately coder.module does not agree with all of the code style. :-(
It's all REALLY boring and trivial whitespace niggles, but try running coder.module over it to get in the habit.
Coder.module report below.

2

  $result = mysql_query("SELECT * FROM uc_favor
   WHERE uid='$user_id'") or die(mysql_error());  
  $result = mysql_fetch_array($result);

No. Must use Drupal DB API

3

function uc_favor_menu(){
  $items['user/%/favorite_order'] = array(
    'title' => 'Favorite Order',
    'page callback' => 'uc_favor_show_favorite_order',
    'page arguments' => array(1),
    'access callback' => TRUE,
    'type' => MENU_LOCAL_TASK,
  );
  return $items;
}

Visually, this looks like a privacy breach? Anyone can see what any other user has favorited?

4

What's happening with the "e-" here looks like it needs documentation. Sure looks weird on review. Also it would appear the message is not always correct?

/**
 * Process Favorite function form
 */
function uc_favor_load_fav_order_submit($form, &$form_state) {
  $clear = '';
  // Check if we want to clear cart's content
  if($form_state['clicked_button']['#post']['clear']){
    $clear = 'e-';
  }
  drupal_set_message(t('Your favorite order has been added to the cart'));
  // redirect using Ubercart CART LINK
  $form_state['redirect'] = 'cart/add/'.$clear.$form_state['values']['link'];
}

... I've not yet tried the module, just scanned the code visually for inconsistencies.

coder.module:

uc_favor.module

Line 23: Control statements should have one space between the control keyword and opening parenthesis
  if(user_is_logged_in()){
Line 23: use a space between the closing parenthesis and the open bracket
  if(user_is_logged_in()){
Line 39: use a space between the closing parenthesis and the open bracket
function uc_favor_save_order($form, &$form_state){
Line 51: Control statements should have one space between the control keyword and opening parenthesis
  foreach($form_state['values']['items'] as $product){
Line 51: use a space between the closing parenthesis and the open bracket
  foreach($form_state['values']['items'] as $product){
Line 53: Control statements should have one space between the control keyword and opening parenthesis
    if($product['remove']!=1 && $product['qty']!=0){
Line 53: use a space between the closing parenthesis and the open bracket
    if($product['remove']!=1 && $product['qty']!=0){
Line 57: Control statements should have one space between the control keyword and opening parenthesis
      if($first>1){
Line 57: use a space between the closing parenthesis and the open bracket
      if($first>1){
Line 61: string concatenation should be formatted with a space separating the operators (dot .) and non-quote terms
      $order_link .= 'p'.$product['nid'].'_q'.$product['qty'];
Line 72: Control statements should have one space between the control keyword and opening parenthesis
  if($first){
Line 72: use a space between the closing parenthesis and the open bracket
  if($first){
Line 76: else statements should begin on a new line
  }else{
Line 77: table names should be enclosed in {curly_brackets}
    mysql_query("DELETE FROM uc_favor WHERE uid='$user->uid'") 
Line 79: missing space after comma
    drupal_set_message(t('You have no favorite order.'),'notice');
Line 92: use a space between the closing parenthesis and the open bracket
function uc_favor_menu(){
Line 103: use a space between the closing parenthesis and the open bracket
function uc_favor_show_favorite_order($user_id = NULL){
Line 105: table names should be enclosed in {curly_brackets}
  $result = mysql_query("SELECT * FROM uc_favor
Line 109: Control statements should have one space between the control keyword and opening parenthesis
  if($result){
Line 109: use a space between the closing parenthesis and the open bracket
  if($result){
Line 123: Control statements should have one space between the control keyword and opening parenthesis
    foreach($products as $product){
Line 123: use a space between the closing parenthesis and the open bracket
    foreach($products as $product){
Line 126: missing space after comma
                                      array('data' => $node->title,'class' => 'desc'),
Line 127: missing space after comma
                                      array('data' => $product['qty'],'class' => 'qty'),
Line 128: missing space after comma
                                      array('data' => uc_currency_format($node->sell_price),'class' => 'price'),
Line 129: missing space after comma
                                      array('data' => uc_currency_format($node->sell_price*$product['qty']),'class' => 'price'),
Line 140: Arrays should be formatted with a space separating each element and assignment operator
                                          'data'=>'<strong>'.t('subtotal').'</strong> '.uc_currency_format($subtotal),
Line 140: string concatenation should be formatted with a space separating the operators (dot .) and non-quote terms
                                          'data'=>'<strong>'.t('subtotal').'</strong> '.uc_currency_format($subtotal),
Line 141: Arrays should be formatted with a space separating each element and assignment operator
                                          'colspan'=>4,
Line 142: Arrays should be formatted with a space separating each element and assignment operator
                                          'class'=>'subtotal',
Line 149: missing space after comma
    $content .= theme_table($header,$rows,array('id' => 'uc_favor-favorite-order'));
Line 155: else statements should begin on a new line
  }else{
Line 190: Control statements should have one space between the control keyword and opening parenthesis
  if($form_state['clicked_button']['#post']['clear']){
Line 190: use a space between the closing parenthesis and the open bracket
  if($form_state['clicked_button']['#post']['clear']){
Line 195: string concatenation should be formatted with a space separating the operators (dot .) and non-quote terms
  $form_state['redirect'] = 'cart/add/'.$clear.$form_state['values']['link'];

sites/all/modules/contributions/uc_favor/uc_favor.install
uc_favor.install

Line -1: Include the CVS keyword $Id$ in each file. This should be in the format // $Id$ or // $Id$ (Drupal Docs)
Line 15: use a space between the closing parenthesis and the open bracket
function faf_fav_or_install(){
Line 23: use a space between the closing parenthesis and the open bracket
function faf_fav_or_uninstall(){
Line 31: use a space between the closing parenthesis and the open bracket
function faf_fav_or_schema(){

string concatenation should be formatted with a space separating the operators (dot .) and non-quote terms

The coding standards report to add a space before, and after the concatenation operator, as for other operators.

Yeah, looks like I need to update the install of coder.module on my testbed - if it's been updated to that change.

@jm.federico: We just review a module / theme per applicant.

Seems like there's a lot of overlap between this and uc_reorder http://drupal.org/project/uc_reorder ? uc_reorder adds a button to the customer's order history so the customer can quickly replicate a previous order. The button adds the contents of the previous order to the cart, which allows the customer to modify the order (add items/delete items/change attributes) or just go straight to checkout. uc_reorder has been on drupal.org for more than a year (and on ubercart.org for another year prior to that), and is being actively maintained.

CVS requirements report that the proposed module must not duplicate the work done from existing project, which doesn't only mean duplicated code.

Tagging all CVS applications with ubercart-related code.

• This is a partial review.
• The points reported in this review are not in order of importance / relevance.
• Most of the times I report the code that present an issue. In such cases, the same error can be present in other parts of the code; the fact I don't report the same issue more than once doesn't mean the same issue is not present in different places.
• Not all the reported points are application blockers; some of the points I report are simple suggestions to who applies for a CVS account. For a list of what is considered a blocker for the application approval, see CVS applications review, what to expect. Keep in mind the list is still under construction, and can be changed to adapt it to what has been found out during code review, or to make the list clearer to who applies for a CVS account.

1. The version line needs to be removed from the .info file.
2. Hook implementation comments should be like the following one:

   /**
    * Implements hook_menu().
    */
   

3. Menu descriptions and titles, as well as schema descriptions, are not passed to t().

4.   if ($uid == $user->uid || user_access('administer users')) {
       return TRUE;
     }
     else {
       return FALSE;
     }
   
   

       return ($uid == $user->uid || user_access('administer users'));
   

Thank you for your contribution!
I am going to update your account so you can opt into security advisory coverage now.
These 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 more contributors chatting on the IRC #drupal-contribute channel. So, come hang out and stay involved.
Thank you, 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.

I thank all the dedicated reviewers as well.