Ability for users to use previous usernames

Refresh.Database/GameDatabaseContext.Registration.cs

@@ -96,20 +96,18 @@ public bool IsEmailQueued(string emailAddress)
         return this.QueuedRegistrations.Any(r => r.EmailAddress == emailAddress);
     }
 
-    public bool IsUsernameTaken(string username, GameUser? userToName = null)
+    public bool IsUsernameTaken(string username)
     {
         if (this.GameUsers.Any(u => u.Username == username)) return true;
         if (this.QueuedRegistrations.Any(r => r.Username == username)) return true;
-        if (this.IsUserDisallowed(username)) return true;
-
-        PreviousUsername? previous = this.PreviousUsernames.FirstOrDefault(p => p.Username == username);
-        // no one has ever had this name before
-        if (previous == null) return false;
-        // this is not the initial owner of the name (only previous owners may be renamed back)
-        if (userToName == null || userToName.UserId != previous.UserId) return true;
 
         return false;
     }
+
+    public bool WasUsernamePreviouslyTaken(string username)
+    {
+        return this.PreviousUsernames.Any(u => u.Username == username);
+    }
 
     public bool IsEmailTaken(string emailAddress)
     {

Refresh.Database/GameDatabaseContext.Users.cs

@@ -19,6 +19,10 @@ public partial class GameDatabaseContext // Users
     private IQueryable<GameUser> GameUsersIncluded => this.GameUsers
         .Include(u => u.Statistics);
 
+    private IQueryable<PreviousUsername> PreviousUsernamesIncluded => this.PreviousUsernames
+        .Include(u => u.User)
+        .Include(u => u.User.Statistics);
+
     [Pure]
     [ContractAnnotation("username:null => null; username:notnull => canbenull")]
     public GameUser? GetUserByUsername(string? username, bool caseSensitive = true)
@@ -121,6 +125,12 @@ public DatabaseList<GameUser> GetMostFavouritedUsers(int skip, int count)
             .Where(u => u.Statistics!.FavouriteCount > 0)
             .OrderByDescending(u => u.Statistics!.FavouriteCount), skip, count);
 
+    public DatabaseList<PreviousUsername> GetPreviousUsernameRecordsByName(string username, int skip, int count)
+    {
+        return new(this.PreviousUsernamesIncluded
+            .Where(u => u.Username == username), skip, count);
+    }
+
     public void UpdateUserData(GameUser user, ISerializedEditUser data, TokenGame game)
     {
         this.Write(() =>
@@ -370,7 +380,7 @@ public void RenameUser(GameUser user, string newUsername, bool force = false)
                 throw new ArgumentException("Username is invalid!", nameof(newUsername));
             }
 
-            if (this.IsUsernameTaken(newUsername, user))
+            if (this.IsUsernameTaken(newUsername))
             {
                 throw new ArgumentException("Username is already taken!", nameof(newUsername));
             }
@@ -385,7 +395,15 @@ public void RenameUser(GameUser user, string newUsername, bool force = false)
             User = user,
             ReplacedAt = this._time.Now,
         });
+
+        // Postgres/EF will try to insert untracked entities by default, which will fail if such entities already exist in DB.
+        // We can't guarantee that both user and its referenced entities (currently just statistics cache) are tracked here,
+        // so we should explicitly update user and explicitly track statistics as unchanged to avoid random inconsistent failures.
+        // TODO do explicitly track referenced entities in other similar DB modification methods as well, to also avoid occasional insertion there.
         this.GameUsers.Update(user);
