leopitt [leopitt]

Changing status

I think it makes more sense to put this into the existing Sagepay module (http://drupal.org/project/uc_protx_vsp_direct), so that all the Sagepay protocols are implemented in one module. This is how it's done for other Ubercart payment modules like PayPal and Authorize.net, for example.

@TR, while I can understand that it is preferable to package all modules together, I cannot see how this would be possible without changing the name of the existing module you refer to.

Sagepay "Go Server" is a distinctly different method of integration to "Direct". If my module was incorporated into http://drupal.org/project/uc_protx_vsp_direct, surely this will be confusing for people, as they will be looking for a module that handles Sagepay Server integration, not Sagepay Direct integration, which is a different method.

So the title of the existing Direct payment module would need to be changed to accommodate it. Is this possible? If so, how should I go about approaching this?

Sagepay "Go Server" is a distinctly different method of integration to "Direct"

Still, it is a method used by Sagepay. Splitting the code into two different modules could confuse users, who would download this module thinking it's the only way to process payments through Sagepay when it's not so.

Okay, so should I be getting in touch with the maintainer of http://drupal.org/project/uc_protx_vsp_direct to get the name changed before I resubmit my module as a part of it?

I'm just wandering what the procedure would be for me from this point to make it happen.

As co-maintainer of uc_protx_vsp_direct, I think merging this work into a new uc_sagepay package containing submodules for each of the supported SagePay payment methods (direct, server, and I believe "form" also exists) makes sense. Separate modules also make more sense to me as I can't really see any reason why someone would want more than one SagePay payment method enabled at once.

Any such merge is made slightly more complex by the fact we have both Drupal 5 and 6 versions of uc_protx_vsp_direct. Perhaps all new work should go into a new uc_sagepay project for Drupal 6 only, and then we can mark uc_protx_vsp_direct 6.x-1.x as "not supported", linking to the new project from the old project page?

Here's a quick review of your code while we figure out what approach we're taking with SagePay in general.

First, please ensure you use consistent indentation - your code mixes tabs and spaces. Drupal standards require two spaces, no tabs.

uc_sagepaygoserver_pg_settings_validate() is not needed. You should set #required to TRUE on the form elements that must be entered.

uc_sagepaygoserver_pg_settings_submit() is not needed. The settings form will save any variables for you if they are keyed correctly. You should define them as e.g. $form['uc_sagepaygoserver_pg_settings']['uc_sagepaygoserver_vendor_name'] instead of just ['vendor_name'].

You should not need to define both a payment method and a payment gateway. Payment gateways are used by the credit card module, when the user can enter a credit card directly on the site. SagePay Server appears to be a payment method only.

The gateway URLs should probably be hard coded into the module, letting the user select the live, test or simulator environment instead of making them find and copy the correct URL from the SagePay documentation.

The VPS Protocol variable should also be hard coded into the module, as the module will almost certainly require updates if the protocol is changed.

The store's currency code is already configured in the Ubercart store settings page. You should use the existing uc_currency_code variable instead of defining another copy.

The _uc_sagepaygoserver_cleaninput function looks a little unwieldy. Maybe you can use Drupal's own check_plain() function instead; I'm not sure exactly what this is trying to do.

When creating redirect URLs in uc_sagepaygoserver_notification you should use Drupal's url() function instead of the global $base_url.

You should also find a way of calling uc_cart_complete_sale() without having to copy the entire function at _uc_sagepaygoserver_uc_cart_complete_sale(). The missing global $user at this point would appear to imply that the automatic user login feature that Ubercart provides will not work with this method as it stands. Perhaps looking at the code of other payment modules will give you a pointer on how to achieve this - you should be able to complete the sale on the thankyou page if that is the first time that the user's browser is returned to the site after payment.

@longwave #14

Thanks for the review. I will get onto these over the next few days.

Hi,

I have not forgotten to deal with these issues, I have been unable to deal with them due to a) being away and b) having other paid client work that has needed to be dealt with.

I will get on to the issues that longwave mentioned in the next couple of days.

Thanks

Just an update to say still waiting to deal with this - should be done by the end of the week.

Perhaps if I find some people from the community to further test this module I can get it progressed

Is there anything else I should be doing to move this along?

Unfortunately I have had no time to look at this yet.

@barrysampson: SagePay Server is a payment method rather than a gateway - it should be listed at /admin/store/settings/payment/edit/methods.

