Add coverage for `URLValidator` (#25591)

app/validators/url_validator.rb

@@ -1,16 +1,31 @@
 # frozen_string_literal: true
 
 class URLValidator < ActiveModel::EachValidator
+  VALID_SCHEMES = %w(http https).freeze
+
   def validate_each(record, attribute, value)
-    record.errors.add(attribute, :invalid) unless compliant?(value)
+    @value = value
+
+    record.errors.add(attribute, :invalid) unless compliant_url?
   end
 
   private
 
-  def compliant?(url)
+  def compliant_url?
-    parsed_url = Addressable::URI.parse(url)
+    parsed_url.present? && valid_url_scheme? && valid_url_host?
-    parsed_url && %w(http https).include?(parsed_url.scheme) && parsed_url.host
+  end
+
+  def parsed_url
+    Addressable::URI.parse(@value)
   rescue Addressable::URI::InvalidURIError
     false
   end
+
+  def valid_url_scheme?
+    VALID_SCHEMES.include?(parsed_url.scheme)
+  end
+
+  def valid_url_host?
+    parsed_url.host.present?
+  end
 end

spec/validators/url_validator_spec.rb

@@ -2,32 +2,64 @@
 
 require 'rails_helper'
 
-RSpec.describe URLValidator, type: :validator do
+describe URLValidator do
+  let(:record_class) do
+    Class.new do
+      include ActiveModel::Validations
+      attr_accessor :profile
+
+      validates :profile, url: true
+    end
+  end
+  let(:record) { record_class.new }
+
   describe '#validate_each' do
-    before do
+    context 'with a nil value' do
-      allow(validator).to receive(:compliant?).with(value) { compliant }
+      it 'adds errors' do
-      validator.validate_each(record, attribute, value)
+        record.profile = nil
+
+        expect(record).to_not be_valid
+        expect(record.errors.first.attribute).to eq(:profile)
+        expect(record.errors.first.type).to eq(:invalid)
+      end
     end
 
-    let(:validator) { described_class.new(attributes: [attribute]) }
+    context 'with an invalid url scheme' do
-    let(:record)    { instance_double(Webhook, errors: errors) }
+      it 'adds errors' do
-    let(:errors)    { instance_double(ActiveModel::Errors, add: nil) }
+        record.profile = 'ftp://example.com/page'
-    let(:value)     { '' }
+
-    let(:attribute) { :foo }
+        expect(record).to_not be_valid
+        expect(record.errors.first.attribute).to eq(:profile)
+        expect(record.errors.first.type).to eq(:invalid)
+      end
+    end
+
+    context 'without a hostname' do
+      it 'adds errors' do
+        record.profile = 'https:///page'
+
+        expect(record).to_not be_valid
+        expect(record.errors.first.attribute).to eq(:profile)
+        expect(record.errors.first.type).to eq(:invalid)
+      end
+    end
 
-    context 'when not compliant?' do
+    context 'with an unparseable value' do
-      let(:compliant) { false }
+      it 'adds errors' do
+        record.profile = 'https://host:port/page' # non-numeric port string causes invalid uri error
 
-      it 'calls errors.add' do
+        expect(record).to_not be_valid
-        expect(errors).to have_received(:add).with(attribute, :invalid)
+        expect(record.errors.first.attribute).to eq(:profile)
+        expect(record.errors.first.type).to eq(:invalid)
       end
     end
 
-    context 'when compliant?' do
+    context 'with a valid url' do
-      let(:compliant) { true }
+      it 'does not add errors' do
+        record.profile = 'https://example.com/page'
 
-      it 'not calls errors.add' do
+        expect(record).to be_valid
-        expect(errors).to_not have_received(:add).with(attribute, any_args)
+        expect(record.errors).to be_empty
       end
     end
   end