Easy-digital-downloads: 3.0 - Determine end result of 'deleting' orders

[cklosowski]

With 3.0 we've got two methods that delete orders.

edd_delete_order

• Deletes the row from the wp_edd_orders table

edd_destroy_order

• Deletes the row from wp_edd_orders
• Deletes the rows from wp_edd_ordermeta
• Deletes any items from the wp_edd_order_items
• Deletes any items from wp_edd_order_itemmeta
• Deletes any items from wp_edd_order_adjustments
• Deletes any items from wp_edd_order_adjustmentmeta

Both of these ignore the fact that you could also have an order refund related to the order you are deleting, which in turn also has items in the 5 other tables (on top of the orders table).

The question comes in, what do we determine as 'deleting' an order?

• All of the data related to the order
• Just the order row itself, leaving the other items all in tact.

As @jjj mentioned here it's possible we should look at allowing people to 'delete' orders, but actually putting them into a 'trash' status (or something similar).

[JJJ]

Missing refunds/children isn’t intended. We will need a Walker class to help with the parent tree and deleting any affected children.

Question: What happens when you delete a customer? Do their orders also get deleted?

In WordPress, what happens when you delete a post? Do comments get deleted?

I think it’s reasonable to expect for metadata to get deleted alongside a thing, but the relational, child, or tertiary data is risky.

We went with the _destroy_ naming convention because it’s new & unused, and also because it implies that the relational goes along with it. Hulk smash. Obliterated. But ohhhh your city lies in dust, my friend.

Pippin really wants to be able to delete orders, and I do agree with his assessment that some kind of cleanup is occasionally necessary - it’s my accounting, I can ruin it if I want to. Personally, I really-really want some kind of permadeath protection on orders, like a Trash or “Deleted” status.

[cklosowski]

So with that logic...I think maybe we should look at the following approach:

1) We alter the edd_delete_order to really become a 'trash' order (penultimate to destroy). It gets put into a state that is not really 'workable' unless it's restored. With this type of state in place, I would say we only affect the order you're touching, and it's adjustments and order items. No meta is deleted b/c we're simply just setting it to a 'trashed' state. This would also leave 'refunds' as they are, without setting them to the 'trashed' state.

2) We keep 'destroy' order as the ultimate, skip trash (or empty trash), hulk smash, including all of it's meta, and child items, and the refunds itself. A 'destroy' is basically acting like the order never occurred, and therefore, the refunds as well.

We'll also have to look at the transactions table, if we're doing the ultimate 'destroy', should that also remove any 'transaction' records in the order_transactions table...I think yes.

[JJJ]

I like that thinking.

Maybe we also introduce edd_trash_% (or whatever) functions, and replace our internal delete usages? That way the delete functions remain comparable?

I think we can do this in a way that doesn’t need to reinvent the (kinda not good) way that Post Statuses work in WordPress.

I am bias towards “trash” because of WordPress and general computing things, but for eCommerce and accounting, there might be a more appropriate word, and I think that’s worth exploring before we introduce it in the 3.0 release.

[DrewAPicture]

We actually had kind of a similar debate in SellBird for handling the deletion of products. We were concerned about items randomly disappearing from orders and other places and users having trouble tracking down what happened.

Ultimately, we decided that outright deleting [products] shouldn't be possible because of the potential for so many things to break. So we went with a process of retaining the product and "discontinuing" it, so not far off from the trash status discussed above. I really think that's the best way to go.

Obviously in the case of EDD, users have a lot more control over their own own data and destiny though. So providing the API to still delete outside the UI is perfectly fine but only with grave consequences attached.

[mintplugins]

The EDD ecosystem has always allowed people to delete their orders, despite the negative consequences that almost always has in the hands of a layperson. Because it always had it before, I can see the argument for retaining that in EDD3.

The concept of "trashing" gives them the option to "undo" that action, which is a good thing. So I am in support of the trash idea. 🗑🚮

Would we expose the "destroy" option anywhere in the User Interface? Or would that be a developer-only function?

[cklosowski]

@mintplugins I'm keen to use the destroy as the Empty Trash action.

If you Delete an item it, and it's associated items are Trashed (but now I'm thinking that if you 'trash' an order it should trash the refunds as well).

If you go to 'Empty Trash' it would then prompt you that you are about to delete all orders, meta, adjustments, and any refunds.

[JJJ]

"Destroy" is developer lingo only.

It's simply the name of the function, and nomenclature we can use when talking about the computational differences between it and Delete.

I agree with @cklosowski that when you trash something, you are also trashing it's relative data.

Here's how I imagine it working:

• You're looking at a list table of "All" orders
• A row action says "Delete"
• Clicking "Delete" runs the edd_trash_order() function (to be created)

Next:

• You're looking at a list table of "Deleted" orders
• One row action says "Restore" and another says "Delete Permanently"
• Clicking "Restore" runs the edd_restore_trashed_order() function (to be created)
• Clicking "Delete Permanently" calls edd_destroy_order() and obliterates it

[cklosowski]

@jjj, so what do you propose we do with the edd_delete_order function? deprecate it and have it run trash order?

