From b71865fc426eacdc30388b82748064b23bab7cf3 Mon Sep 17 00:00:00 2001 From: Jeremy Friesen Date: Mon, 6 Dec 2021 17:07:47 -0500 Subject: [PATCH] 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 Co-authored-by: Michael Kohl --- app/models/organization.rb | 13 +--- app/models/page.rb | 14 +--- app/models/podcast.rb | 8 +- app/models/user.rb | 12 +-- .../unique_cross_model_slug_validator.rb | 45 ++++++++++++ .../unique_cross_model_slug_validator_spec.rb | 73 +++++++++++++++++++ 6 files changed, 123 insertions(+), 42 deletions(-) create mode 100644 app/validators/unique_cross_model_slug_validator.rb create mode 100644 spec/validators/unique_cross_model_slug_validator_spec.rb 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