Fixing various issues around name reservations

Users and Teams share a name reservation pool, and both had issues
around name handling that either resulted simply in unique constraint
violations or in a worse case a possibly inconsistent database.

This commit improves the name validation and test coverage to make sure
that everything is handled correctly, and that forms return proper error
messages on failure.

Author: dutow

app/controllers/verifications_controller.rb

@@ -30,6 +30,7 @@ def handle_register(token)
     metadata = JSON.parse(token.metadata || "{}") rescue {}
     desired_username = metadata["username"]
     user.username = desired_username
+    user.skip_name_reservation = true
     if metadata["password_digest"].present?
       user.password_digest = metadata["password_digest"]
     end

app/models/team.rb

@@ -12,8 +12,9 @@ class Team < ApplicationRecord
 
   validates :name, presence: true
   validates :name, format: { with: /\A[a-zA-Z0-9_\-\.]+\z/ }
+  validate :name_available_in_reservations
 
-  after_commit :reserve_name, on: :create
+  after_create :reserve_name
   after_destroy :release_name_reservation
 
   def member?(user)
@@ -43,6 +44,17 @@ def mentionable_by?(user)
 
   private
 
+  def name_available_in_reservations
+    return if name.blank? || !will_save_change_to_name?
+
+    normalized = NameReservation.normalize(name)
+    existing = NameReservation.find_by(name: normalized)
+    return unless existing
+    return if existing.owner_type == "Team" && existing.owner_id == id
+
+    errors.add(:name, "is already taken")
+  end
+
   def reserve_name
     NameReservation.reserve!(name:, owner: self)
   end

app/models/user.rb

@@ -33,23 +33,37 @@ def shares_team_with?(other_user)
     team_ids.intersect?(other_user.team_ids)
   end
 
+  attr_accessor :skip_name_reservation
+
   validates :username, format: { with: /\A[a-zA-Z0-9_\-\.]+\z/, allow_blank: true }
+  validates :username, uniqueness: { allow_blank: true, case_sensitive: false }
   validates :username, presence: true, on: :registration
+  validate :username_available_in_reservations
 
   before_save :release_old_username_reservation
-  after_commit :reserve_username, on: [ :create, :update ]
+  after_save :reserve_username
   after_destroy :release_name_reservation
 
   private
 
+  def username_available_in_reservations
+    return if username.blank? || !will_save_change_to_username? || skip_name_reservation
+
+    normalized = NameReservation.normalize(username)
+    existing = NameReservation.find_by(name: normalized)
+    return unless existing
+    return if existing.owner_type == "User" && existing.owner_id == id
+
+    errors.add(:username, "is already taken")
+  end
+
   def release_old_username_reservation
     return unless will_save_change_to_username?
-    old_username = username_before_last_save
-    NameReservation.release_for(self) if old_username.present?
+    NameReservation.release_for(self) if username_was.present?
   end
 
   def reserve_username
-    return if username.blank?
+    return if username.blank? || skip_name_reservation
     NameReservation.reserve!(name: username, owner: self)
   end
 

db/schema.rb

@@ -53,6 +53,48 @@
     end
   end
 
+  describe "name reservation" do
+    it "rejects name already reserved by a user" do
+      create(:user, username: "claimed")
+
+      new_team = Team.new(name: "claimed")
+      expect(new_team).not_to be_valid
+      expect(new_team.errors[:name]).to include("is already taken")
+    end
+
+    it "rejects name already reserved by another team" do
+      Team.create!(name: "existing")
+
+      new_team = Team.new(name: "existing")
+      expect(new_team).not_to be_valid
+      expect(new_team.errors[:name]).to include("is already taken")
+    end
+
+    it "rejects name case-insensitively" do
+      create(:user, username: "CaseName")
+
+      new_team = Team.new(name: "casename")
+      expect(new_team).not_to be_valid
+      expect(new_team.errors[:name]).to include("is already taken")
+    end
+
+    it "creates a reservation on create" do
+      new_team = Team.create!(name: "freshteam")
+      reservation = NameReservation.find_by(name: "freshteam")
+      expect(reservation).to be_present
+      expect(reservation.owner_type).to eq("Team")
+      expect(reservation.owner_id).to eq(new_team.id)
+    end
+
+    it "releases reservation on destroy" do
+      new_team = Team.create!(name: "doomed")
+      expect(NameReservation.find_by(name: "doomed")).to be_present
+
+      new_team.destroy!
+      expect(NameReservation.find_by(name: "doomed")).to be_nil
+    end
+  end
+
   describe "#mentionable_by?" do
     it "allows only members to mention private teams" do
       expect(team.mentionable_by?(user)).to be(true)

spec/models/team_spec.rb

