Not utilizing new price handler functionality on attribute level

uc_aac calls uc_currency_format() to display prices, this needs to be changed to uc_price() with a valid location and node context in order for uc_vat to be able to add VAT as necessary.

I haven't personally used this module and from a quick look at the code I'm not quite sure what the _uc_aac_calculate_price() function does with the various prices it calculates, so I am moving this over to uc_aac's queue to see if the module author can help add this support.

Updated the title to better communicate the issue. I have also attached a patch that correctly applies discounts created via the uc_discount module. This patch should also fix the initial issue related to the uc_vat module.

Cheers,

Antoine

Attachment	Size
price_handler.patch	1.14 KB

I've tested the patch. I also upgraded ubercart and uc_vat to the latest .dev versions - but with uc_vat enabled it still doesn't work.

this is what I try:
on the product node page I have a select box to choose between two options of an attribute. the default option adds 0.- to the price the other 200.-
with uc_vat disabled the price changes instantly when changing the attribute options.

then I enable uc_vat. reload the product node page. and try again (without cron for testing purpose)
same as above it works - BUT the prices are still displayed without VAT.

then I run cron. and reload the product node page.
now the price is displayed with VAT. but when I change from the default option to the other the price doesn't update anymore.

as a further test I disabled the price value caching - but that didn't help either. still didn't work.

I put status on needs work - but I'm actually not sure if the work is needed in this module.

one more thing:
shouldn't the option prices also go through the price handler functionality? (see lines 257, 259 & 260)

this might help... the price is actually correctly updated up to line 278 (all line numbers referring to .dev build 7.5.09):

but somehow this 'updated_price' get's directly or indirectly replaced before reaching the screen.

xibun,

Line 278 from the 6.x-1.x-dev looks like the following:

Perhaps you have another patch applied that altered line 278. Please install a fresh 6.x-1.x-dev, apply this patch, and test. Let me know if you are still having issues.

Cheers,

Antoine

Hi Antoine,

I was referring to the .dev version as in CVS (because on my system I have your patch and the patch from andreiashu). that's why I have pasted the code to avoid misunderstanding.. but I guess that was unavoidable ;).

as it's kind of the last line of this module's code I don't think that "un-applying" andreiashu's patch will change anything (but I'm happy to do the test if you think otherwise). I believe the value leaves this module correctly but doesn't reach the display later in the process. but I haven't yet been able to understand if something else overwrites it - or if with uc_vat installed a different variable gets displayed altogether.

one more thing:
shouldn't the option prices also go through the price handler functionality? looking at post #6 these must be lines 275, 277 & 278 in your version.

xibun,

Try this patch on for size. Let me know how it goes.

Cheers,

Antoine

Attachment	Size
price_handler_2.patch	1.59 KB

Updating status to "needs review"!

xibun,

I could not find a correct context to use when passing attribute option prices to uc_price(). Perhaps we could handle this in another issue since it will require modifications to other modules.

Cheers,

Antoine

@cYu: I have tested the patch (post #8) and it is good. I recommend this patch to be applied to the .dev version.

there is still work to be done to be 100% VAT compatible. but I completely agree that this is better done in a new issue - and when I understand this correctly the issue actually needs to be opened with ubercart core. once we have it fixed there we can open a new issue for uc_aac (if needed).

Can you explain to me what module I should install (vat or multicurrency perhaps?) so that I can see where uc_aac is currently lacking and where using uc_price now fixes it? I had been a bit tentative to apply this patch just because of the instability of uc_price and the various issues talking about it's reworking, but I suppose reviewing this patch would be a good way for me to at least get familiar with what is there now.

after my post #10 I found #475474: Price alterer contexts and VAT support. for the current implementation of uc_price() the patch provided by AntoineSolutions would still be a big step forward (I use it with uc_vat). but the chances are high that it will need a big rewrite.

so up to you if you want to apply it as baseline or if you prefer to wait.

The patch was originally created for use with the Discount Framework module found at www.drupal.org/project/uc_discount.

Cheers,

Antoine

updated to be against new dev branch. disregard previous patches, they are for the old branch 6.x-1.x.

New patch for the 6.x-2.x branch attached. This patch will be updated once #474400: Add support for checkbox attributes is committed.

Cheers,

Antoine

Attachment	Size
uc_aac-485606_3.patch	1.56 KB

With #474400: Add support for checkbox attributes now committed, here is a new patch to fix this issue.

Cheers,

Antoine

Attachment	Size
uc_aac-485606_4.patch	2.12 KB

I had to add:

'type' => 'product',

to the attribute context to get it to work with uc_vat.

I'm not sure what the correct type to use is. I was now looking a while for the context respectively type "handbook" but couldn't find it. for that reason I don't change status - but for me it actually works after the change.

I'm not sure what I was thinking. I already pulled the correct context from Ubercart's code and used it in another function in the module. Try this patch and let me know.

Cheers,

Antoine

Attachment	Size
uc_aac-485606_5.patch	2.16 KB

Doh... using the wrong revision in the previous patch. Hopefully this will be the final one.

Cheers,

Antoine

Attachment	Size
uc_aac-485606_6.patch	2.17 KB

your latest patch doesn't work.

it works when we use:

instead of:

as $product is not defined in uc_aac_form_alter().

xibun,

Sorry about that, I don't currently have a price altering module installed, so I haven't been able to verify the results. Thanks for bringing to my attention that $product was not defined, but rather than reloading the node, I actually want to use the node object from the form. This will save us from loading the node a second time and it allows other modules to interact with uc_aac by making changes to the node via hook_form_alter(). Please try this new patch.

Cheers,

Antoine

Attachment	Size
uc_aac-485606_7.patch	3.5 KB

your latest patch is working.

good team work :).

cheers

Indeed, EXCELLENT team work! Thanks so much for you partnership in getting this new module released. This patch has been committed.

Cheers,

Antoine

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