I need an user action to unblock users, and modifying user.module solved this for me.
The action lets me do unblocking using the rules module very straightforward.

Attached is a patch to user.module that implements function user_unblock_user_action().
Patch is done against user.module in drupal 6.13.

Note: This is a patch to solve a particular issue on the site I'm building. There are probably
cleaner ways to deal with this problem. I spent too many hours trying to find a solution already
though, so I solved it this way. I don't propose this is a good solution to go into core, but
rather an example of my feature request.

CommentFileSizeAuthor
#77 interdiff-74_75.txt2.1 KBpoker10
#77 interdiff-69_74.txt2.76 KBpoker10
#77 512042-75.patch5.88 KBpoker10
#75 512042-74_complete.patch5.88 KBstefanos.petrakis@gmail.com
#74 512042-74.patch4 KBstefanos.petrakis@gmail.com
#74 interdiff_69-74.txt3.33 KBstefanos.petrakis@gmail.com
#74 512042-74_tests_only.patch4 KBstefanos.petrakis@gmail.com
#69 interdiff_67-69.txt2.77 KBpoker10
#69 512042-69.patch3.79 KBpoker10
#67 edit_add_user_action-512042-67.patch4.3 KBvbouchet
#54 interdiff_52_54.txt961 bytesfelribeiro
#54 edit_add_user_action-512042-54.patch4.3 KBfelribeiro
#52 interdiff-49-52.txt1.8 KBfelribeiro
#52 edit_add_user_action-512042-52.patch4.33 KBfelribeiro
#49 add_user_action-512042-49.patch3.71 KBvbouchet
#47 add_user_action-512042-47.patch3.77 KBvbouchet
#41 screenshot.png57.42 KBvbouchet
#41 user-unblock-user-action-512042-41.patch3.67 KBvbouchet
#28 unblock-user-action-512042-27.patch3.46 KBCaligan
#24 interdiff.txt1.08 KBryan.gibson
#23 unblock-user-action-512042-23.patch3.44 KBryan.gibson
#20 unblock-user-action-512042-20.patch3.45 KBscottrigby
#20 interdiff.txt2.88 KBscottrigby
#18 512042-18.patch3.41 KBkbasarab
#16 512042-16.patch3.34 KBkbasarab
#14 512042-14.patch3.34 KBkbasarab
#12 512042-12.patch3.34 KBkbasarab
#12 interdiff.txt2.08 KBkbasarab
#11 drupal-512042-11.patch1.27 KBtim.plunkett
#8 512042-unblock-user-action.patch1.28 KBstpaultim
#2 512042-unblock-user-action.patch1.26 KBbojanz
user_module.patch1.29 KBperarnet
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

perarnet’s picture

Title: Add user action to user.module » Add user action "unblock current user"to user.module

edit: changed to a more descriptive title

bojanz’s picture

Title: Add user action "unblock current user"to user.module » Add user action "unblock current user" to user.module
Version: 6.13 » 8.x-dev
Status: Active » Needs review
FileSize
1.26 KB

Seems odd to me that there's no unblock action, while there is a block action. The old hook_user_operations() supports both.
I get this as a feature request for VBO 7.x (because VBO for D7 no longer supports hook_user_operations(), just core actions and rules).
So let's try to add it to core. It's a small amount of code.

Patch should apply to both 7.x and 8.x.

colin_young’s picture

sagannotcarl’s picture

Patch in #2 worked for me.

xjm’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

Patch looks good. We'll want to add an automated test for the functionality, I think. Note also that the Drupal 8.x patch will need to be rerolled, because of #22336: Move all core Drupal files under a /core folder to improve usability and upgrades.

ppatterson-edc’s picture

This patch seems unfinished - it looks like it stops abruptly on line 34?

xjm’s picture

#6: Looks okay to me. Also, it applied fine on testbot. See: http://drupal.org/node/367392

stpaultim’s picture

Status: Needs review » Needs work
FileSize
1.28 KB

OK, I'm a total newbie that just re-rolled this patch under the guidance of webchick and xjm - so hopefully I did this right.

Applied at commit: d2901d051d55bdbb08896af99351e7acabffe4b5

stpaultim’s picture

Status: Needs work » Needs review
tim.plunkett’s picture

Issue tags: -user.module

When this was rerolled, it was correct.

However, #1361228: Make the user entity a classed object happened and now there is no more user_save(). So this needs work again.

Also, it still needs tests.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
1.27 KB

Rerolled to not call user_save().
No other core action has test coverage (except those used to test the action module itself).

kbasarab’s picture

FileSize
2.08 KB
3.34 KB

