Fix reaction notification json data when a reaction is destroyed (#2112)

* Add specs for notification sending when a destroyed reaction is passed

* Keep only persisted reactions data in the notification

app/decorators/user_decorator.rb

@@ -33,7 +33,7 @@ class UserDecorator < ApplicationDecorator
 
   def config_body_class
     body_class = ""
-    body_class = body_class + config_theme.gsub("_", "-")
+    body_class += config_theme.gsub("_", "-")
     body_class = body_class + " " + config_font.gsub("_", "-") + "-article-body"
     body_class
   end

app/models/notification.rb

@@ -126,26 +126,11 @@ class Notification < ApplicationRecord
       return if reaction.points.negative?
       return if receiver.is_a?(User) && reaction.reactable.receive_notifications == false
 
-      aggregated_reaction_siblings = Reaction.where(reactable_id: reaction.reactable_id, reactable_type: reaction.reactable_type).
-        reject { |r| r.user_id == reaction.reactable.user_id }.
-        map { |r| { category: r.category, created_at: r.created_at, user: user_data(r.user) } }
-      json_data = {
-        user: user_data(reaction.user),
-        reaction: {
-          category: reaction.category,
-          reactable_type: reaction.reactable_type,
-          reactable_id: reaction.reactable_id,
-          reactable: {
-            path: reaction.reactable.path,
-            title: reaction.reactable.title,
-            class: {
-              name: reaction.reactable.class.name
-            }
-          },
-          aggregated_siblings: aggregated_reaction_siblings,
-          updated_at: reaction.updated_at
-        }
-      }
+      reaction_siblings = Reaction.where(reactable_id: reaction.reactable_id, reactable_type: reaction.reactable_type).
+        where.not(reactions: { user_id: reaction.reactable.user_id }).
+        order("created_at DESC")
+
+      aggregated_reaction_siblings = reaction_siblings.map { |r| { category: r.category, created_at: r.created_at, user: user_data(r.user) } }
 
       notification_params = {
         notifiable_type: reaction.reactable.class.name,
@@ -162,6 +147,26 @@ class Notification < ApplicationRecord
       if aggregated_reaction_siblings.size.zero?
         notification = Notification.where(notification_params).delete_all
       else
+        recent_reaction = reaction_siblings.first
+
+        json_data = {
+          user: user_data(recent_reaction.user),
+          reaction: {
+            category: recent_reaction.category,
+            reactable_type: recent_reaction.reactable_type,
+            reactable_id: recent_reaction.reactable_id,
+            reactable: {
+              path: recent_reaction.reactable.path,
+              title: recent_reaction.reactable.title,
+              class: {
+                name: recent_reaction.reactable.class.name
+              }
+            },
+            aggregated_siblings: aggregated_reaction_siblings,
+            updated_at: recent_reaction.updated_at
+          }
+        }
+
         previous_siblings_size = 0
         notification = Notification.find_or_create_by(notification_params)
         previous_siblings_size = notification.json_data["reaction"]["aggregated_siblings"].size if notification.json_data

spec/models/notification_spec.rb

@@ -143,6 +143,38 @@ RSpec.describe Notification, type: :model do
       end
     end
 
+    context "when a reaction is destroyed" do
+      let(:comment) { create(:comment, user: user2, commentable: article) }
+      let!(:notification) { create(:notification, user: user, notifiable: comment, action: "Reaction") }
+
+      it "destroys the notification if it exists" do
+        reaction = create(:reaction, reactable: comment, user: user)
+        reaction.destroy
+        Notification.send_reaction_notification_without_delay(reaction, article.user)
+        expect(Notification.where(id: notification.id)).not_to be_any
+      end
+
+      it "keeps the notification if siblings exist" do
+        reaction = create(:reaction, reactable: comment, user: user)
+        create(:reaction, reactable: comment, user: user3)
+        reaction.destroy
+        Notification.send_reaction_notification_without_delay(reaction, article.user)
+        notification.reload
+        expect(notification).to be_persisted
+      end
+
+      it "doesn't keep data of the destroyed reaction in the notification" do
+        reaction = create(:reaction, reactable: comment, user: user)
+        create(:reaction, reactable: comment, user: user3)
+        reaction.destroy
+        Notification.send_reaction_notification_without_delay(reaction, article.user)
+        notification.reload
+        expect(notification.json_data["reaction"]["aggregated_siblings"].map { |s| s["user"]["id"] }).to eq([user3.id])
+        # not the user of the destroyed reaction!
+        expect(notification.json_data["user"]["id"]).to eq(user3.id)
+      end
+    end
+
     context "when reactable is not receiving notifications" do
       it "does not send a notification to the author of a comment" do
         comment = create(:comment, user: user2, commentable: article)

spec/models/user_spec.rb

@@ -36,7 +36,7 @@ RSpec.describe User, type: :model do
     it { is_expected.to validate_presence_of(:username) }
     it { is_expected.to validate_length_of(:username).is_at_most(30).is_at_least(2) }
     it { is_expected.to validate_length_of(:name).is_at_most(100) }
-    it { is_expected.to validate_inclusion_of(:inbox_type).in_array(["open", "private"]) }
+    it { is_expected.to validate_inclusion_of(:inbox_type).in_array(%w[open private]) }
   end
 
   # the followings are failing