I would also mention that any time we run these functions we leave a note

For trashing and restoring it can be stored on the order record itself.

For destroying we can leave it on the customer record.

[JJJ]

deprecate it and have it run trash order?

Not exactly. edd_delete_order() still gets called inside edd_destroy_order() at the very end, because it is how the database row in the wp_edd_orders table gets removed.

But... edd_delete_purchase() needs to be updated to call edd_destroy_order() instead.

[JJJ]

UPDATE: I was wrong.

Order transactions, addresses, and adjustments all inherit the parent order status, so no status change is required. Order items are the only place where the status juggle needs to be performed.

• neither order_transactions nor order_addresses have meta tables, so there is no way to store their previous status
• neither order_adjustments nor order_addresses have a status column, so there is nothing to toggle
• order_transactions has a column that pertains only to the status of the remote transaction, not the WordPress status
• order_items have a status column and a meta table, so when trashed, they will need to juggle their statuses to ensure that the "Restore" functionality knows what status to return to (it won't always be publish or completed, it might have been a single refunded item to an order that was trashed, but is now being restored, etc...)

[cklosowski]

Important note here....if we're 'restoring' an order that was put into 'trash' we need a way to do so that won't trigger any actions (emails, license creation, subscriptions, etc)

It basically needs to be a silent status update.

[JJJ]

It basically needs to be a silent status update.

It’s things like this that are ultimately why the database-only functions are so important. Deep down in the database API, actions do get fired, but not up high.

Let’s assume someone (us) will want to trigger something when the statuses change (which is why the transition_post_status hook in WordPress is so crowded) but at least for now we can limit that exposure until we are confident with how it’s integrated.

[cklosowski]

Ok, backend seems to be working and the unit tests are passing for trashing orders, next up is building out the UI.

[cklosowski]

This is ready for some testing! Just realized I need to make an 'Empty Trash' button which will destroy all items in the trash status...but it's testable from a bulk trash/restore as well as individual trash/restore.

Also, it's working with orders that have refunds, by trashing/restoring all refund items as well, and also retaining their previous states.

[cklosowski]

Ok so I've been working with a Empty Trash, however, this is super akin to the 'Reset Store' when there are large data sets. If we do it, it's going to need to be an AJAX'd process. We still have the bulk action to 'Delete' from the Trash view.

Do we think this is enough for now?

[spencerfinnell]

Reset Store' when there are large data sets. If we do it, it's going to need to be an AJAX'd process.

If that happens it might be a good opportunity to pull out some of the batch-step AJAX processing which I think is tied to exporting/importing currently?

[JJJ]

fwiw, WordPress core doesn’t do anything fancy here at all. It’s prone to timing out a bunch, and giving 504’s, because bulk actions use $_GET on IDs and doesn’t redirect without them.

I agree we should do better, but if we don’t for now, it’s not any worse than what WordPress has had for ten years, and we can always come back and improve this when WordPress does.

[spencerfinnell]

fwiw, WordPress core doesn’t do anything fancy here at all. It’s prone to timing out a bunch, and giving 504’s, because bulk actions use $_GET on IDs and doesn’t redirect without them.

If it can happen with standard Posts I'd say it's an acceptable failure here.

[cklosowski]

My only rebuttal is that we could end up in a really really weird state if
it fails. With abandoned cart items/adjustments/meta, refunds could be
existing for sales that don't. While yes it fails for things like posts and
pages...that isn't their reports stats, etc. Basically, if it fails, we
could end up with abandoned items that are in the other tables, other than
orders. That's my biggest concern.

On Wed, May 15, 2019 at 11:32 AM Spencer Finnell [e-mail]
wrote:

fwiw, WordPress core doesn’t do anything fancy here at all. It’s prone to
timing out a bunch, and giving 504’s, because bulk actions use $_GET on IDs
and doesn’t redirect without them.

If it can happen with standard Posts I'd say it's an acceptable failure
here.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/easydigitaldownloads/easy-digital-downloads/issues/7245?email_source=notifications&email_token=AAKTMLBTUUSV7Y5XLYFYT6DPVRJL3A5CNFSM4HKXUQRKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODVPRUCQ#issuecomment-492771850,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAKTMLCHKKUPKXZMEENBSFDPVRJL3ANCNFSM4HKXUQRA
.

[mintplugins]

I've run a bunch of tests and this is looking pretty solid to me. Nice work @cklosowski.

[mintplugins]

One question that's on-topic but maybe not fully related:

When doing partial refunds, should there be a refund order that exists? I do not have one in my tests. I only get the refund order in the database when I refund the entire order.

[cklosowski]

@mintplugins The partial refunds UI isn't in this branch, and won't be working until #7233 is merged in.

[cklosowski]

Also, @mintplugins confirmed that trashed orders do not have their amounts calculated in totals for sales & earnings, but they are added back in when an order is restored, so nice work on the reports API @DrewAPicture, it just 'worked' :)

[mintplugins]

@cklosowski Do we need to test this issue with all of the functionality from #7233?

[cklosowski]

@mintplugins no, these are separate.