Garanti Bank Virtual POS

Hi there,

Some coding standards issues:
http://ventral.org/pareview/httpgitdrupalorgsandboxboragunesdogan1367214git

And also you should not work in the master branch, more information here: http://drupal.org/node/1127732

Also:

• README.txt is missing, see the guidelines for in-project documentation: http://drupal.org/node/447604
• Remove LICENSE.txt, it will be added by drupal.org packaging automatically.
• Remove "version" from the info file, it will be added by drupal.org packaging automatically.
• Remove "project" from the info file, it will be added by drupal.org packaging automatically.
• Remove all old CVS $Id tags, they are not needed anymore.

Changed to needs review, so that in depth reviews could be done without coding standards issues.

First, thanks for the submission. All new payment methods are always welcome to Drupal.

My comments don't take the above mentioned coding standards and styles into account. So you should run your project through the pareview again (http://ventral.org/pareview/httpgitdrupalorgsandboxboragunesdogan1367214git) after you do the changes I suggest. This way you are one step ahead of us. ;)

Generally speaking you need to comment the code more. There's no comments at all at the moment. Especially when you're dealing with third party API requirements, I'd prefer to explanations on why some things are needed. For example, I'd love to know if the User ID that's being collected is the Drupal Uid or some other. When your code is well documented, it's easy to follow and modify. You should read http://drupal.org/node/1354 for commenting guidelines.

Settings form

1. In the settings form your variables are not named properly. You only need one level and the variables (i.e. form elements) should be prefixed with your module name.
   Example:

   function garanti_bank_virtual_pos_settings_form() {
     $form['garanti_bank_virtual_pos_bank_name'] = array(
       '#type' => 'textfield',
       '#title' => t('Bank Name'),
       '#default_value' => variable_get('garanti_bank_virtual_pos_bank_name', NULL),
       '#size' => 30,
       '#weight' => -3,
       '#required' => TRUE,
     );
   

   See http://drupal.org/coding-standards#naming (Persistent variables).

2. In the same form you are gathering a password. If that really is a real password, shouldn't it be kept a bit more in secret? Consider using a password form element. See http://api.drupal.org/api/drupal/developer%21topics%21forms_api_referenc...
3. Also, I don't see why you'd need to restrict the settings field sizes unless Garanti bank's API defines those restrictions.
4. I suggest validating at least some of the input data. The ones you have the pattern for. URL at least. What about Merchant ID? User ID?

The XML-data

1. Drupal has a function called ip_address() for retrieving the IP. Use that for fetching the ip.
2. Also your code '192.168.1.1'.GetHostByName($REMOTE_ADDR) would output two ip-addresses. I don't believe you want that.
3. Basically there's nothing wrong in building the XML-data the way you're doing it, but using placeholders and str_replace is a bit redundant since you are not using the XML anywhere else. There's really no need to make the code more complicated. Just concat the data inside the XML string and it's more straightforward.
4. You're not using $instalment = 0; anywhere. Also remove the commented code if they're not valid or important for this release.

Response data

1. You are using the following three lines four times in a row
   

     $posf = strpos($result, "<". $response_tag .">");
     $posl = strpos($result, "</". $response_tag .">");
     $posf = $posf + strlen($response_tag) + 2 ;
   

   Think a more abstract way to parse the response code. That way there's less code and it's more resilient to small API changes from the bank.

2. In line 189: You should use uc_price instead of uc_currency_format.
   Example:

     $context = array(
         'revision' => 'formatted-original',
         'type' => 'amount',
       );
       $message = t('Credit card charged: !amount', array('!amount' => uc_price($amount, $context)));
   

   uc_price allows other modules to modify the amount/currency if needed.

   See http://www.ubercart.org/docs/developer/11375/price_api for more info on the context and Ubercart's test_gateway.module for the example use.

3. Starting from line 194 you're using $user->uid, but you haven't declared global $user anywhere. Also, using a global $user is not good practice at this point, since this function could also be fired from cron and then the acting user is not the one that's making the order. Thus, you should use the uid in the order.

Your module is well on its way. There are a lot of small things to be fixed, but don't be discouraged by them. You'll do fine with this.

Closing due to lack of activity. Feel free to reopen if you are still working on this application.

I have fixed some issues and cleaned the module code

https://drupal.org/sandbox/pablov2/2114353

Thanks

This project application is closed. If you want to apply with your own sandbox please open a new issue.