Order hook event

[JDGrimes]

The Order Complete points hook got pulled at the last minute before 1.0.0 was released, mostly due to #12 (see #13). Now, with the advent of the new hooks API, we should introduce this as a points hook. We don't have to worry about the HTGP shortcode right now, because we can just go ahead and have it award a static amount by default. The dynamic features can be added later. See https://github.com/WordPoints/dynamic-points

[JDGrimes]

Previously, we were using the oocommerce_order_status_completed action. However, there is also woocommerce_payment_complete. The difference is that the latter fires when the payment is complete, generally coinciding with the order being marked as processing (if physical goods). The order then only gets marked as complete (original action) after it has been processed. I can imagine that some folks might want it to work one way, while others would want it to work another. Really it makes little difference for digital goods though, only physical goods. I guess maybe we ought to just have a separate event for payment complete if some people want that. I'm also not entirely certain of the reliability of the payment complete action, due to the surrounding logic. But I guess we can investigate that further if we pursue a separate event.

[JDGrimes]

Working on the attributes for the order entity, one of the first is the parent_id. It seems that this is only used for refunds though, which are apparently saved as a child of the parent order. So as far as the basic order entity goes, I don't think that adding the parent is important at this point.

[JDGrimes]

The currency attribute would really benefit from https://github.com/WordPoints/wordpoints/issues/589, since the currencies are stored in three-letter currency code format. Users should be able to select from a dropdown based on get_woocommerce_currencies(), since they won't know what all of the currency codes are.

[JDGrimes]

WooCommerce stores the date_created for an order in Unix timestamp format, so we've reserved the unix_timestamp data type.

[JDGrimes]

There are a bunch of attributes, like the totals and tax amounts, that are decimal numbers (floats). Currently we only have the integer data type as far as numbers go, so I suppose that we need to introduce the decimal_number data type.

[JDGrimes]

Most all of the important attributes, however, are actually stored as post metadata, and not as fields themselves (since orders are stored as a custom post type). So need to introduce the concept of "foreign attributes" that we've discussed before, I guess mainly in the context of the Hooks API module, although I found one reference here: https://github.com/WordPoints/wordpoints/issues/541#issuecomment-253989077.

As far as I can see, there is nothing preventing us from introducing the concept of foreign attributes right now, and no changes needed in core. Anything using the storage info API would need to take note of it, but nothing is at the moment (as far as I know). Anyway, it is as simple as just slightly modifying the syntax already in place for "foreign" relationships. So long as we note the syntax in the docs, we should be OK. Ideally, we should also note this in WordPoints's repo/change log somehow or something. We'll probably want to introduce a base bootstrap class for handling this kind of entity attribute anyway, so that would provide an opportunity.

[JDGrimes]

Even though we've introduced the base class for meta-type attributes upstream, I'm not sure whether we should extend it yet. If we do, we'll have to require WordPoints 2.3.0. Which I guess just means that we have to wait for WordPoints 2.3.0 to be released before we can release. However, I suppose that we already want to get that out ASAP for the sake of the BuddyPress module, so this shouldn't be a problem. I guess the main reason to hold back would be because we don't have the dependency/compatibility API in place yet to protect users from updating one thing without the other. But really, we just need to fix that. So let's plan on requiring 2.3.0 for now, I guess.

[JDGrimes]

We're also requiring WooCommerce 2.7 now, unless we fallback to get the entity attribute values in older versions.

[JDGrimes]

I'm not going to pursue the billing/shipping address things right now, because they are stored in arrays.

The order_key is just use in URLs for validation, I think.

The payment method would be nice to do, but it needs to be enumerable. Actually, that should probably be a relationship anyway, not an attribute. So maybe we could pursue that.

[JDGrimes]

The cart_hash is just and MD5 hash of the carts contents, we don't need that either.

[JDGrimes]

I don't think we need to worry about the custom IP address and user agent either.

[JDGrimes]

