commerce_greek_banks

added information about other modules providing the same functionality (none exist)

Hi there,

You have some coding standards issues to take care about: http://ventral.org/pareview/httpgitdrupalorgsandboxxaristsimpouris140095...

Also it appears that you have code ine the master branch, and no 7.x-1.x branch, more information here: http://drupal.org/node/1127732

Coding issues should be taken care about before in depth review of the module.

Also all warnings have been removed.

Active branch is now 7.x-1.x and not 7.x-1.x-dev as it was before.
Master branch is empty.

changed git clone command to use 7.x-1.x-dev instead of master

The module has been updated to not empty the cart when the users cancels the payment.
Alphabank submodule has been, once again, throughly tested in every possible way (success, fail, cancellation) and is functional.

It has been tested alongside the paypal module too, to ensure that there won't be any complications on the later since it is common for greek e-commerce sites to accept paypal payments too.

Since it has been a long time since the last reply I'm marking this as major.

I do not wish to mark it as RTBC by myself, although I've already tested it and deployed it to one site, because I've helped xaris.tsimpouris with it in a few issues and one could say that I'm a co-maintainer of the project.

Promoting this to critical.

Just gave the code a quick look to help move this along. A couple recommendations:

• Coder may have missed this, but as far as I know variables being used as array keys should not have spaces around them.
  I.e. $my_var[ $key ] should be $my_var[$key]
• The use of one- or two-letter variables can be confusing when they're not $k, $v, or $i - everyone knows what those are. But variables like $t, $l, $md, and $m can be confusing and ultimately retard the process of contributing. Descriptive variable names can go a long way towards the readability of code and thus cut down on development time and debugging. Otherwise, developers might have to follow that variable around different functions to figure out what it's supposed to be.

Hi

Here is my manual review. Good work by the way.

1) commerce_greek_banks.module
your are missing some t() in your form like :
56 '#description' => 'Use the following links when you contact your bank.',

2) commerce_greek_banks.module
You have different variable '$default_text' that can no be translated.

3) commerce_alphabank/commerce_alphabank.pages.inc
watchdog('commerce_alphabank', 'Receiving new order notification for order !order_id.', array('!order_id' => $order_id));
Why are you using the ! placeholders ? instead of the @ ?

4) commerce_alphabank/commerce_alphabank.pages.inc
why are you running check_plain() on $_POST items if you never print them? Do not filter on input, filter on output only. "the golden rule is to store exactly what the user typed. When a user edits a post they created earlier, the form should contain the same things as it did when they first submitted it. This means that conversions are performed when content is output, not when saved to the database." See http://drupal.org/node/28984

5) $delta_pay_id = -1; is never user. Do you need it ?

1 and 2 are fixed in the latest code. We are looking into The rest ASAP.

check_plain() is not used against sql injections, (you are safe from them if you use prepared statements => PDO), it is used for XSS or CSRF attacks.

http://en.wikipedia.org/wiki/Prepared_statement

http://en.wikipedia.org/wiki/Cross-site_request_forgery
http://en.wikipedia.org/wiki/Cross-site_scripting

btw are you aware of http://drupal.org/project/Commerce_eurobank?
have you talked with yannis, to merge his module into yours?

Back to the review:

function commerce_greek_banks_menu() {
  $items = array();

  $items[GB_REVIEW_AND_PROCEED] = array(
    'title' => 'Redirect to Payment Bank',
    'page callback' => 'commerce_greek_banks_pay_to_be_altered',
    'page arguments' => array(),
    'access callback' => 'commerce_greek_banks_completion_access',
    'type' => MENU_CALLBACK,
  );

  $items['admin/commerce/config/greek_banks'] = array(
    'title' => 'Greek Banks Global settings',
    'description' => 'Configuration for all greek banks',
    'page callback' => 'drupal_get_form',
    'page arguments' => array('commerce_greek_banks_settings_form'),
    'access arguments' => array('access administration pages'),
    'type' => MENU_NORMAL_ITEM,
  );

  return $items;
}

/**
 * Make sure anyone can complete their orders through greek banks.
 */
function commerce_greek_banks_completion_access() {
  return TRUE;
}

Any reason why not hardcode it to TRUE?seems like a wasted function call to me, if it is intended to always be TRUE

function commerce_greek_banks_menu() {
  $items = array();

  $items[GB_REVIEW_AND_PROCEED] = array(
    'title' => 'Redirect to Payment Bank',
    'page callback' => 'commerce_greek_banks_pay_to_be_altered',
    'page arguments' => array(),
    'access callback' => TRUE,
    'type' => MENU_CALLBACK,
  );

  $items['admin/commerce/config/greek_banks'] = array(
    'title' => 'Greek Banks Global settings',
    'description' => 'Configuration for all greek banks',
    'page callback' => 'drupal_get_form',
    'page arguments' => array('commerce_greek_banks_settings_form'),
    'access arguments' => array('access administration pages'),
    'type' => MENU_NORMAL_ITEM,
  );

  return $items;
}

@rootatwc: The project of yannisc you are mentioning uses the XML API to communicate with the bank.
Our modules uses the redirection method. Those two are completely different and as we say in our documendation we do not intend to expand to the XML method (it is more expensive for the site because it requires a PCI DSS certificate which costs ~1000euros).

