[D7] Ubercart funds

Hi Aporie,

Please fix the pareview errors reported.

https://pareview.sh/pareview/https-git.drupal.org-project-ubercart_funds...

A function/method name uses the lower camel format when it is, in your case, preRender().

If the class implementing that method is replacing a method another module's class defines, ignore the warning, since you cannot change the method name.

In this case, change the status to Needs review.

PAReview still reports errors. The most important are the following are:

• The permission for the callback on ubercart_funds.module (line 96) should be probably changed.
• There are various not used variables.
• In the installation file, neither t() nor st() should be used directly. The code should use $t = get_t(); and $t('string to translate') to get the translation of a string.

I will make a manual review later today.

The LICENSE.txt file needs to be removed.

  $variables = array(
    'ubercart_funds_commissions',
    'uc_funds_withdrawal_methods',
  );
  // Delete set variables.
  foreach ($variables as $variable) {
    variable_del($variable);
  }

The code is deleting a persistent variable used by another module, or the variable name is wrong and it should be ubercart_funds_withdrawal_methods.

  $schema['uc_funds_user_funds'] = array(
    'description' => 'User Funds',
    'fields' => array(
      'uid' => array(
        'description' => 'The user id of the fund.',
        'type' => 'int',
        'not null' => TRUE,
        'default' => 0,
      ),
      'balance' => array(
        'description' => 'The balance of the user.',
        'type' => 'int',
        'size' => 'big',
        'not null' => TRUE,
        'default' => 0,
      ),
    ),
    'primary key' => array(
      'uid',
    ),
  );

The database table names should be prefixed by the module name.

module_load_include('inc', 'ubercart_funds', 'ubercart_funds.rules');
module_load_include('inc', 'ubercart_funds', 'ubercart_funds.pages');
module_load_include('inc', 'ubercart_funds', 'ubercart_funds.tokens');

Those function calls should not be outside functions.

/**
 * Implements hook_block_info().
 */
function ubercart_funds_block_info() {
  $blocks['admin_site_balance'] = array(
    'info' => t('Site Balance'),
    'cache' => DRUPAL_NO_CACHE,
  );

  $blocks['admin_user_balances'] = array(
    'info' => t('User Balances'),
    'cache' => DRUPAL_NO_CACHE,
  );

  $blocks['account_balance'] = array(
    'info' => t('Account Balance'),
    'cache' => DRUPAL_NO_CACHE,
  );

  $blocks['funds_operations'] = array(
    'info' => t('Account Funds Operations'),
    'cache' => DRUPAL_NO_CACHE,
  );

  return $blocks;
}

Block machine names should be prefixed by the module name. The same is true for routers specific to the module (defined in hook_menu()), and theme functions (defined in hook_theme()). For theme functions, the template name must match the theme name, in Drupal 7. You cannot have site_balance as theme name and ubercart-funds-account-balance as template name.

  $items['user/funds/deposit'] = array(
    'title' => 'Deposit Funds',
    'page callback' => '_ubercart_funds_deposit_funds',
    'access arguments' => array('deposit funds'),
    'type' => MENU_CALLBACK,
  );

Instead of using a page callback that is calling drupal_get_form(), the route should use 'drupal_get_form' as page callback and array('ubercart_funds_deposit_funds') as page arguments.

/**
 * Definition of _ubercart_funds_get_site_balance().
 */
function _ubercart_funds_get_admin_user_balances() {
  if (arg(0) == 'user' && is_numeric(arg(1))) {
    $user_uid = arg(1);
    $user_balance = db_query("SELECT balance FROM uc_funds_user_funds WHERE uid = :uid", array(':uid' => $user_uid))->fetchAssoc();

    return theme('account_balance', array('uid' => $user_uid, 'balance' => $user_balance['balance']));
  }
  else {
    return FALSE;
  }
}