+        if (user.Statistics != null)
+            this.GameUserStatistics.Attach(user.Statistics);
+
         this.SaveChanges();
 
         this.AddNotification("Username Updated", $"An admin has updated your account's username from '{oldUsername}' to '{newUsername}'. " +

Refresh.Interfaces.APIv3/Endpoints/Admin/AdminUserApiEndpoints.cs

@@ -173,7 +173,7 @@ public ApiResponse<ApiExtendedGameUserResponse> UpdateUser(RequestContext contex
                 return new ApiValidationError(ApiValidationError.InvalidUsernameErrorWhen
                     + " Are you sure you used a PSN/RPCN username, or prepended it with ! if it's a fake user?");
 
-            if (database.IsUsernameTaken(body.Username, targetUser))
+            if (database.IsUsernameTaken(body.Username))
                 return ApiValidationError.UsernameTakenError;
 
             database.RenameUser(targetUser, body.Username);

RefreshTests.GameServer/Tests/ApiV3/AdminUserEditApiTests.cs

@@ -1,4 +1,5 @@
 using MongoDB.Bson;
+using Refresh.Database;
 using Refresh.Database.Models.Authentication;
 using Refresh.Database.Models.Moderation;
 using Refresh.Database.Models.Users;
@@ -269,38 +270,58 @@ public void CannotRenameToTakenUsername()
     }
 
     [Test]
-    public void CannotRenameToOtherUsersPreviousName()
+    public void CanRenameUserToDifferentUsersPreviousName()
     {
         using TestContext context = this.GetServer();
 
         GameUser mod = context.CreateUser(null, GameUserRole.Moderator);
         GameUser owner = context.CreateUser("original", GameUserRole.User);
         GameUser target = context.CreateUser("stinker", GameUserRole.User);
+
+        // Ensure we're tracking neither usernames
+        Assert.That(!context.Database.WasUsernamePreviouslyTaken("original"));
+        Assert.That(!context.Database.WasUsernamePreviouslyTaken("stinker"));
 
         context.Database.RenameUser(owner, "original_2");
         GameUser? modifiedOwner = context.Database.GetUserByObjectId(owner.UserId);
         Assert.That(modifiedOwner, Is.Not.Null);
         Assert.That(modifiedOwner!.Username, Is.EqualTo("original_2"));
 
+        // Try to rename stinker to original's previous name
         HttpClient client = context.GetAuthenticatedClient(TokenType.Api, mod);
         ApiAdminUpdateUserRequest request = new()
         {
             Username = "original"
         };
 
-        ApiResponse<ApiExtendedGameUserResponse>? response = client.PatchData<ApiExtendedGameUserResponse>($"/api/v3/admin/users/uuid/{target.UserId}", request, false, true);
-        Assert.That(response?.Error, Is.Not.Null);
-        Assert.That(response!.Error!.StatusCode, Is.EqualTo(BadRequest));
+        ApiResponse<ApiExtendedGameUserResponse>? response = client.PatchData<ApiExtendedGameUserResponse>($"/api/v3/admin/users/uuid/{target.UserId}", request, true, false);
+        Assert.That(response?.Data, Is.Not.Null);
+        Assert.That(response!.Data!.Username, Is.EqualTo("original"));
+        Assert.That(response!.Data!.UserId, Is.EqualTo(target.UserId.ToString()));
 
         context.Database.Refresh();
 
         GameUser? modifiedTarget = context.Database.GetUserByObjectId(target.UserId);
         Assert.That(modifiedTarget, Is.Not.Null);
-        Assert.That(modifiedTarget!.Username, Is.EqualTo("stinker"));
+        Assert.That(modifiedTarget!.Username, Is.EqualTo("original"));
+
+        // Ensure we're tracking both usernames
+        Assert.That(context.Database.WasUsernamePreviouslyTaken("original"));
+        Assert.That(context.Database.WasUsernamePreviouslyTaken("stinker"));
+
+        // Ensure "original" is tracked as previously owned by owner
+        DatabaseList<PreviousUsername> originalHistory = context.Database.GetPreviousUsernameRecordsByName("original", 0, 10);
+        Assert.That(originalHistory.Items.Count, Is.EqualTo(1));
+        Assert.That(originalHistory.Items.First().UserId.ToString(), Is.EqualTo(owner.UserId.ToString()));
+
+        // Ensure "stinker" is tracked as previously owned by target
+        DatabaseList<PreviousUsername> stinkerHistory = context.Database.GetPreviousUsernameRecordsByName("stinker", 0, 10);
+        Assert.That(stinkerHistory.Items.Count, Is.EqualTo(1));
+        Assert.That(stinkerHistory.Items.First().UserId.ToString(), Is.EqualTo(target.UserId.ToString()));
     }
 
     [Test]
-    public void CanRenameUserBackToTheirPreviousName()
+    public void CanRenameUserBackToTheirOwnPreviousName()
     {
         using TestContext context = this.GetServer();
 
@@ -327,6 +348,11 @@ public void CanRenameUserBackToTheirPreviousName()
         GameUser? modifiedOwner2 = context.Database.GetUserByObjectId(owner.UserId);
         Assert.That(modifiedOwner2, Is.Not.Null);
         Assert.That(modifiedOwner2!.Username, Is.EqualTo("original"));
+
+        // Ensure "original" is still also tracked as previously owned by owner
+        DatabaseList<PreviousUsername> originalHistory = context.Database.GetPreviousUsernameRecordsByName("original", 0, 10);
+        Assert.That(originalHistory.Items.Count, Is.EqualTo(1));
+        Assert.That(originalHistory.Items.First().UserId.ToString(), Is.EqualTo(owner.UserId.ToString()));
     }
 
     [Test]

RefreshTests.GameServer/Tests/ApiV3/UserApiTests.cs

@@ -47,7 +47,7 @@ public void RegisterAccount()
     }
 
     [Test]
