This is not a bug but more an improvement on how the module works.
Right now we are doing a md5() string for the VendorTXCode field.
I propose to take in consideration making it an integer (autoincremented primary key).
Why ? Because it is easier to look at your orders, refunds, etc by seeking a number than by looking for a 32 chars long string.

Marking this as minor. It is just an idea :)

Or maybe even better: provide the user the ability to choose between md5-ed VendorTXCode and integer-ed one.

Comments

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Could VendorTXCode just be the integer order ID? I've used this method on a non-Drupal integration and it seemed to work out okay there; as you say it's easier for the store owner to trace transactions back to their original order with this method. Maybe an optional prefix string would be useful if the store owner is sharing their Protx account between multiple sites.

andreiashu’s picture

andreiashu commented

longwave, I also saw the method you describe implemented on a non-drupal website that uses protx and indeed it works and it also looks nicer.
I also think that an optional prefix is a good idea.

hanoii’s picture

hanoii
Primary language Spanish
Location 🇦🇷UTC-3
commented

I rather not have configurable things that are not necessary (speaking of the prefix). There's also a way to easily match the sagepay tx to the ubercart as the TXID is sent in the desc of the order.

However, I also agree that having md5() might be a bit too cryptic and unnecessary, maybe something more as (like the PHP integration kit you donwload from sagepay):

VENDOR+TXNID+TSTAMP+RANDOM

I am just thinking that I need to limit the TXNID to some amount so probably used the last N numbers.

Let's leave this one open, we'll do something at some time.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Won't "store prefix string" + "integer order ID" be unique for every transaction? I can only see there being one payment per order ID, so no need for a timestamp or random data to be added to the end. KISS principle in action :)

hanoii’s picture

hanoii
Primary language Spanish
Location 🇦🇷UTC-3
commented

Maybe.

Just thinking out loud, but, store prefix string means something else to configure, not a big deal, I agree.

I usually used a local dev and the live site for development. And I don't always mirror the db unless I really need it, so sometimes I can run into conflicting txnids. I can also think that some site might be reworked in a way that the order's ID are reset but all of these are just very special cases.

Anyway, I am not that worried about this, definitely a minor issue and I'll look it up soon. Let's leave it open and see if, in the meantime, other users have anything to say about this.

andreiashu’s picture

andreiashu commented
Title: Make VendorTXCode integer » Make VendorTXCode more meaningful

Here is how they do in the osCommerce sage pay module ($oID is the order's id):

$uid = tep_create_random_value(32, 'digits');
$VendorTxCode = substr($oID . '-'. $uid, 0, 40);

To me that seems pretty sane. You can recognize the order id form the vendonTxCode but at the same time it provides uniqueness.

Opinions ?

hanoii’s picture

hanoii
Primary language Spanish
Location 🇦🇷UTC-3
commented
Status: Active » Fixed

I finally worked on this as I was running through a few tests on a new site and this one appeared to be necessary. I implemented both things, something like the osCommerce approach of #6 and an optional prefix configuration as suggested in #4. Unfortunately, this isn't included in the releases I just created today (due to a rounding bug), so it will be in the next one, or just wait for the -dev tarball to be generated. Applied to D5 and D6.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.