diff --git a/app/models/organization.rb b/app/models/organization.rb index e0fe93fa5..d963fd772 100644 --- a/app/models/organization.rb +++ b/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 diff --git a/app/models/page.rb b/app/models/page.rb index b57c63874..cba4761a0 100644 --- a/app/models/page.rb +++ b/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 diff --git a/app/models/podcast.rb b/app/models/podcast.rb index 9e445049c..1b825217f 100644 --- a/app/models/podcast.rb +++ b/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 diff --git a/app/models/user.rb b/app/models/user.rb index 7fa2d66d9..836214d9d 100644 --- a/app/models/user.rb +++ b/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 diff --git a/app/validators/unique_cross_model_slug_validator.rb b/app/validators/unique_cross_model_slug_validator.rb new file mode 100644 index 000000000..ef68d9c24 --- /dev/null +++ b/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 diff --git a/spec/validators/unique_cross_model_slug_validator_spec.rb b/spec/validators/unique_cross_model_slug_validator_spec.rb new file mode 100644 index 000000000..54d92db0d --- /dev/null +++ b/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