-    public void CannotRegisterAccountWithPreviouslyTakenUsername()
+    public void CanRegisterAccountWithPreviouslyTakenUsername()
     {
         using TestContext context = this.GetServer();
         GameUser owner = context.CreateUser("original", GameUserRole.User);
@@ -62,13 +62,46 @@ public void CannotRegisterAccountWithPreviouslyTakenUsername()
             Username = "original",
             EmailAddress = "guy@lil.com",
             PasswordSha512 = "ee26b0dd4af7e749aa1a8ee3c10ae9923f618980772e473f8819a5d4940e0db27ac185f8a0e1d5f84f88bc887fd67b143732c304cc5fa9ad8e6f57f50028a8ff",
-        }, false, true);
+        }, true, false);
+        Assert.That(response, Is.Not.Null);
+        Assert.That(response!.Data, Is.Not.Null);
+
+        context.Database.Refresh();
+        GameUser? newUser = context.Database.GetUserByUuid(response!.Data!.UserId);
+        Assert.That(newUser, Is.Not.Null);
+        Assert.That(newUser!.Username, Is.EqualTo("original"));
+
+        // Ensure the original "original" usage is tracked
+        Assert.That(context.Database.WasUsernamePreviouslyTaken("original"));
+    }
+
+    [Test]
+    public void CannotRegisterAccountWithCurrentlyTakenUsername()
+    {
+        using TestContext context = this.GetServer();
+        GameUser owner = context.CreateUser("original", GameUserRole.User);
+
+        ApiRegisterRequest request = new ApiRegisterRequest
+        {
+            Username = "original",
+            EmailAddress = "guy@lil.com",
+            PasswordSha512 = "ee26b0dd4af7e749aa1a8ee3c10ae9923f618980772e473f8819a5d4940e0db27ac185f8a0e1d5f84f88bc887fd67b143732c304cc5fa9ad8e6f57f50028a8ff",
+        };
+        ApiResponse<ApiAuthenticationResponse>? response = context.Http.PostData<ApiAuthenticationResponse>("/api/v3/register", request, false, true);
         Assert.That(response, Is.Not.Null);
-        Assert.That(response!.Error, Is.Not.EqualTo(null));
-        Assert.That(response.Error!.Name, Is.EqualTo("ApiAuthenticationError"));
+        Assert.That(response!.Error, Is.Not.Null);
 
         context.Database.Refresh();
-        Assert.That(context.Database.GetTotalUserCount(), Is.EqualTo(1));
+
+        // Our initial user still exists and hasn't been touched
+        GameUser? newUser = context.Database.GetUserByObjectId(owner.UserId);
+        Assert.That(newUser, Is.Not.Null);
+        Assert.That(newUser!.Username, Is.EqualTo("original"));
+        Assert.That(newUser!.EmailAddress, Is.Not.EqualTo(request.EmailAddress));
+        Assert.That(newUser!.PasswordBcrypt, Is.Not.EqualTo(request.PasswordSha512));
+
+        // Ensure there is nothing tracked as no rename has happened
+        Assert.That(!context.Database.WasUsernamePreviouslyTaken("original"));
     }
 
     [TestCase("4")]