[D7] Commerce Authorize.Net Card Present

Thank you for your module.
1) Please use http://pareview.sh/ script, that checks most popular code style errors and provides some tips. I see a lot of errors.
2) And change a link from issue description to http://git.drupal.org/sandbox/tidrif/2230849.git, instead tidrif@git.drupal.org:sandbox/tidrif/2230849.git
3) Don't work in "master" branch.

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

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.

This application satisfies all the requirements of the review checklist, has a clean slate on the PAReview, and introduces functionality that doesn't currently exist in contrib. I believe this is RTBC.

Duplication

This sounds like a feature that should live in the existing commerce_authnet project. Module duplication and fragmentation is a huge problem on drupal.org and we prefer collaboration over competition. Please open an issue in the commerce_authnet issue queue to discuss what you need. You should also get in contact with the maintainer(s) to offer your help to move the project forward. If you cannot reach the maintainer(s) please follow the abandoned project process.

If that fails for whatever reason please get back to us and set this back to "needs review".

Hey guys, Rob and I chatted about the module at DrupalCon along with the other Commerce Authorize.Net maintainer David Kitchen. From an integration standpoint, we're actually comfortable with it living apart. The needs of card present transactions are significantly different enough from card not present transactions that allowing them to develop independently makes fine sense.

The only question I have is could this module use commerce_authnet's existing API request function instead of duplicating that bit? In other words, is there some advantage to be gained by making this module dependent on the other as if it were a submodule of the one project - just able to develop independently?

Automated Review

Best practice issues identified by pareview.sh / drupalcs / coder

none

Manual Review

