[D7] Commerce Eurobank

Hi yannisc,
Thank you for your contribution! Please remove LICENSE.txt from your module as well as any other licensing information. That will be added by git when your project is promoted.

Oh, that's nothing! Very common mistake I assure you. Good luck on your application!

Screening cleared:
-Has link to sandbox with module files
-Module duplication Nothing found. However, if there are any similar modules, part of the review process is to mention them and describe how your module is different.
-No LICENSE.txt removed
-Has README.txt Good job on that one.

Please be sure to run your module through the Coder module (http://drupal.org/project/coder) on minor (most) and review coding standards (http://drupal.org/coding-standards) for more information (though your module looks pretty close to standardized).

When working with an external API, if the coding standards will break communication between your module and the API you certainly do not need to comply. As long as you have done what you can, if complying will cause issues then leave it how it is. Coding standards serve a purpose of promoting collaboration by making code uniformly understandable, but nothing is perfect! Thanks for the update!

Well, I only did a screening to check that your module was prepared for review. Since the queue is so long, we generally have been trying to shorten it by reviewing the applications that have been active the longest. Currently, many applications have been in the queue for over a month, so don't be discouraged if the process slows down. Someone will come along to complete the code review. The module may be tested, and we will check especially for secure coding practices and demonstrable knowledge of the Drupal API, as well as an ability to maintain projects through git and the issue queue on drupal.org. Once a reviewer feels your project demonstrates those abilities and all issues have been fixed they will mark it RTBC and a git administrator will come along and grant you the git vetted user role. Hope this helps!

Pretty much! However, if you want to affect the backlog you are free to review other project applications. You can even join the project review group at http://groups.drupal.org/code-review or for information on how to review new project applications visit http://drupal.org/node/894256

well had you read the links posted on #9 you would have your issue set to critical..
I understand that it can be frustrating but as jordojuice noticed this process is run by volunteers.
if you dont like the waiting, then help;)

And finally, are you sure you run your module through coder set to minor ?

I see.
But what i meant is that you could review/help with other issues so the other reviewers, have more time and review yours :)

about the coder, i run it vs your module and i got several indentation and space errors.

moreover, about coding standars.

if ($payment_method['settings']['txn_mode'] == EUROBANK_TXN_MODE_LIVE) {$server_url = EUROBANK_SERVER_LIVE;}
else {$server_url = EUROBANK_SERVER_TEST;}

should be like this

if ($payment_method['settings']['txn_mode'] == EUROBANK_TXN_MODE_LIVE) {
  $server_url = EUROBANK_SERVER_LIVE;
}
else {
  $server_url = EUROBANK_SERVER_TEST;
}

or use the ternary operator (more elegant and saves some lines)

$server_url = ($payment_method['settings']['txn_mode'] == EUROBANK_TXN_MODE_LIVE) ? EUROBANK_SERVER_LIVE : EUROBANK_SERVER_TEST;

Much more readable, don't you agree?

Also, inline commens like this

$safepost = preg_replace('%<CCN>.*</CCN>%i', '<CCN>CC NUMBER NOT SHOWN</CCN>', $post); // remove cc number from log message.

should be

// remove cc number from log message.
$safepost = preg_replace('%<CCN>.*</CCN>%i', '<CCN>CC NUMBER NOT SHOWN</CCN>', $post); 

please read this

Understood.
Yes i will, but you need to commit the changes and set the status back to needs review

EDIT: Do you use coder 7.x-1.x-dev version? (thats what i use and got the minor warnings)

lines 137 to 250 needs proper indentation and there are some control structures that still aren't according to drupal standards.

eg
wrong place for closing brackets:

if ($charge['currency_code'] == 'EUR') {
  $currency = 978;}
else if ($charge['currency_code'] == 'USD') {
  $currency = 840;}
else if ($charge['currency_code'] == 'GBP') {
  $currency = 826;}

wrong indentation:

  if ($payment_method['settings']['txn_type'] == COMMERCE_CREDIT_AUTH_ONLY) {
  $transaction_type = 'AUTH';
  }
  else {
  $transaction_type = 'CAPTURE';
  }

