The {uc_taxed_product_types} is not updated when node type machine name of a product class changes. This could result in incorrect tax calculations.

This could be fixed with a hook_node_type_update() implementation.

Files: 
CommentFileSizeAuthor
#5 1899458-taxed-product-type-update.patch5.14 KBlongwave
PASSED: [[SimpleTest]]: [MySQL] 2,833 pass(es).
[ View ]
#5 1899458-taxed-product-type-update-test-only.patch4.42 KBlongwave
FAILED: [[SimpleTest]]: [MySQL] 2,836 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#3 ubercart-taxed_product_type_udpate-1899458-s1.patch2.3 KBJordan_Fei
PASSED: [[SimpleTest]]: [MySQL] 2,798 pass(es).
[ View ]
#3 ubercart-taxed_product_type_udpate-1899458-s2.patch1.12 KBJordan_Fei
PASSED: [[SimpleTest]]: [MySQL] 2,798 pass(es).
[ View ]
#1 ubercart-taxed_product_type_udpate-1899458.patch2.25 KBJordan_Fei
PASSED: [[SimpleTest]]: [MySQL] 2,800 pass(es).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new2.25 KB
PASSED: [[SimpleTest]]: [MySQL] 2,800 pass(es).
[ View ]

Hi DanZ,

I tried to fix this bug as you described as the attached patch. Please review and test it.

Regards,
Jordan

Status:Needs review» Needs work

Thanks for this. It's good to see some initiative from someone new.

Interesting approach, adding an additional hook to do this. I was expecting just a uc_tax_node_type_update() implementation. It's worth discussing.

What is the advantage of adding an Ubercart-specific hook over just using hook_node_type_update()? Can you see a lot of modules using this? The Drupal hook provides more information.

Actually, I might use it for the Ubercart Stamps.com module, since it does need to respond to product type machine name changes and doesn't care about anything else.

If you're going to add the hook, though, make sure it only responds to Ubercart product type and subtype changes. There's no point in having an Ubercart hook that gets invoked on events that have nothing to do with Ubercart. There's also no point in calling it if the old type and the new type are the same.

You need to have spaces after your commas to meet Drupal style standards. See http://drupal.org/node/1354.

Status:Needs work» Needs review
StatusFileSize
new1.12 KB
PASSED: [[SimpleTest]]: [MySQL] 2,798 pass(es).
[ View ]
new2.3 KB
PASSED: [[SimpleTest]]: [MySQL] 2,798 pass(es).
[ View ]

Hi DanZ,

Before I thought uc_taxes module depends on ubercart core module uc_product, and {uc_taxed_product_types} should depend on product type in uc_product module(although product type
is just content type there), so I added a hook in uc_product product to decouple the uc_taxes with
node type.

But as you said, if there are other modules using the hook_node_type_update, your solution is more simple.

Therefore, I created two patches:
ubercart-taxed_product_type_udpate-1899458-s1.patch: My solution just do some coding format as you mentioned.
ubercart-taxed_product_type_udpate-1899458-s2.patch: Your simple solution.

It is up to you to use which one.

Regards,
Jordan

Hi Jordan_Fei,

Thanks for your contribution. It's looking good - hopefully we can get some testing from some users to confirm that this works without breaking anything. In the meantime, please take a look at http://drupal.org/coding-standards because there are some things in your patch that need to be changed to conform to Drupal coding standards.

StatusFileSize
new4.42 KB
FAILED: [[SimpleTest]]: [MySQL] 2,836 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new5.14 KB
PASSED: [[SimpleTest]]: [MySQL] 2,833 pass(es).
[ View ]

I think the "s2" patch is simpler. I fixed up some more coding standards issues, and added a test (rearranging the other tax tests while I was at it).

Status:Needs review» Needs work

The last submitted patch, 1899458-taxed-product-type-update-test-only.patch, failed testing.

Status:Needs work» Needs review

Status:Needs review» Fixed

Committed. @Jordan_Fei, thanks for fixing this!

Both of the new tests have:

<?php
    $this
->assertText($tax, 'Tax line item displayed.');
?>

Shouldn't the second be something like this?

<?php
    $this
->assertText($tax, 'Tax line item displayed after changing product class node type.');
?>

...or does something informative show up in the text anyway? I still don't know much about tests....

I committed your suggestion. Generally, when debugging test failures you have to look at the source code of the test to figure out what's happening anyway, but improving the assertion messages can't hurt.

hi TR and longwave,

I came back from vacation just now, so didn't reply to you. Thanks for your continuous work on this issue.

Regards,
Jordan

Status:Fixed» Closed (fixed)

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

Version:7.x-3.x-dev» 6.x-2.11

Version:6.x-2.11» 7.x-3.4