Refactoring to use StringAttributeCleaner (#15641)

* Refactoring to use StringAttributeCleaner

In #15281, we introduced the StringAttributeCleaner.  Let's use it!

* Adding StringAttributeCleaner for User properties

* Parameterizing callback for attribute cleaning

* Reading and applying the example for attribute cleaner

app/lib/string_attribute_cleaner.rb

@@ -6,17 +6,20 @@ module StringAttributeCleaner
   # validation happens.
   #
   # @param attributes [Array<Symbol|String>]
+  # @param on [Symbol] register the cleaning on the specified
+  #        ActiveRecord::Base callback
   #
   # @return [void]
   #
   # @example Defining a list of attributes to be cleaned
   #   include StringAttributeCleaner.for(:attribute1, :attribute2)
-  def self.for(*attributes)
+  #   include StringAttributeCleaner.for(:attribute1, :attribute2, on: :before_save)
+  def self.for(*attributes, on: :before_validation)
     Module.new do
       define_singleton_method(:included) do |klass|
         return unless klass < ActiveRecord::Base
 
-        klass.before_validation(:nullify_blank_attributes)
+        klass.public_send(on, :nullify_blank_attributes)
       end
 
       define_method(:nullify_blank_attributes) do

app/models/article.rb

@@ -9,6 +9,7 @@ class Article < ApplicationRecord
   acts_as_taggable_on :tags
   resourcify
 
+  include StringAttributeCleaner.for(:canonical_url, on: :before_save)
   DEFAULT_FEED_PAGINATION_WINDOW_SIZE = 50
 
   attr_accessor :publish_under_org
@@ -102,7 +103,7 @@ class Article < ApplicationRecord
   before_validation :evaluate_markdown, :create_slug
   before_save :update_cached_user
   before_save :set_all_dates
-  before_save :clean_data
+
   before_save :calculate_base_scores
   before_save :fetch_video_duration
   before_save :set_caches
@@ -793,10 +794,6 @@ class Article < ApplicationRecord
     "#{Sterile.sluggerize(title)}-#{rand(100_000).to_s(26)}"
   end
 
-  def clean_data
-    self.canonical_url = nil if canonical_url == ""
-  end
-
   def touch_actor_latest_article_updated_at(destroying: false)
     return unless destroying || saved_changes.keys.intersection(%w[title cached_tag_list]).present?
 

app/models/user.rb

@@ -21,6 +21,7 @@ class User < ApplicationRecord
     end
   end
 
+  include StringAttributeCleaner.for(:email)
   ANY_ADMIN_ROLES = %i[admin super_admin].freeze
   USERNAME_MAX_LENGTH = 30
   USERNAME_REGEXP = /\A[a-zA-Z0-9_]+\z/
@@ -208,8 +209,8 @@ class User < ApplicationRecord
   }
   before_validation :check_for_username_change
   before_validation :downcase_email
+
   # make sure usernames are not empty, to be able to use the database unique index
-  before_validation :verify_email
   before_validation :set_username
   before_validation :strip_payment_pointer
   before_create :create_users_settings_and_notification_settings_records
@@ -567,10 +568,6 @@ class User < ApplicationRecord
     Notification.send_welcome_notification(id, set_up_profile_broadcast.id)
   end
 
-  def verify_email
-    self.email = nil if email == ""
-  end
-
   def set_username
     set_temp_username if username.blank?
     self.username = username&.downcase

spec/lib/string_attribute_cleaner_spec.rb

@@ -13,6 +13,34 @@ RSpec.describe StringAttributeCleaner, type: :lib do
     end
   end
 
+  with_model :AlternateTestClass do
+    table do |t|
+      t.string :string_attribute
+    end
+
+    model do
+      # rubocop:disable RSpec/DescribedClass
+      include StringAttributeCleaner.for(:string_attribute, on: :before_save)
+      # rubocop:enable RSpec/DescribedClass
+    end
+  end
+
+  context "when specifying an :on callback" do
+    it "registers that callback on the model", :aggregate_failures do
+      before_validation_cbs = AlternateTestClass._validation_callbacks.filter_map do |cb|
+        cb.filter if cb.kind == :before
+      end
+
+      before_save_cbs = AlternateTestClass._save_callbacks.filter_map do |cb|
+        cb.filter if cb.kind == :before
+      end
+
+      expect(before_validation_cbs).to eq([])
+      expect(before_save_cbs).to eq([:nullify_blank_attributes])
+      expect(TestClass.new).to respond_to(:nullify_blank_attributes)
+    end
+  end
+
   it "adds a before_validation callback to the including model", :aggregate_failures do
     before_validation_cbs = TestClass._validation_callbacks.filter_map do |cb|
       cb.filter if cb.kind == :before