From ef2a9d58af66f6a83610b671a06319f45f009ee8 Mon Sep 17 00:00:00 2001
From: Matt Jankowski <matt@jankowski.online>
Date: Wed, 11 Sep 2024 11:16:26 -0400
Subject: [PATCH] Add coverage for name/display_name changes in `Tag` model

---
 app/models/tag.rb       | 25 ++++++++++++++++++-------
 spec/models/tag_spec.rb | 34 +++++++++++++++++++++++++++++++++-
 2 files changed, 51 insertions(+), 8 deletions(-)

diff --git a/app/models/tag.rb b/app/models/tag.rb
index acf514919b..899d19a5e9 100644
--- a/app/models/tag.rb
+++ b/app/models/tag.rb
@@ -47,8 +47,10 @@ class Tag < ApplicationRecord
 
   validates :name, presence: true, format: { with: HASHTAG_NAME_RE }
   validates :display_name, format: { with: HASHTAG_NAME_RE }
-  validate :validate_name_change, if: -> { !new_record? && name_changed? }
-  validate :validate_display_name_change, if: -> { !new_record? && display_name_changed? }
+  with_options on: :update do
+    validate :validate_name_change, if: :name_changed?
+    validate :validate_display_name_change, if: :display_name_changed?
+  end
 
   scope :reviewed, -> { where.not(reviewed_at: nil) }
   scope :unreviewed, -> { where(reviewed_at: nil) }
@@ -160,13 +162,22 @@ class Tag < ApplicationRecord
   private
 
   def validate_name_change
-    errors.add(:name, I18n.t('tags.does_not_match_previous_name')) unless name_was.mb_chars.casecmp(name.mb_chars).zero?
+    errors.add(:name, previous_name_error_message) unless matches_name_chars?(name_was.mb_chars)
   end
 
   def validate_display_name_change
-    unless HashtagNormalizer.new.normalize(display_name).casecmp(name.mb_chars).zero?
-      errors.add(:display_name,
-                 I18n.t('tags.does_not_match_previous_name'))
-    end
+    errors.add(:display_name, previous_name_error_message) unless matches_name_chars?(normalized_display_name)
+  end
+
+  def normalized_display_name
+    HashtagNormalizer.new.normalize(display_name)
+  end
+
+  def matches_name_chars?(value)
+    value.casecmp(name.mb_chars).zero?
+  end
+
+  def previous_name_error_message
+    I18n.t('tags.does_not_match_previous_name')
   end
 end
diff --git a/spec/models/tag_spec.rb b/spec/models/tag_spec.rb
index 18dd26be94..a1cc6a064f 100644
--- a/spec/models/tag_spec.rb
+++ b/spec/models/tag_spec.rb
@@ -5,7 +5,39 @@ require 'rails_helper'
 RSpec.describe Tag do
   include_examples 'Reviewable'
 
-  describe 'validations' do
+  describe 'Validations' do
+    describe 'name' do
+      context 'with a new record' do
+        subject { Fabricate.build :tag, name: 'original' }
+
+        it { is_expected.to allow_value('changed').for(:name) }
+      end
+
+      context 'with an existing record' do
+        subject { Fabricate :tag, name: 'original' }
+
+        it { is_expected.to_not allow_value('changed').for(:name).with_message(previous_name_error_message) }
+      end
+    end
+
+    describe 'display_name' do
+      context 'with a new record' do
+        subject { Fabricate.build :tag, name: 'original', display_name: 'OriginalDisplayName' }
+
+        it { is_expected.to allow_value('ChangedDisplayName').for(:display_name) }
+      end
+
+      context 'with an existing record' do
+        subject { Fabricate :tag, name: 'original', display_name: 'OriginalDisplayName' }
+
+        it { is_expected.to_not allow_value('ChangedDisplayName').for(:display_name).with_message(previous_name_error_message) }
+      end
+    end
+
+    def previous_name_error_message
+      I18n.t('tags.does_not_match_previous_name')
+    end
+
     it 'invalid with #' do
       expect(described_class.new(name: '#hello_world')).to_not be_valid
     end