Merge pull request #20978 from jrafanie/fix_notification_missing_subs…

…tituted_values

Fix notification missing substituted values, log deprecation if other places do this

(cherry picked from commit cac4c50)

app/models/miq_server/worker_management/monitor.rb

@@ -158,7 +158,15 @@ def do_system_limit_exceeded
 
       msg = "#{w.format_full_log_msg} is being stopped because system resources exceeded threshold, it will be restarted once memory has freed up"
       _log.warn(msg)
-      MiqEvent.raise_evm_event_queue_in_region(w.miq_server, "evm_server_memory_exceeded", :event_details => msg, :type => w.class.name)
+
+      notification_options = {
+        :name             => name,
+        :memory_usage     => memory_usage.to_i,
+        :memory_threshold => memory_threshold,
+        :pid              => pid
+      }
+
+      MiqEvent.raise_evm_event_queue_in_region(w.miq_server, "evm_server_memory_exceeded", :event_details => msg, :type => w.class.name, :full_data => notification_options)
       stop_worker(w, MiqServer::MEMORY_EXCEEDED)
       break
     end

app/models/notification.rb

@@ -18,6 +18,8 @@ class Notification < ApplicationRecord
   serialize :options, Hash
   default_value_for(:options) { Hash.new }
 
+  validate :complete_bindings
+
   scope :of_type, ->(notification_type) { joins(:notification_type).where(:notification_types => {:name => notification_type}) }
 
   def type=(typ)
@@ -54,6 +56,15 @@ def self.notification_text(name, message_params)
 
   private
 
+  def complete_bindings
+    notification_type.message % to_h[:bindings]
+  rescue ArgumentError, KeyError => e
+    # 1. Deprecate now
+    # 2. Fail validation going forward via errors.add(error_args)
+    error_args = [:options, "text bindings for notification_type: '#{notification_type.name}' failed with error: '#{e.message}' with options: '#{options.inspect}' and message #{notification_type.message.inspect}. Next release will not allow a notification without complete bindings."]
+    ActiveSupport::Deprecation.warn(error_args.join(' '), caller_locations[1..-1].reject {|location| location.label.include?("emit_for_event") })
+  end
+
   def emit_message
     return unless ::Settings.server.asynchronous_notifications
     notification_recipients.pluck(:id, :user_id).each do |id, user|

spec/models/notification_spec.rb

@@ -6,14 +6,13 @@
 
   describe '.of_type' do
     it "only returns notification of the given type" do
-      types = NotificationType.all
-      type_name_one   = types[0].name
-      type_name_two   = types[1].name
-      type_name_three = types[2].name
+      type_name_one   = "request_approved"
+      type_name_two   = "request_denied"
+      type_name_three = "vm_retired"
 
-      Notification.create(:type => type_name_one, :initiator => user)
-      Notification.create(:type => type_name_one, :initiator => user)
-      Notification.create(:type => type_name_two, :initiator => user)
+      Notification.create(:type => type_name_one, :initiator => user, :options => {:subject => "Request 1"})
+      Notification.create(:type => type_name_one, :initiator => user, :options => {:subject => "Request 2"})
+      Notification.create(:type => type_name_two, :initiator => user, :options => {:subject => "Request 3"})
 
       expect(Notification.of_type(type_name_one).count).to eq(2)
       expect(Notification.of_type(type_name_two).count).to eq(1)
@@ -95,7 +94,7 @@
         let!(:peer) { FactoryBot.create(:user_with_group, :tenant => tenant) }
         let!(:non_peer) { FactoryBot.create(:user) }
 
-        subject { Notification.create(:initiator => user, :type => 'automate_tenant_info') }
+        subject { Notification.create(:initiator => user, :type => 'automate_tenant_info', :options => {:message => "This is not the message you are looking for."}) }
 
         it 'sends notification to the tenant of initiator' do
           expect(subject.recipients).to match_array([user, peer])