When purchasing an order that has more then one user role only one role in the order gets added to the account, I believe that all the role expiration stuff from uc_roles works correctly, but it is just the adding of the actually role to an account to results in an error.

I believe the reason for this error is that the same reference to the user $account object is being passed around without updating to include the new roles, so the last added role will always win and the rest do not get added.

Here is the offending code:

function uc_roles_grant($account, $rid, $timestamp, $save_user = TRUE) {

  // First, delete any previous record of this user/role association.
  uc_roles_delete($account, $rid);

  if ($save_user) {
    // Punch the role into the user object.
    $roles_list = $account->roles + array($rid => _uc_roles_get_name($rid));
    user_save($account, array('roles' => $roles_list));
  }

Updating the roles in the $account object as they are also saved to the DB ensures that it will keep track of all roles that should be added, and subsequent calls to this function will not override the previous actions:

function uc_roles_grant($account, $rid, $timestamp, $save_user = TRUE) {

  // First, delete any previous record of this user/role association.
  uc_roles_delete($account, $rid);

  if ($save_user) {
    // Punch the role into the user object.
    $roles_list = $account->roles + array($rid => _uc_roles_get_name($rid));
    $account->roles=$roles_list;
    user_save($account, array('roles' => $roles_list));
  }

It looks like this error previous existed and was fixed back here: (but has somehow crept back in)
http://www.ubercart.org/forum/bug_reports/2019/uc_roles_fail_when_settin...

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

univate’s picture

Status: Active » Needs review
cha0s’s picture

Assigned: Unassigned » cha0s
FileSize
1.05 KB

Could you test this for me, please?

univate’s picture

FileSize
1.19 KB

No that doesn't work as using array_merge - "the keys get reindexed in a continuous way." (see... http://au2.php.net/manual/en/function.array-merge.php)

The recommendation on that page is to use the + operator.

I can see what you are doing with checking that roles is an array, but you probably don't need to do this as by default you should be able to assume that a user object has at least one role of authenticated or anonymous already.

see the user_load() function:

  $result = db_query('SELECT * FROM {users} u WHERE '. implode(' AND ', $query), $params);

  if ($user = db_fetch_object($result)) {
    $user = drupal_unpack($user);

    $user->roles = array();
    if ($user->uid) {
      $user->roles[DRUPAL_AUTHENTICATED_RID] = 'authenticated user';
    }
    else {
      $user->roles[DRUPAL_ANONYMOUS_RID] = 'anonymous user';
    }

I guess there is no harm having this check anyway if you really want, in that case I have tested the following and it works.

$account->roles = (is_array($account->roles) ? $account->roles : array()) + array($rid => _uc_roles_get_name($rid));

Here is my preference though, basically my previous patch, but removed the extra unnecessary variable to clean it up (I have also tested this and it works):

    $account->roles = $account->roles + array($rid => _uc_roles_get_name($rid));
    user_save($account, array('roles' => $account->roles));
univate’s picture

I think this fix may also fix this problem #364024: UC Roles Not Working for Anonymous Users or at least help identify where that issue is, as it sounds like the same problem of subsequent user_save() override previous ones.

rszrama’s picture

Version: 6.x-2.0-beta5 » 6.x-2.0-beta6
Status: Needs review » Fixed

If it didn't work before, it worked in my test of this patch just now. Committing!

Status: Fixed » Closed (fixed)

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

brisath’s picture

Version: 6.x-2.0-beta6 » 6.x-2.x-dev
Status: Closed (fixed) » Active

I'm experiencing this same problem with the latest 6.x-2.x-dev

This is happening for an authenticated user though.

Also, the "expiring roles" show fine on the user page, as if they have been assigned. However, when you look at the roles in drupal, only 1 role is assigned and the user doesn't have access like they should.

joachim’s picture

subscribing.

Also -- if I add two features to a product, one for each of the two roles I want to assign, will the user get two emails??

cha0s’s picture

Assigned: cha0s » Unassigned
thill_’s picture

Status: Active » Needs work

Subscribe, show stopper here, i sense a solution shortly.

joachim’s picture

What about allowing the 'assign role' feature to add several roles at once?
At the moment you can only pick one role, and so to give two roles with a product you need to add two features with a role, each presumably leading two two emails which from the POV of the user is broken.
Or should that be a separate issue?

rszrama’s picture

Status: Needs work » Fixed

That'd be a separate issue. As far as I can tell here the patch I previously applied was either reverted or didn't commit when I thought I had committed it. : P

brisath’s picture

I do like the idea of being able to assign multiple roles at once for a product, rather than having to add each individually. If this is opened as another issue, I'll be following.

joachim’s picture

Status: Fixed » Closed (fixed)
Issue tags: -uc_roles

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