fix: add unique constraint to reactions to avoid race conditions (The-Commit-Company#1702)

* fix: add unique constraint to reactions to avoid race conditions

* fix: label for reactions tooltip

Author: nikkothari22

frontend/src/utils/operations.ts

@@ -81,25 +81,33 @@ export const getUsers = (usersList: string[], count: number, currentUser: string
 
             if (currentUserInList) {
                 const otherUsers = usersList.filter((user, index) => index !== currentUserIndex)
-
-                // Show all users upto 50
-                const userString = otherUsers.slice(0, 50).map((user) => userArray.find((u) => u.name == user)?.full_name).join(', ')
-
                 const remainingUsers = otherUsers.length - 50
                 if (remainingUsers > 0) {
+                    // Show all users upto 50
+                    const userString = otherUsers.slice(0, 50).map((user) => userArray.find((u) => u.name == user)?.full_name).join(', ')
                     return `You, ${userString} and ${remainingUsers} others`
                 } else {
-                    return `You and ${userString}`
+                    // The user string will need to have an ", and" just before the last user
+                    // For example, You, John, Jane, and Henry
+                    const numberOfUsers = otherUsers.length
+                    const userString = otherUsers.slice(0, numberOfUsers - 1).map((user) => userArray.find((u) => u.name == user)?.full_name).join(', ')
+                    const lastUser = userArray.find((u) => u.name == otherUsers[numberOfUsers - 1])?.full_name
+                    return `You, ${userString}, and ${lastUser}`
                 }
             }
             else {
-                const userString = usersList.slice(0, 50).map((user) => userArray.find((u) => u.name == user)?.full_name).join(', ')
-                const remainingUsers = usersList.length - 50
+                const numberOfUsers = usersList.length
 
-                if (remainingUsers > 0) {
+                if (numberOfUsers > 50) {
+                    const userString = usersList.slice(0, 50).map((user) => userArray.find((u) => u.name == user)?.full_name).join(', ')
+                    const remainingUsers = usersList.length - 50
                     return `${userString} and ${remainingUsers} others`
                 } else {
-                    return userString
+                    // The user string will need to have an ", and" just before the last user
+                    // For example, John, Jane, and Henry
+                    const userString = usersList.slice(0, numberOfUsers - 1).map((user) => userArray.find((u) => u.name == user)?.full_name).join(', ')
+                    const lastUser = userArray.find((u) => u.name == usersList[numberOfUsers - 1])?.full_name
+                    return `${userString} and ${lastUser}`
                 }
             }
         }

raven/api/reactions.py

@@ -15,9 +15,6 @@ def react(message_id: str, reaction: str, is_custom: bool = False, emoji_name: s
 	If yes, then unreacts (deletes), else reacts (creates).
 	"""
 
-	# PERF: No need for permission checks here.
-	# The permission checks are done in the controller method for the doctype
-
 	channel_id = frappe.get_cached_value("Raven Message", message_id, "channel_id")
 	channel_type = frappe.get_cached_value("Raven Channel", channel_id, "type")
 
@@ -32,18 +29,9 @@ def react(message_id: str, reaction: str, is_custom: bool = False, emoji_name: s
 	else:
 		reaction_escaped = reaction.encode("unicode-escape").decode("utf-8").replace("\\u", "")
 	user = frappe.session.user
-	existing_reaction = frappe.db.exists(
-		"Raven Message Reaction",
-		{"message": message_id, "owner": user, "reaction_escaped": reaction_escaped},
-	)
 
-	if existing_reaction:
-		# Why not use frappe.db.delete?
-		# Because frappe won't run the controller method for 'after_delete' if we do so,
-		# and we need to calculate the new count of reactions for our message
-		frappe.get_doc("Raven Message Reaction", existing_reaction).delete(delete_permanently=True)
-
-	else:
+	try:
+		# Try to insert the reaction first
 		frappe.get_doc(
 			{
 				"doctype": "Raven Message Reaction",
@@ -55,10 +43,24 @@ def react(message_id: str, reaction: str, is_custom: bool = False, emoji_name: s
 				"reaction_escaped": reaction_escaped,
 			}
 		).insert(ignore_permissions=True)
-	return "Ok"
 
+		calculate_message_reaction(message_id, channel_id)
+		return "Ok"
 
-def calculate_message_reaction(message_id, channel_id: str = None):
+	except frappe.exceptions.UniqueValidationError:
+		# If the reaction already exists, delete it
+		frappe.db.delete(
+			"Raven Message Reaction",
+			filters={"message": message_id, "owner": user, "reaction_escaped": reaction_escaped},
+		)
+
+		calculate_message_reaction(message_id, channel_id)
+		return "Ok"
+	except Exception as e:
+		frappe.throw(_("Error reacting to message {0}").format(str(e)))
+
+
+def calculate_message_reaction(message_id, channel_id: str = None, do_not_publish: bool = False):
 
 	reactions = frappe.get_all(
 		"Raven Message Reaction",
@@ -73,11 +75,11 @@ def calculate_message_reaction(message_id, channel_id: str = None):
 		item_key = reaction_item.reaction_escaped if reaction_item.is_custom else reaction_item.reaction
 		if item_key in total_reactions:
 			existing_reaction = total_reactions[item_key]
-			new_users = existing_reaction.get("users")
-			new_users.append(reaction_item.owner)
+			new_users = set(existing_reaction.get("users"))
+			new_users.add(reaction_item.owner)
 			total_reactions[item_key] = {
-				"count": existing_reaction.get("count") + 1,
-				"users": new_users,
+				"count": len(new_users),
+				"users": list(new_users),
 				"reaction": reaction_item.reaction,
 				"is_custom": reaction_item.is_custom,
 			}
@@ -96,6 +98,10 @@ def calculate_message_reaction(message_id, channel_id: str = None):
 		json.dumps(total_reactions, indent=4),
 		update_modified=False,
 	)
+
+	if do_not_publish:
+		return
+
 	frappe.publish_realtime(
 		"message_reacted",
 		{

raven/patches.txt

@@ -9,4 +9,5 @@ raven.patches.v1_6.create_raven_channel_member_index
 raven.patches.v1_6.migrate_older_raven_users #2
 raven.patches.v2_0.migrate_existing_dm_threads
 raven.patches.v2_0.create_default_workspace
-raven.patches.v2_0.create_default_company_workspace_mapping
+raven.patches.v2_0.create_default_company_workspace_mapping
+raven.patches.v2_4.add_unique_constraint_on_reactions #2

raven/patches/v2_4/add_unique_constraint_on_reactions.py

@@ -0,0 +1,61 @@
+import frappe
+
+from raven.api.reactions import calculate_message_reaction
+from raven.raven_messaging.doctype.raven_message_reaction.raven_message_reaction import (
+	on_doctype_update,
+)
+
+
+def execute():
+	"""
+	Add a unique constraint on Raven Message Reaction on the message, owner and reaction_escaped fields
+	"""
+
+	# Before adding the constraint, we need to delete any existing reactions where the message, owner and reaction_escaped fields are the same
+
+	# For this, we can count the number of reactions where the message, owner and reaction_escaped fields are the same
+	duplicate_reactions = frappe.db.sql(
+		"""
+		SELECT
+			message,
+			owner,
+			reaction_escaped,
+			COUNT(*) AS count
+		FROM
+			`tabRaven Message Reaction`
+		GROUP BY
+			message,
+			owner,
+			reaction_escaped
+		HAVING
+			COUNT(*) > 1
+		""",
+		as_dict=True,
+	)
+	messages_to_be_updated = set()
+
+	for reaction in duplicate_reactions:
+		if reaction.count == 1:
+			continue
+		reaction_ids = frappe.db.get_all(
+			"Raven Message Reaction",
+			filters={
+				"message": reaction.message,
+				"owner": reaction.owner,
+				"reaction_escaped": reaction.reaction_escaped,
+			},
+			fields=["name"],
+		)
+		# Delete all but the first one
+		for reaction_id in reaction_ids[1:]:
+			frappe.delete_doc("Raven Message Reaction", reaction_id.name, delete_permanently=True)
+
+			# Update the count for the reaction
+		messages_to_be_updated.add(reaction.message)
+
+		# Update the count for the reactions
+	for message in messages_to_be_updated:
+		calculate_message_reaction(message_id=message, do_not_publish=True)
+
+	# Add the unique constraint
+	on_doctype_update()

raven/raven_messaging/doctype/raven_message_reaction/raven_message_reaction.py

@@ -4,6 +4,7 @@
 import json
 
 import frappe
+from frappe import _
 from frappe.model.document import Document
 
 from raven.api.reactions import calculate_message_reaction
@@ -32,3 +33,11 @@ def after_insert(self):
 	def after_delete(self):
 		# Update the count for the current reaction
 		calculate_message_reaction(self.message, self.channel_id)
+
+
+def on_doctype_update():
+	frappe.db.add_unique(
+		"Raven Message Reaction",
+		fields=["message", "owner", "reaction_escaped"],
+		constraint_name="unique_reaction",
+	)