Page MenuHomePhorge

Fix exception trying to rename user to their previous username
ClosedPublic

Authored by aklapper on Apr 20 2024, 17:12.

Details

Summary

When trying to rename a user, properly handle when the new username is the old username to avoid an exception.

EXCEPTION: (PhabricatorApplicationTransactionNoEffectException) Transactions have no effect:
  - Transaction (of type "user.rename") has no effect. at [<phorge>/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php:2823]

Closes T15795

Test Plan
  • As an admin, go to /people/manage/123/ and select Change Username. Use the old username as the new username and select Rename User. Without the patch, get an exception. With the patch, get a proper error message within the dialog, as if you tried to set the new username to an empty string.
  • As an admin, go to /people/create/, select Create Standard User, and successfully create a new user account.
  • In an anonymous browser window, go to /auth/start/, click Register New Account, and successfully create a new user account.

Diff Detail

Repository
rP Phorge
Branch
T15795UserRename (branched from master)
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 1202
Build 1202: arc lint + arc unit

Event Timeline

Note that $xaction->getOldValue() in https://we.phorge.it/source/phorge/browse/master/src/applications/people/xaction/PhabricatorUserUsernameTransaction.php$62 seems to always return null. It does not provide the old/current username, while generateOldValue() actually does.

Maybe better to expand the test plan to check user creation as well

aklapper edited the test plan for this revision. (Show Details)

Maybe better to expand the test plan to check user creation as well

Done, I think.

If you followed the test plan and if this works, thanks

sgtm

I would probably put this check after the New username is required. and also after the username validation, but that is still OK

Move additional check after existing checks

Nice catch :) Can reproduce. Tested.

lgtm

This revision is now accepted and ready to land.May 7 2024, 12:27