Paths

Table of Contentst

Differential D25601

Fix exception trying to rename user to their previous username
Needs ReviewPublic
Actions

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

Tags

None

Referenced Files

	Unknown Object (File)
	Thu, May 2, 16:09

	Unknown Object (File)
	Tue, Apr 30, 06:33

	Unknown Object (File)
	Mon, Apr 29, 22:25

	Unknown Object (File)
	Mon, Apr 29, 10:40

	Unknown Object (File)
	Mon, Apr 29, 03:16

	Unknown Object (File)
	Fri, Apr 26, 23:12

	Unknown Object (File)
	Thu, Apr 25, 23:03

	Unknown Object (File)
	Thu, Apr 25, 23:03

View All 14 Files

Subscribers

valerio.bozzolan

Details

Reviewers

None

Group Reviewers

O1: Blessed Committers

Maniphest Tasks

T15795: Unhandled exception trying to rename user to their previous name

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

Branch

T15795UserRename (branched from master)

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 1202
Build 1202: arc lint + arc unit

Event Timeline

aklapper created this revision.Sat, Apr 20, 17:12

Owners added a reviewer: O1: Blessed Committers.Sat, Apr 20, 17:12

Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan, tobiaswiese. · View Herald TranscriptSat, Apr 20, 17:12

aklapper requested review of this revision.Sat, Apr 20, 17:12

Harbormaster completed remote builds in B1193: Diff 1890.Sat, Apr 20, 17:12

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

In D25601#17036, @valerio.bozzolan wrote:

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

Harbormaster completed remote builds in B1202: Diff 1905.Mon, Apr 29, 11:48

Revision Contents
Changeset List

Path

Size

Packages

src/

applications/

people/

xaction/

PhabricatorUserUsernameTransaction.php

4 lines

O1: Blessed Committers

Diff 1905

src/applications/people/xaction/PhabricatorUserUsernameTransaction.php

Loading...