Module doesn't do anything when editing/deleting orders via orders admin area

Thanks! It sounds like you did a lot of work and it looks like you caught a major oversight. I want to give the patch a good review before applying it and I'm not in the middle of actively working on a project that requires this (as I suspect you are). Since you already have a patch, it looks like you're set for now and I'll try to get to this in the next few days.

Hi,

Just started reading through the patch while waiting for an upload to complete. I have a few questions:

In function uc_product_keys_order():

// Get our own copy of the order so we don't modify the one passed in.
$order = $arg1;

In PHP5, you are just copying references to an object, so the code doesn't accomplish what the comment states. You would need

$order = clone $arg1;

if you really need a copy of the order. And this is probably only a shallow copy, so it may still not be safe. In any case, I'm not sure whether you really need a copy or not, so there may not be a problem.

I would like to understand the use cases for administrator-managed orders.

My thinking is that there are three use cases: creating a new order, modifying an existing order and deleting an existing order.

If you create a new order, it should operate just like when the customer creates an order.

If you modify an existing order, then I think there are two use cases: a product is added to the order or a product is removed from the order.

Deleting an order is somewhat similar to modifying an order to removing all the products (at least, as far as my module is concerned).

Does that seem to cover everything?

For new orders and for added products to an existing order, it looks like the code executed is pretty much what the customer would get if they had purchased the products directly. In the latter case, the customer only receives product keys for the added products.

When a product is removed from an order, it looks like you are deleting the associated product key. This seems a little strange because the customer will already have received the product key and I don't see how deleting the key would get it back. My model is to never delete a product key that is assigned. This is why the _uc_product_keys_db_disassociate_keys_from_product() and _uc_product_keys_db_disassociate_keys_from_user() functions only zero out the association to the product or the user.

I noticed that you deleted these functions and (if I am reading the code properly), you are deleting the keys. I don't believe this is the correct behavior, but I'm willing to listen to your reasons for doing so. I could perhaps make this a settable option.

But back to removing products from an order and completely deleting an order, could you explain the cases where an admin would want to do this, given that you can never retrieve a product key once it's been issued?

Regarding #709868: Order is saved before checkout is complete, I encountered this behavior long ago when working on uc_addresses. The whole order process is rather complicated and "saved" may not mean what you think. Also, the different payment systems process orders in all sorts of ways. You might think you can use hook_order() for everything (once the bug is fixed), but I think you might be surprised if you do. In particular, there would probably be cases where people would receive product keys that have not been paid for ("save" does not imply payment received).

This raises another question about orders created by the admin. Should I assume that the admin-created orders are always paid for?

Related to this, someone complained because they had a COD payment type and figured out that customers could easily get all the product keys they wanted for free. I pointed out that it doesn't make sense to use COD for products delivered electronically, but it points out how the various payment systems can surprise you. So hook_order_complete() isn't perfect, but so far it seems a lot safer than hook_order(), given all the many payment modules out there.

Well, my upload has long since finished and it's time to get back to other work.

Ok, I think we may be converging on the right fix, but maybe we should talk a bit more before you try to re-do the patch. I have some other ideas that I want to run by you.

I tried to separate your changes into individual bugs, so I could get a better handle on the whole thing.

The first bug would be the inability to track a key back to its corresponding order line item. I am having a hard time seeing where this causes a problem, but you initially said this would cause "a lot of problems". Maybe if you could list some of these, I could confirm that you are correct. My thinking is that all one cares about is how many keys were sold from a particular sequence. We don't know which software product will actually get used with which key, so a hard connection is not necessary and might even be misleading.

The second bug is the failure of the code when creating orders as admin. This is definitely a problem. For a new order, we want the product key stuff to work just as for a customer-placed order. But we still have to deal with admins who add product keys to or delete product keys from an order, because we have no way of preventing them from doing so, as far as I know.

If the admin adds a product key, all we need to know is how many keys (for that product) are currently associated with the order and how many keys are listed on the order. The difference is the number added, right? And that's how many new keys we have to generate.

Actually if the number is the same or less, then there's nothing we have to do. Despite the fact that the admin removed a line item, the customer still has the key. So there are more keys pointing to the order than the order actually states. This is more an error for the admin in thinking that they can cancel a key that is already in the user's hands. Well, one good thing is that if the admin adds a new product key to the order, this algorithm would not send out another key since the assigned keys would again match the number on the order.

You can revoke a key, of course, but if the customer already activated their product, it might be too late to do any good. And if the customer buys three keys that all work for three products, it doesn't matter which one you revoke as you won't know which one the customer will use with which product. Knowing which line item on the order the key was associated with doesn't really buy you anything.

Finally, if the admin deletes the order, then we just disassociate the keys from the order because we don't want to leave a pointer to a non-existent record. Other than that, there's nothing to do.

Now, let's think about hook_order vs. hook_order_complete. What I worry about is that the Ubercart people haven't really defined all the states that an order goes through and I believe that extensions can add even more states. And then the people who write payment modules aren't always consistent about how they transition an order through the states, so I didn't feel very comfortable relying on the various states. You might know more about this than I do, but I'm just worried about getting a lot of bugs filed about various payment systems not working.

Obviously, for admin orders, hook_order_complete is not going to work. I haven't created orders as admin, but I assume the admin doesn't actually have to run through the payment code, so maybe the state transitions are more predictable. It sounds like you may have a better idea of when an admin is "done" with an order.

Maybe a simpler approach would be to just not do anything at all if an admin modifies an existing order (it would be nice if the module could add a warning to the admin order modification process if the order contained product keys). For additions, the documentation would tell admins to create a new order; for deletions, the documentation would tell the admin not to bother—that horse has already left the gate.

So all we need now is a state that tells us that this is a new order and that the admin is done messing around with it. Is this possible?

Again, let's make sure we're both convinced we all the details pinned down before launching into any recoding. Thanks for helping out with this problem.

OK, I had a window of time and I decide to try to go for a simple fix.

There are no changes to the database.

When an admin-created order transitions to the "completed" state AND the order has no associated product keys assigned, the order is handled just like a customer-placed order.

When an order is deleted, product keys are disassociated from the order (but not deleted).

That's it. If you add a product to an order after it's been completed, no product keys will be assigned. If you delete products from an order, the product keys are not affected.

I added some instructions in the README.txt file for the admin.

These changes will appear in the next developer release dated 2-18-2010 or later.