wrong indentation again:

  if (!isset($payment_method['settings']['merchant_id']) && !isset($payment_method['settings']['hash_password'])) {
    watchdog('commerce_eurobank', $payment_method['settings']['txn_mode'] . 'Merchant id and hash password have not been set in the payment settings. Cannot proceed with transaction', array(), WATCHDOG_ERROR);
    drupal_set_message(t('The Eurobank gateway has not been correctly configured - see watchdog for details. Cannot proceed with payment'), 'error');
    return FALSE;
  } 
//there should be at least two spaces indentation till the end of the function
$custemail = $order->mail;
$merchantref = $order->order_id . "_" . $custemail . "_" . date('U');
$myamount = $charge['amount'];

Those aren't by the coder module.
Those are by me:)

btw thats coder's output for me:

commerce_eurobank.module

    severity: minorLine 34: There should be no trailing spaces

    /** 

    severity: minorLine 56: There should be no trailing spaces

      

    severity: minorLine 75: There should be no trailing spaces

      

    severity: minorLine 86: There should be no trailing spaces

      

    severity: minorLine 95: There should be no trailing spaces

      

    severity: minorLine 135: There should be no trailing spaces

      } 

    severity: minorLine 136: There should be no trailing spaces

      

    severity: minorLine 159: use lowercase html tags to comply with XHTML

    $post = "APACScommand=NewRequest&Data=<?xml version=\"1.0\" encoding=\"UTF-8\"?><JProxyPayLink><Message><Type>PreAuth</Type><Authentication><MerchantID>$merchantid</MerchantID><Password>$hashpassword</Password></Authentication><OrderInfo><Amount>$myamount</Amount><MerchantRef>$merchantref</MerchantRef><MerchantDesc>$merchantdesc</MerchantDesc><Currency>$currency</Currency><CustomerEmail>$custemail</CustomerEmail><Var1>var1</Var1><Var2>var2</Var2><Var3>var3</Var3><Var4>var4</Var4><Var5>var5</Var5><Var6>var6</Var6><Var7>var7</Var7><Var8>var8</Var8><Var9>var9</Var9></OrderInfo><PaymentInfo><CCN>$ccnumber</CCN><Expdate>$ccexp</Expdate><CVCCVV>$cccv</CVCCVV><InstallmentOffset>0</InstallmentOffset><InstallmentPeriod>0</InstallmentPeriod></PaymentInfo></Message></JProxyPayLink>";

    severity: minorLine 168: There should be no trailing spaces

      

    severity: minorLine 176: There should be no trailing spaces

    curl_setopt($curlSession, CURLOPT_RETURNTRANSFER, 1); 

    severity: minorLine 177: There should be no trailing spaces

    curl_setopt($curlSession, CURLOPT_TIMEOUT, 30); 

    severity: minorLine 184: use lowercase html tags to comply with XHTML

    preg_match('%<ERRORCODE>.*</ERRORCODE>%i', $rawresponse, $errorcode);

    severity: minorLine 185: use lowercase html tags to comply with XHTML

    preg_match('%<ERRORMESSAGE>.*</ERRORMESSAGE>%i', $rawresponse, $errormessage);

    severity: minorLine 190: use lowercase html tags to comply with XHTML

    $safepost = preg_replace('%<CCN>.*</CCN>%i', '<CCN>CC NUMBER NOT SHOWN</CCN>', $post);

    severity: minorLine 193: use lowercase html tags to comply with XHTML

    $safepost = preg_replace('%<CVCCVV>.*</CVCCVV>%i', '<CVCVV>XXX</CVCCVV>', $safepost);

    severity: minorLine 196: use lowercase html tags to comply with XHTML

    $safepost = preg_replace('%<Expdate>.*</Expdate>%i', '<Expdate>XXXX</Expdate>', $safepost);

    severity: minorLine 216: use lowercase html tags to comply with XHTML

      $capturepost = "APACScommand=NewRequest&Data=<?xml version=\"1.0\" encoding=\"UTF-8\"?><JProxyPayLink><Message><Type>Capture</Type><Authentication><MerchantID>$merchantid</MerchantID><Password>$hashpassword</Password></Authentication><OrderInfo><Amount>$myamount</Amount><MerchantRef>$merchantref</MerchantRef><MerchantDesc /><Currency /><CustomerEmail /><Var1 /><Var2 /><Var3 /><Var4 /><Var5 /><Var6 /><Var7 /><Var8 /><Var9 /></OrderInfo></Message></JProxyPayLink>";

    severity: minorLine 222: use lowercase html tags to comply with XHTML

      preg_match('%<ERRORCODE>.*</ERRORCODE>%i', $capturerawresponse, $captureerrorcode);

    severity: minorLine 223: use lowercase html tags to comply with XHTML

      preg_match('%<ERRORMESSAGE>.*</ERRORMESSAGE>%i', $capturerawresponse, $captureerrormessage);

    severity: minorLine 245: There should be no trailing spaces

        watchdog('commerce_eurobank', $payment_method['settings']['txn_mode'] . 'Payment failed: %message', array('%message' => $errormsg), WATCHDOG_WARNING); 

    severity: minorLine 253: There should be no trailing spaces

    /** 

    severity: minorLine 260: There should be no trailing spaces

You should fix trailing spaces errors.you can leave the html ones be

• watchdog messages:
  from the watchdog API documentation:

  $message The message to store in the log. See t() for documentation on how $message and $variables interact. Keep $message translatable by not concatenating dynamic values into it!

  code like this :

  watchdog('commerce_eurobank', $payment_method['settings']['txn_mode'] . 'Merchant id and hash password have not been set in the payment settings. Cannot proceed with transaction', array(), WATCHDOG_ERROR);
  

  should be:

  watchdog('commerce_eurobank',  '%method Merchant id and hash password have not been set in the payment settings. Cannot proceed with transaction', array('%method' => $payment_method['settings']['txn_mode']), WATCHDOG_ERROR);
  

  replace the occuring instances.

• variable naming:
  from coding standards documentation:

  variables should be named using lowercase, and words should be separated with an underscore

  code like this :

    $merchantid = $payment_method['settings']['merchant_id'];
    $hashpassword = $payment_method['settings']['hash_password'];
    $merchantdesc = 'site payment';
    .
    .
    .
    $curlSession = curl_init();
    curl_setopt($curlSession, CURLOPT_URL, $server_url);
  

  should be :

  <code>
    $merchant_id = $payment_method['settings']['merchant_id'];
    $hash_password = $payment_method['settings']['hash_password'];
    $merchant_desc = 'site payment';
    .
    .
    .
    $curl_session = curl_init();
    curl_setopt($curl_session, CURLOPT_URL, $server_url);
  

  this provides better consistency and readability.
  replace the occuring instances.

• use drupal's mail function

  mail(variable_get('site_mail'), 'successful auth but failed capture', 'We had a successful auth result, but capture failed! You may have to capture money manually! Check Eurobank Back Office!');
  

  should be:

      $module = 'commerce_eurobank';
      $key = $order->order_id;
      $language = language_default();
      $params = array();
      $from = NULL;
      $email = variable_get('site_mail');
      $send = FALSE;
      $message = drupal_mail($module, $key, $email, $language, $params, $from, $send);
  
      $message['subject'] = t('successful auth but failed capture');
      $message['body'] = t('We had a successful auth result, but capture failed! You may have to capture money manually! Check Eurobank Back Office!');
  
      // Retrieve the responsible implementation for this message.
      $system = drupal_mail_system($module, $key);
  
      // Format the message body.
      $message = $system->format($message);
  
      // Send e-mail.
      $system->mail($message);
  

  This will allow sending tranlated messages
  You could make a function just for this and just call it to keep it cleaner and make it re-usable.

That's it..correct all the above ocuring instances, set the status back to needs review and if everything is ready i will set it to RTBTC

Hello yannis.
I checked and everything is ok.
Congrats!
I set it to RTBC, you will have to wait until a git administrator provides you with full project access

Here is a final coding standards patch:
* End all comments with a full stop
* Comments should not exceed 80 chars
* Fixed indentation with switch/case
* In cases where readability is improved, use ternary assignment
* Use single quotes unless double quotes are needed
* Name of module in .info should match the project
* files[] in .info is only needed for files containing OO classes/interfaces

Once you commit this, mark the application back to RTBC and I will immediately grant you full project creation rights.

Thank you for your patience!

For future commits, please read http://drupal.org/node/52287.
For example, that one could have been Issue #1187352 by tim.plunkett: Coding standards fixes.

I've granted you full project creation rights. Use this ability carefully!