[hotfix] website_url update script should handle parse errors (#14306)

* Add additional test cases to the DUS

And then handle them.

* rubocop changes

Keep let blocks together
prefer blank? to ! + present?

* Test copied script as well

Since the file was fixed, then copied - we want to test the copy that
will actually run (these _should_ be idempotent and running both back
to back will cause no harm).

lib/data_update_scripts/20210722135549_profile_website_url_format.rb

@@ -10,9 +10,15 @@ module DataUpdateScripts
     def fix_or_clear_website_url(profile)
       profile.website_url =
         begin
-          new_url = "https://#{profile.website_url}"
+          new_url = "https://#{profile.website_url.strip}"
           uri = URI.parse(new_url)
-          uri.scheme.present? && uri.host.present? && new_url || ""
+          if acceptable_uri?(uri)
+            new_url
+          else
+            ""
+          end
+        rescue URI::InvalidURIError
+          ""
         end
     end
 
@@ -25,5 +31,11 @@ module DataUpdateScripts
         .where.not("website_url like 'http__/%'")
         .where.not(website_url: [nil, ""])
     end
+
+    def acceptable_uri?(uri)
+      uri.scheme.present? &&
+        uri.host.present? &&
+        uri.user.blank?
+    end
   end
 end

lib/data_update_scripts/20210722160452_profile_website_url_format_fixup.rb

@@ -0,0 +1,41 @@
+module DataUpdateScripts
+  class ProfileWebsiteUrlFormatFixup
+    def run
+      profiles_to_fix.each do |profile|
+        fix_or_clear_website_url(profile)
+        profile.save || log_failure(profile)
+      end
+    end
+
+    def fix_or_clear_website_url(profile)
+      profile.website_url =
+        begin
+          new_url = "https://#{profile.website_url.strip}"
+          uri = URI.parse(new_url)
+          if acceptable_uri?(uri)
+            new_url
+          else
+            ""
+          end
+        rescue URI::InvalidURIError
+          ""
+        end
+    end
+
+    def log_failure(profile)
+      Rails.logger.warn("Attempted to update website_url for profile #{profile.id} but failed with #{profile.errors}")
+    end
+
+    def profiles_to_fix
+      Profile
+        .where.not("website_url like 'http__/%'")
+        .where.not(website_url: [nil, ""])
+    end
+
+    def acceptable_uri?(uri)
+      uri.scheme.present? &&
+        uri.host.present? &&
+        uri.user.blank?
+    end
+  end
+end

spec/lib/data_update_scripts/profile_website_url_format_fixup_spec.rb

@@ -0,0 +1,70 @@
+require "rails_helper"
+require Rails.root.join(
+  "lib/data_update_scripts/20210722160452_profile_website_url_format_fixup.rb",
+)
+
+describe DataUpdateScripts::ProfileWebsiteUrlFormatFixup do
+  let(:nil_profile) { create(:profile, website_url: nil) }
+  let(:invalid_profile) { make_profile_for("www.example.com") }
+  let(:unfixable_profile) { make_profile_for("/local.html") }
+  let(:email_profile) { make_profile_for("dan@forem.com") }
+  let(:spacey_profile) { make_profile_for("  example.com  ") }
+  let(:unparseable_profile) { make_profile_for("this will not parse") }
+  let(:empty_profile) { create(:profile, website_url: "") }
+  let(:valid_profile) { create(:profile, website_url: "https://www.example.com") }
+
+  def make_profile_for(website_url)
+    build(:profile, website_url: website_url).tap do |profile|
+      profile.save(validate: false)
+    end
+  end
+
+  it "does not modify profiles where website url is null" do
+    expect { described_class.new.run }.not_to change(nil_profile, :website_url)
+  end
+
+  it "does not modify profiles where website url is empty" do
+    expect { described_class.new.run }.not_to change(empty_profile, :website_url)
+  end
+
+  it "does not modify profiles where website url is valid" do
+    expect { described_class.new.run }.not_to change(valid_profile, :website_url)
+  end
+
+  it "prepends https:// to invalid urls to make a valid url from hostnames" do
+    expect { described_class.new.run }
+      .to change { invalid_profile.reload.website_url }
+      .from("www.example.com")
+      .to("https://www.example.com")
+  end
+
+  it "clears websites that don't form valid urls by prepending" do
+    expect { described_class.new.run }
+      .to change { unfixable_profile.reload.website_url }
+      .from("/local.html")
+      .to("")
+  end
+
+  it "rejects users in the link" do
+    # what was meant to be an email address is actually a valid user@host url
+    # we just don't want to do this
+    expect { described_class.new.run }
+      .to change { email_profile.reload.website_url }
+      .from("dan@forem.com")
+      .to("")
+  end
+
+  it "trims the input to help parsing" do
+    expect { described_class.new.run }
+      .to change { spacey_profile.reload.website_url }
+      .from("  example.com  ")
+      .to("https://example.com")
+  end
+
+  it "handles parse errors by clearing the website url" do
+    expect { described_class.new.run }
+      .to change { unparseable_profile.reload.website_url }
+      .from("this will not parse")
+      .to("")
+  end
+end

spec/lib/data_update_scripts/profile_website_url_format_spec.rb

@@ -5,17 +5,16 @@ require Rails.root.join(
 
 describe DataUpdateScripts::ProfileWebsiteUrlFormat do
   let(:nil_profile) { create(:profile, website_url: nil) }
+  let(:invalid_profile) { make_profile_for("www.example.com") }
+  let(:unfixable_profile) { make_profile_for("/local.html") }
+  let(:email_profile) { make_profile_for("dan@forem.com") }
+  let(:spacey_profile) { make_profile_for("  example.com  ") }
+  let(:unparseable_profile) { make_profile_for("this will not parse") }
   let(:empty_profile) { create(:profile, website_url: "") }
   let(:valid_profile) { create(:profile, website_url: "https://www.example.com") }
 
-  let(:invalid_profile) do
-    build(:profile, website_url: "www.example.com").tap do |profile|
-      profile.save(validate: false)
-    end
-  end
-
-  let(:unfixable_profile) do
-    build(:profile, website_url: "/local.html").tap do |profile|
+  def make_profile_for(website_url)
+    build(:profile, website_url: website_url).tap do |profile|
       profile.save(validate: false)
     end
   end
@@ -45,4 +44,27 @@ describe DataUpdateScripts::ProfileWebsiteUrlFormat do
       .from("/local.html")
       .to("")
   end
+
+  it "rejects users in the link" do
+    # what was meant to be an email address is actually a valid user@host url
+    # we just don't want to do this
+    expect { described_class.new.run }
+      .to change { email_profile.reload.website_url }
+      .from("dan@forem.com")
+      .to("")
+  end
+
+  it "trims the input to help parsing" do
+    expect { described_class.new.run }
+      .to change { spacey_profile.reload.website_url }
+      .from("  example.com  ")
+      .to("https://example.com")
+  end
+
+  it "handles parse errors by clearing the website url" do
+    expect { described_class.new.run }
+      .to change { unparseable_profile.reload.website_url }
+      .from("this will not parse")
+      .to("")
+  end
 end