Add tests for user role administration

I don't think the @return for user_save() needs to be returned. Also this needs a test. Nice find!

Can we write a SimpleTest for this please? Thanks.

@steve, HEAD installation is broken, so don't pay any heed to the test bot.

Trying to resubmit.

Well the tests pass and the patch makes sense.

Would you mind rolling them into a single patch? Uploading just the test patch is a great way to demonstrate the bug in HEAD, I'll remember that :)

I applied both patches and the tests still failed.

This test is dependant on a fix to Simpletest itself: #335035: drupalPost() incorrectly submits input for disabled elements

Postponed until the fix is in.

Subscribing.

this is a rather annoying HEAD bug that has been lingering for a long time -- can we please break this issue up into a patch that fixes the problem, and a patch for the test ?

i don't see a need to hold up the actual fix just because simpletest is busted...

I agree. The bug fix is ready in #21. The simpletest fix is apparently complicated and need not delay.

The patch doesn't apply anymore.

I consider it drupal_write_record()'s fault for not handling its return value right. Looking at the code, it seems unintentional that it returns FALSE in this situation:

  ...
  else {
    $query = db_update($table)->fields($fields);
    foreach ($primary_keys as $key) {
      $query->condition($key, $object->$key);
    }
    $return = SAVED_UPDATED;
  }

  // Execute the SQL.
  if ($last_insert_id = $query->execute()) {
    if (isset($serial)) {
      // If the database was not told to return the last insert id, it will be
      // because we already know it.
      if (isset($options) && $options['return'] != Database::RETURN_INSERT_ID) {
        $object->$serial = $fields[$serial];
      }
      else {
        $object->$serial = $last_insert_id;
      }
    }
  }
  // If we have a single-field primary key but got no insert ID, the
  // query failed.
  elseif (count($primary_keys) == 1) {
    $return = FALSE;
  }

After a call to db_update(), it doesn't make sense to check for an insert ID or count the number of primary keys. The "insert ID" is instead the affected row count--and the part that overwrites SAVED_UPDATED to FALSE is trying to detect a failed INSERT.

Changing that last part to:

-  // If we have a single-field primary key but got no insert ID, the
-  // query failed.
-  elseif (count($primary_keys) == 1) {
+  // Return FALSE if we expected an insert ID and it was not returned.
+  elseif (count($primary_keys) == 1 && isset($options['return']) && $options['return'] == Database::RETURN_INSERT_ID) {
     $return = FALSE;
   }

would seem to make more sense. This would make drupal_write_record() reliably return SAVED_UPDATED for updates rather than being hit-or-miss, since detecting if a db_update() failed isn't possible (as far as I can tell?) with the new DB API.

The problem with drupal_write_record() has been fixed, which now fixes this issue as well. See #626790: drupal_write_record returns FALSE for valid update queries

I apologize, I didn't notice that this still needs a test. If comment #26 is correct, the test apparently depends on #335035: drupalPost() incorrectly submits input for disabled elements which has been stalled for months. Is there a way we can make the test not depend on the SimpleTest fixes?

I wrote at test for user roles. I found a small error in user_role_load(). It was using is_int() on a number that can also be a string because it came from the url.

The error appears when trying to delete a role (that is not possible right now because of that). It is a small one line fix.

The tests still rely on #335035: drupalPost() incorrectly submits input for disabled elements being committed.

#39: 334671.patch queued for re-testing.

Reroll

Another reroll. The other one certainly did not pass tests.

Weird. The patch does not fail testing on my machine. Sorry testbot, I am making you try again.

#44: 334671_2.patch queued for re-testing.

OK, maybe the xpath likes a[contains better than a[starts-with. Both still works on my machine, but lets see what the test here says.

OK, test here with no xpath in it. Weird, but let's see if this will pass.

-  $field = is_int($role) ? 'rid' : 'name';
+  $field = is_numeric($role) ? 'rid' : 'name';

This isn't correct, unfortunately - see #619584: Deleting user role throws PHP notices and prevents delete operation for the full discussion of how to fix that bug. For that reason, I'd suggest removing that line as well as the testAddRole() test from this patch, since it's being fixed over at that other issue.

***

The other tests look pretty good on a quick glance. However, in addition to asserting that the correct text appears on the screen and that the correct role checkboxes are checked, the most important thing that needs to be asserted here is that if we actually load the user account object, $account->roles contains the correct role list - and the current patch does not do that.

+  function testCreateUserAndAssignRole()  {

Shouldn't this really be named something more like testAssignAndRemoveRole()?

+    $user = $this->drupalCreateUser();

Normally we don't user $user here so as not invite confusion with the global $user object. I've seen $web_user used in the tests in this case, or even just $account should be fine.

+    $user2 = user_load_by_name($edit['name']);

No need to use a different variable name here; since it's in a different test, you can just use whatever same variable name was chosen above.

Reroll after #619584: Deleting user role throws PHP notices and prevents delete operation was committed. The test now checks that the user object has the role too - that makes a lot of sense - thanks David :)

#53: 334671_5.patch queued for re-testing.

#53: 334671_5.patch queued for re-testing.

subscribing

Patch needs reroll against HEAD (can't right now) BUT test is fully functional and code reviewed.

I applied the patch cleanly to HEAD and tests do not fail, so setting back.

#53: 334671_5.patch queued for re-testing.

#53: 334671_5.patch queued for re-testing.

2 months after the roll. Does it still apply?

Committed to CVS HEAD. Thanks.