@barrysampson,

Thanks for pointing this out: The Readme instructions are incorrect, I forgot to update them. I will do this shortly.

longwave is correct, the module is configured at /admin/store/settings/payment/edit/methods

Pick the Sagepay Go Server payment method. Expand the "Sagepay Go Server settings" fields.

The options contained there should be fairly self-explanatory: Choose between using Sagepay's Simulator, Test and Live modes, enter in your vendor name.

Thank you :)

I feel like I'm coming to this discussion late, but 2 weeks ago I was looking for a Sagepay Server module, didn't find anything on Drupal.org, but found Leo's post on Ubercart:

http://www.ubercart.org/forum/development/13910/sagepay_go_server_integr...

I wasn't able to contact Leo and ended up implementing my own over the last week, which has been an interesting challenge!

Leo has now been in touch and pointed me to this thread and I'm very happy to help support the Server method and contribute anything useful from my own work.

I will be testing with Leo's module and reviewing how it compares to my own work over the next couple of days.

Can I make a couple of suggestions immediately?
1. This thread should be renamed to Sagepay [Server] payment module or something similar. Just being called 'leopitt' doesn't help much and probably reduces its ranking on a search.
2. Could we rename all references to SagepayGoServer to just SagepayServer? My understanding is that the 'Go' part just refers to the package you have with Sagepay, which means you're using their all-in-one package rather than your own merchant agreement or custom package. The API method is simply 'Server' - there is nothing specifically 'Go' about the API.

While I agree that the Direct and Server methods are quite distinct and should be separate submodules, there is certainly some possibility to share a common include file of helper functions. My own module is patched together from bits of uc_protx_vsp_direct, uc_paypal and Sagepay's developers' PHP-kit, so I've certainly seen a few bits of common functionality in the uc_protx_vsp_direct code, e.g. building and posting messages to sagepay using curl.

Andy

I've found a couple of problems with running on Postgres instead of MySQL, both related to the different handling of attempts to insert data that exceeds the declared length of a varchar. MySQL silently truncates the data, but Postgres throws it up as an error. I'll avoid the argument about which of these behaviours is better, but enough to say that it currently doesn't work on Postgres (8.3).

- when installing, INSERT INTO drupal_uc_order_statuses (...) VALUES ('uc_sagepaygoserver_pending_results', ...); in .../uc_sagepaygoserver.install on line 113 exceeds varchar(32) for order_status_id. You could just get away with 'uc_sagepayserver_pending_result'.

- wherever there's a call to watchdog('uc_sagepaygoserver', ...) this exceeds the varchar(16) for the type field. I also had this problem with my module where I used the type 'uc_sagepay_server', so we can both agree to move to 'uc_sagepayserver', although this is really a fault with the watchdog() function for not handling this better.

Andy

Here is a new version of the module.

Changes:

• Renamed to uc_sagepayserver
• Removed all instances of "Sagepay Go Server" and replaced with "Sagepay Server"
• Amended .install file in order to import data from installations of uc_sagepaygoserver
• Amended readme.txt instructions

Sorry for not giving any further feedback before now.

I've been exploring a problem with my implementation of the Sagepay Server method in that emails to customer and sales admin are sent twice on order completion. This is triggered by the conditional action trigger "Customer completes checkout", which I explicitly call through uc_cart_complete_sale() in my notification callback and implicitly through drupal_goto('cart/checkout/complete') at the end of my order complete page.

Comparing this with both Leo's Sagepay code and the Paypal code, I can see that both of these effectively trigger order completion just in the order completion page - which sounds right on the surface - but I'd like to question this and hopefully get some clarification from Ubercart 'experts' on the correct behaviour.

For me, the point at which payment is confirmed in the Sagepay Server method is the notification callback. Getting to the order complete page relies on browser redirects, which is not completely reliable.

The situation with Paypal is similar, although a little more complicated: the background notification (IPN) is called asynchronously, but is much more reliable than the return to the order response page. The IPN will retry repeatedly over a long period if it cannot initially make a confirmed connection. The page redirect (PDT) back from Paypal may not happen at all: it is possible to configure Paypal so the user is automatically redirected back after payment completion, but it's also possible that Paypal is configured to allow the customer to arbitrarily close their browser and not bother returning to your site at all!

