The prices per role table stores the vid number, but it removes the previous revisions' prices. This is a particular issue for a site which implements workflow control where changes must be approved/published as updating the prices would involve the creation of a new revision - but then the published version of prices would default to the products default price.

I've identified the problem as being in uc_price_per_role_nodeapi:

      ...
      case 'insert':
      case 'update':
        db_query("DELETE FROM {uc_price_per_role_prices} WHERE nid = %d", $node->nid);
      ...
CommentFileSizeAuthor
#8 1080772-8-fix-revision-prices.patch976 bytesfenstrat
#5 revision-fix-for-saving-prices-1080772-5.patch921 byteshoruskol
#2 revision-fix-for-saving-prices-1080772-2.patch21.08 KBhoruskol

Comments

horuskol’s picture

horuskol commented
Status: Active » Needs work

I have a patch ready to commit in git, but I don't know what version to apply - should it be 6.x-1.1 or master?

horuskol’s picture

horuskol commented
StatusFileSize
new revision-fix-for-saving-prices-1080772-2.patch21.08 KB

Patch file built against master (6.x-1.1)

horuskol’s picture

horuskol commented
Status: Needs work » Needs review

Forgot to set to "needs review"

fenstrat’s picture

fenstrat
he/him
Primary language English
Location Australia
commented
Status: Needs review » Needs work

Your patch includes uc_price_per_role.module twice, once diff'd against itself and once against /dev/null - you need to remove the second one.

As for the change it does seem logical. The {uc_price_per_role_prices} table is storing prices per revision but as you've noted hook_nodeapi() is deleting all revisions when a node is updated.

horuskol’s picture

horuskol commented
Status: Needs work » Needs review
StatusFileSize
new revision-fix-for-saving-prices-1080772-5.patch921 bytes

I just had a look, and it seems the patch has also caught the backup file that Kate automatically generates (you can tell by the ~ at the end). I guess because this file doesn't exist, it's diff'd against /dev/null - sorry. I've learnt to read the patch file to make sure this happens again.

I have rebuilt and re-uploaded the patch (after checking).

In hindsight - I think the problem was the use of 'git add -a' from the instructions... looks like git doesn't ignore "hidden" files on that switch.

fenstrat’s picture

fenstrat
he/him
Primary language English
Location Australia
commented
Status: Needs review » Reviewed & tested by the community

Good to go. Does what it says on the box. Revision prices don't get nuked on node update.

+++ b/uc_price_per_role.module
@@ -279,8 +279,8 @@ function uc_price_per_role_nodeapi(&$node, $op, $a3 = NULL, $a4 = NULL) {
       case 'insert':
-      case 'update':
         db_query("DELETE FROM {uc_price_per_role_prices} WHERE nid = %d", $node->nid);

Also, I'd say that this is totally superfluous as it's not run on node update any more. There should never be any prices in {uc_price_per_role_prices} on node insert as this is a new node.

fenstrat’s picture

fenstrat
he/him
Primary language English
Location Australia
commented
Status: Reviewed & tested by the community » Needs work

Whoo, I just realised that the patch in #5 doesn't do what is intended in all cases.

It does achieve the goal of not nuking any previous revision prices. However if you resave a node with revisioning disabled then you'll get 2 entries in the {uc_price_per_role_prices} with the same nid and vid. Problem. So what needs to be done is a query to see if there is any entry with the nid and vid if so update, otherwise insert.

fenstrat’s picture

fenstrat
he/him
Primary language English
Location Australia
commented
Status: Needs work » Needs review
StatusFileSize
new 1080772-8-fix-revision-prices.patch976 bytes

This is a far more straight forward approach. It only removes the current vid about to be saved.

horuskol’s picture

horuskol commented

nice catch - hadn't thought about non-revisioned systems.

fenstrat’s picture

fenstrat
he/him
Primary language English
Location Australia
commented
Status: Needs review » Reviewed & tested by the community

RTBC as per #9

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Reviewed & tested by the community » Fixed

Committed #8 to 6.x and 7.x, thanks for the patch.

Status: Fixed » Closed (fixed)

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