Merge branch '336452-fix-terms-deletion' into 'master'

Prevent terms from being created if blank

See merge request gitlab-org/gitlab!66437

app/models/application_setting/term.rb

@@ -3,13 +3,12 @@
 class ApplicationSetting
   class Term < ApplicationRecord
     include CacheMarkdownField
-    include NullifyIfBlank
 
     has_many :term_agreements
 
     cache_markdown_field :terms
 
-    nullify_if_blank :terms
+    validates :terms, presence: true
 
     def self.latest
       order(:id).last

app/services/application_settings/update_service.rb

@@ -67,8 +67,10 @@ module ApplicationSettings
     end
 
     def update_terms(terms)
+      return unless terms.present?
+
       # Avoid creating a new terms record if the text is exactly the same.
-      terms = terms&.strip
+      terms = terms.strip
       return if terms == @application_setting.terms
 
       ApplicationSetting::Term.create(terms: terms)

db/migrate/20210720083432_change_application_setting_terms_not_null.rb

+# frozen_string_literal: true
+
+class ChangeApplicationSettingTermsNotNull < ActiveRecord::Migration[6.1]
+  def up
+    execute("UPDATE application_setting_terms SET terms = '' WHERE terms IS NULL")
+    change_column_null :application_setting_terms, :terms, false
+  end
+
+  def down
+    change_column_null :application_setting_terms, :terms, true
+  end
+end

db/schema_migrations/20210720083432

+6096780be4fae007485f150a019fc4555153e4b22b893d5fe29be36834d970a9
\ No newline at end of file

db/structure.sql

@@ -9216,7 +9216,7 @@ ALTER SEQUENCE appearances_id_seq OWNED BY appearances.id;
 CREATE TABLE application_setting_terms (
     id integer NOT NULL,
     cached_markdown_version integer,
-    terms text,
+    terms text NOT NULL,
     terms_html text
 );
 

spec/models/application_setting/term_spec.rb

@@ -3,7 +3,7 @@
 require 'spec_helper'
 
 RSpec.describe ApplicationSetting::Term do
-  it { is_expected.to nullify_if_blank(:terms) }
+  it { is_expected.to validate_presence_of(:terms) }
 
   describe '.latest' do
     it 'finds the latest terms' do

spec/services/application_settings/update_service_spec.rb

@@ -23,8 +23,8 @@ RSpec.describe ApplicationSettings::UpdateService do
     context 'when the passed terms are blank' do
       let(:params) { { terms: '' } }
 
-      it 'does create terms' do
-        expect { subject.execute }.to change { ApplicationSetting::Term.count }.by(1)
+      it 'does not create terms' do
+        expect { subject.execute }.not_to change { ApplicationSetting::Term.count }
       end
     end