In the process, we also need to introduce an entity restriction for the order entity. Basically though, all orders are restricted, that is, they are never publicly viewable by everyone. I don't think we've come across that before exactly, and I was just wondering exactly what we should name it. Kind of setting the precedent here. Should we go ahead an just use nonpublic, or something more generic like default? I guess that it is good to be as explicit as possible. So nonpublic might be preferable. However, it isn't just a restriction for nonpublic orders, it is a restriction for all orders. For the posts, they may be nonpublic based on status, and so for them we call it status_nonpublic. In this case though, it isn't based on status, it is just all the time, so I guess going with just nonpublic makes sense (since it is unqualified).

[JDGrimes]

We should probably also reverse the event when the order is fully refunded. Actually, I guess this brings up an issue with the dynamic points concept, and that is that it makes it possible to have "partial reversals", if you will. The dynamic value has changed, rather than the event itself being reversed. This is similar to when a condition is no longer met. Currently we don't handle that, see https://github.com/WordPoints/hooks-api/issues/116#issuecomment-200555484. However, this is more specific to this particular application/extension/reactor. It isn't specific to WooCommerce thought, so we don't need to worry about it here, until an API is worked out. It's probably best to figure out how to handle that generically upstream, and then we can come back and create a ticket for it here.

Aside from that though, I suppose that we do need to handle total refunds. Maybe even partial refunds should reverse this. Or should refunds reverse this at all? Do they really reverse the event? That is somewhat in the eyes of the beholder, but arguably it doesn't change the fact that an order is completed, which is what this event is about. Unless the order status changes (does it automatically even? maybe we don't have to worry about this). If we were hooking into payment complete, that would make more sense. I guess we may need to partly just consider what the majority use-case is going to be and plan for that. (Though it might make more sense to use payment completed then. Maybe we'll even have to give users an option here.)

[JDGrimes]

In WC_Ajax::refund_line_items(), the order status is changed to 'refunded' if the total amount has been refunded. Otherwise, the 'woocommerce_order_partially_refunded' action is just called instead. So this will automatically reverse for full refunds, if we just reverse on status change as planned.

[JDGrimes]

I was having some trouble in relation to the reversals, and one part of it was caused by the order arg for the reversal action not having the value set on the order entity. The value was there, but when set_value() was called it wouldn't end up actually getting set on the entity. This was because the wc_get_order() function ultimately ends up checking some things in relation to the order, like its post type, to determine what class to instantiate with the ID. But these checks fail because the order isn't in the database anymore.

Actually, they were failing because we hook directly to delete_post without ensuring that it is actually an order getting deleted. The products in the order were being deleted first, and as a result of that, the delete_post action was being called with a product post ID, instead of an order post ID. When trying to retrieve the order with wc_get_order(), it would detect that this wasn't an order post type, and fail. As a result, the value of the entity would end up being not set, as pointed out above.

This was not affecting the points reactor, only the legacy points reactor. The reason being that the legacy reactor performs a query on the points logs when the regular check of the hook hit logs fails (as it does in this case, since the arg values were set when it originally fired). That query skips the meta query part entirely since there is no legacy meta key set for the reaction, since it isn't actually an imported reaction. And so all unreversed logs of that type are detected and reversed.

This brings up the question as to why exactly we are testing the legacy reactor against event types that it shouldn't be tested against in the first place. But I suppose it did save us here, because it uncovered a bug that the regular reactor didn't seem to have any trouble with. Then again, that was because the regular reactor was still performing the query involving the GUID of the entity, which though empty, wouldn't match anything in the database. So you might say this is a sanity check missing in the legacy reactor.

[JDGrimes]

All in all, it is probably better just to fire the action only for the correct post type to start with, as we do for the action when registered for the other post types, by using WordPoints_Hook_Action_Post_Type. Let's try using that here as well.

[JDGrimes]

The problem with trying to use WordPoints_Hook_Action_Post_Type is that when it calls $post_entity->get_the_attr_value( 'post_type' ) it will get null, because the WC_Order class doesn't expose post-like attributes in any way. So I guess it would be better to just do a custom action object.

[JDGrimes]

That takes care of that, but then there is an issue with the status change reversals doing something similar. I'm not sure what the difference is, but for the status change reversals the value of the entity is properly set but the 'legacy_meta_key' reaction setting isn't set. That means that when no matching hits are found in the database, the logs are consulted and the meta query part is skipped so all the unreversed ones are returned. Setting the legacy_meta_key reaction setting fixes it.

I guess actually a part of the problem here is that no hook hits are being found, because really they should be, there is one that is supposed to get reversed in this case. But for some reason it finds no hits matching, and so both of the hits are reversed.

[JDGrimes]

This is actually caused by the exception that we throw from in wp_die() getting caught by the WooCommerce Ajax callback and interpreted as an error, which will cause it to delete the order in question. This is apparently causing the second log to be reversed, though I'm not sure why since the order isn't for that log.

[JDGrimes]

Well, the reason is that by that time the other hit has been reversed, and so the legacy extension is checking the logs without specifying the order ID in the meta query, as noted above, which will grab the other log which is unreversed.

This again demonstrates the hazards of the legacy extension, and possibly some sanity logic should be added here. However, it is good for us to have realized that we were also triggering deletion of the order Actually, it isn't the order itself that is being deleted, but the refund that is being created. Still I guess this brings up the issue of what we should do when a refund is reversed. But in this case it is actually just being aborted, not reversed.

[JDGrimes]

Now that that is figured out, it is back to getting the rest of the entities set up properly. I just realized that the date_created property/field/meta for the order is actually stored in the post_date field (and the date_modified field is stored in the post_modified field, although we hadn't really even done anything with that). The reason this is a bit of an oddity is that in the database the post_date field is a MySQL DATETIME field, whereas it seems that the date_created for an order is presented as a Unix timestamp. So I guess what we'd need to do is convert the attribute value that we return to the same format as is in the database.

[JDGrimes]

I was going to add an entity attribute for the total_tax order property, but it seems that this property is only a convenience dynamically calculated at run-time. It isn't actually stored in the database, the cart_tax (_order_tax) and shipping_tax (_order_shipping_tax) are only stored separately, no total. So I guess we can't represent that as an attribute. Someday folks might be able to use calculations with conditions, etc., though, and then they would be able to add the shipping tax and regular tax together if they needed to. (Or perhaps our query API will expand to support something like this eventually.)

[JDGrimes]

Note that likewise we only have the grand total available, the cart total is calculated dynamically.

[JDGrimes]

Remaining items include the billing and shipping address info.

Above we said (https://github.com/WordPoints/woocommerce/issues/23#issuecomment-277458853):

I'm not going to pursue the billing/shipping address things right now, because they are stored in arrays.

However, in the database each element is actually stored in its own meta key. So we could pursue this here if we wanted to.

For each of these their is the two address lines, the city, the company, the country, the first and last names, the state, and the postal code. The billing address also includes an email and phone number. There is also an index for each which I guess is all of these combined.

I guess there is really nothing stopping us from implementing these, but some of them (phone and email) should be special data types (and aren't that useful otherwise). The rest should possibly be data types too. Like the city, even though it is just a string, also has geographical coordinates relating to it (not in the code or the database, but in a literal sense). At some point we might want to introduce custom conditions for that data type, like "is within distance" of some other location. We could probably just change the data type then though.

However, all things considered, I think we shouldn't try to add these right now, we can do it in the next release if we want.

[JDGrimes]

There are also the boolean attribute_prices_include_tax, which I'm not sure is much use.

The only other thing is the so-called discount_tax (or _cart_discount_tax), which is the amount that was saved on taxes because of the discounts applied to the cart. I suppose maybe we could go ahead and do that.

[JDGrimes]

Note also that orders are password protected, since WooCommerce generates a key to fill the password field. However, it doesn't appear that the post_content field is really used, or at least we don't have an entity for it, so there doesn't seem to be much point for us to add an entity restriction based on this. Besides, we already restrict the entire entity, so restricting the individual attributes further is not really necessary (unless they are restricted from users who usually can view the order).