Packages not deleted when order is deleted

A decision needs to be made here: Should the shipments and packages really be deleted if the order is deleted? My opinion is "yes", but that's not the only possibility.

If the orphaned shipment and packages get to stay, then they need to be self-sufficient without their corresponding orders. Their order_id fields need to be set to NULL on delete. They need UI support to get to them. They need to be tied to the users who placed the orders to get things like the e-mail address.

But we also have to make a hook_update_N function that looks for orphaned packages and shipments until update and delete them.

Yep. I realized this last night. It will clean up any mess already there.

I'll take this one.

Question: Do we want to make a "clean up" button that will do this, or just count on the hook_update_N and the improved code to keep it clean?

Here's a first pass, with just the hook_uc_order(), but not the hook_update_N().

This also prevents backend users from deleting orders with a shipment or package that has a tracking number, unless the user has administrative privileges or the "Unconditionally delete orders" permission.

If I'm reading the hook_update_N() docs correctly, it looks like the cleanup is going to have to be some fairly raw database operations, without using the schemas or calling any functions that have hooks in them. It will just have to drop the records from the tables, without any other niceties. That means that any other modules that would want to know about shipments or packages being deleted won't be informed here, and will have to do their own cleanup, if they are interested in that sort of thing. Does that sound correct?

Also, I'm figuring on calling it uc_shipping_update_7301(), to avoid a naming conflict with #1911278: Expose shipping data to views. Does that sound right? Will that cause problems if it is committed before uc_shipping_update_7300()?

Also, not sure this is really a bug; nothing bad actually happens if we don't delete this data.

Not from Ubercart's current perspective, but any modules that work back from packages to orders blow up if the packages are there and the orders aren't (guess how I learned that?).

Here it is with the cleanup hook uc_shipping_update_7301().

I'm not sure how efficient this sort of db_delete() is, but it works. I tried a bunch of stuff with subqueries and ->notExists(), but they didn't work at all.

I'm not sure if this requires using the batch sandbox and showing progress, etc. These could be some pretty big database operations if there are a lot of package and shipment records around.

I am not very bothered about the performance of db_delete() in hook_update_N(), it will only ever be called once during upgrades of sites that use uc_shipping.

I'm worried about Web timeouts during the update if the tables are big. I don't know if I should be worried or not. It's three separate queries, so there's a pretty obvious way to divide it up. I didn't do it because I haven't figured out the batch/sandbox thing.

However, in hook_uc_order('can_delete') surely it is more efficient to use a count query with a condition that checks for tracking numbers, rather than loop through all the results?

It's not terrible as-is, because it only loops through the shipments on that one order. However, it's probably worth improving, since this gets called once for every order displayed on the orders view.

We should also short circuit if the first query returns FALSE, as there is little point running the second query in this case.

Definitely.

I'll take a shot at all this in a couple days.

This patch incorporates the changes suggested in #10. It seems to work for my tests.

The uc_shipping_update_7301() here will have to be renamed uc_shipping_update_7300() if this is committed before #1911278: Expose shipping data to views.

@fiftyz, any luck yet?

On another note, this seems like something that should probably go into an auto-test, but I'm not sure if the hook_update_N() stuff can work that way. I guess I'll have to learn about auto-tests sooner or later.