logic in function uc_roles_action_order_renew is wrong. [#569920]

function uc_roles_action_order_renew($order) {
  // Load the order's user and exit if not available.
  if (!($account = user_load($order->uid))) {
    return;
  }

  // Loop through all the products on the order.
  foreach ($order->products as $product) {
    // Look for any role promotion features assigned to the product.
    $roles = db_query("SELECT * FROM {uc_roles_products} WHERE nid = %d", $product->nid);

    while ($role = db_fetch_object($roles)) {
      // Product model matches, or was 'any'.
      if (!empty($role->model) && $role->model != $product->model) {
        continue;
      }

      // Determine the expiration timestamp for the role.
      $expiration = _uc_roles_product_get_expiration($role, $product->qty, $existing_role->expiration);

      // Grant the role to the user.
      uc_roles_grant($account, $role->rid, $expiration, TRUE, !$settings['message']);

      // Get the new expiration (if applicable)
      $new_expiration = db_fetch_object(db_query("SELECT * FROM {uc_roles_expirations} WHERE uid = %d AND rid = %d", $account->uid, $role->rid));

      // Trigger role email.
      ca_pull_trigger('uc_roles_notify_'. $op, $order, $new_expiration);

      // Leave an order comment.
      $existing_role = db_fetch_object(db_query("SELECT * FROM {uc_roles_expirations} WHERE uid = %d AND rid = %d", $account->uid, $role->rid));
      if (!is_null($existing_role->expiration)) {
        $op = 'renew';
        $comment = t('Customer user role %role renewed.', array('%role' => _uc_roles_get_name($role->rid)));
      }
      else {
        $op = 'grant';
        $comment = t('Customer granted user role %role.', array('%role' => _uc_roles_get_name($role->rid)));
      }

      uc_order_comment_save($order->order_id, $user->uid, $comment);
    }
  }
}

correct version should looks like

function uc_roles_action_order_renew($order) {
  // Load the order's user and exit if not available.
  if (!($account = user_load($order->uid))) {
    return;
  }

  // Loop through all the products on the order.
  foreach ($order->products as $product) {
    // Look for any role promotion features assigned to the product.
    $roles = db_query("SELECT * FROM {uc_roles_products} WHERE nid = %d", $product->nid);

    while ($role = db_fetch_object($roles)) {
      // Product model matches, or was 'any'.
      if (!empty($role->model) && $role->model != $product->model) {
        continue;
      }

      // Determine the expiration timestamp for the role.
      $expiration = _uc_roles_product_get_expiration($role, $product->qty, $existing_role->expiration);

      // Leave an order comment.
      $existing_role = db_fetch_object(db_query("SELECT * FROM {uc_roles_expirations} WHERE uid = %d AND rid = %d", $account->uid, $role->rid));
      if (!is_null($existing_role->expiration)) {
        $op = 'renew';
        $comment = t('Customer user role %role renewed.', array('%role' => _uc_roles_get_name($role->rid)));
        // Grant the role to the user.
        uc_roles_renew($account, $role->rid, $expiration);
      }
      else {
        $op = 'grant';
        $comment = t('Customer granted user role %role.', array('%role' => _uc_roles_get_name($role->rid)));
        // Grant the role to the user.
        uc_roles_grant($account, $role->rid, $expiration, TRUE);

      }

      uc_order_comment_save($order->order_id, $user->uid, $comment);

      // Get the new expiration (if applicable)
      $new_expiration = db_fetch_object(db_query("SELECT * FROM {uc_roles_expirations} WHERE uid = %d AND rid = %d", $account->uid, $role->rid));

      // Trigger role email.
      ca_pull_trigger('uc_roles_notify_'. $op, $order, $new_expiration);
    }
  }
}

Comment	File	Size	Author
#31	569920_role_emails.patch	1.3 KB	Island Usurper
#13	569920_action_renew.patch	4.13 KB	Island Usurper
#11	569920_action_renew.patch	3.89 KB	Island Usurper
#6	ubercart-569920.patch	2.52 KB	esrms
#4	ubercart-569920.patch	2.44 KB	esrms
#2	ubercart-569920.patch	2.11 KB	univate

Comments

Comment #1

esrms commented 6 September 2009 at 14:37

function uc_roles_action_order_renew($order) {
  // Load the order's user and exit if not available.
  if (!($account = user_load($order->uid))) {
    return;
  }

  // Loop through all the products on the order.
  foreach ($order->products as $product) {
    // Look for any role promotion features assigned to the product.
    $roles = db_query("SELECT * FROM {uc_roles_products} WHERE nid = %d", $product->nid);

    while ($role = db_fetch_object($roles)) {
      // Product model matches, or was 'any'.
      if (!empty($role->model) && $role->model != $product->model) {
        continue;
      }

      // Leave an order comment.
      $existing_role = db_fetch_object(db_query("SELECT * FROM {uc_roles_expirations} WHERE uid = %d AND rid = %d", $account->uid, $role->rid));

      // Determine the expiration timestamp for the role.
      $expiration = _uc_roles_product_get_expiration($role, $product->qty, $existing_role->expiration);

      if (!is_null($existing_role->expiration)) {
        $op = 'renew';
        $comment = t('Customer user role %role renewed.', array('%role' => _uc_roles_get_name($role->rid)));
        // Grant the role to the user.
        uc_roles_renew($account, $role->rid, $expiration);
      }
      else {
        $op = 'grant';
        $comment = t('Customer granted user role %role.', array('%role' => _uc_roles_get_name($role->rid)));
        // Grant the role to the user.
        uc_roles_grant($account, $role->rid, $expiration, TRUE);

      }

      uc_order_comment_save($order->order_id, $user->uid, $comment);

      // Get the new expiration (if applicable)
      $new_expiration = db_fetch_object(db_query("SELECT * FROM {uc_roles_expirations} WHERE uid = %d AND rid = %d", $account->uid, $role->rid));

      // Trigger role email.
      ca_pull_trigger('uc_roles_notify_'. $op, $order, $new_expiration);
    }
  }
}

Comment #2

univate commented 7 September 2009 at 04:38

Status	File	Size
new	ubercart-569920.patch	2.11 KB

Its a little hard to understand the problem without any explanation of whats going on?

I have created a patch so I (and anyone else) can actually see what you are changing.

Comment #3

mikejoconnor commented 7 September 2009 at 09:45

Status:

Active

» Postponed (maintainer needs more info)

I appreciate the patch, could you please provide an explanation to go along with it?

Comment #4

esrms commented 7 September 2009 at 13:30

Status	File	Size
new	ubercart-569920.patch	2.44 KB

Note: Code quality is not assured.
Updated the patch to include the following changes:
1. Function signature from "function uc_roles_action_order_renew($order)" to "function uc_roles_action_order_renew($order, $settings)"
So, later, uc_roles_grant($account, $role->rid, $expiration, TRUE, !$settings['message']); statement makes sense.
2. Change the execution order, so variables have values assigned before been used.
3. Use uc_roles_renew to renew existing role before uc_roles_notify_renew is triggered.

Comment #5

mikejoconnor commented 8 September 2009 at 08:50

Status:

Postponed (maintainer needs more info)

» Needs review

esrms

Thanks, the patch looks good, however I still need to test it.

Comment #6

esrms commented 8 September 2009 at 12:01

Status	File	Size
new	ubercart-569920.patch	2.52 KB

Updated patch:
Change the expiration caculation to:
$expiration = _uc_roles_product_get_expiration($role, $product->qty, !is_null($existing_role->expiration) ? $existing_role->expiration : NULL);

Comment #7

univate commented 9 September 2009 at 01:50

possibly related issue: #571822: uc_role expiration end time fixes

Comment #8

Chad_Dupuis commented 9 September 2009 at 12:58

subscribing

Comment #9

Island Usurper commented 9 September 2009 at 17:02

Status:

Needs review

» Needs work

Part of this was just committed to the Bazaar repository in a similar issue. If you don't have Bazaar, I'll see about updating the CVS dev version so you can reroll the patch.

Comment #10

Island Usurper commented 9 September 2009 at 19:26

Issue tags:

+Release blocker

There's something goofy with the roles module, or there wouldn't be as many issues about it as there are. Let's consolidate those here, since it looks like they're concentrated around this function.

Comment #11

Island Usurper commented 10 September 2009 at 14:36

Status:

Needs work

» Needs review

Status	File	Size
new	569920_action_renew.patch	3.89 KB

Here's the patch as it applies to the latest version in Bazaar. The two functions uc_roles_grant() and uc_roles_renew() didn't work the same way with regard to the drupal_set_message(), so I added the $silent parameter to uc_roles_renew() and fixed up the messages a bit.

Comment #12

esrms commented 10 September 2009 at 23:04

+
+ // Grant the role to the user.
+ uc_roles_renew($account, $role->rid, $expiration, !$settings['message']);

Actually, the comment should be something like:
// Renew the role of the user.

It's my mistake.

Comment #13

Island Usurper commented 11 September 2009 at 13:44

Status	File	Size
new	569920_action_renew.patch	4.13 KB

Easy enough to fix.

Comment #14

jasont28 commented 11 September 2009 at 17:15

First of all, it is great to see this issue has been found by others and being worked on. Great work guys!

Second, it seems we now have two separate codes being implemented here, esrms and Island Usurper. Island Usurper, yours doesn't build upon Esrms' code, it is a different approach. Is it fair to say that before it gets out of hand, we choose one code to build upon instead of numerous codes being thrown in here? This would make it easier for others to make additions knowing there is only one code to work with.

Currently the process for esrms is:

Checking if there is an existing role and finding the expiration date if it exists
If it does exist, renew variable is set and role is renewed with the renew comment being set.
If it doesn't exist, grant variable is set and role is granted with grant comment being set.
Comment is saved
Find new expiration and send notification to the user

Currently the process for Island Usurper is:

Checking if there is an existing role and finding the expiration date if it exists
Find new expiration and send notification to the user (This presumes we are using the default Ubercart roles.ca.inc file as no other mention of ca_pull_trigger('uc_roles_notify_'. $op, $order, $new_expiration); is used.)
If an existing role does exist, renew variable is set and role is renewed with the renew comment being set.
If it doesn't exist, grant variable is set and role is granted with grant comment being set.
Comment is saved.

The difference between the two is that Island Usurpers code doesn't renew or grant the roles prior to getting the new expiration date and send notification. Should we blend the two together or choose one code to work upon?

Comment #15

Island Usurper commented 11 September 2009 at 19:17

I think the differences between the patches are due to the fact that esrms' is based off of RC6, and mine is based off the current HEAD in Ubercart's Bazaar repository. There might still be some differences, but they should end up causing the same things to happen. The reason this happened is because another patch to this function got committed that did part of what esrms' patch does. Mine should just be getting the rest of it in.

I've updated CVS so that it has all of the changes that Bazaar does. Hopefully looking at that version of the function will make it clear why I updated the patch.

Comment #16

mairav commented 15 September 2009 at 03:34

I have rc6 version. Does the dev version include this patch?
Do you know when will the rc7 be launched?

This is for a site that will be launched in some days, and I'm not sure to use a dev version... what do you recommend me?

(sorry for my english)

Comment #17

Island Usurper commented 15 September 2009 at 12:41

The dev version does not include this patch, but it should still apply cleanly.

The plan is to fix all of the Ubercart issues that have been tagged as "Release blocker" (like this one) by Friday, and then release rc7.

Comment #18

mairav commented 15 September 2009 at 14:40

Ok, I've already applyed the patch given in http://drupal.org/node/569920#comment-2017570 (as I have the rc6 version) and it seems to work fine. Now its sending mails again (It stop sending them in rc6).
Thanks for this patchs!

I'll be waiting for the rc7! I'm really happy with ubercart! thanks again.

Comment #19

Island Usurper commented 15 September 2009 at 16:00

Status:

Needs review

» Fixed

Glad to hear it. Thanks to everyone who worked on this. Committed to the Bazaar repository.

Comment #20

jasont28 commented 16 September 2009 at 15:10

Great work guys.

Island Usurper, that makes sense as to why your patch changes mentioned here were not working for me. Whilst waiting for the Rc7 update, I have used the patch from esrms and it works great. All roles are provided correctly and all emails are sent as desired. I look forward to the Rc7 update. Great work esrms and Island Usurper! Big thanks.

Jason

Comment #21

vood002 commented 21 September 2009 at 18:24

Thanks for the work on this

Comment #22

mairav commented 24 September 2009 at 13:57

I installed the rc7 release that is supposed to fix this problem, but as far as I tested it, the mails are not being sent again. Could someone install the new release? are the mails being sent for you?

Comment #23

Island Usurper commented 24 September 2009 at 18:08

I'm getting emails from purchasing roles. Make sure that the order is getting to the status it needs to according to the conditional actions for granting roles.

Comment #24

mairav commented 24 September 2009 at 19:15

I have the default conditional actions that say the "completed" status has to be reached to grant or renew a role. The rol is granted but the email is not sent. The operation gets the completed status. I don't know what could I be doing wrong.

Comment #25

Island Usurper commented 24 September 2009 at 19:36

OK, when I said "getting emails", I was really talking about how Devel will capture emails and send them to the watchdog since my test sites don't have SMTP. Check your devel settings to see if that got changed at some point.

The emails are actually sent by a different conditional action that is triggered by the role being granted, so check that it is set up properly as well.

Comment #26

mairav commented 24 September 2009 at 20:05

I also have the default action "Notify customer when a role is granted", with no changes.
I don't have devel enabled now, and I made no changes since it worked with the patch, and stopped working after I installed the rc7.
I'm sorry I can't tell you much, It's my first site with drupal, I've learned a lot, but still need to learn a lot more.

Comment #27

mairav commented 25 September 2009 at 15:05

I enabled devel again, change to Log only in SMTP Library in devel settings, bought a role for an user.
In the watchdog, I got only the email about the order, but no email for the role assignment.

Comment #28

jasont28 commented 26 September 2009 at 15:16

The emails (role granted or renewed) are not sending after updating to rc7 for me either. Everything was working perfectly with rc6 after adding the patch. No changes were made in the conditional actions or any other settings. I will spend some time reviewing the code and see what I can find.

Comment #29

BenK commented 29 September 2009 at 23:21

Version:	6.x-2.0-rc6	» 6.x-2.0-rc7
Status:	Fixed	» Active

I just upgraded to Ubercart 6.x-2.0-rc7 and the role notification e-mails are not working for me either. I will investigate further as well....

P.S. The issue appears to be confined to role notification e-mails. File downloads notification e-mails are working fine for me in rc7.

Thanks,
Ben

Comment #30

sgdev commented 30 September 2009 at 00:56

I am running rc7 and noticing a similar, yet different, problem. An email is being sent to the user, but not the right one.

When a user is granted a role for the first time, they are receiving the "renewed" email rather than the "granted" email. The message says the role was renewed, yet the user never had the role prior to placing an order.

Comment #31

Island Usurper commented 20 October 2009 at 15:19

Status:

Active

» Needs review

Status	File	Size
new	569920_role_emails.patch	1.3 KB

I think I found the problem. The expiration argument passed to the notification trigger could sometimes be FALSE instead of an object. ca_pull_trigger() checks the types of the arguments passed into it, so when that happened, it refused to perform any of the email actions. Patch makes sure that the most up-to-date data on the new role's expiration is given.

In my tests, I didn't have any confusion between granting and renewing emails. This may have been fixed after rc7 was released.

Comment #32

jasont28 commented 20 October 2009 at 15:47

Great work Island Usurper. Good to have this issue finally under wraps. I tested both new and existing users, both received the necessary email notifications. If there is any issue with the expiration reminders I'll let you know.

Jason