• Duplication
• No
• Multiple applications
• No
• README.txt
• Follows the guidelines for in-project documentation. Although it could see some improveent around how to configure, use and install the module.
• Master Branch
• Follows the guidelines for master branch.
• 3rd party code
• Follows the guidelines for 3rd party code.
• Individual user account
• Follows the guidelines for individual user accounts.
• Code too short/too simple
• Follows the guidelines for project length and complexity.
• Licensing
• No
• Security
• No security issues identified.
• Coding style & Drupal API usage

  • commerce_authnet_card_present_aim_credit_form()
    ...
    $default_amount = number_format(commerce_currency_amount_to_decimal($transaction->amount, $transaction->currency_code), 2, '.', '');

    This should use commerce_currency_load() to gather the number format settings.

  • function commerce_authnet_card_present_aim_credit_form_validate($form, &$form_state) {
    ...
      if (!is_numeric($amount) || $amount <= 0)
    

    From element_validate_integer_positive(), I'd recommend you use <code>$value !== '' && (!is_numeric($value) || intval($value) != $value || $value <= 0)

  • function commerce_authnet_card_present_aim_credit_form_submit($form, &$form_state) {
    ...
      $amount = number_format($form_state['values']['amount'], 2, '.', '');

    This should use commerce_currency_load().

  • function commerce_authnet_card_present_aim_credit_form_submit($form, &$form_state) {
    ...
            drupal_set_message(t('The transaction must be setted before a credit can be issued. This usually takes 24 hours'), 'error');

    Spelling issue for the word setted.

  • function commerce_authnet_card_present_aim_submit_form_submit($payment_method, $pane_form, $pane_values, $order, $charge) {
    ...
      $nvp = array(
        'x_type' => commerce_authnet_txn_type($txn_type),
        'x_method' => 'CC',
        'x_amount' => number_format(commerce_currency_amount_to_decimal($charge['amount'], $charge['currency_code']), 2, '.', ''),

    This should use commerce_currency_load().

  • (*) You should list curl in a hook_requirements in your .install file. An example at http://cgit.drupalcode.org/oauth/diff/?id=8e2d9d8

  The starred items (*) are fairly big issues and warrant going back to Needs Review. The rest of the comments in the code walkthrough are recommendations.

@tidrif, thank you for your contribution!

Manual Review:

1. You have used following code in commerce_authnet_card_present_aim_void_form() and commerce_authnet_card_present_aim_credit_form() functions.

$form_state['order'] = $order;
$form_state['transaction'] = $transaction;

I think you should use check_plain as these two parameters are coming through hook_menu().

2. One major thing that I have analyzed, you have created card-swipe.js file that contains very small code. I think we can directly add this js in footer with form rendered array. Secondly you have included this js using .info file, it means once this module will installed this js will also included to every page that looks weird.

Here is my suggestion to you either you inline add your js code or add this file with page rendered array or form rendered array.

// Added js file.
$form['#attached']['js'] = array(
    drupal_get_path('module', 'commerce_authnet_card_present') . '/js/card-swipe.js',
);

or

// Added inline js.
$form['#attached']['js']["(function($) {
  Drupal.behaviors.commerceAuthnetCardPresentSwipe = {
    attach: function(context, settings) {
      $('#edit-payment-details-card-present-track', context).focus();
    }
  }
})(jQuery);"] = array( 'type' => 'inline');

In similar way, you can also add the js on page with page rendered array. It would ensure your js code is always adding once and only for your page.

PS: Above code is a sample code, there can be some TYPO :)

Thanks Again!

Automated Review

Best practice issues identified by pareview.sh / drupalcs / coder. No findings @ http://pareview.sh/pareview/httpgitdrupalorgsandboxtidrif2230849git

Manual Review

README.txt/README.md

(+) No: Follows the guidelines for in-project documentation and the README Template. The documentation in the README is a little light on how to use. If it is very similar to other payment systems in commerce, simply link to some general documentation.

Coding style & Drupal API usage

1. (+) .info file references js file. It shouldn't be included on every page view on the site. Attach it to the necessary form(s) using #attached. Or on hook_page_build().
2. .module file: Line 71 and 98. Don't call time(), rather use the constante REQUEST_TIME. Better performance.
3. (+) Don't use HTML in t() calls. For emphasis, %arg_name. See https://api.drupal.org/api/drupal/includes%21bootstrap.inc/function/form.... Using HTML in t() makes translation more difficult.
   

   $form['login'] = array(
       '#type' => 'textfield',
       '#title' => t('API Login ID'),
       '#description' => t('Your API Login ID is different from the username you use to login to your Authorize.Net account. Once you login, browse to your Account tab and click the <em>API Login ID and Transaction Key</em> link to find your API Login ID. If you are using a new Authorize.Net account, you may still need to generate an ID.'),
       '#default_value' => $settings['login'],
       '#required' => TRUE,
     );

4. (+) This wasn't address from earlier:
   

   Spelling issue for the word setted.

   function commerce_authnet_card_present_aim_credit_form_submit($form, &$form_state) {
   ...
           drupal_set_message(t('The transaction must be setted before a credit can be issued. This usually takes 24 hours'), 'error');

5. Rending #markup makes it very difficult for later theming. Please using something that allows you to add some type of unique CSS class. I'd suggest https://api.drupal.org/api/drupal/includes%21theme.inc/function/theme_ht....
   

   function commerce_authnet_card_present_aim_void_form($form, &$form_state, $order, $transaction) {
   ...
     $form['markup'] = array(
       '#markup' => t('Are you sure that you want to void this transaction?'),
     );

6. I disagree with point #1 in #2231731-19: [D7] Commerce Authorize.Net Card Present. These are objects loaded via the hook_menu from a numeric order and payment id. There is no security vulnerability from using them. See commerce_payment_transaction_load() and commerce_order_load().

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

However, none of these are blockers so I'm moving this to RTBC. Assigning to mpdonadio in case he has time to perform a final review in the next week.

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

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards).
  

  
  FILE: /home/klausi/pareview_temp/js/card-swipe.js
  --------------------------------------------------------------------------------
  FOUND 1 ERROR AFFECTING 1 LINE
  --------------------------------------------------------------------------------
   1 | ERROR | Missing file doc comment
  --------------------------------------------------------------------------------
  
  Time: 262ms; Memory: 10.25Mb
  

• No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

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.

manual review:

• project page is outdated: "It is a port of the Commerce Authorize.Net module" => not true anymore, it depends on that module now, correct?

But otherwise looks good to me, so ...

Thanks for your contribution, tidrif!

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.