So for both Sagepay and Paypal, if the method relies on returning to the order complete page, the payment will definitely be stored in the database, but the "Customer completes checkout" trigger may not be fired and no notification emails sent to either customer or sales (plus other actions like stock decrement and order status update).

Should these be changed so that the trigger is fired in the notification callback, not the order response page? What is the Ubercart 'official' (design intention) for when the trigger should be fired?

If there's a more appropriate place on the Ubercart site to ask this sort of question, do let me know.

Andy

Hi Andy

I agree that having the order completed on the thankyou page is not ideal as it relies upon the browser being redirected, after payment has actually been taken.

I think that the reason I've gone down this root lies in uc_cart_complete_sale (see messages #14 and #21 in this thread).

uc_cart_complete_sale relies upon a global user object to function properly. When Sagepay Server notifies the Drupal site that the payment has been completed, their is no global user object due to the fact that the notification comes from Sagepay, not the customer's browser, and is therefore missing the session data. So calling uc_cart_complete_sale in response to the Sagepay notification will not work.

My original solution to this had been to create a new function which basically reproduces uc_cart_complete_sale, except without relying upon session data. This route though obviously introduces the problem of redundant code, as suggested in #14.

I'm not sure how to get around this problem. It seems similar issues have popped up in a number of different guises:

- http://drupal.org/node/658470
- http://drupal.org/files/issues/644538_OVERVIEW.TXT (http://drupal.org/node/644538)
- http://www.ubercart.org/forum/development/12856/uc_order_complete_sale_a...

In short I believe this is a problem with uc_cart_complete_sale, which was obviously never designed to be called remotely.

Suggestions much appreciated.

1. Schema descriptions are not passed through t(); see what Drupal core code does.
2. <?php
  db_query("DELETE FROM {variable} WHERE name LIKE 'uc_sagepayserver_%%'");
?>

   The code would delete the variables of any module with a name starting with uc_sagepayserver_; there could be a module called uc_sagepayserver_advanced, and the code would delete also its variables.

3. Strings used in the user interface should be in sentence case.
4. <?php
  $ch = curl_init();
  // Set cURL options
  curl_setopt($ch, CURLOPT_URL, $server);
  curl_setopt($ch, CURLOPT_HEADER, 0);
  curl_setopt($ch, CURLOPT_VERBOSE, 1);
  curl_setopt($ch, CURLOPT_POST, 1);
  curl_setopt($ch, CURLOPT_POSTFIELDS, $data);
  curl_setopt($ch, CURLOPT_RETURNTRANSFER, 1);
  curl_setopt($ch, CURLOPT_SSL_VERIFYPEER, FALSE);
  curl_setopt($ch, CURLOPT_SSL_VERIFYHOST, 1);
  curl_setopt($ch, CURLOPT_NOPROGRESS, 0);
  curl_setopt($ch, CURLOPT_FOLLOWLOCATION, 0);
  curl_setopt($ch, CURLOPT_TIMEOUT, 30);
  // Start the session and get the response
  $response = curl_exec($ch);
?>

   Why isn't the code using drupal_http_request()?

5. <?php
  $site_name = variable_get('site_name', NULL);
?>

   The default value for that variable is Drupal.

6. <?php
    db_query(
      "INSERT INTO {uc_sagepayserver_transactions} (".
        "order_id, ".
        "vendor_transaction_code, ".
        "uc_sagepayserver_transaction_id, ".
        "status, ".
        "status_detail, ".
        "security_key, ".
        "received".
      ") VALUES (".
        "'%d',".
        "'%s',".
        "'%s',".
        "'%s',".
        "'%s',".
        "'%s',".
        "'%d'".
      ");",
      array(
        $order->order_id,
        $vendor_tx_code,
        $nvp_response['VPSTxId'],
        $nvp_response['Status'],
        $nvp_response['StatusDetail'],
        $nvp_response['SecurityKey'],
        time()
      )
    );
?>

   The query could be simply written as

   <?php
    db_query(
      "INSERT INTO {uc_sagepayserver_transactions} (order_id, vendor_transaction_code, uc_sagepayserver_transaction_id, status, status_detail, security_key, received) VALUES ('%d','%s','%s','%s','%s','%s','%d')",
      array(
        $order->order_id,
        $vendor_tx_code,
        $nvp_response['VPSTxId'],
        $nvp_response['Status'],
        $nvp_response['StatusDetail'],
        $nvp_response['SecurityKey'],
        time()
      )
    );
