Extracting cross-model uniqueness validator (#15640)

* Extracting cross-model uniqueness validator

Prior to this commit, we had somewhat duplicated logic across four
models.  In adding this validator we're consolidating the logic and
tidying up each of the other models; letting the validator know which
models are part of the "uniqueness gang."

* Adding conditional test for validator

* Bump for travis

* Apply suggestions from code review

Co-authored-by: Michael Kohl <citizen428@forem.com>

Co-authored-by: Michael Kohl <citizen428@forem.com>

app/models/organization.rb

@@ -67,7 +67,7 @@ class Organization < ApplicationRecord
   validates :unspent_credits_count, presence: true
   validates :url, length: { maximum: 200 }, url: { allow_blank: true, no_local: true }
 
-  validate :unique_slug_including_users_and_podcasts, if: :slug_changed?
+  validates :slug, unique_cross_model_slug: true, if: :slug_changed?
 
   mount_uploader :profile_image, ProfileImageUploader
   mount_uploader :nav_image, ProfileImageUploader
@@ -150,15 +150,4 @@ class Organization < ApplicationRecord
   def bust_cache
     Organizations::BustCacheWorker.perform_async(id, slug)
   end
-
-  def unique_slug_including_users_and_podcasts
-    slug_taken = (
-      User.exists?(username: slug) ||
-      Podcast.exists?(slug: slug) ||
-      Page.exists?(slug: slug) ||
-      slug&.include?("sitemap-")
-    )
-
-    errors.add(:slug, "is taken.") if slug_taken
-  end
 end

app/models/page.rb

@@ -6,7 +6,7 @@ class Page < ApplicationRecord
   validates :slug, presence: true, format: /\A[0-9a-z\-_]*\z/
   validates :template, inclusion: { in: TEMPLATE_OPTIONS }
   validate :body_present
-  validate :unique_slug_including_users_and_orgs, if: :slug_changed?
+  validates :slug, unique_cross_model_slug: true, if: :slug_changed?
 
   before_validation :set_default_template
   before_save :evaluate_markdown
@@ -50,18 +50,6 @@ class Page < ApplicationRecord
     errors.add(:body_markdown, "must exist if body_html or body_json doesn't exist.")
   end
 
-  def unique_slug_including_users_and_orgs
-    slug_exists = (
-      User.exists?(username: slug) ||
-      Organization.exists?(slug: slug) ||
-      Podcast.exists?(slug: slug) ||
-      slug.include?("sitemap-")
-    )
-    return unless slug_exists
-
-    errors.add(:slug, "is taken.")
-  end
-
   # As there can only be one global landing page, we want to ensure that
   # data integrity is preserved by setting `landing_page` to `false` for all
   # other pages if the current one was transformed into a landing page

app/models/podcast.rb

@@ -20,7 +20,8 @@ class Podcast < ApplicationRecord
             uniqueness: true,
             format: { with: /\A[a-zA-Z0-9\-_]+\Z/ },
             exclusion: { in: ReservedWords.all, message: "slug is reserved" }
-  validate :unique_slug_including_users_and_orgs, if: :slug_changed?
+
+  validates :slug, unique_cross_model_slug: true, if: :slug_changed?
 
   after_save :bust_cache
 
@@ -54,11 +55,6 @@ class Podcast < ApplicationRecord
 
   private
 
-  def unique_slug_including_users_and_orgs
-    slug_exists = User.exists?(username: slug) || Organization.exists?(slug: slug) || Page.exists?(slug: slug)
-    errors.add(:slug, "is taken.") if slug_exists
-  end
-
   def bust_cache
     return unless path
 

app/models/user.rb

@@ -163,7 +163,7 @@ class User < ApplicationRecord
   end
 
   validate :non_banished_username, :username_changed?
-  validate :unique_including_orgs_and_podcasts, if: :username_changed?
+  validates :username, unique_cross_model_slug: true, if: :username_changed?
   validate :can_send_confirmation_email
   validate :update_rate_limit
   # NOTE: when updating the password on a Devise enabled model, the :encrypted_password
@@ -439,16 +439,6 @@ class User < ApplicationRecord
     UserBlock.blocking?(blocker_id, id)
   end
 
-  def unique_including_orgs_and_podcasts
-    username_taken = (
-      Organization.exists?(slug: username) ||
-      Podcast.exists?(slug: username) ||
-      Page.exists?(slug: username)
-    )
-
-    errors.add(:username, "is taken.") if username_taken
-  end
-
   def non_banished_username
     errors.add(:username, "has been banished.") if BanishedUser.exists?(username: username)
   end

app/validators/unique_cross_model_slug_validator.rb

@@ -0,0 +1,45 @@
+##
+# Validates if the give attribute is used across the reserved spaces.
+class UniqueCrossModelSlugValidator < ActiveModel::EachValidator
+  class_attribute :model_and_attribute_name_for_uniqueness_test
+
+  # Why a class attribute?  Allow for other implementations to extend
+  # this behavior.
+  self.model_and_attribute_name_for_uniqueness_test = {
+    Organization => :slug,
+    Page => :slug,
+    Podcast => :slug,
+    User => :username
+  }
+
+  DEFAULT_MESSAGE = "is taken".freeze
+
+  def validate_each(record, attribute, value)
+    return unless already_exists?(value: value, record: record)
+
+    record.errors.add(attribute, options[:message] || DEFAULT_MESSAGE)
+  end
+
+  private
+
+  ##
+  # Answers the question if it's okay for the record to use the given value.
+  #
+  # @param value [String] the value we're to check in the various classes
+  # @param record [ActiveRecord::Base] the record that we're attempting to validate
+  #
+  # @return [TrueClass] if the value already exists across the various classes.
+  # @return [FalseClass] if the given value is not already used.
+  #
+  # @see CLASS_AND_ATTRIBUTE_NAME_FOR_UNIQUNESS_TEST
+  def already_exists?(value:, record:)
+    return false unless value
+    return true if value.include?("sitemap-")
+
+    model_and_attribute_name_for_uniqueness_test.detect do |model, attribute|
+      next if record.is_a?(model)
+
+      model.exists?(attribute => value)
+    end
+  end
+end

spec/validators/unique_cross_model_slug_validator_spec.rb

@@ -0,0 +1,73 @@
+require "rails_helper"
+
+RSpec.describe UniqueCrossModelSlugValidator do
+  subject(:record) { validatable.new.tap { |m| m.name = name } }
+
+  let(:validatable) do
+    Class.new do
+      def self.name
+        "Validatable"
+      end
+      include ActiveModel::Validations
+      attr_accessor :name
+
+      def initialize
+        @enforce_validation = true
+      end
+
+      attr_accessor :enforce_validation
+      alias_method :enforce_validation?, :enforce_validation
+
+      validates :name, unique_cross_model_slug: true, if: :enforce_validation?
+    end
+  end
+
+  context "when if option is false" do
+    let(:name) { "sitemap-happy" }
+
+    before { record.enforce_validation = false }
+
+    it { is_expected.to be_valid }
+  end
+
+  context "when name includes sitemap-" do
+    let(:name) { "sitemap-happy" }
+
+    it { is_expected.not_to be_valid }
+  end
+
+  context "when name exists in User model" do
+    let(:user) { create(:user) }
+    let(:name) { user.username }
+
+    it { is_expected.not_to be_valid }
+  end
+
+  context "when name exists in Organization model" do
+    let(:org) { create(:organization) }
+    let(:name) { org.slug }
+
+    it { is_expected.not_to be_valid }
+  end
+
+  context "when name exists in Podcast model" do
+    let(:org) { create(:podcast) }
+    let(:name) { org.slug }
+
+    it { is_expected.not_to be_valid }
+  end
+
+  context "when name exists in Page model" do
+    let(:org) { create(:page) }
+    let(:name) { org.slug }
+
+    it { is_expected.not_to be_valid }
+  end
+
+  context "when name is something different" do
+    let(:org) { create(:organization) }
+    let(:name) { "not-#{org.slug}" }
+
+    it { is_expected.to be_valid }
+  end
+end