Problem/Motivation

An order total including a product will increase every time a product is loaded and then saved under the following conditions:

1. VAT is applied
2. A product is configured with the VAT set to "Include tax in this price"
3. The product is loaded and saved

This is quite nefarious. It seems that on every save, the order total goes up by the price of the product.

Proposed resolution

Still to come

Original report byj0rd

Original posting by j0rd:
I'm submitting this bug, which I believe relates to Commerce Stock according to other users. I'll probably need to use this module in the future so it would be great if we could look into it and get it resolved if there's a problem.

Here's the original issue in the Drupal Commerce issue queue:
http://drupal.org/node/1260602#comment-4930890

Comments

guy_schneerson’s picture

j0rd do you know that this issue relates to alpha3? Because earlier versions of the module required the user to create rules and if those where not setup correctly may have caused an issue.

HenrikBak If this issue relates to earlier versions it may be a good idea to upgrade the module and make sure that any user created rules are removed, make sure you backup first (:,
If it is the latest version was it upgraded? Maybe old rules are still around

If this is still an issue I can have a look, if you want me to check the site you can email me details to guy@blue-bag.com, note that I will not look at live sites only dev/test sites
.
I am going to do some work on the module this weekend so if I get more feedback I can look into it before Monday otherwise it will be my next available weekend.

j0rd’s picture

not sure what version is applies to actually, so if you believe it's fixed, it probably is.

You'll have to ask HenrikBak

guy_schneerson’s picture

Status: Active » Postponed (maintainer needs more info)

setting to need more information but will be happy to look at if i get more information from HenrikBak (see #1)

olofbokedal’s picture

Version: 7.x-1.0-alpha3 » 7.x-1.x-dev
Priority: Normal » Critical
Status: Postponed (maintainer needs more info) » Active

This happens for me as well, with the latest dev version.

I've traced it down to the "Decrease stock when completing the order process" rule. I've disabled all of the other rules, so there shouldn't be a conflict of some sort. By studying the code of that rule, I believe that the problem resides in the commerce_stock_adjust() function.

After the product has been updated, I can see that the price components has been added once again, hence the double price. This is what the price field contains after the update:

Array
(
    [amount] => 53500
    [currency_code] => SEK
    [data] => Array
        (
            [components] => Array
                (
                    [0] => Array
                        (
                            [name] => base_price
                            [price] => Array
                                (
                                    [amount] => 42800
                                    [currency_code] => SEK
                                    [data] => Array
                                        (
                                        )

                                )

                            [included] => 1
                        )

                    [1] => Array
                        (
                            [name] => tax|general_moms
                            [price] => Array
                                (
                                    [amount] => 10700
                                    [currency_code] => SEK
                                    [data] => Array
                                        (
                                            [tax_rate] => Array
                                                (
                                                    [name] => general_moms
                                                    [display_title] => Moms 25%
                                                    [description] => 
                                                    [rate] => .25
                                                    [type] => vat
                                                    [rules_component] => 1
                                                    [price_component] => tax|general_moms
                                                    [calculation_callback] => commerce_tax_rate_calculate
                                                    [module] => commerce_tax_ui
                                                    [title] => general_moms
                                                    [admin_list] => 1
                                                )

                                        )

                                )

                            [included] => 1
                        )

                    [2] => Array
                        (
                            [name] => base_price
                            [price] => Array
                                (
                                    [amount] => 42800
                                    [currency_code] => SEK
                                    [data] => Array
                                        (
                                        )

                                )

                            [included] => 1
                        )

                    [3] => Array
                        (
                            [name] => tax|general_moms
                            [price] => Array
                                (
                                    [amount] => 10700
                                    [currency_code] => SEK
                                    [data] => Array
                                        (
                                            [tax_rate] => Array
                                                (
                                                    [name] => general_moms
                                                    [display_title] => Moms 25%
                                                    [description] => 
                                                    [rate] => .25
                                                    [type] => vat
                                                    [rules_component] => 1
                                                    [price_component] => tax|general_moms
                                                    [calculation_callback] => commerce_tax_rate_calculate
                                                    [module] => commerce_tax_ui
                                                    [title] => general_moms
                                                    [admin_list] => 1
                                                )

                                        )

                                )

                            [included] => 1
                        )

                )

            [include_tax] => general_moms
        )

)

As you can see, the base_price and tax component has been added once again, which equals a double price.

Simply by saving the product through the administration pages changes the price back to normal.

olofbokedal’s picture

I've tried to update the product without using the wrapper:

$product->commerce_stock['und'][0]['value'] += intval($qty);
$result = commerce_product_save($product);

instead of

$wrapper = entity_metadata_wrapper('commerce_product', $product);

$new_stock = $wrapper->commerce_stock->value() + intval($qty);
$wrapper->commerce_stock->set($new_stock);
$result = $wrapper->save();

This results in the same behavior. Maybe this problem is related to another module? The price components aren't being duplicated here. Can't see anything wrong with this code.

guy_schneerson’s picture

Hi @ojohansson,
this is very strange as the commerce_stock_adjust() function and its calling function commerce_stock_decrease_by_line_item(), loads updates and saves a product and not a line item.

can you confirm that the rule is set up as follows:

event: Completing the checkout process

Conditions: None

Actions:
Loop
Parameter: List: [commerce_order:commerce...
List item: Current list item (list_item)
Decrease the product stock level, given a line item
Parameter: Line item: [list-item]

If this is the case is it possible to get a sanitized copy of the site and DB to try and debug this.

olofbokedal’s picture

I've just installed Commerce Kickstart and this module, and reproduced everything from a fresh install. I've enabled a tax rate for the products that exists from the start, just to get some data in the data array of the price field.

I looked through the code of the Commerce Price module, and found this in the hook_field_presave() function:

// Convert amounts to integers and serialize data arrays before saving.
foreach ($items as $delta => $item) {
  // Serialize an existing data array.
  if (isset($item['data']) && is_array($item['data'])) {
    $items[$delta]['data'] = serialize($item['data']);
  }
}

This is where the components are added every time. If you save a product from the administration pages, the $item['data'] only contains information about the tax, but if you load a product through commerce_product_load() function, the price components are already there, resulting in duplicates.

This can't be related to Commerce Stock, since everything seems to be loaded and saved in the appropriate ways?

guy_schneerson’s picture

@ojohansson, nice job at tracking it down, i am not on the latest version of kickstart and i havent got a tax rate currently so that may be why i am not getting this issue.
i suspect you are correct about it not being a commerce_stock issue. i will update my system and try and recreate the issue.
We shoul bring it up on IRC and create an issue Q in commerce if we are sure it is not related to stock

olofbokedal’s picture

Title: Order total doubled or tripled when commerce_stock is enabled. » Product price increases when saving a product that has been loaded through commerce_product_load()
Project: Commerce Stock » Commerce Core
Component: Code » Price

This is definitely not related to Commerce Stock.

I put these lines in my template.php, just to confirm my suspicions:

$product = commerce_product_load(97);
commerce_product_save($product);

These lines duplicates the price components.

Found this similar issue #1123026: commerce_product_load() followed by commerce_product_save() causes 100x price increase on front-end

However, that was some time ago, and the cause of the problems doesn't seem to be the same.

rszrama’s picture

Priority: Critical » Major

I'm going to need more to go on if this is coming back to the Commerce queue... can you provide more information or debug data to show what exactly the $product looks like before and after the save?

olofbokedal’s picture

Sure.

This is the entire product object right after I've loaded it through commerce_product_load():

stdClass Object
(
    [product_id] => 1
    [sku] => PROD-01
    [type] => product
    [language] => und
    [title] => Product One
    [uid] => 1
    [status] => 1
    [created] => 1315316438
    [changed] => 1315317105
    [data] => 
    [commerce_price] => Array
        (
            [und] => Array
                (
                    [0] => Array
                        (
                            [amount] => 1000
                            [currency_code] => USD
                            [data] => Array
                                (
                                    [components] => Array
                                        (
                                            [0] => Array
                                                (
                                                    [name] => base_price
                                                    [price] => Array
                                                        (
                                                            [amount] => 800
                                                            [currency_code] => USD
                                                            [data] => Array
                                                                (
                                                                )

                                                        )

                                                    [included] => 1
                                                )

                                            [1] => Array
                                                (
                                                    [name] => tax|moms
                                                    [price] => Array
                                                        (
                                                            [amount] => 200
                                                            [currency_code] => USD
                                                            [data] => Array
                                                                (
                                                                    [tax_rate] => Array
                                                                        (
                                                                            [name] => moms
                                                                            [display_title] => moms
                                                                            [description] => 
                                                                            [rate] => .25
                                                                            [type] => vat
                                                                            [rules_component] => 1
                                                                            [price_component] => tax|moms
                                                                            [calculation_callback] => commerce_tax_rate_calculate
                                                                            [module] => commerce_tax_ui
                                                                            [title] => moms
                                                                            [admin_list] => 1
                                                                        )

                                                                )

                                                        )

                                                    [included] => 1
                                                )

                                        )

                                    [include_tax] => moms
                                )

                        )

                )

        )

    [field_image] => Array
        (
        )

    [commerce_stock] => Array
        (
            [und] => Array
                (
                    [0] => Array
                        (
                            [value] => 10
                        )

                )

        )

    [rdf_mapping] => Array
        (
        )

)

And this is the same object right after I've saved it through commerce_product_save().

stdClass Object
(
    [product_id] => 1
    [sku] => PROD-01
    [type] => product
    [language] => und
    [title] => Product One
    [uid] => 1
    [status] => 1
    [created] => 1315316438
    [changed] => 1315464411
    [data] => 
    [commerce_price] => Array
        (
            [und] => Array
                (
                    [0] => Array
                        (
                            [amount] => 1000
                            [currency_code] => USD
                            [data] => Array
                                (
                                    [components] => Array
                                        (
                                            [0] => Array
                                                (
                                                    [name] => base_price
                                                    [price] => Array
                                                        (
                                                            [amount] => 800
                                                            [currency_code] => USD
                                                            [data] => Array
                                                                (
                                                                )

                                                        )

                                                    [included] => 1
                                                )

                                            [1] => Array
                                                (
                                                    [name] => tax|moms
                                                    [price] => Array
                                                        (
                                                            [amount] => 200
                                                            [currency_code] => USD
                                                            [data] => Array
                                                                (
                                                                    [tax_rate] => Array
                                                                        (
                                                                            [name] => moms
                                                                            [display_title] => moms
                                                                            [description] => 
                                                                            [rate] => .25
                                                                            [type] => vat
                                                                            [rules_component] => 1
                                                                            [price_component] => tax|moms
                                                                            [calculation_callback] => commerce_tax_rate_calculate
                                                                            [module] => commerce_tax_ui
                                                                            [title] => moms
                                                                            [admin_list] => 1
                                                                        )

                                                                )

                                                        )

                                                    [included] => 1
                                                )

                                            [2] => Array
                                                (
                                                    [name] => base_price
                                                    [price] => Array
                                                        (
                                                            [amount] => 800
                                                            [currency_code] => USD
                                                            [data] => Array
                                                                (
                                                                )

                                                        )

                                                    [included] => 1
                                                )

                                            [3] => Array
                                                (
                                                    [name] => tax|moms
                                                    [price] => Array
                                                        (
                                                            [amount] => 200
                                                            [currency_code] => USD
                                                            [data] => Array
                                                                (
                                                                    [tax_rate] => Array
                                                                        (
                                                                            [name] => moms
                                                                            [display_title] => moms
                                                                            [description] => 
                                                                            [rate] => .25
                                                                            [type] => vat
                                                                            [rules_component] => 1
                                                                            [price_component] => tax|moms
                                                                            [calculation_callback] => commerce_tax_rate_calculate
                                                                            [module] => commerce_tax_ui
                                                                            [title] => moms
                                                                            [admin_list] => 1
                                                                        )

                                                                )

                                                        )

                                                    [included] => 1
                                                )

                                        )

                                    [include_tax] => moms
                                )

                        )

                )

        )

    [field_image] => Array
        (
        )

    [commerce_stock] => Array
        (
            [und] => Array
                (
                    [0] => Array
                        (
                            [value] => 10
                        )

                )

        )

    [rdf_mapping] => Array
        (
        )

)

The only two lines causing this are

$product = commerce_product_load(1);
commerce_product_save($product);

This is a fresh Commerce Kickstart installation, with tax included for the price field. As you can see, the data array for the price field has been altered, causing duplicate components. These components will be added once again if I would save the product again.

rszrama’s picture

Ahh, it's helpful to see that this may be a relic of VAT included price stuff. Will have to investigate that further.

HenrikBak’s picture

Do you guys need any information from me? (I created the original issue)

I'm running Commerce 7.x-1.0 and Commerce Stock 7.x-1.0-alpha3. I have a VAT tax rate enabled on all my products.

queenvictoria’s picture

Subscribe
Same scenario: Commerce 7.x-1.x-dev, commerce stock 7.x-1.x-dev, VAT on products.

@rszrama - anything I can do to help? Are we looking at commerce_tax first?

olofbokedal’s picture

@rszrama as queenvictoria said, if there's anything we can do to help, please let us know.

rszrama’s picture

Feel free to investigate through the Tax code; I'm doing a bit of other contrib stuff and haven't had time to come back to this yet.

HenrikBak’s picture

Any news on this issue? It's currently preventing my shop from launching because I need to have some sort of stock management and VAT tax on my products.

As mentioned earlier the issue is related to the commerce_product_load function combined with VAT tax, but I haven't been able to figure out a workaround yet - so any help would be appreciated...

olofbokedal’s picture

I'm having a hard time figuring out the flow for when the price field is loading.

I think the problem is somewhere where the tax module interacts with the price module though. Don't know if the problem is caused by the price or the tax module. But since the components gets added once more every time I save a product, I think the problem is caused by the price module.

Anyhow, I think we should be looking into how the price and tax modules interact via the price component array.

reecemarsland’s picture

sub

rszrama’s picture

Issue tags: +1.1 blocker

Tagging.

johnkennedy’s picture

sub

johnkennedy’s picture

This is a hack, but it meant that I could get a site past this bug until we have a patch in commerce price. Disclaimer disclaimer, if you plan to use this, test it in your own setup extensively.

function commerce_payment_express_reset_products($order){
  $order_wrapper = entity_metadata_wrapper('commerce_order', $order);
  foreach ($order_wrapper->commerce_line_items as $line_item_wrapper) {
      $product = commerce_product_load($line_item_wrapper->commerce_product->product_id->value());
      unset($product->commerce_price['und'][0]['data']['components']);
      commerce_product_save($product);
  }
}

Executed on the $order object last.

boardyuk’s picture

I've had a look through the commerce_payment module but can't figure out where to put the hack in #22 at all. Can anyone let me know where to put this to test please?

Really need to get this working on a site I'm working on at the moment.

Thanks Adam

johnkennedy’s picture

You can put the function anywhere, but you should change its name to suit the naming convention. if you are already building a module, something like function your_module_reset_products($order).

Then you can either put a reference to it in your payment module, or as a rule which executes php code on event "Completing the checkout process" weighted so that it runs after "Decrease stock when completing the order process".

John

ricovandevin’s picture

subscribing

Hoping for an official patch to be released soon.

Rico.

Taxoman’s picture

Priority: Major » Critical

This issue - also with "critical" status, has been marked as a duplicate of this one: #1308542: Order subtotal wrong.

abaier’s picture

Thanks for this, I am having the same problem. Every time a product is sold and the stock decreases, the next time this product is bought, the product price is added another time to the order total. After manually saving this specific product afterwards the price is corrected...

@ john_evelops

Could you please describe the integration of your hack in post #22 more precisely? I am not very used to coding, so I don't really know where to put the code and how to run it after the completion of the order. For your information, I have three product-types that have to be considered.

Thanks in advance for your help!

AJ

rfay’s picture

Assigned: Unassigned » rfay

I'll take a look at this today. Probably will need some help from Ryan.

rfay’s picture

I'm not able to replicate this so far, using a simple VAT and enabling Commerce Stock, and using the "load/save product in the template.php" technique.

Would any of you with the problem be willing to share a database with me? randy at randyfay dot com by dropbox or email or whatever.

I *did* see when editing the product the default price had the VAT included in it.

rfay’s picture

Status: Active » Postponed (maintainer needs more info)

I'd love to see a solution to this, but waiting for somebody to demonstrate it, preferably with a database. @ojohansson, are you still able to recreate this on a simple commerce kickstart install?

HenrikBak’s picture

rfay: I have only encountered this issue when the product price is VAT inclusive. As far as I remember the problem disappears when I set the product price excluding the VAT.

HenrikBak’s picture

And I have just been able to replicate the issue. I did the following:

- Installed Commerce Kickstart 7.x-1.0-rc5 with example products
- Created a VAT tax
- Set the price of "Product One" to 10$ VAT inclusive
- executed the commerce_product_load from template.php

rfay’s picture

Status: Postponed (maintainer needs more info) » Active

@HenrikBak, Could you please attach (or send me) a database and your template.php?

HenrikBak’s picture

StatusFileSize
new422.76 KB

@rfay I have attached the DB from my quick local test setup

In template.php i simply inserted the following:

$product = commerce_product_load(1);
commerce_product_save($product);

The issue is really easy to replicate though if you follow my instructions above. Guess I forgot to mention that I also called the commerce_product_save function though.

rfay’s picture

Thanks - I'll take a look later today or tomorrow.

rfay’s picture

Title: Product price increases when saving a product that has been loaded through commerce_product_load() » If VAT is included in a product price, the order total goes up by that much every time the product is re-saved

Updating title; wrote an issue summary at the top.

rfay’s picture

Here's what happens in this case:

Every time the product is loaded, two new components are added to it. If it gets saved... it multiplies.

A new base price component (equal to previous base + real base) is added, and a new vat component is added. Lots of components!

rfay’s picture

Assigned: rfay » Unassigned

Will take a further look tomorrow with some coaching on how product pricing works. I've never been there before.

rszrama’s picture

Priority: Critical » Major

Talked through this with Randy, and there are three points to be made:

  1. Obviously, I need to lay some serious documentation down on price components. The function I first hinted at above, commerce_tax_field_attach_load(), is responsible for separating out the base price of a product from its VAT when you input prices including VAT. This price amount and components then become the starting point for product line items that reference this product. The fact that these components are split apart on product load is unique to products with VAT inclusive pricing. It should only be the Tax module that ever needs to do this at the product level; other component adjustments will be made at the line item level. In fact, to properly calculate included VAT, this must be the first operation that happens on hook_field_attach_load() to these price field components arrays.
  2. This means that we can safely update our commerce_tax_field_attach_load() to wipe the components array before adding the base price / tax components for the included VAT. If in the future, some module turns up that also wants to monkey with components when products are loaded (and not when line items for those products are being created, which again is the overwhelming scenario), it must make sure it makes its adjustments after the Tax module.
  3. As Randy pointed out, we also need to clear the components array for product price fields on save. There's no reason to save the components, and normally it won't happen. If you look at commerce_tax_field_attach_form() and commerce_tax_price_field_validate(), you'll see that we do this for saving that happens through the UI. Right now you could duplicate this in your code prior to your commerce_product_save() and everything would be just fine. We should be able to implement a commerce_product_presave() to do this automatically, though. I'm trying to think of drawbacks to that and coming up with none.

I'm demoting this to a "major" priority not because it isn't a bad bug but because it doesn't render a system unusable. It affects a subset of instances where you're using both VAT inclusive product pricing and a module or custom code that manually saves and then reloads products in the same pageload without clearing component arrays - those modules could just as easily imitate commerce_tax_price_field_validate() and the problem would be half-solved before we ever did my #2 suggestion above.

rfay’s picture

Status: Active » Needs review
StatusFileSize
new7.34 KB

Here's a patch for review.

It does these things:

1. hook_update_N() to remove components from products where they've already mistakenly stuck.
2. Adds a module identifier to components, so that module can determine whether it's safe to remove.
3. Adds a commerce_tax_commerce_product_presave() that removes its components before saving, since they should not be saved with the product. They're dynamically created on every load.

Status: Needs review » Needs work
abaier’s picture

Hey!

I just wanted to ask if the test-problems with the patch will be resolved soon?
Or is the patch already working properly, so I could apply it?

In my case the total amount in the cart will only be multiplied if the stock module is active.
It seems to be depending on the times a product was sold before. The first order is calculated
right, the following not. Saving the specific product in the backend manually solves the problem.

Thanks for your help

rfay’s picture

Status: Needs work » Needs review
rfay’s picture

Anybody can request a retest... I just did. Hoping you can test this in your environment. Don't forget to run update.php.

Status: Needs review » Needs work
olofbokedal’s picture

Status: Needs work » Needs review

Sorry for my late reply.

I've just tested the patch on our active site (in a dev. environment), and it works as intended. I've simply tested the entire flow for purchasing a product, and the price stays the same. If I take a look at the price component array in the database, it's empty, as it should be.

Haven't tested any other scenarios though, since this was the only time it happened in our case.

Status: Needs review » Needs work
rfay’s picture

Status: Needs work » Needs review
StatusFileSize
new7.17 KB

This one just changes the for loop to

    if ($field['type'] == 'commerce_price' && !empty($product->{$field_name})) {

(adding the !empty)

I'm not sure how the test gets an empty $product->{$field_name} in this situation.

abaier’s picture

Thx, for the info. I tested the patch and it seems to work also for my case, especially in combination with the stock module.

After adding the !empty to the price module the order confirmation e-mails were sent incorrectly. Only the total amount
was included, but subtotal, shipping and vat were missing... so I changed it back.

rfay’s picture

@toni4i are you saying that #48 gave you trouble? If so, please explain exactly what trouble. It does pass tests now, but of course that's not everything :-)

abaier’s picture

sorry, it DOES work...

first I applied the patch in #40 and it worked properly. then I only changed the code line "if ($field['type'] == 'commerce_price' && !empty($product->{$field_name})) {" in the price module (manually) without applying the new patch #48 ... that resulted in the missing elements ... after applying the new patch it seems to do its job.

thanks!

rszrama’s picture

In case anyone is wondering, here's a simple Rule you can use in conjunction with a product with VAT included in its price to easily duplicate the bug:

{ "rules_save_referenced_product" : {
    "LABEL" : "Save the referenced product",
    "PLUGIN" : "reaction rule",
    "REQUIRES" : [ "rules", "commerce_product_reference" ],
    "ON" : [ "commerce_product_calculate_sell_price" ],
    "IF" : [
      { "data_is" : { "data" : [ "commerce-line-item:type" ], "value" : "product" } }
    ],
    "DO" : [
      { "entity_save" : { "data" : [ "commerce-line-item:commerce-product" ] } }
    ]
  }
}

We're ultimately going to go with a more limited approach than #40. We really just need to blow away any existing components on hook_field_attach_load() in the Tax module, because Tax isn't price component sensitive. We must start from scratch, and therefore we'll have to mandate that any module that wants to preset components on a product price for calculation must use the same approach (and must be weighted after the Tax module).

Talked it over with Randy, and so long as the presave is working fine (which I've moved into the product controller itself), then we also shouldn't need to include the update function. It would be quite an expensive task for any site with a sizable product catalog for no gain since we're going to be ignoring component arrays on load from here on out anyways. We need components wiped uniformly for every product price, so I don't see any problem with the code being in the controller. Additionally, because this happens before hook_field_presave(), we don't have to mess with unserialization.

The code to add module designations to components can still be reopened in a child issue if necessary.

Letting the test bot take at crack at this before I commit it.

rszrama’s picture

Component: Price » Tax
Status: Needs review » Fixed

Alrighty, thanks for the assist, test bot! If this leaves any dangling issues for anyone, feel free to reopen. In my testing, this seems to accommodate new installs and existing installs with components stored for prices, even if those components are never actually cleared from the database. As I mentioned above, I'm not putting in an update function right now because it isn't strictly necessary and could be quite costly for large product catalogs; if need be, we can devise a script here to clean product prices. Just let me know.

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

Anonymous’s picture

Issue summary: View changes

Added an issue summary