Adds validations to avoid creating unnecessary PNs (#14621)

* Adds validations to avoid creating unnecessary PNs

* Fix conditional + add tests

* Trigger CI

* Trigger CI

* Add missing rpush mock in spec

* Mock before testing d'oh

* Extend ability to mock rpush in helpers

* Remove unnecessary user3 from spec

app/models/device.rb

@@ -12,6 +12,11 @@ class Device < ApplicationRecord
   validates :token, uniqueness: { scope: %i[user_id platform consumer_app_id] }
 
   def create_notification(title, body, payload)
+    # There's no need to create notifications for Consumer Apps that aren't
+    # operational. This happens when credentials aren't configured or delivery
+    # errors have been raised (i.e. expired authentication certificates)
+    return unless consumer_app.operational?
+
     if android?
       android_notification(title, body, payload)
     elsif ios?

app/queries/consumer_apps/rpush_app_query.rb

@@ -29,6 +29,9 @@ module ConsumerApps
     attr_reader :app_bundle, :consumer_app
 
     def recreate_ios_app!
+      # If the ConsumerApp doesn't have credentials there's no need to create it
+      return if consumer_app.auth_credentials.blank?
+
       app = Rpush::Apns2::App.new
       app.name = app_bundle
       app.certificate = consumer_app.auth_credentials.to_s.gsub("\\n", "\n")
@@ -41,6 +44,9 @@ module ConsumerApps
     end
 
     def recreate_android_app!
+      # If the ConsumerApp doesn't have credentials there's no need to create it
+      return if consumer_app.auth_credentials.blank?
+
       app = Rpush::Gcm::App.new
       app.name = app_bundle
       app.auth_key = consumer_app.auth_credentials.to_s

spec/queries/consumer_apps/rpush_app_query_spec.rb

@@ -15,5 +15,16 @@ RSpec.describe ConsumerApps::RpushAppQuery, type: :query do
       expect(rpush_app).to be_instance_of(Rpush::Apns2::App)
       expect(rpush_app.name).to eq(consumer_app.app_bundle)
     end
+
+    it "returns nil if ConsumerApp is not operational" do
+      bad_consumer_app = create(:consumer_app, auth_key: nil)
+
+      mock_rpush(bad_consumer_app, empty: true)
+
+      rpush_app = described_class.call(app_bundle: bad_consumer_app.app_bundle, platform: bad_consumer_app.platform)
+
+      expect(bad_consumer_app.operational?).to be false
+      expect(rpush_app).to be_nil
+    end
   end
 end

spec/services/push_notifications/send_spec.rb

@@ -20,6 +20,11 @@ RSpec.describe PushNotifications::Send, type: :service do
     }
   end
 
+  before do
+    allow(ApplicationConfig).to receive(:[]).with("RPUSH_IOS_PEM").and_return("dGVzdGluZw==")
+    allow(ApplicationConfig).to receive(:[]).and_return("stub")
+  end
+
   context "with no devices for user" do
     it "does nothing", :aggregate_failures do
       described_class.call(**params)
@@ -29,11 +34,6 @@ RSpec.describe PushNotifications::Send, type: :service do
   end
 
   context "with devices for one user" do
-    before do
-      allow(ApplicationConfig).to receive(:[]).with("RPUSH_IOS_PEM").and_return("dGVzdGluZw==")
-      allow(ApplicationConfig).to receive(:[]).with("COMMUNITY_NAME").and_return("Forem")
-    end
-
     it "creates a notification and enqueues it" do
       device = create(:device, user: user)
       mocked_objects = mock_rpush(device.consumer_app)
@@ -62,9 +62,6 @@ RSpec.describe PushNotifications::Send, type: :service do
     let(:consumer_app) { create(:consumer_app) }
 
     before do
-      allow(ApplicationConfig).to receive(:[]).with("RPUSH_IOS_PEM").and_return("dGVzdGluZw==")
-      allow(ApplicationConfig).to receive(:[]).with("COMMUNITY_NAME").and_return("Forem")
-
       create(:device, user: user, consumer_app: consumer_app)
       create(:device, user: user2, consumer_app: consumer_app)
     end
@@ -91,4 +88,34 @@ RSpec.describe PushNotifications::Send, type: :service do
       sidekiq_assert_enqueued_jobs(1, only: PushNotifications::DeliverWorker)
     end
   end
+
+  context "when associated to operational vs non-operational ConsumerApp" do
+    it "doesn't create any new (RPush) Push Notifications if non-operational" do
+      bad_consumer_app = create(:consumer_app, auth_key: nil)
+      create(:device, user: user, consumer_app: bad_consumer_app)
+      bad_params = {
+        user_ids: [user.id],
+        title: "Alert 3",
+        body: "some other alert!",
+        payload: ""
+      }
+
+      mocked_objects = mock_rpush(bad_consumer_app)
+
+      described_class.call(**bad_params)
+
+      # rpush_notification was saved 0 times -> No new notifications were created
+      expect(mocked_objects[:rpush_notification]).to have_received(:save!).exactly(0).times
+    end
+  end
+
+  it "does create new (RPush) Push Notifications if operational" do
+    device = create(:device, user: user)
+    mocked_objects = mock_rpush(device.consumer_app)
+
+    described_class.call(**params)
+
+    # rpush_notification was saved 1 times -> One new notifications was created
+    expect(mocked_objects[:rpush_notification]).to have_received(:save!).exactly(1).times
+  end
 end

spec/support/rpush_helpers.rb

@@ -1,5 +1,9 @@
 module RpushHelpers
-  def mock_rpush(consumer_app)
+  # This method creates the mocks necessary to test RPush related features since
+  # we use the fakeredis gem and don't have a working Redis server in CI.
+  # The param `empty` allows us to conditionally mock if the app exists or not
+  # Example: Rpush::Apns2::App.where(...).first => nil/app (empty true/false)
+  def mock_rpush(consumer_app, empty: false)
     if consumer_app.android?
       rpush_class = Rpush::Gcm::App
       rpush_notification_class = Rpush::Gcm::Notification
@@ -21,7 +25,7 @@ module RpushHelpers
     allow(rpush_notification_class).to receive(:new).and_return(rpush_notification)
 
     relation = double
-    allow(relation).to receive(:first).and_return(rpush_app)
+    allow(relation).to receive(:first).and_return(empty ? nil : rpush_app)
     allow(rpush_class).to receive(:where).and_return(relation)
 
     original = consumer_app.method(:save)