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.
Comments
Comment #1
perarnet CreditAttribution: perarnet commentededit: changed to a more descriptive title
Comment #2
bojanz CreditAttribution: bojanz commentedSeems 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.
Comment #3
colin_young CreditAttribution: colin_young commentedSubscribe (from #1293600: No option for unblock user.
Comment #4
sagannotcarl CreditAttribution: sagannotcarl commentedPatch in #2 worked for me.
Comment #5
xjmPatch 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.
Comment #6
ppatterson-edc CreditAttribution: ppatterson-edc commentedThis patch seems unfinished - it looks like it stops abruptly on line 34?
Comment #7
xjm#6: Looks okay to me. Also, it applied fine on testbot. See: http://drupal.org/node/367392
Comment #8
stpaultim CreditAttribution: stpaultim commentedOK, 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
Comment #9
stpaultim CreditAttribution: stpaultim commentedComment #10
tim.plunkettWhen 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.
Comment #11
tim.plunkettRerolled to not call user_save().
No other core action has test coverage (except those used to test the action module itself).
Comment #12
kbasarab CreditAttribution: kbasarab commentedI don't know that this will pass but here is a first attempt. On my local I keep getting this error when testing:
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.
Comment #14
kbasarab CreditAttribution: kbasarab commentedHad a whitespace issue anyways but that was an odd Failure. Trying again...
Comment #16
kbasarab CreditAttribution: kbasarab commentedUrgh. Sorry About that. Took out a bracket accidentally when removing the whitespace.
Comment #18
kbasarab CreditAttribution: kbasarab commentedHad 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.
Comment #19
xjmThanks @kbasarab! This test coverage looks great.
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.
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.)
Would be good to add a comment explaining this condition as well. (I do realize this is simply parallel to
user_block_user_action()
.)Minor note: missing periods on these comments.
Comment #20
scottrigbyReroll & interdiff attached:
WebTestBase::drupalCreateContext()
for more useful context-based testing: #1671218: Add create context method to test base classComment #21
xjmOoops, 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.
Comment #22
scottrigby@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).Comment #23
ryan.gibson CreditAttribution: ryan.gibson commentedOkay, made the changes from #21.
Comment #24
ryan.gibson CreditAttribution: ryan.gibson commentedAnd interdiff.
Comment #25
xjmOops, 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.
Comment #26
xjmComment #27
Caligan CreditAttribution: Caligan commentedComment #28
Caligan CreditAttribution: Caligan commentedDocblock correction.
Comment #29
Caligan CreditAttribution: Caligan commentedFollowup to #25 at #1683794: User action "block user" docblock correction.
Comment #30
ryan.gibson CreditAttribution: ryan.gibson commentedWell, 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.
Comment #31
ryan.gibson CreditAttribution: ryan.gibson commentedAlright, 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.
Comment #32
ryan.gibson CreditAttribution: ryan.gibson commentedSo someone else can review...
Comment #33
stefank CreditAttribution: stefank commentedHi
I just want to ask if this functionality is going to be applied in Drupal 7 core, maybe in the next update.
Many Thanks
Comment #34
DarrellDuane CreditAttribution: DarrellDuane commentedI'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?
Comment #35
tim.plunkettThis needs to go into D8 first.
Comment #36
Anonymous (not verified) CreditAttribution: Anonymous commentedThis patch doesn't apply and doesn't work anymore, as D8's codebase has completely changed in the meantime.
Comment #37
StryKaizerAfter 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
Comment #38
alberto56 CreditAttribution: alberto56 commentedPatch #2 works fine even with the latest version of D7, in case you need it.
Comment #39
DarrellDuane CreditAttribution: DarrellDuane commentedOk, 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.
Comment #41
vbouchetHi,
The situation seems a little bit messy between patches and Drupal versions.
I created a new patch including:
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.
Comment #42
gaurav.goyal CreditAttribution: gaurav.goyal at Innoraft commentedReview done.
#41 is working for me.
Comment #44
SGhosh CreditAttribution: SGhosh commentedPatch #41 -
Patch reviewed and tested. Confirming that it resolves the issue and works as required.
Comment #45
SGhosh CreditAttribution: SGhosh commentedComment #46
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedNice patch, but there are a couple issues.
Comment #47
vbouchetI 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.
Comment #49
vbouchetUpdate files path.
Comment #50
Sophie.SKThis patch worked well for me. Thanks @vbouchet!
Comment #51
Fabianx CreditAttribution: Fabianx as a volunteer commentedNice work, unfortunately there is a little more work to do:
block should be unblock in this line.
The watchdog messages are never asserted.
It might be good to bring that back, too.
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.
Comment #52
felribeiro CreditAttribution: felribeiro at CI&T commentedI did the Fabian suggestions in #51.
Comment #53
Fabianx CreditAttribution: Fabianx as a volunteer commentedAlmost 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.
Comment #54
felribeiro CreditAttribution: felribeiro at CI&T commentedThank you @Fabianx
Comment #57
Fabianx CreditAttribution: Fabianx as a volunteer commentedBack 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!
Comment #58
Fabianx CreditAttribution: Fabianx as a volunteer commentedComment #59
stefan.r CreditAttribution: stefan.r commentedComment #60
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedShould 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.
Comment #61
stefan.r CreditAttribution: stefan.r commentedComment #62
joseph.olstadseeing 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.
Comment #63
joseph.olstadBumping to 7.70. This didn't make it into 7.60
Comment #64
joseph.olstadMaintain '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)
Comment #65
mcdruidAFAICS there's a mistake in the comments in the patch in #54:
...is in
user_block_user_action()
, whereas:...is in
user_unblock_user_action()
I'd think these should be the other way around.
Comment #66
MustangGB CreditAttribution: MustangGB commentedComment #67
vbouchetI have the same understanding than @mcdruid, hence updating the patch.
Comment #68
MustangGB CreditAttribution: MustangGB commentedComment #69
poker10 CreditAttribution: poker10 at ActivIT s.r.o. commentedPatch #67 still has problems with comments.
function user_block_user_action()
should have:function user_unblock_user_action()
should have: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 haveassertWatchdogMessage()
andclearWatchdog()
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 globalassertWatchdogMessage()
which would be used by all tests (where needed).Attaching updated patch with interdiff.
Comment #70
poker10 CreditAttribution: poker10 at ActivIT s.r.o. commentedComment #71
stefanos.petrakis@gmail.comAfter 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:
If we agree on this, then the patch would need to be updated.
Comment #72
poker10 CreditAttribution: poker10 at ActivIT s.r.o. commentedThanks @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):
This is caused by the logic in the
user_save()
function:So I think we do not need to put the protection here, as this new unblock action will use the
user_save()
function.Comment #73
stefanos.petrakis@gmail.comThanks @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):
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()).
Comment #74
stefanos.petrakis@gmail.comThis is what I have in mind.
Comment #75
stefanos.petrakis@gmail.comPatch was not complete, uploading again.
Comment #77
poker10 CreditAttribution: poker10 at ActivIT s.r.o. commentedI 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).
Comment #79
mcdruidDraft 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!
Comment #80
poker10 CreditAttribution: poker10 at ActivIT s.r.o. commentedI have updated the Change record.