There are three minor issues with the uc_discounts_nodeapi() function. I thought putting them together as cleanup is easier for everybody than creating three separate issues, but let me know if I'm wrong.
1. It reloads the node ("$product = node_load(array('nid' => $node->nid));"). However, as we're in nodeapi and op == view, the node has just been loaded. $product will equal $node, so there's no need to reload it (even in preview).
2. Needless check for "variable_get('uc_discounts_product_discount_price', 0)" on line 193, just "if ($discount_price != $node->sell_price)" is enough. The variable check has already been done in the "parent" if-statement, thus the variable_get always evaluates to true in this case.
3. The text " (discounted from ...)" is inflexible and untranslatable. I've changed this to use Drupal's t() function with placeholders for the prices here.
Comment | File | Size | Author |
---|---|---|---|
uc_discounts_nodeapi_cleanup.patch | 1.36 KB | haffmans |
Comments
Comment #1
haffmans CreditAttribution: haffmans commentedComment #2
psynaptic CreditAttribution: psynaptic commentedThanks for the patch, tszcheetah.
This is something I've done a little work on and have changed it in a version of uc_discounts I'm running on a live site. I agree with what you've done but it seems the patch is against a previous version of uc_discounts. Could you reroll against the latest DRUPAL-5?
Comment #3
psynaptic CreditAttribution: psynaptic commentedComment #4
psynaptic CreditAttribution: psynaptic commentedIt seems I had changed this function and not committed it. CVS reported that uc_discounts.module needed patching so I reverted and found this patch is actually against the latest DRUPAL-5. Sorry for the confusion.
Comment #5
psynaptic CreditAttribution: psynaptic commentedI've used the patch from the original post and come up with this:
The reason I did it like this was because the existing functions theme_uc_product_sell_price() and theme_uc_product_display_price() are like so:
Does anyone have any comments on this? This is certainly an improvement on what it was before but I'm still not happy with it. As a developer I just want sell price and discounted price in the $node object. So $node->sell_price would be the original price and $node->discount_price would be the discount price.
How do we add $node->discount_price? I tried but was unsuccessful.
Comment #6
haffmans CreditAttribution: haffmans commentedFirst: As for
theme('uc_discounts_display_price', $discount_price);
I suppose you can just call the 'original' uc theme function for that, theme_uc_product_display_price, with the discount price as parameter:theme('uc_product_display_price', $discount_price);
Saves you overriding that function, since you made no changes anyway, and themers don't have to change two functions to change the looks of the display price in all cases. The drawback is that in a theme you can't change the looks of a discounted display price, which is possible with the new theme function.
As for getting $node->discount_price: try this in nodeapi() (i.e. the same function) with $op == "load". You should be able to modify the actual node object there without losing the changes. Haven't tried yet though, so I'm not entirely sure if that'll work (depends on if the prices etc. have been loaded before the discount module hooks in).
Other than that I like the solution, much better than what it was before.
Comment #7
psynaptic CreditAttribution: psynaptic commentedHi tszcheetah, thanks again for commenting. I agree with you about using the original theme function, so will go with that now until we find a need to have the uc_discounts_display_price one.
I'll commit this as it is since it does improve the situation and we can always make further changes later.
I'll see if I can add that $node->discount_price now.
Comment #8
psynaptic CreditAttribution: psynaptic commentedI've made a few improvements to this now:
The main differences are:
I'll commit this although it's still open for debate if anyone has any suggestions.
Comment #9
joachim CreditAttribution: joachim commentedWe're mixing SPAN and DIV, but this is to match current UC output.
Filed a bug on UC here: http://drupal.org/node/325106
When that gets fixed we can follow to match.
Comment #10
Anonymous (not verified) CreditAttribution: Anonymous commentedAutomatically closed -- issue fixed for two weeks with no activity.
Comment #11
joachim CreditAttribution: joachim commentedhttp://drupal.org/node/325106 has been fixed, so we should make that change to the SPAN.
Comment #12
manarth CreditAttribution: manarth commentedNodeapi is looking for a valid node->type before applying any discount actions to the node.
Right now, it's calling uc_product_node_info() to get a list of valid types. However, this function doesn't include custom node modules which implement hook_product_types to declare the node as a product.
Instead of:
You could use:
Can you think of any reason not to go with this approach?