Delete previous Q&A pair if no feedback received using atomic operation

Author: kris-szlapa

packages/slackBotFunction/app/slack/slack_events.py

@@ -34,39 +34,29 @@
 
 
 def cleanup_previous_unfeedback_qa(conversation_key, current_message_ts, session_data):
-    """Delete previous Q&A pair if no feedback received"""
+    """Delete previous Q&A pair if no feedback received using atomic operation"""
     try:
         previous_message_ts = session_data.get("latest_message_ts")
         # Skip if no previous message or it's the same as current
         if not previous_message_ts or previous_message_ts == current_message_ts:
             return
 
-        # Check if previous Q&A has any feedback
-        feedback_exists = check_feedback_exists(conversation_key, previous_message_ts)
-
-        if not feedback_exists:
-            # Delete unfeedback Q&A pair for privacy compliance
-            previous_qa_key = f"qa#{conversation_key}#{previous_message_ts}"
-            table.delete_item(Key={"pk": previous_qa_key, "sk": "turn"})
-            logger.info("Deleted unfeedback Q&A for privacy", extra={"message_ts": previous_message_ts})
+        # Atomically delete Q&A only if no feedback received
+        table.delete_item(
+            Key={"pk": f"qa#{conversation_key}#{previous_message_ts}", "sk": "turn"},
+            ConditionExpression="attribute_not_exists(feedback_received)",
+        )
+        logger.info("Deleted unfeedback Q&A for privacy", extra={"message_ts": previous_message_ts})
 
+    except ClientError as e:
+        if e.response.get("Error", {}).get("Code") == "ConditionalCheckFailedException":
+            logger.info("Q&A has feedback - keeping for user", extra={"message_ts": previous_message_ts})
+        else:
+            logger.error("Error cleaning up Q&A", extra={"error": str(e)})
     except Exception as e:
         logger.error("Error cleaning up unfeedback Q&A", extra={"error": str(e)})
 
 
-def check_feedback_exists(conversation_key, message_ts):
-    """Check if any feedback exists for this Q&A pair"""
-    try:
-        response = table.query(
-            KeyConditionExpression="pk = :pk",
-            ExpressionAttributeValues={":pk": f"feedback#{conversation_key}#{message_ts}"},
-            Limit=1,
-        )
-        return len(response.get("Items", [])) > 0
-    except Exception:
-        return False
-
-
 def store_qa_pair(conversation_key, user_query, bot_response, message_ts, session_id, user_id):
     """
     Store Q&A pair for feedback correlation
@@ -308,6 +298,11 @@ def store_feedback_with_qa(
             feedback_item["additional_feedback"] = additional_feedback[:4000]
 
         table.put_item(Item=feedback_item)
+
+        # Mark Q&A as having received feedback to prevent deletion
+        if message_ts:
+            _mark_qa_feedback_received(conversation_key, message_ts)
+
         logger.info("Stored feedback with Q&A context", extra={"pk": pk, "sk": sk})
 
     except Exception as e:
@@ -357,7 +352,7 @@ def store_feedback(
             "pk": pk,
             "sk": sk,
             "conversation_key": conversation_key,
-            "feedback_type": feedback_type,  # 'positive' | 'negative' | 'additional'
+            "feedback_type": feedback_type,
             "user_id": user_id,
             "channel_id": channel_id,
             "created_at": now,
@@ -370,15 +365,19 @@ def store_feedback(
         if message_ts:
             feedback_item["message_ts"] = message_ts
         if user_query:
-            feedback_item["user_query"] = user_query[:1000]  # small excerpt to keep items compact
+            feedback_item["user_query"] = user_query[:1000]  # Truncate to keep items compact
         if additional_feedback:
-            feedback_item["additional_feedback"] = additional_feedback[:4000]
+            feedback_item["additional_feedback"] = additional_feedback[:4000]  # Truncate to keep items compact
 
         if condition:
             table.put_item(Item=feedback_item, ConditionExpression=condition)
         else:
             table.put_item(Item=feedback_item)
 
+        # Mark Q&A as having received feedback to prevent deletion
+        if message_ts:
+            _mark_qa_feedback_received(conversation_key, message_ts)
+
         logger.info(
             "Stored feedback",
             extra={
@@ -488,6 +487,18 @@ def update_session_latest_message(conversation_key, message_ts):
         logger.error("Error updating session latest message", extra={"error": str(e)})
 
 
+def _mark_qa_feedback_received(conversation_key, message_ts):
+    """Mark Q&A record as having received feedback to prevent deletion"""
+    try:
+        table.update_item(
+            Key={"pk": f"qa#{conversation_key}#{message_ts}", "sk": "turn"},
+            UpdateExpression="SET feedback_received = :val",
+            ExpressionAttributeValues={":val": True},
+        )
+    except Exception as e:
+        logger.error("Error marking Q&A feedback received", extra={"error": str(e)})
+
+
 def query_bedrock(user_query, session_id=None):
     """
     Query Amazon Bedrock Knowledge Base using RAG (Retrieval-Augmented Generation)