I don't know that this will pass but here is a first attempt. On my local I keep getting this error when testing:

08-Jun-2012 13:42:43 UTC] Uncaught PHP Exception Drupal\Core\Database\DatabaseExceptionWrapper: "SQLSTATE[42S02]: Base table or view not found: 1146 Table 'd8git.simpletest687327watchdog' doesn't exist: SELECT 1 FROM {watchdog} WHERE message = :message AND variables = :variables LIMIT 0, 1; Array

This applies tim.plunkett's patch and then takes a stab at writing a test case for it based on comment module's implementation of action tests.

Assuming Failure, looking to see what TestBot has to say though.

Status: Needs review » Needs work

The last submitted patch, 512042-12.patch, failed testing.

kbasarab’s picture

Status: Needs work » Needs review
FileSize
3.34 KB

Had a whitespace issue anyways but that was an odd Failure. Trying again...

Status: Needs review » Needs work

The last submitted patch, 512042-14.patch, failed testing.

kbasarab’s picture

Status: Needs work » Needs review
FileSize
3.34 KB

Urgh. Sorry About that. Took out a bracket accidentally when removing the whitespace.

Status: Needs review » Needs work

The last submitted patch, 512042-16.patch, failed testing.

kbasarab’s picture

Status: Needs work » Needs review
FileSize
3.41 KB

Had to enable dblog to get Watchdog to work (forgot about that) and also changes second test so that we load the blocked user again before unblocking said user.

xjm’s picture

Status: Needs review » Needs work
Issue tags: -Needs tests

