From b170e9e5de5b651df172bd8ac589e6361cf9c934 Mon Sep 17 00:00:00 2001
From: Martin Emde <martin.emde@gmail.com>
Date: Mon, 9 Oct 2023 12:20:08 -0700
Subject: [PATCH] Add better error messages for Ownership uniqueness validation

---
 app/models/ownership.rb       | 15 ++++++++++++++-
 test/models/ownership_test.rb | 36 ++++++++++++++++++++++++-----------
 2 files changed, 39 insertions(+), 12 deletions(-)

diff --git a/app/models/ownership.rb b/app/models/ownership.rb
index 327d34b9e9e..dd095a0c7f1 100644
--- a/app/models/ownership.rb
+++ b/app/models/ownership.rb
@@ -4,7 +4,7 @@ class Ownership < ApplicationRecord
   belongs_to :authorizer, class_name: "User"
   has_many :api_key_rubygem_scopes, dependent: :destroy
 
-  validates :user_id, uniqueness: { scope: :rubygem_id }
+  validate :validate_unique_user
 
   delegate :name, to: :user, prefix: :owner
   delegate :name, to: :authorizer, prefix: true, allow_nil: true
@@ -72,4 +72,17 @@ def unconfirmed?
   def safe_destroy
     destroy if unconfirmed? || rubygem.owners.many?
   end
+
+  def validate_unique_user
+    return unless rubygem && user
+    ownerships = persisted? ? Ownership.where.not(id: id) : Ownership
+    other = ownerships.find_by(rubygem:, user:)
+    return unless other
+
+    if other.confirmed?
+      errors.add :user_id, "is already an owner of this gem"
+    else
+      errors.add :user_id, "is already invited to be an owner of this gem"
+    end
+  end
 end
diff --git a/test/models/ownership_test.rb b/test/models/ownership_test.rb
index 9347a346321..1823f7a7249 100644
--- a/test/models/ownership_test.rb
+++ b/test/models/ownership_test.rb
@@ -13,17 +13,6 @@ class OwnershipTest < ActiveSupport::TestCase
   should have_db_index %i[user_id rubygem_id]
   should have_many(:api_key_rubygem_scopes).dependent(:destroy)
 
-  context "with ownership" do
-    setup do
-      @ownership = create(:ownership)
-      create(:version, rubygem: @ownership.rubygem)
-    end
-
-    subject { @ownership }
-
-    should validate_uniqueness_of(:user_id).scoped_to(:rubygem_id)
-  end
-
   context "by_indexed_gem_name" do
     setup do
       @ownership = create(:ownership)
@@ -108,6 +97,31 @@ class OwnershipTest < ActiveSupport::TestCase
       refute_predicate ownership, :valid?
       assert_contains ownership.errors[:rubygem], "must exist"
     end
+
+    should "not create with a duplicate unconfirmed user and rubygem" do
+      existing_ownership = create(:ownership, :unconfirmed)
+      ownership = build(:ownership, user: existing_ownership.user, rubygem: existing_ownership.rubygem)
+
+      refute_predicate ownership, :valid?
+      assert_contains ownership.errors[:user_id], "is already invited to be an owner of this gem"
+    end
+
+    should "not create with a duplicate confirmed user and rubygem" do
+      existing_ownership = create(:ownership)
+      ownership = build(:ownership, user: existing_ownership.user, rubygem: existing_ownership.rubygem)
+
+      refute_predicate ownership, :valid?
+      assert_contains ownership.errors[:user_id], "is already an owner of this gem"
+    end
+
+    should "not update to a duplicate confirmed user and rubygem" do
+      existing_ownership = create(:ownership)
+      ownership = create(:ownership, :unconfirmed, rubygem: existing_ownership.rubygem)
+      ownership.user = existing_ownership.user
+
+      refute_predicate ownership, :valid?
+      assert_contains ownership.errors[:user_id], "is already an owner of this gem"
+    end
   end
 
   context "#valid_confirmation_token?" do