FIX: Rejected emails should not be cleaned up before their logs (#17648)

* FIX: Rejected emails should not be cleaned up before their logs

If we delete the rejected emails before we delete their associated logs
we will receive 404 errors trying to inspect an email message for that
log.

* don't add a blank line

* test for max value as well

* pr cleanup and add migration

* Fix failing test

config/locales/server.en.yml

@@ -2432,6 +2432,7 @@ en:
       search_tokenize_chinese_enabled: "You must disable 'search_tokenize_chinese' before enabling this setting."
       search_tokenize_japanese_enabled: "You must disable 'search_tokenize_japanese' before enabling this setting."
       discourse_connect_cannot_be_enabled_if_second_factor_enforced: "You cannot enable DiscourseConnect if 2FA is enforced."
+      delete_rejected_email_after_days: "This setting cannot be set lower than the delete_email_logs_after_days setting or greater than %{max}"
 
     placeholder:
       discourse_connect_provider_secrets:

config/site_settings.yml

@@ -1266,7 +1266,7 @@ email:
   raw_rejected_email_max_length: 4000
   delete_rejected_email_after_days:
     default: 90
-    max: 36500
+    validator: "DeleteRejectedEmailAfterDaysValidator"
   enable_secondary_emails:
     client: true
     default: true

db/migrate/20220726164831_fix_delete_rejected_email_after_days_site_setting.rb

@@ -0,0 +1,26 @@
+# frozen_string_literal: true
+class FixDeleteRejectedEmailAfterDaysSiteSetting < ActiveRecord::Migration[6.1]
+  def up
+    delete_rejected_email_after_days = DB.query_single("SELECT value FROM site_settings WHERE name = 'delete_rejected_email_after_days'").first
+    delete_email_logs_after_days = DB.query_single("SELECT value FROM site_settings WHERE name = 'delete_email_logs_after_days'").first
+
+    # These settings via the sql query return nil if they are using their default values
+    unless delete_email_logs_after_days
+      delete_email_logs_after_days = DeleteRejectedEmailAfterDaysValidator::MAX
+    end
+
+    # Only update if the setting is not using the default and it is lower than 'delete_email_logs_after_days'
+    if delete_rejected_email_after_days != nil && delete_rejected_email_after_days.to_i < delete_email_logs_after_days.to_i
+      execute <<~SQL
+      UPDATE site_settings
+      SET value = #{delete_email_logs_after_days.to_i}
+      WHERE name = 'delete_rejected_email_after_days'
+      SQL
+    end
+
+  end
+
+  def down
+    raise ActiveRecord::IrreversibleMigration
+  end
+end

lib/validators/delete_rejected_email_after_days_validator.rb

@@ -0,0 +1,17 @@
+# frozen_string_literal: true
+
+class DeleteRejectedEmailAfterDaysValidator
+  MAX = 36500
+
+  def initialize(opts = {})
+    @opts = opts
+  end
+
+  def valid_value?(value)
+    @valid = value.to_i >= SiteSetting.delete_email_logs_after_days && value.to_i <= MAX
+  end
+
+  def error_message
+    I18n.t("site_settings.errors.delete_rejected_email_after_days", max: MAX) if !@valid
+  end
+end

spec/lib/validators/delete_rejected_email_after_days_spec.rb

@@ -0,0 +1,15 @@
+# frozen_string_literal: true
+
+describe DeleteRejectedEmailAfterDaysValidator do
+
+  it 'is not valid if value is smaller than the value of SiteSetting.delete_email_logs_after_days' do
+    SiteSetting.delete_email_logs_after_days = 90
+
+    expect { SiteSetting.delete_rejected_email_after_days = 89 }.to raise_error(Discourse::InvalidParameters)
+  end
+
+  it "is not valid if value is greater than #{DeleteRejectedEmailAfterDaysValidator::MAX}" do
+    expect { SiteSetting.delete_rejected_email_after_days = DeleteRejectedEmailAfterDaysValidator::MAX + 1 }.to raise_error(Discourse::InvalidParameters)
+  end
+
+end