ikos [ikos]

Hello, and thank you for applying for a CVS account.

Please carefully read what reported in http://drupal.org/cvs-application/requirements about what you need to do when you need a CVS account to be a co-maintainer.
I apologize if this is not a co-maintainer application, but as you reported you want to add two module to the e-commerce project, I would guess you want to be a co-maintainer.

Actually, from talking to him it seems he wants to submit new projects. Obviously for this, we need an actual project zip to review - I am putting this back to needs work.

As per requirements, the motivation should include more than two sentences that describe the module features, and compare the proposed module with the existing solutions.

@kiamlaluno: @ikos was here in Paris for Drupal Commerce code sprint. He's doing a good job with UK payment gateways. It's not for e-commerce module. But for Drupal Commerce module. And he need to contribute his module as a contrib.

I think it's fair giving him access for checking in his code to drupal CVS.

Thanks!

@recidive: That's fine, but a CVS application is approved after the code is reviewed, and the code doesn't contain any of the issues listed in http://drupal.org/node/539608.

So the first thing is to make sure we've met the requirements of https://drupal.org/node/539608.

Very cool - subscribing and marking for review. Thanks for wanting to contribute to the community! These are some annoying hoops to jump through but you'll get through them!

I did a quick code inspection and here's what I've found:

First of all you module namespace can be only "commerce_sagepay", there's no need for it to be "commerce_payment_sagepay".

Module PHP files should start with:

<?php
// $Id$

It's missing the "// $Id$" part.

// SagePay transaction mode definitions:

Code comments should end with a full stop.

/** 
 * Implementation of hook_menu
 */

In Drupal 7 hook declaration documentation should be in the form:

/** 
 * Implements hook_menu().
 */

/**
 * Implementation of hook_redirect_form 
 **/

This should be

/**
 * Implements of hook_redirect_form().
 */

Look I removed the extra '*' from the end of the code comment

  // calculate the total value of the order to be sent to SagePay

Code comments should start with capital letter and, again, it should end with a full stop.

print_r('key is ' . $Key);

We probably don't need it here.

  $strPost = "VendorTxCode=" . $strVendorTxCode;
  $strPost .= "&Amount=" . $total['amount'];
  $strPost .= "&Currency=" . $total['currency_code'];
  $strPost .= "&Description=" . $payment_method->settings['order_description'];
  $strPost .= "&SuccessURL=" . $GLOBALS['base_url'] . '/commerce-sagepay-callback';
  $strPost .= "&FailureURL=" . $GLOBALS['base_url'] . '/commerce-sagepay-callback';
  
  $strPost .= "&CustomerName=" . $address['first_name'] . " " . $address['last_name'];
  $strPost .= "&CustomerEmail=" . $order->mail;
  $strPost .= "&BillingAddress=" . $address['organisation_name'] . "\n" . $address['thoroughfare'] . "\n" . $address['locality'] . "\n";
  $strPost .= "&BillingPostcode=" . $address['postal_code'];

Here you probably want to make this an array like this to make the code more reable:

$query = array(
  'VendorTxCode' => $strVendorTxCode,
  'Amount' => $total['amount'],
);

Then you can:

$query_string = implode('&', $query);

Or if you want this URL encoded, you can use this drupal function to do that for you:

$query_string = drupal_http_build_query($query);

Whenever possible, make use of single quotes as it's faster and easy to read.

Also there are some code commented out. You should remove all of them.

Please make sure code indentation is correctly, e.g. _commerce_sagepay_get_tokens() indentation is completely out of the whack.

In the .info file you don't need that part as it is added by drupal.org packing script:

; Information added by drupal.org packaging script on 2011-01-16
version = "7.x-1.x-dev"
core = "7.x"
project = "commerce"
datestamp = "1295136648"

  $query = array(
  'VendorTxCode' => $strVendorTxCode,
  'Amount' => $total['amount'],
  'Currency' => $total['currency_code'],

Nit: Please indent the members of the array by 2 spaces.

    watchdog('commerce_sagepay', 'Payment confirmation received for order ' . $order_id);

All of the concatenated watchdogs are done wrong - watchdog() is a strange beast that takes arguments in the same way t() does so that the messages are translatable.

function commerce_sagepay_callback() {
  
  $sagepay_callback = $_GET;
  $encrypted_response = $sagepay_callback['crypt'];
  // $enc_key = $payment_method->settings['enc_key'];
  $enc_key = '7CRqn5wrXajtNQY2';

All of these raise questions.

• Where will $payment_method come from?
• It doesn't seem very useful to assign $sagepay_callback = $_GET - it just hides from the rest of the code that you're dealing with unsanitized data. And note that it might be easier to get the extra arguments passed in the function signature - see menu_example in Examples project.
• hook_menu() said that this was going to receive arg(1)... What happened to that?

function _commerce_sagepay_get_tokens($tokenizedstring) {

  // List the possible tokens.
  $tokens = array(
  "Status",
  "StatusDetail",
  "VendorTxCode",

1. Nit: Please indent members of the array.

2. This function operates on unsanitized data. Does it know that? Could evil data presented to this callback (which is unprotected by permissions) do evil things? Should the tokenized values be sanitized? I suspect this needs review.

3. I think you might be able to make a cleaner function with strtok() or *something*. It looks like a lot of work. I'm certainly not insisting, just suggesting another possible approach.

  switch ($tokens['Status']) {

This switch statement doesn't have a default case and it certainly needs one.

Finally, I *really* encourage you to document the response format you're working with in commerce_sagepay_callback(). You'll be happy for this a thousand times over, because every time you work on the code you'll be able to look at the function and know what it is you're trying to parse. Pointers to additional information about this entire exchange would be useful as well.

Thanks again for being willing to contribute to this and your patience in jumping through these hoops. Many of the things I've mentioned are for the long-term health of the project, so don't actually have to be addressed here, but you might consider doing it anyway.

Note that not all problems need to be fixed up to get CVS, and further review and enhancement will be done before you get this in.

I think you'll be a great committer and consider this application RTBC. Thanks so much for being willing to contribute to the community.

While you're celebrating, please check all your watchdogs, as there's still at least one broken one (I think you just have a '.' instead of a ',':

        watchdog('commerce_sagepay', 'Payment callback received from SagePay for order %order_id with status code %status' . array('%order_id'=> $order_id, '%status'=>$tokens['Status']));

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.