When finishing a site for a client, we noticed that some products weren't getting proper adjusted SKUs assigned at cart add based on selected product attributes. The issue arises when the attributes have been re-ordered on the attribute edit page for a product after the original product adjustments were applied... The reason is that the unique key for these options is a serialized array of the attribute id and the id of the selected option. These get serialized in the order that they appear on the page when the user adds a product to their cart, which if the display order is changed after all of the SKU adjustments have been set, the array elements will not be in the same order.

With this bug in effect, customers cannot manage their cart, because the model doesn't get correctly applied to their cart item. In this case, they can't remove products from their cart, and checkout doesn't seem to work right either.

My suggestion is to just make sure that a ksort is used whenever you serialize those elements either for comparison or during db insertion/update of product adjustments. That way the data stored is independent of display order, since that is modifiable by the store manager at any point.

This would mean that arrays for pre-existing options would also need to be updated with the ksort applied. So, I've attached three patches... One for uc_attribute.admin.inc, one for uc_attribute.php, and a short php script that could go in the install file (I ran it from drush) that updates the existing adjustments with the ksort.

Any feedback would be appreciated... For now I've just had my customer revert the display order of the attributes so that the serialized options match. I'd appreciate some feedback on this, as want to make sure there aren't any unintended consequences :)

I didn't mark it critical, though it's a pretty big deal on our end, since pretty much all of my client's products use a few attributes. For now I'll tell them to not mess with display order, though.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

stockliasteroid’s picture

Status: Needs review » Needs work

I'm going to have to re-post those patches... I wasn't using ksort correctly, and I made the wrong change to uc_attribute.admin.inc. I've got it working now, though, so I'll repost those patches later today.

stockliasteroid’s picture

New patches/update script...

Island Usurper’s picture

Tabs are gross (coding standards). Please set your editor to emulate tabs with 2 spaces.

Otherwise, I think this looks like a good idea.

hanoii’s picture

subscribe

stockliasteroid’s picture

FileSize
348 bytes

Sorry, Textmate wasn't set on soft tabs for some reason.

Here's a new reset file. The diffs had soft tabs, I believe.

Island Usurper’s picture

Status: Needs work » Needs review
FileSize
2.07 KB

The changes to uc_attribute.admin.inc still weren't using ksort() correctly, but I was going to move the reset script to an update function anyway. Here's a patch with the changes I've got so far.

stockliasteroid’s picture

So it wasn't... Looks like I accidentally re-posted my original patch, as my final version is identical to what you have in your new patch. Looks fine to me...

Island Usurper’s picture

Status: Needs review » Fixed
FileSize
2.52 KB

Heh, it looks fine, but it's broken. We can't have the ksort() in uc_product_adjustments_form_submit() because $values['combo_array'] is already a string by that point.

Here's an updated version that sorts in the form building function instead, and this seems to work right.

Committed to the Bazaar repo.

Status: Fixed » Closed (fixed)

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