Thanks @kbasarab! This test coverage looks great.

  1. +++ b/core/modules/user/lib/Drupal/user/Tests/UserActionsTest.phpundefined
    @@ -0,0 +1,69 @@
    +  function testUserBlockUnBlockActions() {
    

    Do we want to add additional test coverage for when the uid is provided via the context parameter? (And when is that, anyway? See below.)

    Additional test coverage might include a test module to test the "current user" part of it (when no parameters are passed and it falls back to the current user), but that's less important.

  2. +++ b/core/modules/user/user.moduleundefined
    @@ -3408,6 +3408,12 @@ function user_action_info() {
    +      'label' => t('Unblock current user'),
    
    @@ -3436,6 +3442,26 @@ function user_block_user_action(&$entity, $context = array()) {
    + * Unblocks the current user.
    

    I'd change this to "Unblocks a user." It might be the current user, but it might also be a different user provided from the passed-in entity or context. Also, let's get some parameter descriptions for this docblock.

    (We should have a followup issue for the block action for these points as well.)

  3. +++ b/core/modules/user/user.moduleundefined
    @@ -3436,6 +3442,26 @@ function user_block_user_action(&$entity, $context = array()) {
    +  elseif (isset($context['uid'])) {
    +    $uid = $context['uid'];
    +  }
    

    Would be good to add a comment explaining this condition as well. (I do realize this is simply parallel to user_block_user_action().)

  4. +++ b/core/modules/user/lib/Drupal/user/Tests/UserActionsTest.phpundefined
    @@ -0,0 +1,69 @@
    +    // Block a user
    ...
    +    // Unblock a user
    

    Minor note: missing periods on these comments.

scottrigby’s picture

Status: Needs work » Needs review
FileSize
2.88 KB
3.45 KB

Reroll & interdiff attached:

  1. I'm adding a follow up issue to add a WebTestBase::drupalCreateContext() for more useful context-based testing: #1671218: Add create context method to test base class
  2. Changed `current` to `a` user
  3. Added a quick inline comment. See 1.
  4. minor but hey - that's what coding standards are for :)
xjm’s picture

Issue tags: +Novice
+++ b/core/modules/user/lib/Drupal/user/Tests/UserActionsTest.phpundefined
@@ -0,0 +1,69 @@
+    $this->assertWatchdogMessage('Blocked user %name.', array('%name' => $unblocked_user->name), t('Found watchdog message'));
...
+    $this->assertWatchdogMessage('Unblocked user %name.', array('%name' => $blocked_user->name), t('Found watchdog message'));

Ooops, missed this on the previous review. The assertion message texts do not need to be translated. See: http://drupal.org/simpletest-tutorial-drupal7#t

Tagging novice for an easy fix.

scottrigby’s picture

@xjm, I said I'd add a quick note about the history of user_block_user_action() - these were added in #148410: Actions in core (see #42, #50).

ryan.gibson’s picture

Okay, made the changes from #21.

ryan.gibson’s picture

FileSize
1.08 KB

And interdiff.

xjm’s picture

Status: Needs review » Needs work
Issue tags: -Novice
+++ b/core/modules/user/user.moduleundefined
@@ -3441,6 +3447,27 @@ function user_block_user_action(&$entity, $context = array()) {
 /**
+ * Unblocks the current user.
+ *

Oops, this docblock is still wrong. (It should say "Unblocks a user." or perhaps "Unblocks a user, defaulting to the current user.")

Let's also get that followup filed to make these same corrections for the existing block action.

xjm’s picture

Issue tags: +Novice
Caligan’s picture

Assigned: Unassigned » Caligan
Caligan’s picture

Status: Needs work » Needs review
FileSize
3.46 KB

Docblock correction.

Caligan’s picture

ryan.gibson’s picture

Assigned: Caligan » Unassigned
Status: Needs review » Needs work

Well, I tested the patch on Chrome, FF, and Safari. I ran into an issue when unblocking users.

The patch applies cleanly. I created a test user - I then blocked the account -this worked fine. But, when I tried to unblock the user, the page loads endlessly. If I close the browser window and then navigate to the page again, the user is unblocked, so that part is working.

ryan.gibson’s picture

Alright, I need to update this again - I think I jumped the gun - the unblocking action does take place, but it takes approximately 3 minutes to complete. Not sure if this is expected/acceptable or not. That being the case, I don't know if this should be marked for needs work or not.

This is on a fresh Drupal install.

ryan.gibson’s picture

Status: Needs work » Needs review

So someone else can review...

stefank’s picture

Hi

I just want to ask if this functionality is going to be applied in Drupal 7 core, maybe in the next update.
Many Thanks

DarrellDuane’s picture

Version: 8.x-dev » 7.x-dev
Status: Needs review » Reviewed & tested by the community

I've found that the patch in #8 works great for Drupal 7, and is ready to be incorporated into the core code.
I don't see a need to write a test for this code. The code is copied directly from the same code that performs the Block Current User operation, and slightly updated to Unblock instead of Block the user.

Perhaps I'm not following proper procedure, but I have gone ahead and changed the version of this issue to be 7.x-dev and changed the status to reviewed & tested by the community in the hopes that we can get this patch into the Drupal 7 core ASAP.

Do we have to wait for a Drupal 8 version of this code to be fixed before we can fix Drupal 7 ? Or, should I start a new issue on d.o. to get the Drupal 7 patch included?

tim.plunkett’s picture

Version: 7.x-dev » 8.x-dev
Status: Reviewed & tested by the community » Needs review

This needs to go into D8 first.

Anonymous’s picture

Issue summary: View changes
Status: Needs review » Needs work

This patch doesn't apply and doesn't work anymore, as D8's codebase has completely changed in the meantime.

StryKaizer’s picture

Version: 8.0.x-dev » 7.x-dev

After investigating this issue, it turns out that both the Block/Unblock action as the test coverage is already committed in 8.0.x

Actions:
core/modules/user/src/Plugin/Action/BlockUser.php
core/modules/user/src/Plugin/Action/UnblockUser.php

Tests:
core/modules/user/src/Tests/UserAdminTest.php

The test in #28 is written for an old drupal 8 version, and needs to be rewritten for D7.
Tagging this issue as drupal 7.x again

alberto56’s picture

Patch #2 works fine even with the latest version of D7, in case you need it.

DarrellDuane’s picture

Status: Needs work » Reviewed & tested by the community

Ok, changing this back to Reviewed & Tested by the community for Drupal 7.x since D8 is covered. Lets get it into D7, Thanks @alberto56 and @StryKaizer.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 28: unblock-user-action-512042-27.patch, failed testing.

vbouchet’s picture

Status: Needs work » Needs review
FileSize
3.67 KB
57.42 KB

Hi,

The situation seems a little bit messy between patches and Drupal versions.

I created a new patch including:

  • #2 from @bojanz patch which seems the only one for D7
  • I adapted the comment of user_unblock_user_action()
  • Re-roll/adapt D8 test case (based on #28 from @Caligan).

See screenshot for test case result.

Edit: Right after submitting this patch I was wondering if I should have re-rolled real D8 test cases instead of @Caligan one (I don't know if these tests differ). Let if know if it's the case and I will do it.

gaurav.goyal’s picture

Review done.

#41 is working for me.

SGhosh’s picture

Patch #41 -
Patch reviewed and tested. Confirming that it resolves the issue and works as required.

SGhosh’s picture

Status: Needs review » Reviewed & tested by the community
David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs work

Nice patch, but there are a couple issues.

  1. The documentation says that it unblocks the current user if none is specified, but unlike the "block user" action there is no code to do that.
  2. The tests look like they would be more useful if they also checked the user object to see if it's actually blocked/unblocked, rather than just testing the watchdog messages. Maybe we can look at where this got added to Drupal 8 (perhaps in #1846172: Replace the actions API) and see if there are any tests added there we could partially backport?
vbouchet’s picture

Status: Needs work » Needs review
FileSize
3.77 KB

I updated the patch to use the current user in case of no user is given. I update the test to check the user status instead of checking logs.

Status: Needs review » Needs work

The last submitted patch, 47: add_user_action-512042-47.patch, failed testing.

vbouchet’s picture

Status: Needs work » Needs review
FileSize
3.71 KB

Update files path.

Sophie.SK’s picture

Status: Needs review » Reviewed & tested by the community

This patch worked well for me. Thanks @vbouchet!

Fabianx’s picture

Status: Reviewed & tested by the community » Needs work

Nice work, unfortunately there is a little more work to do:

  1. +++ b/modules/user/user.module
    @@ -3749,6 +3755,36 @@ function user_block_user_action(&$entity, $context = array()) {
    +  // If neither of those are valid, then block the current user.
    

    block should be unblock in this line.

  2. +++ b/modules/user/user.test
    @@ -2445,3 +2445,64 @@ class UserValidateCurrentPassCustomForm extends DrupalWebTestCase {
    +    $this->clearWatchdog();
    

    The watchdog messages are never asserted.

    It might be good to bring that back, too.

  3. +++ b/modules/user/user.test
    @@ -2445,3 +2445,64 @@ class UserValidateCurrentPassCustomForm extends DrupalWebTestCase {
    +    $blocked_user = user_load($unblocked_user->uid);
    ...
    +    $unblocked_user = user_load($unblocked_user->uid);
    

    It might be a good idea to reset the entity cache before calling user_load to ensure that the data is actually persisted to the database.

felribeiro’s picture

Status: Needs work » Needs review
FileSize
4.33 KB
1.8 KB

I did the Fabian suggestions in #51.

Fabianx’s picture

+++ b/modules/user/user.test
@@ -2601,14 +2601,18 @@
+    clearstatcache();
...
+    clearstatcache();

Almost there, but clearstatcache() is not the right function.

We need to use:

https://api.drupal.org/api/drupal/modules!user!user.module/function/user...

with user_load($uid, TRUE) to reset the static cache.

Status: Needs review » Needs work

The last submitted patch, 54: edit_add_user_action-512042-54.patch, failed testing.

The last submitted patch, 54: edit_add_user_action-512042-54.patch, failed testing.

Fabianx’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: +Pending Drupal 7 commit

Back to RTBC, only cosmetical changes, so also marking for commit with the caveat of:

- assertWatchdogMessage needs to be checked if we really need it (or if this is already pre-existing in the code base)

This is just a note to myself.

Great work, everyone!

Fabianx’s picture

Issue tags: -Novice
stefan.r’s picture

Assigned: Unassigned » Fabianx
David_Rothstein’s picture

Should this be saved for a Drupal 7.60 release? We aren't as strict about that kind of thing in Drupal 7 as in Drupal 8 - however, as a new feature, it will get more "publicity" if we do that.

Also, since it adds a new translated string, it will get more time to have it translated if we don't commit it right before a release.

stefan.r’s picture

Issue tags: +Drupal 7.60 target
joseph.olstad’s picture

seeing as the next release IS going to be 7.60, might as well throw this in now. It's got tests and is pending commit.

joseph.olstad’s picture

Issue tags: -Drupal 7.60 target +Drupal 7.70 target

Bumping to 7.70. This didn't make it into 7.60

joseph.olstad’s picture

Maintain 'Pending Drupal 7 commit' and RTBC
Bumping up to 7.68

1) Has test coverage (included with patch #54)
2) Is already in D8 (see comment #37)

mcdruid’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: -Pending Drupal 7 commit

AFAICS there's a mistake in the comments in the patch in #54:

// If neither of those are valid, then unblock the current user.

...is in user_block_user_action(), whereas:

// If neither of those are valid, then block the current user.

...is in user_unblock_user_action()

I'd think these should be the other way around.

MustangGB’s picture

Issue tags: -Drupal 7.68 target +Drupal 7.69 target
vbouchet’s picture

I have the same understanding than @mcdruid, hence updating the patch.

MustangGB’s picture

Issue tags: -Drupal 7.69 target +Drupal 7.70 target
poker10’s picture

Patch #67 still has problems with comments.

function user_block_user_action() should have:

// If neither of those are valid, then block the current user.

function user_unblock_user_action() should have:

// If neither of those are valid, then unblock the current user.

Updating the patch to reflect this.

I also think that adding a function assertWatchdogMessage() in specific test is not a good idea. I have done a quick search and found that in comment.test we already have assertWatchdogMessage() and clearWatchdog() functions, but in other tests simple selects are used. This way we are going to duplicate existing code and in addition we will not be able to reuse these functions in other tests. So I think the best way here is to use only basic selects and create a follow-up issue to cleanup similar watchdog usages in tests and create a global assertWatchdogMessage() which would be used by all tests (where needed).

Attaching updated patch with interdiff.

poker10’s picture

Assigned: Fabianx » Unassigned
Issue tags: +Needs change record
stefanos.petrakis@gmail.com’s picture

Status: Needs review » Needs work

After reading through the changes in this issue, I came up with this issue #3311563: Safeguarding against UnblockUser::execute()'s method unblocking the anonymous user; the idea there is also relevant here, namely:

Should the user_unblock_user_action action take responsibility for disallowing the anonymous user from becoming activated (unblocked) to avoid possible vulnerabilities?

If we agree on this, then the patch would need to be updated.

poker10’s picture

Status: Needs work » Needs review

Thanks @stefanos.petrakis. I have checked this for D7 and it does not seems that it is possible to unblock anonymous user this way. The attempt to unblock anonymous user in D7 will instead try to create a new user (and throws an error):

$account = user_load(0);
$account = user_save($account, array('status' => 1));
PDOException: SQLSTATE[23505]: Unique violation: 7 ERROR: duplicate key value violates unique constraint "users_name_key" DETAIL: Key (name)=() already exists.: INSERT INTO users (uid, name, pass, mail, theme, signature, signature_format, created, changed, access, login, status, timezone, language, picture, init, data) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4, :db_insert_placeholder_5, :db_insert_placeholder_6, :db_insert_placeholder_7, :db_insert_placeholder_8, :db_insert_placeholder_9, :db_insert_placeholder_10, :db_insert_placeholder_11, :db_insert_placeholder_12, :db_insert_placeholder_13, :db_insert_placeholder_14, :db_insert_placeholder_15, :db_insert_placeholder_16); Array ( ) in drupal_write_record() (line 7537 of /includes/common.inc).

This is caused by the logic in the user_save() function:

if (!isset($account->is_new)) {
  $account->is_new = empty($account->uid);
}
...
if (is_object($account) && !$account->is_new) {
  ...
}
else {
  // Allow 'uid' to be set by the caller. There is no danger of writing an
  // existing user as drupal_write_record will do an INSERT.
  if (empty($account->uid)) {
    $account->uid = db_next_id(db_query('SELECT MAX(uid) FROM {users}')->fetchField());
  }
  ...
}

So I think we do not need to put the protection here, as this new unblock action will use the user_save() function.

stefanos.petrakis@gmail.com’s picture

Status: Needs review » Needs work

Thanks @poker10, I had a look too after reading your comment.
It is true, the snippet you provided demonstrates how this will not allow the status to change to 1 for the anonymous user.
Should we rely on that safeguard though?
The following snippet will work (hacking away at this, bear with me):

$a = user_load(0);

// Following two lines may be coming from a hook_user_load implementation
$a->is_new = FALSE;
$a->original = clone $a;

// Anonymous user will be saved with a status of 1.
user_save($a, ['status' => 1]);

So, my counter-argument is, why not simply safeguard this with a condition, just to make sure that a $uid that is empty or 0 will not be processed at all. That will skip further logic to become involved (e.g. user_save()).

stefanos.petrakis@gmail.com’s picture

This is what I have in mind.

The last submitted patch, 74: 512042-74_tests_only.patch, failed testing. View results

poker10’s picture

FileSize
5.88 KB
2.76 KB
2.1 KB

I think this looks good.

I have fixed some small documentation and comment issues (see interdiff-74_75.txt). I would also rather use direct drupal.org domain instead of some 3rd party service to link to this issue. And the last think, I have changed the watchdog call to the WATCHDOG_WARNING, probably it is a better severity for this case.

Attaching the final patch (with two interdiffs, interdiff-69_74.txt which was not present and interdiff-74_75.txt). +1 from me to RTBC (if tests passes on this patch).

  • mcdruid committed d5e4215 on 7.x
    Issue #512042 by vbouchet, kbasarab, stefanos.petrakis, poker10,...
mcdruid’s picture

Status: Needs review » Fixed
Issue tags: -Needs change record

Draft CR: https://www.drupal.org/node/3325143 ... this is just a placeholder for now (I'm concentrating on reviewing and committing).

Thank you to everyone that contributed!

poker10’s picture

I have updated the Change record.

Status: Fixed » Closed (fixed)

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