According to the README, if a user has the 'change own username' permission, the username should be able to set their username. Currently, if the user has that permission, the username field is still not appearing on the user edit page. This needs to be fixed. A quick grep of the current code shows that the string 'change own username' doesn't appear, so obviously the permission isn't even being paid attention to.

CommentFileSizeAuthor
#22 interdiff.txt1.17 KBandypost
#19 3034828-19.patch2.35 KBandypost
#19 3034828-19-test-only.patch1.49 KBandypost
#19 interdiff.txt807 bytesandypost
#18 3034828.patch2.34 KBasherry
#12 interdiff_10-12.txt636 bytesvacho
#12 3034828-admins-users-when-allowed-change-username-12.patch921 bytesvacho
#12 after_patch.png64.66 KBvacho
#12 before_patch.png59.75 KBvacho
#10 3034828-admins-users-when-allowed-change-username-10.patch871 bytesgreggles
#2 email_registration-Allow_username_changing-3034828-2.patch1.3 KBben coleman

Comments

Ben Coleman created an issue. See original summary.

ben coleman’s picture

ben coleman commented
Version: 8.x-1.0-rc6 » 8.x-1.x-dev
Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new email_registration-Allow_username_changing-3034828-2.patch1.3 KB

The attached patch implements the above - if the user has the 'change own username' permission, the username field is presented for normal editing. I don't check for whether the user is editing his own profile, or whether the user has the 'Administer users' permission, as I assume that if the user can get to the user edit page, one of these two situations applies. This means that a user that has the 'Administer users' permission *and* the 'change own username' permission will be able to change other user's usernames. Tweaking will be required if this is not acceptable.

Note that this patch expects that the patch from #3024558-19: Username regenerated improperly after rc6 release has been applied.

Status: Needs review » Needs work

The last submitted patch, 2: email_registration-Allow_username_changing-3034828-2.patch, failed testing. View results

ben coleman’s picture

ben coleman commented

Hmmm. Is there a way to specify to the test mechanism that another patch needs to be applied in addition to the patch being tested?

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented

There is not. You could help write tests for that other patch - that's the last thing it needs to get committed :)

brandonratz’s picture

brandonratz commented

Would be great to have this working properly. I was unable to apply this patch after applying https://www.drupal.org/files/issues/2019-03-04/Strange-username-logic-af...

ben coleman’s picture

ben coleman commented

Brandon: note that I used #3024558-19: Username regenerated improperly after rc6 release (patch from comment 19), while you used the patch from comment 44. You might (at least temporarily) try the comment 19 patch before trying the patch for this issue.

Since #3024558: Username regenerated improperly after rc6 release still seems to be somewhat in flux, and the code from this issue overlaps with the code form there, it might be best to wait until #3024558: Username regenerated improperly after rc6 release is settled and commitded before finalizing a patch for this issue.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented

#3024558: Username regenerated improperly after rc6 release was committed, so this can now be re-rolled.

It also seems like this needs fixes in 2 places:
* registration
* account management

The latest patch only addresses registration.

And, ideally, this should get tests to make sure we don't break it again.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented

Pardon me, this affects both registration and account management since the form gets re-used. But another important detail is handling an admin looking at a user's form.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented
Status: Needs work » Needs review
StatusFileSize
new 3034828-admins-users-when-allowed-change-username-10.patch871 bytes

Still needs tests, but here's a patch for review.

andypost’s picture

andypost
he/him
Primary language Russian
commented

It needs check that form element exists at least

vacho’s picture

vacho commented
StatusFileSize
new before_patch.png59.75 KB
new after_patch.png64.66 KB
new 3034828-admins-users-when-allowed-change-username-12.patch921 bytes
new interdiff_10-12.txt636 bytes

I update the code with #11 enhancement and test the solution.

Before the patch review:
https://www.drupal.org/files/issues/2019-05-08/before_patch.png

After the patch review:
https://www.drupal.org/files/issues/2019-05-08/after_patch.png

kle’s picture

kle commented

A good solution is very welcome. My own hack only resets the name#type to 'textfield' ...

d.fisher’s picture

d.fisher commented

Patch in #12 fails for me.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented

@darren.fisher - do you mean it fails to apply or that it doesn't solve the problem as you experience it. If it doesn't solve the problem, could you provide more details about your configuration and what you expect to see and what you do see?

d.fisher’s picture

d.fisher commented

Sorry greggles I kind of wrote that as a bit of a throwaway without much context. My bad! The patch failed to apply against 1.0.0-rc7 using Drupal core 8.6.15. Didn't test against dev as I needed to use in production. Hope that helps a bit more.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented

@vacho can you clarify what situation your patch helps vs. the prior?

asherry’s picture

asherry commented
StatusFileSize
new 3034828.patch2.34 KB

Yeah I agree, it'd be great to get more details @vacho @andypost. What's the context where $form['account']['name']['#type'] wouldn't be set?
Could we maybe move that to a separate ticket and release this?

I fixed a small typo with \Drupal::CurrentUser instead of \Drupal::currentUser, and added a test for this, but this is based on #10 and not #12.

This is pretty crucial for our instance.
Thanks so much @greggles!

andypost’s picture

andypost
he/him
Primary language Russian
commented
StatusFileSize
new interdiff.txt807 bytes
new 3034828-19-test-only.patch1.49 KB
new 3034828-19.patch2.35 KB

@asherry Sorry my bad, mixed with default value in #3024558: Username regenerated improperly after rc6 release

Here's small clean-up and test-only patch because it's bug

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs review » Reviewed & tested by the community

Ready for commit

  • andypost committed e9d1b78 on 8.x-1.x 
    Issue #3034828 by andypost, vacho, greggles, Ben Coleman, asherry:...
andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Reviewed & tested by the community » Fixed
StatusFileSize
new interdiff.txt1.17 KB

Cleaned on commit

Status: Fixed » Closed (fixed)

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