Add coverage for name/display_name changes in Tag model

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? }
+  with_options on: :update do
-  validate :validate_display_name_change, if: -> { !new_record? && display_name_changed? }
+    validate :validate_name_change, if: :name_changed?
+    validate :validate_display_name_change, if: :display_name_changed?
+  end
 
   scope :pending_review, -> { unreviewed.where.not(requested_review_at: nil) }
   scope :usable, -> { where(usable: [true, nil]) }
@@ -158,13 +160,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, previous_name_error_message) unless matches_name_chars?(normalized_display_name)
-      errors.add(:display_name,
+  end
-                 I18n.t('tags.does_not_match_previous_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

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