To quote myself from the project description:

Payment methods for as many as possible, greek banks for Drupal Commerce.

This module will deal only with redirection methods.
When using redirection your client is redirected from your website to a secure server provided by the bank on the last step of checkout. On the secure server he enters his credit-card info and the bank deals with the rest.

As you can see these are two different methods and their modules have nothing in common. We have to override URLS and send and receive POST data, validation happens on the bank server. The XML method stores credit-cards and validates them.

Our intentions are to gather all possible banks (including eurobank) that use the redirection method under one module, but only redirection.
So the idea is that there will be two eurobank modules, one that uses XML and one that uses Redirection. It will be up to the developer to decide which one fits his needs.

Back to the review, I'll let xaris.tsimpouris see into this as he is better acquainted with the code. I deal with the translations and the ease of use more and I wouldn't like to say something invalid.

i am so sorry, i should bother reading it before asking.you are perfectly clear, thanks

No problem. It actually took me quite some time to clear out all the differences myself back in 2009 when I first dealt with all the aspect of electronic-commerce and the greek banks

Hi,
I'm sorry for the delay!

We do really need more hands in the application queue and highly recommend to get a review bonus so we can come back to your application sooner.

Review:
http://drupal.org/node/1431424 Not exactly a review but I've taken part in this project application. Alas the original poster of that application never contacted me.

1. Very little problem with coding standard.
FILE: ...es/commerce_greek_banks/commerce_alphabank/commerce_alphabank.pages.inc
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
58 | ERROR | There must be no blank line following an inline comment

2. Security?
Maybe you could provide API manual in English because I have serious doubts about security!
How can you know this payment is done?
I mean by that.. You can not be sure that $_POST query came from bank, because there is no validation.
Because of that You couldn't be sure that payment is really done.

#1 by kaido: Fixed

I just couldn't stay quiet :D..
I would think about that.
1. Admin form () could be at admin.inc or some like that..

$items['admin/commerce/config/greek_banks'] = array(
    'title' => 'Greek Banks Global settings',
    'description' => 'Configuration for all greek banks',
    'page callback' => 'drupal_get_form',
    'page arguments' => array('commerce_greek_banks_settings_form'),
    'access arguments' => array('access administration pages'),
    'type' => MENU_NORMAL_ITEM,
    'file' => 'admin.inc',
  );

2. Why not?

function commerce_greek_banks_form_commerce_checkout_form_payment_alter(&$form, &$form_state, $form_id) {
  if (!isset($form['bank'])) {
    $default_text_proceed = t('Use the button below to proceed to the payment server.');
    $text = variable_get('greek_banks_proceed_text', $default_text_proceed);
    $form['help']['#markup'] = '<div class="checkout-help">' . $text . '</div>';
  }
}

3. Also I would put alfabank settings to different file and include this only if really needed.
function commerce_alphabank_commerce_payment_method_info() {
include 'alfabank_settings_form.inc';
....
These are not problems or anything.. I just think this would be nice way to handle these things ;).
But as far I see this module could be a project :).

kaido24 thank you for your suggestions, Xaris is away right but I'm pretty sure he'll see all three of them.

As for number 3, it is supposed to be loaded from the modules page in drupal if the drupal-admin enables it. It isn't loaded by default.
I'll se to it with Xaris ASAP!

Thanks

Sorry for the delay, but you have not listed any reviews of other project applications in your issue summary as strongly recommended in the application documentation.

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

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

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. Get a review bonus and we will come back to your application sooner.

manual review:

1. "define('AB_PAY_COMPLETED', 'checkout/alphabank/success');": all constants must be prefixed with your module's name to avoid name collisions. Also for "define('ORDER_SESSION_TAG', 'remember_order_id');" and others.
2. "'access arguments' => array('access administration pages'),": The administration menu callback should probably use "administer site configuration" - which implies the user can change something - rather than "access administration pages" which is about viewing but not changing configurations. Or you could of course reuse a commerce permission or create your own.
3. commerce_greek_banks_completion_access(): this function is not needed. Just set 'access callback' to TRUE in hook_menu().
4. "module_invoke_all('greek_bank_links')": if your module invokes its own hooks you should document them in a dedicated *.api.php file. See http://drupal.org/node/161085#api_php
5. "'#description' => t($text),": do not send dynamic variables to t() where possible, as this cannot be detected by automated translation extraction tools.
6. "'Redirect here after succesful payment. Type &ltfront&gt for the frontpage. Default value is "' . GB_REVIEW_AND_PROCEED . '" and is suggested.'": do not embed variables directly into translatable strings, use placeholders with t() instead.
7. commerce_greek_banks_form_commerce_checkout_form_payment_alter(): do not print user provided text without prior sanitization. See http://drupalscout.com/knowledge-base/drupal-text-filtering-cheat-sheet-... and http://drupal.org/node/28984
8. "'page callback' => 'commerce_greek_banks_pay_to_be_altered',": that page callback does not exist?

Where is the module? it isn't in the git repository

git clone is now pointing to 7.x-1.x