I'd like to see this done in two ways:

  1. Add a minimum interval setting that limits the cart refresh to carts that have not been refreshed in that interval. We can default this to something minimal for now to not change the functional expectation of existing sites while still giving us a performance gain: even something as low as 5 seconds should be sufficient.
  2. Allow a site to restrict the cart refresh to the order owner so that the refresh process will be prevented for anyone other than user $order->uid or, in the case of anonymous orders, users with the $order->order_id in their cart order IDs session variable.

Settings for these will be a part of the existing order settings form in a new fieldset, expanded by default, called "Shopping cart refresh." I can add some descriptive text in there to explain the process and intention behind the settings.

For the second, it will have to be clear that sites depending on this functionality to allow administrators to create orders on behalf of their customers and have their prices refresh automatically will need to leave the setting off for now.

However, let's also prioritize #1645252: Allow administrators to trigger "checkout completed" for the next release as well to bring some full solutions to bear for these use cases.

CommentFileSizeAuthor
#2 commerce_cart.conditional_refresh.patch8.54 KBmjpa
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

rszrama’s picture

Issue summary: View changes
mjpa’s picture

Attaching the first stab at a patch to provide this, feel free to rip it to shreds :P

The patch provides a setting to choose a method of checking if a cart should be refreshed:

  1. Always refresh - the current situation
  2. Only refresh if the owner matches
  3. Only refresh if the cart is the current user's cart - basically checks the order id matches the one returned from commerce_cart_order_id()

There is then a second setting that controls how often to refresh - defaulting to always to maintain the current behaviour. I've picked some timings based on what I felt was reasonable, again, feel free to suggest others!

In terms of storing the last refresh timestamp, I've gone with a column on the commerce_order table as storing it in the data array would mean a commerce_order_save() and negate any performance improvements in checking if the order has actually changed - a single column can be updated with a simple database query.

Finally, I've gone with adding a "Cart settings" page rather than adding to the existing Order settings page seeing as the Order module doesn't require it... which I was surprised at!

mjpa’s picture

Status: Active » Needs review
rszrama’s picture

Status: Needs review » Fixed

Thanks for this first pass, mjpa! I changed a few things, but the bulk is the same. I see what you were doing using the new property on the order object, but to provide a lighter weight update process instead of adding a column, I've changed it to just update the serialized data array if a refresh doesn't trigger an order save. I just had to update both the commerce_order and commerce_order_revision tables. I think in Commerce 2.x we already have on the roadmap the need for a variety of dedicated timestamp fields, so I'll check that and add a refresh timestamp to the list.

I liked the option for only refreshing the user's current cart order. I also added a new setting to force the cart refresh on /cart and /checkout/* paths regardless of the other settings so a site can use delayed updates in general but still have constant updates when the user is viewing the checkout form / updating the order.

Ahh, I just remembered, though, I wanted to add an update function to set the update delay to 0 seconds for existing sites with a message to adjust the setting there if desire. For new sites it will retain the default delay of 15 seconds. I'll add a quick follow-up commit to set it.

Commit: http://drupalcode.org/project/commerce.git/commitdiff/50030a6

rszrama’s picture

mjpa’s picture

Awesome! Thanks, Ryan and I like changes - my first thought was to use the data blob but decided against it for some reason which now escapes me!

Status: Fixed » Needs work

The last submitted patch, 2: commerce_cart.conditional_refresh.patch, failed testing.

mjpa’s picture

Status: Needs work » Fixed

Of course the patch will fail when it's already been applied! Silly test bot...

rszrama’s picture

lol, yeah, I unstuck the test bot yesterday with a workaround for the Entity API issue that was throwing a bunch of notices in tests. : P

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.