@@ -0,0 +1,88 @@
+# frozen_string_literal: true
+
+require "rails_helper"
+
+RSpec.describe User, type: :model do
+  describe "username validations" do
+    it "rejects username already taken by another user" do
+      user1 = create(:user, username: "taken")
+      user2 = create(:user)
+
+      user2.username = "taken"
+      expect(user2).not_to be_valid
+      expect(user2.errors[:username]).to include("has already been taken")
+    end
+
+    it "rejects username case-insensitively against other users" do
+      create(:user, username: "TakenName")
+      user2 = create(:user)
+
+      user2.username = "takenname"
+      expect(user2).not_to be_valid
+      expect(user2.errors[:username]).to include("has already been taken")
+    end
+
+    it "rejects username already reserved by a team" do
+      Team.create!(name: "teamname")
+      user = create(:user)
+
+      user.username = "teamname"
+      expect(user).not_to be_valid
+      expect(user.errors[:username]).to include("is already taken")
+    end
+
+    it "rejects username case-insensitively against teams" do
+      Team.create!(name: "MyTeam")
+      user = create(:user)
+
+      user.username = "myteam"
+      expect(user).not_to be_valid
+      expect(user.errors[:username]).to include("is already taken")
+    end
+
+    it "allows setting a new unique username" do
+      user = create(:user)
+      expect(user.update(username: "unique_name")).to be(true)
+      expect(NameReservation.find_by(name: "unique_name")).to be_present
+    end
+
+    it "allows updating other attributes without affecting username reservation" do
+      user = create(:user, username: "myname")
+      expect(user.update(mention_restriction: :teammates_only)).to be(true)
+
+      reservation = NameReservation.find_by(name: "myname")
+      expect(reservation).to be_present
+      expect(reservation.owner_type).to eq("User")
+      expect(reservation.owner_id).to eq(user.id)
+    end
+
+    it "creates a name reservation when username is set" do
+      user = create(:user)
+      user.update!(username: "reserved_name")
+
+      reservation = NameReservation.find_by(name: "reserved_name")
+      expect(reservation).to be_present
+      expect(reservation.owner_type).to eq("User")
+      expect(reservation.owner_id).to eq(user.id)
+    end
+
+    it "releases old reservation and creates new one when username changes" do
+      user = create(:user, username: "oldname")
+      expect(NameReservation.find_by(name: "oldname")).to be_present
+
+      user.update!(username: "newname")
+      expect(NameReservation.find_by(name: "oldname")).to be_nil
+      expect(NameReservation.find_by(name: "newname")).to be_present
+    end
+
+    it "rolls back username change if reservation fails" do
+      user = create(:user, username: "original")
+      Team.create!(name: "blocked")
+
+      expect(user.update(username: "blocked")).to be(false)
+      user.reload
+      expect(user.username).to eq("original")
+      expect(NameReservation.find_by(name: "original")).to be_present
+    end
+  end
+end

spec/models/user_spec.rb

@@ -0,0 +1,57 @@
+# frozen_string_literal: true
+
+require "rails_helper"
+
+RSpec.describe "Settings::Usernames", type: :request do
+  def sign_in(user)
+    al = create(:alias, user: user, email: "login-#{user.id}@example.com")
+    user.person.update!(default_alias_id: al.id) if user.person.default_alias_id.nil?
+    Alias.by_email(al.email).update_all(verified_at: Time.current)
+    user.update!(password: "secret", password_confirmation: "secret") unless user.password_digest.present?
+    post session_path, params: { email: al.email, password: "secret" }
+  end
+
+  describe "PATCH /settings/username" do
+    it "updates username successfully" do
+      user = create(:user)
+      sign_in(user)
+
+      patch settings_username_path, params: { user: { username: "new_username" } }
+      expect(response).to redirect_to(settings_profile_path)
+      expect(flash[:notice]).to eq("Username updated")
+      expect(user.reload.username).to eq("new_username")
+    end
+
+    it "shows error when username is taken by another user" do
+      create(:user, username: "taken")
+      user = create(:user, username: "original")
+      sign_in(user)
+
+      patch settings_username_path, params: { user: { username: "taken" } }
+      expect(response).to redirect_to(settings_profile_path)
+      expect(flash[:alert]).to match(/already been taken/i)
+      expect(user.reload.username).to eq("original")
+    end
+
+    it "shows error when username is reserved by a team" do
+      Team.create!(name: "devteam")
+      user = create(:user, username: "original")
+      sign_in(user)
+
+      patch settings_username_path, params: { user: { username: "devteam" } }
+      expect(response).to redirect_to(settings_profile_path)
+      expect(flash[:alert]).to match(/already taken/i)
+      expect(user.reload.username).to eq("original")
+    end
+
+    it "preserves old reservation when update is rejected" do
+      Team.create!(name: "blocked")
+      user = create(:user, username: "keeper")
+      sign_in(user)
+
+      patch settings_username_path, params: { user: { username: "blocked" } }
+      expect(user.reload.username).to eq("keeper")
+      expect(NameReservation.find_by(name: "keeper")).to be_present
+    end
+  end
+end