- Issue created by @lauriii
- Merge request !10593Issue #3493122 by lauriii, jquijano: Users are able to block themselves from Drupal โ (Open) created by Unnamed author
Modified AccountForm to prevent status field rendering only if the account editor is the same user to prevent self-blocking
- ๐บ๐ธUnited States nicxvan
Pretty sure this is intentional and required: #8-75: Let users cancel their accounts โ
- ๐บ๐ธUnited States nicxvan
Yeah this should be closed works as designed.
This is required in some places.
- ๐ซ๐ฎFinland lauriii Finland
We have the "Cancel own user account" and "Select method for cancelling account" permissions which make sense for the use case of cancelling your own user. As a requirement, that makes sense because on some sites, you may want to allow users to remove their own user.
If a site is legally mandated to keep the information about author even after canceling the user, the site administrator, would have to disable the "Select method for cancelling account" permission from users, and configure the user cancellation to "Disable the account and keep its content".
I'm not sure I see what's the use case where you'd explicitly have to be able to block your own user outside of the canceling scenario.
- ๐บ๐ธUnited States smustgrave
Agree with @lauriii in #7. Thinking of something like facebook where I can cancel my account but I can't block.
Moving to NW for test coverage
Added test case to validate the existence of status field for 2 situations:
- When user edits itself
- When user edits other user
- First commit to issue fork.
- ๐ฌ๐งUnited Kingdom oily Greater London
PHPSTAN: broken +80 chars comment into two lines in test.
- ๐ฌ๐งUnited Kingdom oily Greater London
Removed a line from the test, just assert for first radio button. See example in UserSubAdminTest.php line 40.
- ๐ฌ๐งUnited Kingdom oily Greater London
Test-only test fails as it should. Pipeline all green.
- ๐ฎ๐ณIndia sagarmohite0031
Hello,
Tested and verified on Drupal 11.
MR applied successfully.
Its working as expected "Status" field is hidden from the user when rendered for the current user.
Attachments - ๐ฌ๐งUnited Kingdom oily Greater London
Thank you for the review @sagarmohite0031. Based on that I am changing to RTBTC.
- ๐ง๐ชBelgium kristiaanvandeneynde Antwerp, Belgium
Please don't RTBC based on someone else's comment unless they explicitly state they have reviewed the code. For all we know @sagarmohite0031 only applied the patch without checking the code.
Left some feedback that warrants setting back to NW.
- ๐จ๐ฆCanada Charlie ChX Negyesi ๐Canada
Usually form fields are hidden from access via #access instead of removing them to keep the form structure intact for alters.
- ๐ง๐ชBelgium kristiaanvandeneynde Antwerp, Belgium
Yeah I mentioned that option in the review. In this case I would even consider disabling it so admins don't face varying forms depending on which user they're editing and end up getting confused.
Something along the lines of: As an administrator, you cannot block your own account. Either [cancel it] or ask another administrator to block it for you.
- ๐ฌ๐งUnited Kingdom oily Greater London
@kristiaanvandeneynde Thank you for #19. I will check what the docs say on it. The value of the RTBTC was the evidence in screenshots which although not fullproof is a lot better than a simple statement. If what you say is true then if the review had merely stated, 'checked the code. All good.' then that would ensure the review was authentic? Hmm. I think what makes reviews reliable is having experienced contributors like yourself do the review. But there is no mechanism for that.
- ๐ง๐ชBelgium kristiaanvandeneynde Antwerp, Belgium
For what it's worth I always considered the R in RTBC as "Reviewed the code" whereas the T means "Tested the outcome / changes". Non-technical users can provide great feedback for the latter, but at least one technical person needs to check the former for an issue to truly be RTBC. It causes a lot of noise for committers if a bunch of issues are set to RTBC and none of them meet the code quality gates.
- ๐ฌ๐งUnited Kingdom oily Greater London
#23 Thank you, interesting to see it from your angle as a committer.
Reverted previous changes, added a condition to #access attribute to make it false if user self-editing according to @kristiaanvandeneynde's suggestions. Updated branch with lastest changes from main 11.x branch.
- ๐บ๐ธUnited States smustgrave
Appears to need a rebase. Have not re-reviewed
If you are another contributor eager to jump in, please allow the previous poster at least 48 hours to respond to feedback first, so they have the opportunity to finish what they started!
- ๐ง๐ชBelgium kristiaanvandeneynde Antwerp, Belgium
As I mentioned earlier, I think even better UX would be to disable the field and have a description explaining why it's disabled. But I'm not blocking the issue on that as you could argue an admin would be able to figure out that they're editing their own account and it would be unwise to block their own account. Will leave to committers to decide whether we need the better UX here.
There is still a line of documentation referring to people as objects, which we really should not do. If you resolve that and make sure there's no merge conflict, then I'm fine to sign off on this.
- Status changed to Needs review
about 2 months ago 2:17am 17 February 2025 Hi folks,
I've updated the fork and rebased the branch. I re-applied my changes and incorporated @kristiaanvandeneynde's UX suggestions. Essentially, I had to rewrite all (although it wasn't that much).
- ๐บ๐ธUnited States smustgrave
Rebase seems good and feedback appears to be addressed.
- ๐ง๐ชBelgium kristiaanvandeneynde Antwerp, Belgium
Looks good to me too. Thanks a lot!
One minor nitpick that shouldn't hold up committing this is that the message assumes the editor is an admin. This will almost always be the case, but perhaps the message could be cut down to:
You cannot modify your own account status
@kristiaanvandeneynde Message has been cut down, also rebased branch to include latest changes from 11.x
- ๐ซ๐ฎFinland lauriii Finland
Sorry about the back and forth, but we should not be showing the radio button. There's usually nothing that the user can do to make the radio button active (other than to logout as another user). Therefore, we should apply progressive disclosure and not show the field at all. Also, it's really hard to see there being an expectation to block yourself outside of just deleting or canceling your account, so it definitely seems just extra noise there.
- ๐ง๐ชBelgium kristiaanvandeneynde Antwerp, Belgium
I tried to look through the commit history but could not find the previous version where #access was being set. Did anyone force push to the branch?
Either way, the MR needs to go back to (also) hiding the field by setting #access to FALSE when you're on your own profile edit form.
- First commit to issue fork.
- ๐ง๐ชBelgium kristiaanvandeneynde Antwerp, Belgium
@prabha1997 are you also going to fix the test that fails now that we went back to using #access? Then feel free to assign the issue to you while you are working on it so others don't duplicate the same work.
- ๐ง๐ชBelgium kristiaanvandeneynde Antwerp, Belgium
Cool, thanks for being a good sport about the conflicting instructions.
Although in #27 I mentioned "Will leave to committers to decide whether we need the better UX here.", but I like the fact that you were proactive about it. If the suggestion gets rejected, it's easy to revert the commit and return to the previous state that was already RTBCed.
Well, if people don't force push and remove the history, that is ๐
- ๐ง๐ชBelgium kristiaanvandeneynde Antwerp, Belgium
Ah dammit, removed the assignment (because I refreshed a tab where it wasn't assigned yet) and can't add it back. Sorry!
- ๐ฎ๐ณIndia prabha1997
@kristiaanvandeneynde
Iโm new to writing test cases at the moment, Iโm exploring why the test is failing locally. Iโm working on understanding and resolving the issue. Apologies for any inconvenience caused. Hi folks,
@prabha1997 It's okay, I have already reviewed this test case, so it's fixed. The MR status is now OK.
- ๐ง๐ชBelgium kristiaanvandeneynde Antwerp, Belgium
Thanks @jquijano and no worries @prabha1997.