Merge branch 'jswain_remove_release_highlight_image_url_validation' into 'master'

Remove image_url requirement on ReleaseHighlights

See merge request https://gitlab.com/gitlab-org/gitlab/-/merge_requests/86279



Merged-by: Allison Browne <abrowne@gitlab.com>
Approved-by: Jeremy Jackson <jjackson@gitlab.com>
Approved-by: Allison Browne <abrowne@gitlab.com>
Approved-by: Eugie Limpin <elimpin@gitlab.com>
Reviewed-by: Jay <jswain@gitlab.com>
Co-authored-by: Jay Swain <jswain@gitlab.com>

lib/release_highlights/validator/entry.rb

@@ -6,12 +6,16 @@ class Validator::Entry
     include ActiveModel::Validations::Callbacks
 
     AVAILABLE_IN = %w(Free Premium Ultimate).freeze
+    HYPHENATED_ATTRIBUTES = [:self_managed, :gitlab_com].freeze
 
     attr_reader :entry
+    attr_accessor :available_in, :description, :gitlab_com, :image_url, :name, :published_at, :release, :self_managed,
+      :stage
 
     validates :name, :description, :stage, presence: true
-    validates :'self-managed', :'gitlab-com', inclusion: { in: [true, false], message: "must be a boolean" }
-    validates :documentation_link, :image_url, public_url: { dns_rebind_protection: true }
+    validates :self_managed, :gitlab_com, inclusion: { in: [true, false], message: "must be a boolean" }
+    validates :documentation_link, public_url: { dns_rebind_protection: true }
+    validates :image_url, public_url: { dns_rebind_protection: true }, allow_nil: true
     validates :release, numericality: true
     validate :validate_published_at
     validate :validate_available_in
@@ -67,11 +71,13 @@ def value_for(key)
       index = entry.children.find_index(node)
 
       next_node = entry.children[index + 1]
+
       next_node&.to_ruby
     end
 
     def find_node(key)
-      entry.children.find { |node| node.try(:value) == key.to_s }
+      formatted_key = key.in?(HYPHENATED_ATTRIBUTES) ? key.to_s.dasherize : key.to_s
+      entry.children.find { |node| node.try(:value) == formatted_key }
     end
   end
 end

spec/lib/release_highlights/validator/entry_spec.rb

@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-RSpec.describe ReleaseHighlights::Validator::Entry do
+RSpec.describe ReleaseHighlights::Validator::Entry, type: :model, feature_category: :onboarding do
   subject(:entry) { described_class.new(document.root.children.first) }
 
   let(:document) { YAML.parse(File.read(yaml_path)) }
@@ -22,34 +22,26 @@
     context 'with an invalid entry' do
       let(:yaml_path) { 'spec/fixtures/whats_new/invalid.yml' }
 
-      it 'returns line numbers in errors' do
-        subject.valid?
-
-        expect(entry.errors[:available_in].first).to match('(line 6)')
-      end
+      it { is_expected.to be_invalid }
     end
 
     context 'with a blank entry' do
-      it 'validate presence of name, description and stage' do
-        subject.valid?
-
-        expect(subject.errors[:name]).not_to be_empty
-        expect(subject.errors[:description]).not_to be_empty
-        expect(subject.errors[:stage]).not_to be_empty
-        expect(subject.errors[:available_in]).not_to be_empty
-      end
-
-      it 'validates boolean value of "self-managed" and "gitlab-com"' do
-        allow(entry).to receive(:value_for).with(:'self-managed').and_return('nope')
-        allow(entry).to receive(:value_for).with(:'gitlab-com').and_return('yerp')
-
-        subject.valid?
-
-        expect(subject.errors[:'self-managed']).to include(/must be a boolean/)
-        expect(subject.errors[:'gitlab-com']).to include(/must be a boolean/)
-      end
-
-      it 'validates URI of "url" and "image_url"' do
+      it { is_expected.to validate_presence_of(:name).with_message(/can't be blank \(line [0-9]+\)/) }
+      it { is_expected.to validate_presence_of(:description).with_message(/can't be blank/) }
+      it { is_expected.to validate_presence_of(:stage).with_message(/can't be blank/) }
+      it { is_expected.to validate_presence_of(:self_managed).with_message(/must be a boolean/) }
+      it { is_expected.to validate_presence_of(:gitlab_com).with_message(/must be a boolean/) }
+      it { is_expected.to allow_value(nil).for(:image_url) }
+
+      it {
+        is_expected.to validate_presence_of(:available_in)
+          .with_message(/must be one of \["Free", "Premium", "Ultimate"\]/)
+      }
+
+      it { is_expected.to validate_presence_of(:published_at).with_message(/must be valid Date/) }
+      it { is_expected.to validate_numericality_of(:release).with_message(/is not a number/) }
+
+      it 'validates URI of "documentation_link" and "image_url"' do
         stub_env('RSPEC_ALLOW_INVALID_URLS', 'false')
         allow(entry).to receive(:value_for).with(:image_url).and_return('https://foobar.x/images/ci/gitlab-ci-cd-logo_2x.png')
         allow(entry).to receive(:value_for).with(:documentation_link).and_return('')
@@ -60,16 +52,8 @@
         expect(subject.errors[:image_url]).to include(/is blocked: Host cannot be resolved or invalid/)
       end
 
-      it 'validates release is numerical' do
-        allow(entry).to receive(:value_for).with(:release).and_return('one')
-
-        subject.valid?
-
-        expect(subject.errors[:release]).to include(/is not a number/)
-      end
-
       it 'validates published_at is a date' do
-        allow(entry).to receive(:value_for).with(:published_at).and_return('christmas day')
+        allow(entry).to receive(:published_at).and_return('christmas day')
 
         subject.valid?
 
@@ -77,7 +61,7 @@
       end
 
       it 'validates available_in are included in list' do
-        allow(entry).to receive(:value_for).with(:available_in).and_return(['ALL'])
+        allow(entry).to receive(:available_in).and_return(['ALL'])
 
         subject.valid?