Support refunds

I'm just looking through the docs at SagePay (V2.23). It seems that in case the response is successful (contains 'status' => OK) we should store all the relevant information into the database for later use: VendorTxCode, VPSTxId, SecurityKey, TxAuthNo.
Some of these fields will be required for the refunds functionality while others are just good to keep.

I've still got some 50+ pages to read tough.

So basically we will need to create a protx/sagepay table that will record all payment information received from sagepay.
We will also need a protx_refunds table that has the following fields: order_id, amount, reason.

Question: are we going to need a hook_update_N ?

Any thoughts, suggestions, questions, etc are welcomed.

EDIT: also some suggestions for table names will be welcomed :)

Cheers

While writing the patch I just found out that they got the URLs wrong in their manual :(
Eg. https://test.sagepay.com/Simulator/VSPServerGateway.asp?Service=Vendoref...
INSTEAD of https://test.sagepay.com/Simulator/VSPServerGateway.asp?Service=VendorRe...
Amazing... at this level I wouldn't thought it is possible...

Hi andreiashu,

A few comments on this one. About creating a table, I first thought it the same way as you, but the previous maintainer of this module pointed me that there was no need to have a table of our own because everything was being stored in the order's admin comment queue. Back then it wasn't really my decision but now that I got to work it that way I guess we can stick to it, because it makes sense and somehow ease the development of the project not maintaining a schema structure.

Said this, all the information you need to add support to refund should be on the comment db of ubercart. The only caveat is that you'll have to parse it. I already added support for an AUTHORIZE/AUTHENTICATE scheme which the process is similar to the REFUND in terms of protocol.

So for instance you can look at the _uc_protx_vsp_direct_comments_to_protx() function, which basically strip most of the information you need to do any further action of the transactions.

Beside all this, I am interested in knowing how you plan on doing the user interface of the refund, how will it work, is there in ubercart a place where refund transactions can occur (in some other gateway), etc.

Thanks for working into this, and, would you be able to backport this to the D5 branch if we got it properly working?
a.=

Hi hanoii,

About the table creation: I think the current approach (parsing order comments) is not a long term solution. We are doing a lot of preg_matches which kills performance. I think that the comments table is for comments, not for storing sensitive data about transactions to ProtX (or Sage :) ).
Having our own tables does indeed imply some more work on the developing side but also provides a lot of flexibility in the long term. For example: SagePay call you (doesn't really matter the reason) regarding one of the transactions received from your system. How can you get more info from your system about that particular transaction ? I don't think you can just search/preg_match on all the comments table. Probably there would be many more reasons why your suggested approach won't be good in the long term, but for now that is all I can think of.
I can help on cleaning the code, but this will be in a different issue if you are oki with this :)

About the interface: unfortunately Ubercart 2 doesn't have support for refunds (more info here). So my approach was to add a local task at 'admin/store/orders/[order_id]/refunds'. Also all the refunding actions are logged at 'admin/store/orders/17/log'.

About backporting: I not really sure if I'll have time to backport this. For now I just want to focus on bringing the 6.x version to a nice form.

I attached a patch with what I think would be a nice approach. Please have a look and state your opinions.
I had to copy-paste a lot of code from the main module, but if this gets in I will clean the code in both files in a followup issue.

Thanks,
Andrei

Setting the proper status.

Edit: I forgot to mention that the patch is missing a hook_update_N. This is because I don't not exactly what should N be in our case (we have a dev version).
So to test it, just apply the patch, uninstall the module (you'll get some errors but it is normal) and install it again.

While I agree with some of your comments, I think this is, at least for now, a matter of personal preference. If SagePay calls and ask you for any information regarding that order, you can easily see the admin's comment of the order (only available to admin) and give any information about that in an interface that's already built in in ubercart (no preg_match done there) and the user is very much used to. By going through to what's proposed in this patch in terms of the new table, more work is needed. FIrst you need to create the interface that will show the store's admin all the information store on that table, otherwise he never will be able to provide with that info (unless he's a developer and now to access the db), there's also work around the authenticate/authorize scheme in which you would have to also use the database.

About the performance issue, I am not worried about that. A few preg_match() hits on refunds or authenticate/authorize transactions won't kill the site. I am even wondering what's faster, a db_query() call or a preg_match on a small string.

Anyway, I think this is a major change, something I am not eager to do at the moment unless I really feel is necessary and not because of personal preferences. Also I would like to hear other voices about this but not to do it now.

Said this, I agree that we should continue to discuss this on a different issue, and I am interest in seeing a refund feature come into the module and I really appreciate what you are doing. I'd like to keep the patch short in order to be easily reviewed and tested and for that we need to work in the way the module is working right now, and not reworking it completely at the same time with a new database schema, making it too many things to test.

To share thoughts about the interface, I have to give it a thought and a look. What I did with the authorize/authenticate was to add a SagePay pane to the order's admin view with an authorize button. Very much alike to the process credit card one from ubercart. I would think this is not a bad idea either to the refund, although a new screen with SagePay commands might something as well.

I will have to give this a proper thought when I have some time.
Thanks,
a.=

Hi,
Thanks for your evaluation.
Indeed it might be only a personal preference creating a new table for the transaction history.
But I think at least we will need a table for the refunds history (or you think we should store those only in the comments as well ?).

About the interface: indeed, we should give the possibility to implement other SagePay features in the future. I'm not really sure how to do all of this in ubercart... I'm still new (just started playing this week with it). So I'll need some advices/guiding.

About the performance of preg_match/db_query: in your function you loop though DB results (so db_query) + preg_match a lot against each result. So the "SELECT this transaction from protx_transaction_table BY primary Key" is going to be faster afaik. Same thing applies for when you will want to compute the SUM of previous refunds.

Anyway, which direction you choose to take (hopefully a good one) I'm willing to help. :)