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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Jordan_Fei’s picture

Status: Active » Needs review
FileSize
2.25 KB

Hi DanZ,

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

Regards,
Jordan

DanZ’s picture

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.

Jordan_Fei’s picture

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

TR’s picture

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.

longwave’s picture

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.

longwave’s picture

Status: Needs work » Needs review
longwave’s picture

Status: Needs review » Fixed

Committed. @Jordan_Fei, thanks for fixing this!

DanZ’s picture

Both of the new tests have:

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

Shouldn't the second be something like this?

    $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....

longwave’s picture

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.

Jordan_Fei’s picture

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.

fcortez’s picture

Version: 7.x-3.x-dev » 6.x-2.11
fcortez’s picture

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