Due to erroneous handling of database access in lines 234-245 function inactive_user_cron(), users are never removed from table inactive_users.

These lines should be changed to:

// reset notifications if recent user activity
    $users = db_query('SELECT uid FROM {inactive_users} WHERE uid <> 1'));
    if ($users) {
      while($uid = db_fetch_object($users)) {
        $u = db_fetch_object(db_query('SELECT access, name FROM {users} WHERE uid = %d', $uid->uid));
        if ($u->access > time() - 604800) {
          // user activity in last week, remove from inactivity table
          db_query('DELETE FROM {inactive_users} WHERE uid = %d', $uid->uid);
          watchdog('user', t('recent user activity: %user removed from inactivity list', array('%user' => $u->name)), WATCHDOG_NOTICE, l(t('edit user'), "admin/user/edit/$uid->uid"));
        }
      }
    }

The bugs are in all versions of this module.

Plus, in general, some of the database queries are substantially not efficient.

Comments

ekrispin’s picture

ekrispin commented

Sorry, there is one extra ")" in second line.

It should be:

// reset notifications if recent user activity
    $users = db_query('SELECT uid FROM {inactive_users} WHERE uid <> 1');
    if ($users) {
      while($uid = db_fetch_object($users)) {
        $u = db_fetch_object(db_query('SELECT access, name FROM {users} WHERE uid = %d', $uid->uid));
        if ($u->access > time() - 604800) {
          // user activity in last week, remove from inactivity table
          db_query('DELETE FROM {inactive_users} WHERE uid = %d', $uid->uid);
          watchdog('user', t('recent user activity: %user removed from inactivity list', array('%user' => $u->name)), WATCHDOG_NOTICE, l(t('edit user'), "admin/user/edit/$uid->uid"));
        }
      }
    }
deekayen’s picture

deekayen commented
Status: Active » Needs work

Don't you think it can be cut down a lot more than that? I haven't tried it, but I'm picturing something like:

db_query('DELETE FROM {inactive_users} WHERE uid IN (SELECT uid FROM {inactive_users} AS iu INNER JOIN {users} AS u ON u.uid = iu.uid WHERE u.access > %d)', array(time() - 6048800));
deekayen’s picture

deekayen commented
Status: Needs work » Needs review

Supposedly closed by #661882: Updated test case and several fixes. Someone needs to verify.

deekayen’s picture

deekayen commented
Status: Needs review » Closed (duplicate)

#77835: Inactive_user does not delete users