Problem/Motivation

When a test user is never logged in the account is not blocked.

Steps to reproduce

  1. Create a new test account.
  2. Run cron
  3. Account is not blocked

Proposed resolution

In \Drupal\role_test_accounts\RoleTestAccountsManager::setStatusForRoleTestAccounts replace

      foreach ($this->getAllRoleTestAccounts() as $user) {
        if ($user->getLastAccessedTime()) {
          if ($user->isActive() && ($this->time->getRequestTime() - $block_after) > $user->getLastAccessedTime()) {
            $user->block();
            $user->save();
          }
          elseif ($user->isBlocked() && ($this->time->getRequestTime() - $block_after) < $user->getLastAccessedTime()) {
            $user->activate();
            $user->save();
          }
        }
      }

with

      foreach ($this->getAllRoleTestAccounts() as $user) {
        if ($user->isActive() && ($this->time->getRequestTime() - $block_after) > $user->getLastAccessedTime()) {
          $user->block();
          $user->save();
        }
        elseif ($user->isBlocked() && ($this->time->getRequestTime() - $block_after) < $user->getLastAccessedTime()) {
          $user->activate();
          $user->save();
        }
      }

Remaining tasks

  • Create patch
  • Review patch

Comments

perry.franken created an issue. See original summary.

perry.franken’s picture

StatusFileSize
new1.14 KB

Block users who are never logged in.

perry.franken’s picture

Issue summary: View changes
idebr’s picture

Status: Active » Needs review
idebr’s picture

Role test accounts are now blocked when the module is enabled, which somewhat defeats the purpose of this module.

bartlangelaan’s picture

Maybe it would be better to use the user creation time when the user has never logged in.

So, something like:

$last_access = $user->getLastAccessedTime() ?? getCreatedTime();

and always use $last_access instead of $user->getLastAccessedTime().

This makes sure that the test users are not immediately blocked after they are created, but are still blocked after a certain amount of time.

bartlangelaan’s picture

Status: Needs review » Needs work
perry.franken’s picture

StatusFileSize
new2.39 KB

new patch with suggested solution

perry.franken’s picture

Status: Needs work » Needs review
bartlangelaan’s picture

Status: Needs review » Needs work

I think the new solution looks good!

However, it looks like you accidentally added the previous patch to this patch.

Can you clean the patch and re-submit it?

perry.franken’s picture

StatusFileSize
new1.08 KB

Fixed the patch

perry.franken’s picture

perry.franken’s picture

Status: Needs work » Needs review
avpaderno’s picture

bartlangelaan’s picture

Status: Needs review » Needs work

While the patch looks good, it looks like it doesn't work. This is because $user->getLastAccessedTime(); always returns a string. When an account was never accessed, it returns "0". This doesn't invoke the null coalescing operator, so it doesn't give the desired effect.

Something like this is more likely to work:

        $last_access = $user->getLastAccessedTime();
        if (!$last_access) {
          $last_access = $user->getCreatedTime();
        }
perry.franken’s picture

StatusFileSize
new1.14 KB
perry.franken’s picture

Status: Needs work » Needs review
bartlangelaan’s picture

Status: Needs review » Reviewed & tested by the community

Looks good!

  • perry.franken authored b2da845 on 8.x-1.x
    Issue #3221711 by perry.franken, bartlangelaan, idebr: When a user is...
idebr’s picture

Status: Reviewed & tested by the community » Fixed

Committed, thanks!

Status: Fixed » Closed (fixed)

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