The documentation comment should be more descriptive than Definition of _ubercart_funds_get_site_balance(). which is just giving known information (the function name).

  else {
    drupal_not_found();
    drupal_drupal_exit()();
  }

What is wrong in that code?

/**
 * Definition of ubercart_funds_configure_fees_submit().
 */

There are standard documentation comments for submission and validation handlers.

  drupal_goto('cart/checkout');

That is not the way submission handlers do redirects.

  if (is_numeric($withdraw_amount) && !(floatval($withdraw_amount) > 0)) {
    form_set_error('amount', t('Value must be greater than 0.'));
    return FALSE;
  }

  if (!$user->data || !array_key_exists($method, $user->data)) {
    form_set_error('methods', t('Please %enter_details for this withdrawal method first.',
    array('%enter_details' => l(t('enter details'), 'user/funds/manage/withdrawal-methods/' . $method))));
    return FALSE;
  }

  $amount_plus_fees = ($withdraw_amount + $commission) * 100;

  if ($amount_plus_fees > $user_balance['balance']) {
    form_set_error('amount', t('You cannot withdraw %amount using this payment method. Commission is %commission.',
    array(
      '%amount' => $withdraw_amount,
      '%commission' => uc_currency_format($commission),
    )));
    return FALSE;
  }

Validation handlers don't return Boolean values; they just set an error message for the form or the form element. Validation handlers then don't return after the first error found.

  if (!is_numeric($escrow_amount)) {
    form_set_error('amount', t('Value must be Numeric'));
    return FALSE;
  }

  if (is_numeric($escrow_amount) && !(floatval($escrow_amount / 100) > 0)) {
    form_set_error('amount', t('Value must be greater than 0'));
    return FALSE;
  }

There isn't the need to give two different error messages (and duplicate code) when it's sufficient to use The value must be a positive integer. as error message.

  if ($user->name == $user_exist->name) {
    form_set_error('username', t('You cannot make an escrow payment to yourself.'));
    return FALSE;
  }

There isn't any need to compare usernames when it's enough to compare the user IDs.

  $transaction = new stdClass();
  $transaction->issuer = $issuer->uid;
  $transaction->type = 'Escrow Payment';
  $transaction->recipient = $recipient->uid;
  $transaction->created = time();
  $transaction->brut_amount = $escrow_amount;
  $transaction->commission = $commission;
  $transaction->net_amount = $escrow_after_commission;
  $transaction->status = 'Pending';
  $transaction->notes = $form_state['values']['notes'];

  // Update issuer balance to provision the funds.
  $issuer_balance = db_query("SELECT * FROM uc_funds_user_funds WHERE uid = :uid", array(':uid' => $issuer->uid))->fetchAssoc();
  $issuer_balance['balance'] -= $escrow_after_commission;
  drupal_write_record('uc_funds_user_funds', $issuer_balance, 'uid');

What happens if the uc_funds_user_funds table doesn't have records for the user?

  global $user;
  $uid = $user->uid;

  $transaction = entity_load_single('transaction', $transaction_id);

  $issuer = $transaction->issuer;
  $recipient = $transaction->recipient;

  if ($transaction->status !== "Completed") {
    if ($type == "cancel") {
      if ($uid === $issuer || $uid === $recipient) {
        return TRUE;
      }
    }
    if ($type == "release" && $uid === $issuer) {
      return TRUE;
    }
  }

Is it necessary to use strict comparisons?

module_load_include() is the correct function, and it is used from Drupal core modules. You cannot use it outside functions, but the same is true for include and include_once, which could cause problems when a module file is included by Drupal core (especially in early bootstrap time).

What I meant is that drupal_write_record('uc_funds_user_funds', $issuer_balance, 'uid') only updates an existing record; it doesn't create one if it doesn't exist. If you need to either update an existing record or create it, db_merge() is the function for that task.

Thank you for the explanation.

I didn't see any security issue. I will approve the application later today.

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.

I apologize for the delay. I planned to approve this application yesterday, but I didn't have time.