Fix a validation error when creating notifications #2525 (#2999)

* Use activerecord-import upsert to deal with the race condition

* Return a more consistent result form Notifications::Reactions::Send

app/services/notifications/reactions/send.rb

@@ -18,6 +18,7 @@ module Notifications
         new(*args).call
       end
 
+      # @return [OpenStruct, #action, #notification_id]
       def call
         return unless receiver.is_a?(User) || receiver.is_a?(Organization)
 
@@ -32,7 +33,6 @@ module Notifications
           notifiable_type: reaction.reactable_type,
           notifiable_id: reaction.reactable_id,
           action: "Reaction"
-          # user_id or organization_id: receiver.id
         }
         if receiver.is_a?(User)
           notification_params[:user_id] = receiver.id
@@ -41,7 +41,8 @@ module Notifications
         end
 
         if aggregated_reaction_siblings.size.zero?
-          notification = Notification.where(notification_params).delete_all
+          Notification.where(notification_params).delete_all
+          OpenStruct.new(action: :deleted)
         else
           recent_reaction = reaction_siblings.first
 
@@ -53,15 +54,36 @@ module Notifications
           notification.json_data = json_data
           notification.notified_at = Time.current
           notification.read = false if json_data[:reaction][:aggregated_siblings].size > previous_siblings_size
-          notification.save!
+
+          notification_id = save_notification(notification)
+
+          OpenStruct.new(action: :saved, notification_id: notification_id)
         end
-        notification
       end
 
       private
 
       attr_reader :reaction, :receiver
 
