Clarify Ownership uniqueness validations when owned/invited (#4119)

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, I18n.t("activerecord.errors.models.ownership.attributes.user_id.already_confirmed")
+    else
+      errors.add :user_id, I18n.t("activerecord.errors.models.ownership.attributes.user_id.already_invited")
+    end
+  end
 end

config/locales/de.yml

@@ -55,6 +55,11 @@ de:
         unpwn:
         blocked:
       models:
+        ownership:
+          attributes:
+            user_id:
+              already_confirmed:
+              already_invited:
         version:
           attributes:
             gem_full_name:

config/locales/en.yml

@@ -64,6 +64,11 @@ en:
         unpwn: has previously appeared in a data breach and should not be used
         blocked: "domain '%{domain}' has been blocked for spamming. Please use a valid personal email."
       models:
+        ownership:
+          attributes:
+            user_id:
+              already_confirmed: "is already an owner of this gem"
+              already_invited: "is already invited to this gem"
         version:
           attributes:
             gem_full_name:

config/locales/es.yml

@@ -66,6 +66,11 @@ es:
         unpwn:
         blocked:
       models:
+        ownership:
+          attributes:
+            user_id:
+              already_confirmed:
+              already_invited:
         version:
           attributes:
             gem_full_name:

config/locales/fr.yml

@@ -66,6 +66,11 @@ fr:
         unpwn:
         blocked:
       models:
+        ownership:
+          attributes:
+            user_id:
+              already_confirmed:
+              already_invited:
         version:
           attributes:
             gem_full_name:

config/locales/ja.yml

@@ -59,6 +59,11 @@ ja:
         unpwn: 過去にデータ侵害を受けたためお使いになれません
         blocked: ドメイン %{domain} はスパムのため差し止められました。正しい個人のEメールを使ってください。
       models:
+        ownership:
+          attributes:
+            user_id:
+              already_confirmed:
+              already_invited:
         version:
           attributes:
             gem_full_name:

config/locales/nl.yml

@@ -58,6 +58,11 @@ nl:
         unpwn:
         blocked:
       models:
+        ownership:
+          attributes:
+            user_id:
+              already_confirmed:
+              already_invited:
         version:
           attributes:
             gem_full_name:

config/locales/pt-BR.yml

@@ -65,6 +65,11 @@ pt-BR:
         unpwn: já apareceu anteriormente em um vazamento de dados e não deve ser utilizada
         blocked:
       models:
+        ownership:
+          attributes:
+            user_id:
+              already_confirmed:
+              already_invited:
         version:
           attributes:
             gem_full_name:

config/locales/zh-CN.yml

@@ -60,6 +60,11 @@ zh-CN:
         unpwn: 曾出现过数据泄露，不应该再使用
         blocked: 域名 '%{domain}' 因发送垃圾邮件已被禁用。请使用另外有效的个人邮箱。
       models:
+        ownership:
+          attributes:
+            user_id:
+              already_confirmed:
+              already_invited:
         version:
           attributes:
             gem_full_name:

config/locales/zh-TW.yml

@@ -55,6 +55,11 @@ zh-TW:
         unpwn:
         blocked:
       models:
+        ownership:
+          attributes:
+            user_id:
+              already_confirmed:
+              already_invited:
         version:
           attributes:
             gem_full_name:

test/functional/api/v1/owners_controller_test.rb

@@ -241,7 +241,20 @@ def self.should_respond_to(format)
         should respond_with :unprocessable_entity
 
         should "respond with error message" do
-          assert_equal "User has already been taken", @response.body
+          assert_equal "User is already an owner of this gem", @response.body
+        end
+      end
+
+      context "owner has already been invited" do
+        setup do
+          post :create, params: { rubygem_id: @rubygem.slug, email: @second_user.email }
+          post :create, params: { rubygem_id: @rubygem.slug, email: @second_user.email }
+        end
+
+        should respond_with :unprocessable_entity
+
+        should "respond with error message" do
+          assert_equal "User is already invited to this gem", @response.body
         end
       end
 

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 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