Add feedback button handlers and unify message handler

Author: kris-szlapa

packages/slackBotFunction/app/slack/slack_events.py

@@ -37,15 +37,16 @@ def process_async_slack_event(slack_event_data):
         raw_text = event["text"]
         user_id = event["user"]
         channel = event["channel"]
-        thread_ts = event.get("thread_ts", event["ts"])
-
         # figure out if this is a DM or channel thread
         if event.get("channel_type") == "im":
             conversation_key = f"dm#{channel}"
             context_type = "DM"
+            thread_ts = event.get("thread_ts", event["ts"])
         else:
-            conversation_key = f"thread#{channel}#{thread_ts}"
+            thread_root = event.get("thread_ts", event["ts"])
+            conversation_key = f"thread#{channel}#{thread_root}"
             context_type = "thread"
+            thread_ts = thread_root
 
         # clean up the user's message
         user_query = re.sub(r"<@[UW][A-Z0-9]+(\|[^>]+)?>", "", raw_text).strip()
@@ -94,7 +95,7 @@ def process_async_slack_event(slack_event_data):
             {"type": "section", "text": {"type": "mrkdwn", "text": response_text}},
             {
                 "type": "section",
-                "text": {"type": "plain_text", "text": BOT_MESSAGES.get("feedback_prompt", "Was this helpful?")},
+                "text": {"type": "plain_text", "text": BOT_MESSAGES["feedback_prompt"]},
             },
             {
                 "type": "actions",
@@ -109,7 +110,6 @@ def process_async_slack_event(slack_event_data):
                     {
                         "type": "button",
                         "text": {"type": "plain_text", "text": "No"},
-                        "style": "danger",
                         "action_id": "feedback_no",
                         "value": feedback_value,
                     },

packages/slackBotFunction/app/slack/slack_handlers.py

@@ -112,7 +112,7 @@ def app_mention_handler(event, ack, body, client):
         try:
             client.chat_postMessage(
                 channel=channel_id,
-                text=BOT_MESSAGES.get("feedback_thanks", "Thank you for your feedback."),
+                text=BOT_MESSAGES["feedback_thanks"],
                 thread_ts=thread_root,
             )
         except Exception as e:
@@ -124,18 +124,14 @@ def app_mention_handler(event, ack, body, client):
     trigger_async_processing({"event": event, "event_id": event_id, "bot_token": bot_token})
 
 
-def dm_message_handler(event, ack, body, client):
+def dm_message_handler(event, event_id, body, client):
     """
     Direct messages:
     - 'feedback:' prefix -> store as conversation-scoped additional feedback (no model call).
     - otherwise -> forward to async processing (Q&A).
     """
     if event.get("channel_type") != "im":
         return  # not a DM; the channel handler will evaluate it
-    ack()
-    event_id = _gate_common(event, body)
-    if not event_id:
-        return
 
     text = (event.get("text") or "").strip()
     channel_id = event["channel"]
@@ -161,7 +157,7 @@ def dm_message_handler(event, ack, body, client):
         try:
             client.chat_postMessage(
                 channel=channel_id,
-                text=BOT_MESSAGES.get("feedback_thanks", "Thank you for your feedback."),
+                text=BOT_MESSAGES["feedback_thanks"],
                 thread_ts=thread_root,
             )
         except Exception as e:
@@ -172,7 +168,7 @@ def dm_message_handler(event, ack, body, client):
     trigger_async_processing({"event": event, "event_id": event_id, "bot_token": bot_token})
 
 
-def channel_message_handler(event, ack, body, client):
+def channel_message_handler(event, event_id, body, client):
     """
     Channel messages:
     - Ignore top-level messages (policy: require @mention to start).
@@ -182,10 +178,6 @@ def channel_message_handler(event, ack, body, client):
     """
     if event.get("channel_type") == "im":
         return  # handled in the DM handler
-    ack()
-    event_id = _gate_common(event, body)
-    if not event_id:
-        return
 
     text = (event.get("text") or "").strip()
     channel_id = event["channel"]
@@ -197,7 +189,9 @@ def channel_message_handler(event, ack, body, client):
     try:
         resp = table.get_item(Key={"pk": conversation_key, "sk": "session"})
         if "Item" not in resp:
+            logger.info(f"No session found for thread: {conversation_key}")
             return  # not a bot-owned thread; ignore
+        logger.info(f"Found session for thread: {conversation_key}")
     except Exception as e:
         logger.error(f"Error checking thread session: {e}")
         return
@@ -222,7 +216,7 @@ def channel_message_handler(event, ack, body, client):
         try:
             client.chat_postMessage(
                 channel=channel_id,
-                text=BOT_MESSAGES.get("feedback_thanks", "Thank you for your feedback."),
+                text=BOT_MESSAGES["feedback_thanks"],
                 thread_ts=thread_root,
             )
         except Exception as e:
@@ -233,6 +227,68 @@ def channel_message_handler(event, ack, body, client):
     trigger_async_processing({"event": event, "event_id": event_id, "bot_token": bot_token})
 
 
+def unified_message_handler(event, ack, body, client):
+    """Handle all message events - DMs and channel messages"""
+    ack()
+    event_id = _gate_common(event, body)
+    if not event_id:
+        return
+
+    # Route to appropriate handler based on message type
+    if event.get("channel_type") == "im":
+        # DM handling
+        dm_message_handler(event, event_id, body, client)
+    else:
+        # Channel message handling
+        channel_message_handler(event, event_id, body, client)
+
+
+def feedback_yes_handler(ack, body, client):
+    """Handle positive feedback button clicks."""
+    ack()
+    try:
+        feedback_data = json.loads(body["actions"][0]["value"])
+        store_feedback(
+            feedback_data["ck"],
+            None,
+            "positive",
+            body["user"]["id"],
+            feedback_data["ch"],
+            feedback_data.get("tt"),
+            feedback_data.get("mt"),
+        )
+        client.chat_postMessage(
+            channel=feedback_data["ch"],
+            text=BOT_MESSAGES["feedback_positive_thanks"],
+            thread_ts=feedback_data.get("tt"),
+        )
+    except Exception as e:
+        logger.error(f"Error handling positive feedback: {e}")
+
+
+def feedback_no_handler(ack, body, client):
+    """Handle negative feedback button clicks."""
+    ack()
+    try:
+        feedback_data = json.loads(body["actions"][0]["value"])
+        store_feedback(
+            feedback_data["ck"],
+            None,
+            "negative",
+            body["user"]["id"],
+            feedback_data["ch"],
+            feedback_data.get("tt"),
+            feedback_data.get("mt"),
+        )
+        client.chat_postMessage(
+            channel=feedback_data["ch"],
+            text=BOT_MESSAGES["feedback_negative_thanks"],
+            thread_ts=feedback_data.get("tt"),
+        )
+    except Exception as e:
+        logger.error(f"Error handling negative feedback: {e}")
+
+
 # ================================================================
 # Registration (kept minimal to satisfy complexity checkers)
 # ================================================================
@@ -241,8 +297,9 @@ def channel_message_handler(event, ack, body, client):
 def setup_handlers(app):
     """Register handlers. Intentionally minimal—no branching here."""
     app.event("app_mention")(app_mention_handler)
-    app.event("message")(dm_message_handler)
-    app.event("message")(channel_message_handler)
+    app.event("message")(unified_message_handler)
+    app.action("feedback_yes")(feedback_yes_handler)
+    app.action("feedback_no")(feedback_no_handler)
 
 
 # ================================================================

packages/slackBotFunction/tests/test_feedback.py

@@ -52,5 +52,28 @@ def test_feedback_storage_with_additional_text(mock_store_feedback, mock_env):
 
 def test_feedback_message_empty_text(mock_env):
     """Test that empty feedback doesn't crash"""
-    # This test just ensures no crash occurs with empty feedback
-    assert True  # Placeholder test
+    from app.slack.slack_handlers import _gate_common, _strip_mentions, _conversation_key_and_root
+    from app.slack.slack_events import get_conversation_session
+
+    # Test _gate_common helper functions
+    result = _gate_common({}, {})
+    assert result is None
+
+    result = _gate_common({"bot_id": "B123"}, {"event_id": "evt123"})
+    assert result is None
+
+    # Test strip mentions
+    result = _strip_mentions("<@U123> hello world")
+    assert result == "hello world"
+
+    # Test conversation key for DM
+    event = {"channel": "D123", "ts": "456", "channel_type": "im"}
+    key, root = _conversation_key_and_root(event)
+    assert key == "dm#D123"
+    assert root == "456"
+
+    # Test error handling
+    with patch("app.core.config.table") as mock_table:
+        mock_table.get_item.side_effect = Exception("DB error")
+        result = get_conversation_session("test-conv")
+        assert result is None

packages/slackBotFunction/tests/test_slack_handlers.py

@@ -32,7 +32,7 @@ def test_setup_handlers_registers_correctly(mock_env):
         setup_handlers(mock_app)
 
     # Verify all handlers are registered
-    assert mock_app.event.call_count == 3  # app_mention and 2 message handlers
+    assert mock_app.event.call_count == 2  # app_mention and unified message handler
 
 
 def test_app_mention_handler(mock_env):
@@ -199,7 +199,7 @@ def decorator(func):
         mock_ack = Mock()
         mock_body = {
             "user": {"id": "U123"},
-            "actions": [{"value": "conv-key|test query"}],
+            "actions": [{"value": '{"ck": "conv-key", "ch": "C123", "tt": "123", "mt": "456"}'}],
             "channel": {"id": "C123"},
             "message": {"ts": "123"},
         }
@@ -209,7 +209,7 @@ def decorator(func):
             yes_handler(mock_ack, mock_body, mock_client)
 
             mock_ack.assert_called_once()
-            mock_store.assert_called_once_with("conv-key", "test query", "positive", "U123")
+            mock_store.assert_called_once_with("conv-key", None, "positive", "U123", "C123", "123", "456")
             mock_client.chat_postMessage.assert_called_once()
 
 
@@ -242,7 +242,7 @@ def decorator(func):
         mock_ack = Mock()
         mock_body = {
             "user": {"id": "U123"},
-            "actions": [{"value": "conv-key|test query"}],
+            "actions": [{"value": '{"ck": "conv-key", "ch": "C123", "tt": "123", "mt": "456"}'}],
             "channel": {"id": "C123"},
             "message": {"ts": "123"},
         }
@@ -252,7 +252,7 @@ def decorator(func):
             no_handler(mock_ack, mock_body, mock_client)
 
             mock_ack.assert_called_once()
-            mock_store.assert_called_once_with("conv-key", "test query", "negative", "U123")
+            mock_store.assert_called_once_with("conv-key", None, "negative", "U123", "C123", "123", "456")
             mock_client.chat_postMessage.assert_called_once()
 
 

packages/slackBotFunction/tests/test_state_management.py

@@ -136,3 +136,26 @@ def test_trigger_async_processing_error(mock_boto_client, mock_boto_resource, mo
 
     mock_boto_client.assert_called_once_with("lambda")
     mock_lambda_client.invoke.assert_called_once()
+
+
+@patch("slack_bolt.App")
+@patch("aws_lambda_powertools.utilities.parameters.get_parameter")
+@patch("boto3.resource")
+def test_handler_missing_slack_event(mock_boto_resource, mock_get_parameter, mock_app, mock_env):
+    """Test handler with missing slack_event in async processing"""
+    mock_get_parameter.side_effect = [
+        json.dumps({"token": "test-token"}),
+        json.dumps({"secret": "test-secret"}),
+    ]
+    mock_boto_resource.return_value.Table.return_value = Mock()
+
+    if "app.handler" in sys.modules:
+        del sys.modules["app.handler"]
+
+    from app.handler import handler
+
+    event = {"async_processing": True}
+    context = Mock()
+
+    result = handler(event, context)
+    assert result["statusCode"] == 400