Add support for Product revisions

Somewhere along the way, I forgot or neglected to add revision tables for products and line items. Damien's cooking up support for line item revisions, and I suppose we may just need to add product revisions at the same time.

+1 for products

would revisions for products probably be a part of the 1.0?

Renaming.

revisions would be a prerequisite for an integration with the workflows of Workbench Moderation
http://drupal.org/node/1215974#comment-4724686

Subscribing

I have created a patch for support the product's revision management.

Basically I made the modifications based on the commerce_order module which already supports versioning.

In the commerce_product.install I updated the schema:

• I added the {commerce_product}.revision_id field
• I added the {commerce_product_revision} table, which will hold the product's revisions

Also I wrote a new hook_update which:

• creates the new {commerce_product}.revision_id column if not exists
• creates the new {commerce_product_revision} table if not exists
• updates the {commerce_product} table keys
• checks if we have products already saved and for the existing products creates the current revisions in the {commerce_product_revision} table and updates the {commerce_product}.revision_id

In commerce_product.module I updated the entity info specifying the revision table and the revision entity key.

I also updated the commerce_product_product_form definition and submit function. I added the revision checkbox to allow users to create new revisions.

I also created a new basic test method in the CommerceProductCRUDTestCase called testCommerceProductRevisions, whixh ensures that a newly created product is having a revision_id. And if a product is saved with revision set to TRUE the revision_id will be changed.

Fixing the module path generation.

Fixing the module path generation.

Let's try again on the right HEAD. Setting it to 1.1 could be why it failed to apply. : ?

Patch recreated.

Patch recreated with prefixes.

Skimming the patch, I don't see any updates to the product controller's save() method supporting revisions. For example, in the order controller we set the "changed" timestamp, so I'd presume we'd need to for products as well. Is this revision functionality now being taken care of by the parent controller and the order code is deprecated? Or was it just not tested for this patch?

I checked, the changed property was updated, but I modified the controller's save method to be similar with the order controller's save method, this means, now the revision_timestamp, revision_hostname and even a log message is saved also with a revision.

I attach the file with the proper issue status to be tested automatically.

@skipyT: you don't need to re-upload the patch, just change the issue status and the testbot will test the last patch that wasn't tested.

Now, on to an actual review:

