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
Comment #1
guy_schneerson commentedj0rd 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.
Comment #2
j0rd commentednot sure what version is applies to actually, so if you believe it's fixed, it probably is.
You'll have to ask HenrikBak
Comment #3
guy_schneerson commentedsetting to need more information but will be happy to look at if i get more information from HenrikBak (see #1)
Comment #4
olofbokedal commentedThis 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:
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.
Comment #5
olofbokedal commentedI've tried to update the product without using the wrapper:
instead of
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.
Comment #6
guy_schneerson commentedHi @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.
Comment #7
olofbokedal commentedI'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:
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?
Comment #8
guy_schneerson commented@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
Comment #9
olofbokedal commentedThis is definitely not related to Commerce Stock.
I put these lines in my template.php, just to confirm my suspicions:
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.
Comment #10
rszrama commentedI'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?
Comment #11
olofbokedal commentedSure.
This is the entire product object right after I've loaded it through commerce_product_load():
And this is the same object right after I've saved it through commerce_product_save().
The only two lines causing this are
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.
Comment #12
rszrama commentedAhh, it's helpful to see that this may be a relic of VAT included price stuff. Will have to investigate that further.
Comment #13
HenrikBak commentedDo 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.
Comment #14
queenvictoria commentedSubscribe
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?
Comment #15
olofbokedal commented@rszrama as queenvictoria said, if there's anything we can do to help, please let us know.
Comment #16
rszrama commentedFeel 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.
Comment #17
HenrikBak commentedAny 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...
Comment #18
olofbokedal commentedI'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.
Comment #19
reecemarsland commentedsub
Comment #20
rszrama commentedTagging.
Comment #21
johnkennedy commentedsub
Comment #22
johnkennedy commentedThis 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.
Executed on the $order object last.
Comment #23
boardyuk commentedI'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
Comment #24
johnkennedy commentedYou 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
Comment #25
ricovandevin commentedsubscribing
Hoping for an official patch to be released soon.
Rico.
Comment #26
Taxoman commentedThis issue - also with "critical" status, has been marked as a duplicate of this one: #1308542: Order subtotal wrong.
Comment #27
abaier commentedThanks 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
Comment #28
rfayI'll take a look at this today. Probably will need some help from Ryan.
Comment #29
rfayI'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.
Comment #30
rfayI'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?
Comment #31
HenrikBak commentedrfay: 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.
Comment #32
HenrikBak commentedAnd 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
Comment #33
rfay@HenrikBak, Could you please attach (or send me) a database and your template.php?
Comment #34
HenrikBak commented@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.
Comment #35
rfayThanks - I'll take a look later today or tomorrow.
Comment #36
rfayUpdating title; wrote an issue summary at the top.
Comment #37
rfayHere'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!
Comment #38
rfayWill take a further look tomorrow with some coaching on how product pricing works. I've never been there before.
Comment #39
rszrama commentedTalked through this with Randy, and there are three points to be made:
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.
Comment #40
rfayHere'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.
Comment #42
abaier commentedHey!
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
Comment #43
rfay#40: commerce.included_tax_components_dynamically_calculated_1267550_40.patch queued for re-testing.
Comment #44
rfayAnybody can request a retest... I just did. Hoping you can test this in your environment. Don't forget to run update.php.
Comment #46
olofbokedal commentedSorry 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.
Comment #48
rfayThis one just changes the for loop to
(adding the !empty)
I'm not sure how the test gets an empty $product->{$field_name} in this situation.
Comment #49
abaier commentedThx, 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.
Comment #50
rfay@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 :-)
Comment #51
abaier commentedsorry, 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!
Comment #52
rszrama commentedIn 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:
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.
Comment #53
rszrama commentedAlrighty, 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.
Comment #54.0
(not verified) commentedAdded an issue summary