From 293ff5653a955ffd9d00ffcc71a99b35c6e95ca8 Mon Sep 17 00:00:00 2001
From: Daniel Uber <dan@forem.com>
Date: Thu, 17 Feb 2022 16:30:39 -0600
Subject: [PATCH] Fix integer > nil error when sending reaction notifications
 (#16627)

* don't set nil when size not called

recently, #16570 worked around an issue where aggregated siblings
wasn't present, by only calling size if present.

Unfortunately, this causes a comparison of integer (new json_data
aggregated siblings count) with nil (result of the safe navigation for dig()&.size
assigned nil to previous_siblings_size, when we expected it to be
0). The initial error on the same data moved farther down the controller.

If old_json_data is present, but does not have an array in
reaction.aggregated_siblings, we want to have previous size be zero.

Remove guard clause and previous assignment

The issue was not that old_json_data was nil (it came from an existing
notification) but that there were missing keys (or rather that the
json data for some notifications didn't have a reaction key at all).

Remove the guard clause and either set to the size, or zero if there
is none. I don't understand what the guard clause was for, so replace the
safety by adding a nil safe call to dig. I don't _believe_ this is
possible but in case it was we can shorten the check here.

* Add test to cover missing json_data['reaction'] key

* Remove unneeded temporary variable
---
 app/services/notifications/reactions/send.rb    |  4 +---
 .../notifications/reactions/send_spec.rb        | 17 +++++++++++++++++
 2 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/app/services/notifications/reactions/send.rb b/app/services/notifications/reactions/send.rb
index 66fbf9341..6ff3e6e5f 100644
--- a/app/services/notifications/reactions/send.rb
+++ b/app/services/notifications/reactions/send.rb
@@ -54,11 +54,9 @@ module Notifications
 
           json_data = reaction_json_data(recent_reaction, aggregated_reaction_siblings)
 
-          previous_siblings_size = 0
           notification = Notification.find_or_initialize_by(notification_params)
 
-          old_json_data = notification.json_data
-          previous_siblings_size = old_json_data.dig("reaction", "aggregated_siblings")&.size if old_json_data
+          previous_siblings_size = notification.json_data&.dig("reaction", "aggregated_siblings")&.size || 0
 
           notification.json_data = json_data
           notification.notified_at = Time.current
diff --git a/spec/services/notifications/reactions/send_spec.rb b/spec/services/notifications/reactions/send_spec.rb
index 7f0158058..f6270eec8 100644
--- a/spec/services/notifications/reactions/send_spec.rb
+++ b/spec/services/notifications/reactions/send_spec.rb
@@ -170,6 +170,23 @@ RSpec.describe Notifications::Reactions::Send, type: :service do
     end
   end
 
+  # regression test for #16627 and #16570
+  context "when a found reaction has unexpected json data" do
+    let(:existing_notification) do
+      create(
+        :notification,
+        json_data: { user: {}, article: {} },
+        action: "Reaction",
+        user: user,
+      )
+    end
+
+    it "does not raise error" do
+      reaction = create(:reaction, reactable: existing_notification.notifiable)
+      expect { described_class.call(reaction_data(reaction), user) }.not_to raise_error
+    end
+  end
+
   context "when a receiver is an organization" do
     let(:organization) { create(:organization) }