?>

   Why isn't the code using drupal_write_record()?

7. <?php
    header('Location: ' . $nvp_response['NextURL']);
    exit();
?>

   The code should allow the other modules to exit by invoking their implementation hook_exit().

I have amended the code according to comments at #42... All except for:

changing curl to drupal_http_request - I cannot get drupal_http_request to work, but will look into this some more. I don't know at the moment if there's a difference in how drupal_http_request packages the data vs. curl, but presently Sagepay is unable to grab the required info. It could be that I'm not implementing it correctly. I'll have to do some more digging around here. The existing http://drupal.org/project/uc_protx_vsp_direct module uses curl rather than drupal_http_request. Unless one of the http://drupal.org/project/uc_protx_vsp_direct maintainers can confirm that there is a problem with using drupal_http_request, I'll have another crack at it.

also have not replaced db_query with db_write_record yet - need to understand better how to implement it.

Made a few other changes to the code not mentioned here, minor cleanups.

• Replaced curl with drupal_http_request()
• Implemented drupal_write_record() to insert row to uc_sagepayserver_transactions table
• Removed obsolete transaction_auth_no field from uc_sagepayserver_transactions hook_schema
• Implemented hook_update_N to remove transaction_auth_no field from uc_sagepayserver_transactions in previous versions of schema

Hi
Thanks for the changes made last night, looks good. I had done a similar (but as it was my first foray into a drupal module code, basic) change to work out what was going on and figured out that no data was being returned, and now I get the message in my druapl log 'No data returned from Sagepay Server notification URL', so this is much easier to debug.

I am currently testing with a Simulator account and have successfully run Direct transactions (using http://drupal.org/project/uc_protx_vsp_direct ), so I'm assuming my ip/mask are set up correctly. Server payments are turned on according to the settings on the test.sagepay site. But I am not getting any data back from them, does anyone recognise this? Any ideas?

@All: A CVS application is not a project issue queue. The purpose of a CVS application is to verify if the proposed module / theme adheres to Drupal coding standards, if the module is a duplication of exiting projects (or it duplicates the work done in existing projects), if the written code is secure code, and if the author understands the API exposed by Drupal; any reports about bugs in the code should wait until the proposed project does not have its own page.

Hi,

Does (or could) this module support the Sage Pay Authenticate and Authorise function?

http://www.sagepay.com/help/faq/what_is_authenticate_and_authorise

Many thanks

Barry

Hi

Just wondering how far this is from being released.

Thankyou for your review kiamlaluno.

marcus178, I hope to have a development / beta release up inside the next month.

Hi,

Thanks for putting the effort into creating this, I had envisaged having to make exactly this myself. On testing (in TEST mode) I encountered this:

ERROR: VPSSignature does not match the expected signature.

I dumped both variables and they were indeed different. No error is displayed to the user, they are taken back to the order review page. Every other part of the transaction is successful. Is $signature_string made up of the right variables? I'm not sure (unless something has changed with Sage Pay) why else I may get this error.

Line 231 - 237:

When I have used this before, each variable was passed through the cleanInput() function. Does this still need to be done, this is probably causing the difference...

Has this or a similar function been implemented? Anyone else with the same problem?

Edit: It magically started working, this may have been caused by the casing of the Vendor Name

Hello all, just thought I'd point out I installed this module and got the following errors, easily fixable of course :-)

Warning: Call-time pass-by-reference has been deprecated; If you would like to pass it by reference, modify the declaration of db_change_field(). If you would like to enable call-time pass-by-reference, you can set allow_call_time_pass_reference to true in your INI file in /home/fhlinux008/s/skiweb.uk.com/user/htdocs/sites/all/modules/uc_sagepayserver/uc_sagepayserver.install on line 133

Warning: Call-time pass-by-reference has been deprecated; If you would like to pass it by reference, modify the declaration of db_rename_table(). If you would like to enable call-time pass-by-reference, you can set allow_call_time_pass_reference to true in your INI file in /home/fhlinux008/s/skiweb.uk.com/user/htdocs/sites/all/modules/uc_sagepayserver/uc_sagepayserver.install on line 135

Thanks
Alun

I have created a project page for this module:

http://drupal.org/project/uc_sagepayserver