Backport the "placed" timestamp from 2.x

Hi @jsacksick,

• I think "placed" is the best, in line with "changed" and "created".
• I would also agree that it's best using the hook instead of Rules.
• I think that if this is done, it should be properly corrected i.e. "created" value should be time created. There should be though a special mention in the Release Notes so that people can update their code or Views that may assume that "created" holds time placed. But more people should vote on that matter ...
• Upgrading should be done, but better do it in MySQL rather than in PHP i.e. don't load all order rows, copy value from "created" to "placed", and save them. Instead issue an UPDATE `commerce_order` SET `placed` = `changed` and that should create no issues at all.

I'm just concerned that some installs might not be using the created timestamp the way we assume they are

That's a good point, it has to be optional then. Maybe the best would be to provide a script to manually upgrade and mention it in the release notes. It may be an option to ask a Yes/No question during the installation to run the script, but we would need a UI as well for those upgrading via the browser.
If people think it's not worth the hassle, then I guess we don't do it at all and leave it up to the user.

but we would need a UI as well for those upgrading via the browser.

In Commerce Reports there's a batch operation for generating tax records. The big question, then, is where do we want to show this and then possibly hide it, if ran. My only wonder is if we can provide a Drush script which piggybacks on the same batch operation. Which someone wrote a script, I think, for Commerce Reports to do the taxes that way.

All for

1) Setting it in hook, no need for a new rules action. This should be as concrete as when the created and updated timestamps are set.
2) Provide batch operation UI/Drush to allow people to populate field if they choose for old orders.

Per discussions with jsacksick, we are missing tests for this field. And then the no-Rules bit.