+      # when a notification exists in the db already it's safe to just save
+      # when it doesn't, there could be a race condition when 2 jobs try to create duplicate notifications concurrently
+      # in this case upsert is used to rely on postgres constraints and update or insert depending on if the record exists in the db at this point
+      # currently, activerecord-import upsert is used
+      # when the app is upgraded to Rails 6 this can be refactored to use rails upsert
+      def save_notification(notification)
+        if notification.persisted?
+          notification.save!
+          notification.id
+        else
+          import_result = Notification.import! [notification],
+                                               on_duplicate_key_update: {
+                                                 conflict_target: %i[notifiable_id notifiable_type user_id organization_id action],
+                                                 columns: %i[json_data notified_at read]
+                                               }
+          import_result.ids.first
+        end
+      end
+
       def reaction_json_data(recent_reaction, siblings)
         {
           user: user_data(recent_reaction.user),

spec/models/notification_spec.rb

@@ -163,14 +163,18 @@ RSpec.describe Notification, type: :model do
       it "sends a notification to the author of a comment" do
         comment = create(:comment, user: user2, commentable: article)
         reaction = create(:reaction, reactable: comment, user: user)
-        Notification.send_reaction_notification_without_delay(reaction, reaction.reactable.user)
-        expect(user2.notifications.count).to eq 1
+        perform_enqueued_jobs do
+          Notification.send_reaction_notification(reaction, reaction.reactable.user)
+          expect(user2.notifications.count).to eq 1
+        end
       end
 
       it "sends a notification to the author of an article" do
         reaction = create(:reaction, reactable: article, user: user2)
-        Notification.send_reaction_notification_without_delay(reaction, reaction.reactable.user)
-        expect(user.notifications.count).to eq 1
+        perform_enqueued_jobs do
+          Notification.send_reaction_notification(reaction, reaction.reactable.user)
+          expect(user.notifications.count).to eq 1
+        end
       end
     end
 
@@ -239,34 +243,38 @@ RSpec.describe Notification, type: :model do
     end
 
     it "creates positive reaction notification" do
-      reaction = Reaction.create!(
+      reaction = article.reactions.create!(
         user_id: user2.id,
-        reactable_id: article.id,
-        reactable_type: "Article",
         category: "like",
       )
-      notification = Notification.send_reaction_notification_without_delay(reaction, reaction.reactable.user)
-      expect(notification).to be_valid
+      perform_enqueued_jobs do
+        expect do
+          Notification.send_reaction_notification(reaction, reaction.reactable.user)
+        end.to change(Notification, :count).by(1)
+      end
     end
 
     it "does not create negative notification" do
       user2.add_role(:trusted)
-      reaction = Reaction.create!(
+      reaction = article.reactions.create!(
         user_id: user2.id,
-        reactable_id: article.id,
-        reactable_type: "Article",
         category: "vomit",
       )
-      notification = Notification.send_reaction_notification_without_delay(reaction, reaction.reactable.user)
-      expect(notification).to eq nil
+      perform_enqueued_jobs do
+        expect do
+          Notification.send_reaction_notification(reaction, reaction.reactable.user)
+        end.not_to change(Notification, :count)
+      end
     end
 
     it "destroys the notification properly" do
       reaction = create(:reaction, user: user2, reactable: article, category: "like")
-      Notification.send_reaction_notification_without_delay(reaction, reaction.reactable.user)
-      reaction.destroy!
-      Notification.send_reaction_notification_without_delay(reaction, reaction.reactable.user)
-      expect(Notification.count).to eq 0
+      perform_enqueued_jobs do
+        Notification.send_reaction_notification(reaction, reaction.reactable.user)
+        reaction.destroy!
+        Notification.send_reaction_notification(reaction, reaction.reactable.user)
+        expect(Notification.count).to eq 0
+      end
     end
   end
 

spec/services/notifications/reactions/send_spec.rb

@@ -23,13 +23,15 @@ RSpec.describe Notifications::Reactions::Send, type: :service do
     end
 
     it "creates a correct notification" do
-      notification = described_class.call(reaction_data(article_reaction), user)
+      result = described_class.call(reaction_data(article_reaction), user)
+      notification = Notification.find(result.notification_id)
       expect(notification.user_id).to eq(user.id)
       expect(notification.notifiable).to eq(article)
     end
 
     it "creates a notification with the correct json" do
-      notification = described_class.call(reaction_data(article_reaction), user)
+      result = described_class.call(reaction_data(article_reaction), user)
+      notification = Notification.find(result.notification_id)
       expect(notification.json_data["user"]["id"]).to eq(user2.id)
       expect(notification.json_data["user"]["name"]).to eq(user2.name)
       expect(notification.json_data["reaction"]["reactable_id"]).to eq(article.id)
@@ -49,13 +51,15 @@ RSpec.describe Notifications::Reactions::Send, type: :service do
     end
 
     it "creates a correct notification" do
-      notification = described_class.call(reaction_data(article_reaction), user)
+      result = described_class.call(reaction_data(article_reaction), user)
+      notification = Notification.find(result.notification_id)
       expect(notification.notifiable).to eq(article)
       expect(notification.notified_at).not_to be_nil
     end
 
     it "creates a notification with the correct json" do
-      notification = described_class.call(reaction_data(article_reaction), user)
+      result = described_class.call(reaction_data(article_reaction), user)
+      notification = Notification.find(result.notification_id)
       expect(notification.json_data["user"]["id"]).to eq(user2.id)
       expect(notification.json_data["user"]["name"]).to eq(user2.name)
       expect(notification.json_data["reaction"]["reactable_id"]).to eq(article.id)
@@ -77,7 +81,8 @@ RSpec.describe Notifications::Reactions::Send, type: :service do
       end
 
       it "returns the same notification" do
-        notification = described_class.call(reaction_data(article_reaction), user)
+        result = described_class.call(reaction_data(article_reaction), user)
+        notification = Notification.find(result.notification_id)
         expect(notification.id).to eq(old_notification.id)
       end
 
@@ -120,6 +125,12 @@ RSpec.describe Notifications::Reactions::Send, type: :service do
       described_class.call(reaction_data(destroyed_reaction), user)
       expect(Notification.where(id: notification.id)).not_to be_any
     end
+
+    it "returns deleted action" do
+      notification
+      result = described_class.call(reaction_data(destroyed_reaction), user)
+      expect(result.action).to eq(:deleted)
+    end
   end
 
   context "when a reaction is destroyed but it has siblings" do
@@ -144,6 +155,12 @@ RSpec.describe Notifications::Reactions::Send, type: :service do
       expect(notification.json_data["user"]["username"]).to eq(user3.username)
       expect(notification.json_data["reaction"]["aggregated_siblings"].map { |s| s["user"]["id"] }).to eq([user3.id])
     end
+
+    it "returns saved action" do
+      result = described_class.call(reaction_data(destroyed_reaction), user)
+      expect(result.action).to eq(:saved)
+      expect(result.notification_id).to eq(notification.id)
+    end
   end
 
   context "when a receiver is an organization" do
@@ -156,7 +173,8 @@ RSpec.describe Notifications::Reactions::Send, type: :service do
     end
 
     it "creates a correct notification" do
-      notification = described_class.call(reaction_data(article_reaction), organization)
+      result = described_class.call(reaction_data(article_reaction), organization)
+      notification = Notification.find(result.notification_id)
       expect(notification.organization_id).to eq(organization.id)
       expect(notification.user_id).to be_nil
       expect(notification.notifiable).to eq(article)