Nilify blanks on the Site model

The optional fields of the site form come through as empty strings when left
blank. These are then saved in the database, when ideally these would be saved
as `nil` in order to preserve the previous behaviour of adding them through
Transition Config for consistency.

We already have a module that handles the nilification of blanks, however we
need to make a couple of changes:
- We add an "except list" of attributes that we don't want to nilify, such as
  Booleans that should be false and not nil (false.blank? == true).
- We allow blank for `special_redirect_strategy` as this will now be nilified
  before save.

app/models/concerns/nilify_blanks.rb

@@ -7,7 +7,15 @@ module NilifyBlanks
 
   def nilify_blanks
     attributes.each do |column, value|
+      next if nilify_except.include?(column.to_sym)
+
       self[column] = nil if value.blank?
     end
   end
+
+private
+
+  def nilify_except
+    []
+  end
 end

app/models/site.rb

@@ -2,6 +2,8 @@
 require "./lib/transition/path_or_url"
 
 class Site < ApplicationRecord
+  include NilifyBlanks
+
   GLOBAL_TYPES = %w[redirect archive].freeze
   SPECIAL_REDIRECT_STRATEGY_TYPES = %w[via_aka supplier].freeze
 
@@ -23,7 +25,7 @@ class Site < ApplicationRecord
   validates :organisation, presence: true
   validates :homepage, presence: true, non_blank_url: true
   validates :abbr, uniqueness: true, presence: true, format: { with: /\A[a-zA-Z0-9_-]+\z/, message: "can only contain alphanumeric characters, underscores and dashes" }
-  validates :special_redirect_strategy, inclusion: { in: SPECIAL_REDIRECT_STRATEGY_TYPES, allow_nil: true }
+  validates :special_redirect_strategy, inclusion: { in: SPECIAL_REDIRECT_STRATEGY_TYPES, allow_blank: true }
   validates :global_new_url, presence: { if: :global_redirect? }
   validates :global_new_url, absence: { if: :global_archive? }
   validates :global_new_url,
@@ -135,4 +137,8 @@ def should_remove_unused_view?
   def remove_all_hits_view
     Postgres::MaterializedView.drop(precomputed_view_name)
   end
+
+  def nilify_except
+    %i[global_redirect_append_path query_params precompute_all_hits_view]
+  end
 end

spec/factories/site_forms.rb

@@ -16,6 +16,16 @@
       special_redirect_strategy { "via_aka" }
     end
 
+    trait :with_blank_optional_fields do
+      homepage_title { "" }
+      homepage_furl { "" }
+      global_type { "" }
+      global_new_url { "" }
+      query_params { "" }
+      global_redirect_append_path { false }
+      special_redirect_strategy { "" }
+    end
+
     trait :with_aliases do
       aliases { "aaib.gov.uk,aaib.com" }
     end

spec/models/site_spec.rb

@@ -143,6 +143,40 @@
     end
   end
 
+  describe "nillifying blanks before validation" do
+    let(:site) { create :site, homepage_furl: "" }
+
+    subject { site.homepage_furl }
+
+    it { is_expected.to be_nil }
+
+    context "attributes not nilified" do
+      describe "#global_redirect_append_path" do
+        let(:site) { create :site, global_redirect_append_path: false }
+
+        subject { site.global_redirect_append_path }
+
+        it { is_expected.to be false }
+      end
+
+      describe "#global_redirect_append_path" do
+        let(:site) { create :site, query_params: "" }
+
+        subject { site.query_params }
+
+        it { is_expected.to eq "" }
+      end
+
+      describe "#precompute_all_hits_view" do
+        let(:site) { create :site, precompute_all_hits_view: false }
+
+        subject { site.precompute_all_hits_view }
+
+        it { is_expected.to be false }
+      end
+    end
+  end
+
   describe "#transition_status" do
     let!(:site) { create :site_without_host }
 

spec/requests/site_creation_spec.rb

@@ -61,4 +61,28 @@
       expect(Site.last.extra_organisations.where(title: "Government digital service")).to exist
     end
   end
+
+  context "with blank optional fields" do
+    let(:params) { attributes_for(:site_form, :with_blank_optional_fields) }
+
+    it "nullifies blanks when creating the new site" do
+      post organisation_sites_path(organisation), params: { site_form: params }
+
+      attributes = {
+        abbr: "aaib",
+        homepage: "https://www.gov.uk/government/organisations/air-accidents-investigation-branch",
+        tna_timestamp: Time.strptime("20141104112824", "%Y%m%d%H%M%S"),
+        global_redirect_append_path: false,
+        global_new_url: nil,
+        special_redirect_strategy: nil,
+        global_type: nil,
+        homepage_title: nil,
+        homepage_furl: nil,
+        query_params: "",
+      }
+
+      expect(Site.last.attributes.with_indifferent_access).to include attributes
+      expect(Site.last.organisation).to eq organisation
+    end
+  end
 end