+++ b/modules/checkout/commerce_checkout.rules_defaults.inc
@@ -35,6 +35,22 @@ function commerce_checkout_default_rules_configuration() {
+  $rule = rules_reaction_rule();
+  $rule->label = t('Set the order placed date to the checkout completion date');
+  $rule->tags = array('Commerce Checkout');
+  $rule->active = variable_get('enable_commerce_checkout_order_placed_date_update', TRUE);
+
+  $rule
+    ->event('commerce_checkout_complete')
+    ->action('data_set', array(
+      'data:select' => 'commerce-order:placed',
+      'value:select' => 'site:current-date',
+    ));
+
+  $rule->weight = -10;
+
+  $rules['commerce_checkout_order_placed_date_update'] = $rule;
+

Remove from Rules. Just do it in the main process manually.

Improved patch attached.

Changes compared to previous patch

• Removed the Rule for setting the "placed" property.
• Set the "placed" property via hook upon checkout completion.
• Copy the value of the "created" property to the "placed" property for existing records in the database in the update function.
• Updated the order form to allow editing the "placed" property.
• Added token support for the "placed" property.
• Added a Placed column in the commerce_user_orders View, some changes were made in the existing patch but they were incomplete. Removed the Created column.
• Added a Placed column in the commerce_orders View, some changes were made in the existing patch but they were incomplete.
• The variable 'enable_commerce_checkout_order_created_date_update' now defaults to FALSE so that the "created" property contains the creation time in new installations. Backwards compatibility strategy explained below.

Upgrade strategy

Reasoning

I've given a bit more though on how to maintain backward compatibility so that we don't break existing installations, and how to handle the database upgrade. I think the best is to do the database upgrade i.e. copying the values from "created" to "placed" column for existing orders on all sites. This is for the following reasons.

• The majority of sites probably keep the default behavior that sets the "created" property to the time the checkout was completed. We won't change that behavior for existing sites so that any Views or code that rely on the "created" property as the checkout completion time will still work. The values that we will copy over to the "placed" property will be correct as well. If they use the default Views, the "placed" property will be identical to the "created" property so there's no change on data displayed to users either.
• Some sites may have disabled the rule that sets the "created" property to the checkout completion time. Those sites will be storing the creation time in the "created" property and they would not be storing the checkout completion time at all, unless they have their own way of doing it which is not something that would interfere with the current patch. There is not much of a consequence for those sites either as they can simply ignore the new property and not use it or display it anywhere and all remains the same. The only change would be if they use the default orders Views, in which case the user would falsely see the creation time instead of the checkout completion time for existing orders, but even that is better than nothing. If the site owners do not want to display the checkout completion time, they can simply customize the default Views. It is extremely unlikely that there will be any site storing anything else other than the creation time in the "created" property if they have disabled the rule, and if they do so it is their own responsibility because it is not its intended usage and falls outside the scope of the Commerce module.
• Since we are introducing this new property, and it already exists in 2.x, moving forward we will be relying on this column. Sites would need to adjust to it and I don't see any use case where they wouldn't want to.

Implementation details

• We copy the values of the "created" column to the "placed" column for all upgrades.
• The new "placed" column is set to the checkout completion time when the checkout completes, on all sites.
• We keep the "enable_commerce_checkout_order_created_date_update" variable and the corresponding Rule for backwards compatibility, but we use a FALSE value as the default so that the "created" property is the creation time in new installations.
• In the update function, if the variable is not set we set it to TRUE which is the current default so that we don't change the behavior in current sites. If it is set, we leave it as is for the same reson.
• We make a mention in the Release Notes so that site administrators can manually do the change when they want.

Let me know what you think about all this.

Remaining tasks

Tests, I'll have a look into it.

Proposed release notes

Something along the lines:

Currently the "commerce_checkout_order_created_date_update" Rule (enabled by default) and the corresponding "enable_commerce_checkout_order_created_date_update" variable, update the "created" property to hold the checkout completion time. This behavior is now deprecated and a new order property is introduced in this release for this purpose. The value for the new column will be automatically populated during the upgrade for existing orders so that it holds the creation time or checkout completion time, depending on whether the site had that Rule enabled.

Sites that have the Rule enabled do not have to do anything, but it is recommended that they update any code or Views using the "created" property as the checkout completion time to use the new "placed" property and disable the Rule and set the variable to FALSE.

Sites that have the Rule disabled should be aware that the new "placed" property will be holding the order creation time for orders created prior to this update, rather than nothing. If that is an issue, they can maintain the current behavior by simply not use the new "placed" property in their code and Views and override the default Views provided by the module if they currently use them.

For more information about the changes follow the corresponding issue: https://www.drupal.org/node/2888407

+++ b/modules/order/commerce_order.module
@@ -341,6 +341,14 @@ function commerce_order_commerce_checkout_pane_info() {
+  $order->placed = REQUEST_TIME;
+  commerce_order_save($order);

Wait, do we need to save here? Shouldn't invoker call the save?

Wait, do we need to save here? Shouldn't invoker call the save?

You're right, I'll remove the hook implementation and I'll do the following. Tested, it works.

function commerce_checkout_complete($order) {
  rules_invoke_all('commerce_checkout_complete', $order);
  $order->placed = REQUEST_TIME;
}

@jsacksick I agree about the query. I'm confused with what you mean with the rest.

The meaning of "The new "placed" column is unconditionally set to the checkout completion time on all sites." refers to setting it from now on, it does not refer to updating existing orders. We do know when checkout is completed for an order when it happens, what is the information that we don't have? I highlighted this to point out the difference with the previous patch that introduced a variable to control the behavior, and which is TRUE by default and would do exactly the same unless you would explicitly set the variable to FALSE. I've amended the comment to make clear what I mean.

Do you mean "I think it shouldn't be set before" or "I think it should be set before"? Because I do have it after ...

Actually taking into account the order status for existing orders when doing the database upgrade (which should be the case) makes things more complicated ... It should be done not only for the "completed" status, but also for "checkout_complete", "pending", "processing", "completed" and there could be many more statuses introduced by custom or contrib modules, such as the ones added by Commerce POS.

This might be a reason to consider again the solution to ask the users to do it via the admin UI, allowing them to choose which order statuses to consider as having gone past the "checkout_complete" phase.

In line with the comments above, I'm posting a new patch.

Changes compared to #8

• Do not populate the values for the new property automatically during module upgrade. Rather provide the option to do it manually in the order settings page. Site admins can choose which order statuses they consider to be past the checkout completion stage and update only orders in those statuses. Batch API is used for executing the operations.
• Do not set the "placed" property via hook, do it in commerce_checkout_complete() instead.

Remaining tasks

• Fix failing tests, related to the order edit form.
• Write tests for the new field.
• Update release notes

Questions

The ability exists for site admins to simulate the checkout process for an order. This can happen for orders that have already completed checkout, such as making a product change, or applying a discount. In these cases, do we want the checkout completion time to be updated as well? Or do we want to keep the first time that the order was "placed"? How is this handled in Commerce 2?

I'd vote for the second with the ability to change this behavior using a variable.

Changes compared to #14

• Fixed broken tests
• Added test for the order:placed token
• Moved the form fieldset that allowed admins to populate the new "placed" property for orders prior to the upgrade into a new page called Module Upgrades. Done in a way that enables other modules can do the same in the future so that they can add optional upgrade actions.

Remaining Tasks

• Write more tests for the new field
• Update release notes

Remaining Questions

• The ability exists for site admins to simulate the checkout process for an order. This can happen for orders that have already completed checkout, such as making a product change, or applying a discount. In these cases, do we want the checkout completion time to be updated as well? Or do we want to keep the first time that the order was "placed"? How is this handled in Commerce 2? I'd vote for the second with the ability to change this behavior using a variable.
• Is the new Module Upgrades page introduced with this patch the right way and the right place for allowing site admins to perform optional upgrade actions?

Tests should now be fixed ...

Changes compared to #16

• Added test to ensure that the "placed" property is set when the order completes checkout.
• Added test to ensure that the "placed" property is not updated when the order completes checkout more than once.

Remaining tasks

None, unless something arises from reviewing the items below.

Review needed for the following

I'd like to get some public opinion on the following decisions I've made with this patch.

• The ability exists for site admins to simulate the checkout process for an order. This can happen for orders that have already completed checkout, such as after making a product change, or applying a discount. In these cases, do we want the checkout completion time to be updated as well? Or do we want to keep the first time that the order was "placed"? How is this handled in Commerce 2? The current patch implements the latter, which is what I'd vote for - I think it's important as a store owner to know when the order was originally placed, any alterations are added as a revision. We could also introduce a variable for configuring this behavior.
• Is the new Module Upgrades page introduced with this patch the right way and the right place for allowing site admins to perform optional upgrade actions?

Proposed release notes

A new order property is introduced with this release that holds the order's checkout completion time. The current behavior of many sites that were storing it in the "created" property is now deprecated. It is recommended to visit the new Commerce Module Upgrades admin page in your store (admin/commerce/config/upgrades) and follow the instructions there for populating the new "placed" property for existing orders.

For more information about the changes follow the corresponding issue: https://www.drupal.org/node/2888407

Reviewed this, ran some local tests (on a site w/ 22k devel generated orders), and it all looks good. I made some minor changes including setting the empty placeholder to "-" for the Placed column similar to other columns in the orders view. Other than that, gonna commit it and will set the release notes accordingly!

Question: i just upgraded Commerce from 7.x-1.13 to 7.x-1.14, and ran update.php

i see that 'placed' is now a column in table commerce_order, and that the drush process commerce-order-update-placed-timestamp will update the 'placed' column from 'created', for existing orders, if that is desired

what i don't understand is: i just tried a new order, with the new 7.x-1.14 code, and 'placed' was NOT populated for that order!

what am i missing? is there a Rule that needs to be added or updated? if so, shouldn't it be in the .install file for 7.x-1.14?

i am surprised that the code isn't actually populating it!

please enlighten me!