Saving prices per role removes previous revisions' prices [#1080772]

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);
      ...

Comment	File	Size	Author
#8	1080772-8-fix-revision-prices.patch	976 bytes	fenstrat
#5	revision-fix-for-saving-prices-1080772-5.patch	921 bytes	horuskol
#2	revision-fix-for-saving-prices-1080772-2.patch	21.08 KB	horuskol

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

horuskol CreditAttribution: horuskol commented 4 March 2011 at 01:26

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?

Comment #2

horuskol CreditAttribution: horuskol commented 14 March 2011 at 23:23

File	Size
revision-fix-for-saving-prices-1080772-2.patch	21.08 KB

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

Comment #3

horuskol CreditAttribution: horuskol commented 14 March 2011 at 23:25

Status:

Needs work

» Needs review

Forgot to set to "needs review"

Comment #4

fenstrat

he/him

English

Australia

CreditAttribution: fenstrat commented 19 March 2011 at 14:13

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.

Comment #5

horuskol CreditAttribution: horuskol commented 22 March 2011 at 23:08

Status:

Needs work

» Needs review

File	Size
revision-fix-for-saving-prices-1080772-5.patch	921 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.

Comment #6

fenstrat

he/him

English

Australia

CreditAttribution: fenstrat commented 25 March 2011 at 11:42

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.

Comment #7

fenstrat

he/him

English

Australia

CreditAttribution: fenstrat commented 3 April 2011 at 01:15

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.