Disallow the deletion of an order with payments without explicit permission / approval

Yes, we should probably go ahead and do this. This only alternative would be to delete the payment transactions automatically when the order is deleted, but that sounds more dangerous than requiring transaction deletions would be inconvenient. What do you think of exceptions for user 1?

I have looked at how Drupal Commerce prevents other entities such as products to be deleted if they are referenced, and implemented a similar solution to prevent orders from being deleted when they have one or more payment transactions referencing them.

The patch from #3 looks good, but I think it should be noted that it has a small change from the *_can_delete pattern: It doesn't introduce a commerce_order_can_delete() function and relies only on the newly added hook_commerce_order_can_delete() called in the order controller.

Attached patch fixes the newline issue from commerce_payment.module.

@rszrama #1: User 1 can delete an order by mistake just like any other user. Maybe we need a new permission for this? Or an extra confirmation/warning?

If we were going to implement this, I'd prefer to use the established pattern. It's not perfect in itself, but it's better to be consistently imperfect so we can bring everything up to date at once. That said, it's not like we don't already have a confirmation form. I wonder if what we need to do is just add to the order delete confirmation that there are payments recorded for the order; and in the event that the user has access to delete orders with payments, perhaps add a button to cascade the delete operation to the transactions themselves. Just a thought - a kind of way to preserve the ability to delete orders with payments for users with permission but to make it even more useful for clearing out test orders that happen to have test payment transactions.

Agree with Ryan on #6. I have a live site with a bunch of orphaned payments from deleted test orders. It is stuffing up reports - very annoying.

Agree with @amateescu #5. User 1 may not understand how deleting orders would result in future issues with orphaned payments. Strong confirmation/warning highly recommend. Perhaps with a link to documentation on proper procedure?

Documentation on transitioning from a test site with authorized test payments to a live site would be really helpful for those of us less immersed in Commerce.

Hi all, Is there a way of deleting orphaned payments? I have a few that are appearing in reports from testing phase and can't seem to find a way of deleting them.

Is using the API out of bounds for you? If not, I'd just write a script that uses commerce_payment_delete() to find and get rid of them. Need an assist on that?

Hi Ryan, thanks for your reply. Yes writing something to do this is way out of my scope. As a stop gap workaround I've cleared the tables in my database for commerce_payment_transaction and commerce_payment_transaction_revision.

It would be very helpful if you could help with a script that I could use during testing phase of the site so that I can remove any orphaned payments before handover.

Going forward, what is best practice not to have any orphaned payments appear in the system? Should I delete the payment transactions before the order?

Many thanks.

What is the status of this issue and patch? Will it be rolled into one of the branches?

I've re-rolled the patch in #4 to apply cleanly to 1.8.

agree, payment transaction should be deleted automatically when delete the order

This in combination with http://ryanszrama.com/blog/05-12-2014/beware-innodbs-autoincrement-reset... can cause issues during checkout when a new order is created that has an existing payment transaction.

For example, yesterday I had an issue where a customer couldn't checkout because there was already a payment transaction on the order because the site admin had deleted a test order with the same order id.

The latest patch in #14 looks pretty good, although I'm not sold on the delete routine in the entity class. Might be a more elegant way to approach that and we should probably add some way to notify the user that (and why) their order deletion failed.

I shortly glanced at the patch from #14 and it looks good, though it needs some work because of odd indentation.

But also it seems to permanently lock orders with referenced payment transactions, couldn't we make this optional through a setting? I came here via #1816370: Deleting an order with commerce_order_delete() does not delete the related payment transactions because of orphan payment transactions appearing on new orders (all within a dev environment). If a setting is added my guess is that on order deletion:

1. the order deletion should be prevented or
2. the order's referencing payment transactions should also be deleted.

Also see Clean up test payment transaction from drupal commerce site at Stack Exchange.

NB: Just to be complete, #11 states that the name of the function to delete payment transactions is commerce_payment_delete(), but this should probably be commerce_payment_transaction_delete().

Where are we on this? I am still coming across orphaned payment transactions on my system causing a few minor headaches for my clients. I would slightly prefer the approach of not allowing an order deletion if there are payment transactions against it.

So, coming back to this, I actually think what's needed here is something more in line with my comments in #6 and lmeur's in #17. The reason we prevent deletion of ordered products is because that actually ruins the integrity of historical order data. There's literally no way around that, and it will result in fatal errors, so it makes sense to prevent deletion at the API level.

However, with payments, the issue isn't about data integrity. You won't get any fatal errors or anything - you'll just have leftover data hanging around in the database. It's annoying but not site-breaking, though it does cause funny issues when test orders are deleted and Innodb resets its auto_increment counter as Andy pointed out.

In any event, here's what I believe the "best of both worlds" approach should be:

1. Add an API function called commerce_payment_order_has_payments() to commerce_payment.module using the same arguments / contents as commerce_payment_commerce_order_can_delete() in the patch. This will give other functions a quick way to check if an order has payments.
2. Alter the order delete form from commerce_payment.module to see if an order has payments. If so and the user does not have access to administer payments or delete payments, disable the submit button and change ht message to indicate, "This order has payment transactions attached to it. You do not have sufficient permissions to delete it."
3. If the user does have access to administer or delete payments, then a requird checkbox should be shown titled, "Confirm deletion of the order and all of its related payment transactions." Use the form states system (see examples in Commerce code by grepping for #states) to disable the submit button until that checkbox is checked.
4. The alter hook needs to add an additional submit handler to the delete form that then checks the $form_state['values'] array for that checkbox and deletes all payments attached to the order as well.

I couldn't believe it when i figured out this was why none of my payment methods would kick in. commerce_payment_order_balance was almost always 0 or negative because we deleted all old test orders (hundreds? thousands?), started testing again and the old payments were still in there!!! i wasted a solid 10-12 hours trying to figure out why the payment methods wouldn't get triggered because of this. for now, i'm just adding my own hook_commerce_order_delete implementation, because it's fine for our use case to always delete transaction data if the orders are deleted. i honestly don't know in what world you would want the transaction data if you don't have the order data, but what do i know... * sigh *

Feel free to submit a patch per my comments in #20 if you have the time. ; )

Here's a patch that follows sir Ryan's guidance in #20 except that the payments are deleted during hook_commerce_order_delete() instead of adding a submit handler to the UI form. This makes sure that if the order is deleted elsewhere (ie: rules) the payments will go with it.

I kept the hook_commerce_order_can_delete() even though it's not implemented because I think it's good to have and I'll be implementing it on a few projects!

Wow cool story... I uploaded the wrong patch in #24 then deleted the folder that contained the correct one. Good thing I was using PHPstorm and was able undo the folder deletion! Here's the correct patch!

The patch in #26 worked for me on the latest dev.

When I didn't have "Administer payments" permission and tried to delete an order that had payments. I was informed that I could not delete the order because I didn't have permission.

When I gave that account "Administer payments" permission, I was able to delete the order and confirmed that the payment details were also removed from the Database.

Thanks

I can confirm that patch #26 works, transaction data is deleted along with orders.

Patch #26 works. Tested for users with and without the "Administer payments" permission.

Thanks everyone for the code work and the various reviews. Very helpful in green lighting this. Confirmed and committed!