+++ b/modules/product/commerce_product.install
@@ -29,6 +29,13 @@ function commerce_product_schema() {
+      'revision_id' => array(
+        'description' => 'The current {commerce_product_revision}.revision_id version identifier.',
+        'type' => 'int',
+        'unsigned' => TRUE,
+        'not null' => TRUE,
+        'default' => 0,
+      ),

Should be before product 'type'.

+++ b/modules/product/commerce_product.install
@@ -88,6 +95,124 @@ function commerce_product_schema() {
+      'current_revision' => array(
+        'table' => 'commerce_product_revision',
+        'columns'=> array('revision_id' => 'revision_id'),
+       ),
+    ),

Should be before 'creator'. There's a oddity here, in other schemas this key is called owner.

Anyway, the 'revision_id' must also be added to this schema as a unique key.

+++ b/modules/product/commerce_product.install
@@ -88,6 +95,124 @@ function commerce_product_schema() {
+    'description' => 'The base table for products.',

Should be : 'Saves information about each saved revision of a {commerce_product}.',

+++ b/modules/product/commerce_product.install
@@ -88,6 +95,124 @@ function commerce_product_schema() {
+      'type' => array(
+        'description' => 'The {commerce_product_type}.type of this product in this version.',
+        'type' => 'varchar',
+        'length' => 255,
+        'not null' => TRUE,
+        'default' => '',
+      ),
+      'language' => array(
+        'description' => 'The {languages}.language of this product in this version.',
+        'type' => 'varchar',
+        'length' => 32,
+        'not null' => TRUE,
+        'default' => '',
+      ),

Not needed.

+++ b/modules/product/commerce_product.install
@@ -88,6 +95,124 @@ function commerce_product_schema() {
+      'uid' => array(
+        'description' => 'The {users}.uid that created this product in this version.',
+        'type' => 'int',
+        'not null' => TRUE,
+        'default' => 0,
+      ),

Should be 'revision_uid'.

+++ b/modules/product/commerce_product.install
@@ -88,6 +95,124 @@ function commerce_product_schema() {
+      'revision_hostname' => array(
+        'description' => 'The IP address that created this order.',
+        'type' => 'varchar',
+        'length' => 128,
+        'not null' => TRUE,
+        'default' => '',
+      ),

We're editing a product, not an order. And I don't think it's needed anyway.

+++ b/modules/product/commerce_product.install
@@ -88,6 +95,124 @@ function commerce_product_schema() {
+      'created' => array(
+        'description' => 'The Unix timestamp when the product  in this version was created.',
+        'type' => 'int',
+        'not null' => TRUE,
+        'default' => 0,
+      ),
+      'changed' => array(
+        'description' => 'The Unix timestamp when the product  in this version was most recently saved.',
+        'type' => 'int',
+        'not null' => TRUE,
+        'default' => 0,
+      ),

Again, not needed :)

+++ b/modules/product/commerce_product.install
@@ -135,3 +260,85 @@ function commerce_product_update_7101() {
+  $indexes_new = array(
+  );

Empty array spanned on two lines?

+++ b/modules/product/includes/commerce_product.forms.inc
@@ -57,6 +57,29 @@ function commerce_product_product_form($form, &$form_state, $product) {
+  $form['revision_information']['log'] = array(
+    '#type' => 'textarea',
+    '#title' => t('Revision log message'),
+    '#rows' => 4,
+    '#description' => t('Provide an explanation of the changes you are making. This will help other authors understand your motivations.'),
+  );
+  $form['revision_information']['log']['#states'] = array(
+    'invisible' => array(
+      'input[name="revision"]' => array('checked' => FALSE),
+    ),
+  );

Don't see any reason to define #states separately.

+++ b/modules/product/tests/commerce_product.test
@@ -196,6 +196,31 @@ class CommerceProductCRUDTestCase extends CommerceBaseTestCase {
+    $fields = array('product_id', 'sku', 'type', 'title', 'uid');

This isn't used anywhere in the test.

There's also a problem in the product controller's create method, which doesn't contain the default value for 'revision_id'.

Attached patch should fix all of these, but I still need to manually test the upgrade path and the new tests.

New patch with the following improvementes:
- cleaned-up commerce_product_update_7102
- added revision support in commerce_product_ui.module (product types)
- tested the upgrade path, everything worked as expected

Todo:
- look at the tests

Cleaned-up the tests a bit and added a new one.

This looks ready to go :)

#21 looks good, except that on Jan 17 there was a commit that added commerce_product_update_7102 to the .install. So I re-rolled patch, changing this new update to 7103.

Are there any plans for building a mechanism/interface for restoring the different revisions?

Hmm, we'll obviously need that, but I think it can be a follow-up issue for either integration with an existing module like diff or at least Views. (We have a similar issue to integrate order revisions with Views, for example.)

You are missing a line break at the end of the install file. ;)

Added the line break and tweaked the update functions some more by adding field schema definitions inside them.

I'm not sure if we should do the same for {commerce_product_revision}, in case someone runs this update after we make some changes to it and create another update function for those changes.

I'm talking about this code:

  $schema = commerce_product_schema();

  if (!db_table_exists('commerce_product_revision')) {
    db_create_table('commerce_product_revision', $schema['commerce_product_revision']);
  }

About the update function:

- the schema change part (the top part) is going to be executed for each batch. Let's move that into a if (!isset($sandbox['progress'])) {} or similar
- you cannot use commerce_product_schema() in there. The usual way is to duplicate the definition inside the update function

Also, revision_id should be NULLable, with a default of NULL so as to avoid deadlocks in MySQL (see #1363826: Deadlock issues when saving orders concurrently and #1369332: Avoid deadlock issues on the {node} table).

Fixed items from #26 and cleaned-up the update function some more. Now the top part deals only with schema changes and the second with adding revisions for existing products.

I reviewed the patch, I didn't find any problem. I tested it manually also.

It seems ok for me.

Alrighty, reviewed this most of the afternoon to make sure nothing would go horribly wrong if we released 1.2 tomorrow. I tweaked the UI a bit for revisions so that the log message is always available (i.e. moved the permission check to the checkbox instead of the whole fieldset on the product edit form) and that the title of the element changes for creation vs. update messages.

I changed the update function that creates initial revisions to batch on 50 instead of 10 to go faster; if anyone thinks I'm crazy for allowing 50 inserts and 50 updates in a single HTTP request, lemme know... but it can't take more than a second or two and it seems preferable to 5 times as many HTTP requests. I tested it with 500 products and there were no issues, took all of 3 or 4 seconds total.

Commit: http://drupalcode.org/project/commerce.git/commitdiff/62c5bb5

This broke tests, see http://qa.drupal.org/commerce

I was able to recreate easily in a trivial test. $form_state['values']['revision'] doesn't exist (commerce_product.forms.inc:174, in commerce_product_product_form_submit())

  if ($form_state['values']['revision'] || !empty($form_state['values']['log'])) {
    $product->revision = TRUE;
    $product->log = $form_state['values']['log'];
  }

Looks like it just needs this. Running local tests.

Seems odd that it wasn't written this way to begin with...

#31 passes tests in my local environment. I'd say commit.

Trying a different patch that accommodates the default value of the revision checkbox as expected even if the user does not have access to change the setting.

I should've caught that in my initial review :(

No worries. I also changed the code to always trigger a revision in the event that a message was entered because I changed where your access check sat. I missed it, too, but a simply empty() still wouldn't have properly accommodated it fully. : )

#33: 1207242.revision_check.patch queued for re-testing.

And of course I'm sitting here re-queuing a patch that won't ever apply... because it's already been committed. Heh.

Fixed! : P

Am I missing something here? I recognize that revisions are now being saved incrementally, and see the revision log message box at the bottom of the product edit form, but is there any way for a user to diff those revisions, or even view a revision that isn't live, like is expected of normal node revisions?

Nope, you're not missing anything. That functionality needs to be added most likely through integration with Views. There's a separate issue for integrating revision logs with Views, though node diffs were always handled through the third party Diff module.

In case it helps anyone else - I think the issue referred to in #42 is #1827292: Integrate Product Revisions with Views, which I have added as a related issue.

That issue has been fixed, and there is a revisions tab with a list of old revisions. However as far as I can tell, you still can't view a revision (to see product-type-specific fields) or revert to it. If anyone knows otherwise, please do correct me.