packages/slackBotFunction/tests/test_feedback.py

@@ -136,39 +136,24 @@ def test_cleanup_previous_unfeedback_qa(mock_table, mock_env):
     cleanup_previous_unfeedback_qa("conv-key", "123", session_data)
     mock_table.delete_item.assert_not_called()
 
-    # Test with different message timestamp and no feedback
+    # Test with different message timestamp - atomic delete
     mock_table.reset_mock()
     session_data = {"latest_message_ts": "456"}
-    with patch("app.slack.slack_events.check_feedback_exists", return_value=False):
-        cleanup_previous_unfeedback_qa("conv-key", "123", session_data)
-        mock_table.delete_item.assert_called_once_with(Key={"pk": "qa#conv-key#456", "sk": "turn"})
+    cleanup_previous_unfeedback_qa("conv-key", "123", session_data)
+    mock_table.delete_item.assert_called_once_with(
+        Key={"pk": "qa#conv-key#456", "sk": "turn"}, ConditionExpression="attribute_not_exists(feedback_received)"
+    )
 
-    # Test with different message timestamp and existing feedback
+    # Test with ConditionalCheckFailedException (feedback exists)
     mock_table.reset_mock()
-    with patch("app.slack.slack_events.check_feedback_exists", return_value=True):
-        cleanup_previous_unfeedback_qa("conv-key", "123", session_data)
-        mock_table.delete_item.assert_not_called()
-
-
-@patch("app.slack.slack_events.table")
-def test_check_feedback_exists(mock_table, mock_env):
-    """Test check_feedback_exists function"""
-    from app.slack.slack_events import check_feedback_exists
-
-    # Test with existing feedback
-    mock_table.query.return_value = {"Items": [{"feedback_type": "positive"}]}
-    result = check_feedback_exists("conv-key", "123")
-    assert result is True
-
-    # Test with no feedback
-    mock_table.query.return_value = {"Items": []}
-    result = check_feedback_exists("conv-key", "123")
-    assert result is False
+    from botocore.exceptions import ClientError
 
-    # Test with exception
-    mock_table.query.side_effect = Exception("DB error")
-    result = check_feedback_exists("conv-key", "123")
-    assert result is False
+    error = ClientError({"Error": {"Code": "ConditionalCheckFailedException"}}, "DeleteItem")
+    mock_table.delete_item.side_effect = error
+    cleanup_previous_unfeedback_qa("conv-key", "123", session_data)
+    mock_table.delete_item.assert_called_once_with(
+        Key={"pk": "qa#conv-key#456", "sk": "turn"}, ConditionExpression="attribute_not_exists(feedback_received)"
+    )
 
 
 @patch("app.slack.slack_events.table")
@@ -223,9 +208,9 @@ def test_cleanup_previous_unfeedback_qa_error_handling(mock_table, mock_env):
     from app.slack.slack_events import cleanup_previous_unfeedback_qa
 
     session_data = {"latest_message_ts": "456"}
-    with patch("app.slack.slack_events.check_feedback_exists", side_effect=Exception("Error")):
-        # Should not raise exception
-        cleanup_previous_unfeedback_qa("conv-key", "123", session_data)
+    mock_table.delete_item.side_effect = Exception("DB error")
+    # Should not raise exception
+    cleanup_previous_unfeedback_qa("conv-key", "123", session_data)
 
 
 @patch("app.slack.slack_events.table")