Merge pull request #419 from OperationCode/irving/fix_issues_post_python_upgrade

fix high severity issues raised during code review

Author: Irving Popovetsky

.gitignore

@@ -115,4 +115,7 @@ venv.bak/
 dmypy.json
 
 # Pyre type checker
-.pyre/
+.pyre/
+
+# Claude
+settings.local.json

CLAUDE_CODE_REVIEW.md docs/CLAUDE_CODE_REVIEW.mdCLAUDE_CODE_REVIEW.md renamed to docs/CLAUDE_CODE_REVIEW.md

@@ -16,7 +16,10 @@ async def _get_requested_mentor(
         if not requested_mentor:
             return None
         mentor = await airtable.get_row_from_record_id("Mentors", requested_mentor)
-        email = mentor["Email"]
+        # mentor could be {} if the record wasn't found or an error occurred
+        email = mentor.get("Email")
+        if not email:
+            return None
         slack_user_id = await _slack_user_id_from_email(email, slack)
         return f" Requested mentor: <@{slack_user_id}>"
     except SlackAPIError:
@@ -39,10 +42,15 @@ async def _get_matching_skillset_mentors(
     if not skillsets:
         return ["No skillset Given"]
     mentors = await airtable.find_mentors_with_matching_skillsets(skillsets)
-    mentor_ids = [
-        await _slack_user_id_from_email(mentor["Email"], slack, fallback=mentor["Slack Name"])
-        for mentor in mentors
-    ]
+    mentor_ids = []
+    for mentor in mentors:
+        email = mentor.get("Email")
+        fallback = mentor.get("Slack Name", "Unknown Mentor")
+        if email:
+            mentor_id = await _slack_user_id_from_email(email, slack, fallback=fallback)
+            mentor_ids.append(mentor_id)
+        elif fallback:
+            mentor_ids.append(fallback)
     return [f"<@{mentor}>" for mentor in mentor_ids]
 
 

pybot/endpoints/airtable/utils.py

@@ -25,7 +25,17 @@ async def mentor_request_submit(action: Action, app: SirBot):
 
     username = action["user"]["name"]
     user_info = await slack.query(methods.USERS_INFO, {"user": action["user"]["id"]})
-    email = user_info["user"]["profile"]["email"]
+    email = user_info["user"]["profile"].get("email")
+
+    if not email:
+        logger.warning(f"User {action['user']['id']} has no email in Slack profile")
+        error_attachment = {
+            "text": ":warning: Your Slack profile doesn't have an email address. Please update your profile and try again. :warning:",
+            "color": "danger",
+        }
+        request.attachments = [error_attachment]
+        await request.update_message(slack)
+        return
 
     airtable_response = await request.submit_request(username, email, airtable)
 
@@ -127,7 +137,16 @@ async def claim_mentee(action: Action, app: SirBot):
         event = MentorRequestClaim(action, slack, airtable)
         if event.is_claim():
             user_info = await slack.query(methods.USERS_INFO, {"user": event.clicker})
-            clicker_email = user_info["user"]["profile"]["email"]
+            clicker_email = user_info["user"]["profile"].get("email")
+
+            if not clicker_email:
+                logger.warning(f"Mentor {event.clicker} has no email in Slack profile")
+                event.attachment["text"] = (
+                    f":warning: <@{event.clicker}>'s Slack profile has no email address. :warning:"
+                )
+                event.should_update = False
+                await event.update_message()
+                return
 
             mentor_records = await airtable.find_records(
                 table_name="Mentors", field="Email", value=clicker_email
@@ -140,5 +159,5 @@ async def claim_mentee(action: Action, app: SirBot):
 
         await event.update_message()
 
-    except Exception as ex:
-        logger.exception("Exception while updating claim", ex)
+    except Exception:
+        logger.exception("Exception while updating claim")

pybot/endpoints/slack/actions/mentor_request.py

@@ -1,6 +1,8 @@
 import logging
 import random
 
+import aiohttp
+
 from pybot._vendor.sirbot import SirBot
 from pybot._vendor.sirbot.plugins.slack import SlackPlugin
 from pybot._vendor.slack import methods
@@ -91,12 +93,35 @@ async def slash_lunch(command: Command, app: SirBot):
 
     slack = app["plugins"]["slack"].api
 
-    request = lunch.get_yelp_request()
-    async with app.http_session.get(**request) as r:
-        r.raise_for_status()
-        message_params = lunch.select_random_lunch(await r.json())
-
-        await slack.query(methods.CHAT_POST_EPHEMERAL, message_params)
+    try:
+        request = lunch.get_yelp_request()
+        async with app.http_session.get(**request) as r:
+            r.raise_for_status()
+            message_params = lunch.select_random_lunch(await r.json())
+    except aiohttp.ClientResponseError as e:
+        logger.error(f"Yelp API HTTP error for {command['user_name']}: {e.status} {e.message}")
+        message_params = {
+            "user": command["user_id"],
+            "channel": command["channel_id"],
+            "text": "Sorry, I couldn't reach the Yelp API right now. Please try again later.",
+        }
+    except aiohttp.ClientError as e:
+        logger.error(f"Yelp API connection error for {command['user_name']}: {e}")
+        message_params = {
+            "user": command["user_id"],
+            "channel": command["channel_id"],
+            "text": "Sorry, I couldn't connect to Yelp. Please try again later.",
+        }
+
+    if message_params is None:
+        # No restaurants found in the area
+        message_params = {
+            "user": command["user_id"],
+            "channel": command["channel_id"],
+            "text": "Sorry, I couldn't find any restaurants in that area. Try a different zip code!",
+        }
+
+    await slack.query(methods.CHAT_POST_EPHEMERAL, message_params)
 
 
 @catch_command_slack_error

pybot/endpoints/slack/commands.py

@@ -36,7 +36,7 @@ def service(self, new_service):
             self.clear_errors()
 
     @property
-    def skillsets(self) -> [str]:
+    def skillsets(self) -> list[str]:
         if self.skillset_fields:
             return [field["text"] for field in self.skillset_fields]
         return []
@@ -97,6 +97,13 @@ async def submit_request(self, username: str, email: str, airtable: AirtableAPI)
             params["Additional Details"] = self.details
 
         service_records = await airtable.find_records("Services", "Name", self.service)
+        if not service_records:
+            return {
+                "error": {
+                    "type": "SERVICE_NOT_FOUND",
+                    "message": f"Service '{self.service}' not found in Airtable",
+                }
+            }
         params["Service"] = [service_records[0]["id"]]
         return await airtable.add_record("Mentor Request", {"fields": params})
 

pybot/endpoints/slack/message_templates/mentor_request.py

@@ -86,7 +86,10 @@ async def link_backend_user(
     """
 
     user_info = await slack_api.query(methods.USERS_INFO, {"user": slack_id})
-    email = user_info["user"]["profile"]["email"]
+    email = user_info["user"]["profile"].get("email")
+    if not email:
+        logger.warning(f"User {slack_id} has no email in profile, skipping backend link")
+        return
 
     async with session.patch(
         f"{BACKEND_URL}/auth/profile/admin/",

pybot/endpoints/slack/utils/event_utils.py

@@ -28,11 +28,20 @@ def get_yelp_request(self):
             "headers": self.AUTH_HEADER,
         }
 
-    def select_random_lunch(self, lunch_response: dict) -> dict:
-        location_count = len(lunch_response["businesses"])
+    def select_random_lunch(self, lunch_response: dict) -> dict | None:
+        """
+        Select a random restaurant from the Yelp response.
+
+        Returns None if no businesses are found, allowing the caller to handle
+        the empty result gracefully.
+        """
+        businesses = lunch_response.get("businesses", [])
+        if not businesses:
+            logger.warning(f"No restaurants found for {self.user_name}")
+            return None
 
-        selected_location = randint(0, location_count - 1)
-        location = lunch_response["businesses"][selected_location]
+        selected_location = randint(0, len(businesses) - 1)
+        location = businesses[selected_location]
 
         logger.info(f"location selected for {self.user_name}: {location}")
 

pybot/endpoints/slack/utils/slash_lunch.py

@@ -37,7 +37,12 @@ async def _depaginate_records(self, url, params, offset):
         while offset:
             params["offset"] = offset
             response = await self.get(url, params=params)
-            records.extend(response["records"])
+            # Check for error response during pagination
+            if "error" in response:
+                error_msg = response["error"].get("message", "Unknown error")
+                logger.error(f"Airtable API error during pagination: {error_msg}")
+                break
+            records.extend(response.get("records", []))
             offset = response.get("offset")
 
         return records
@@ -50,7 +55,7 @@ def table_url(self, table_name, record_id=None):
 
     async def get_name_from_record_id(self, table_name: str, record_id):
         if self.record_id_to_name[table_name]:
-            return self.record_id_to_name[table_name][record_id]
+            return self.record_id_to_name[table_name].get(record_id)
 
         url = self.table_url("Services")
         params = {"fields[]": "Name"}
@@ -63,10 +68,13 @@ async def get_name_from_record_id(self, table_name: str, record_id):
             return None
 
         records = res_json["records"]
+        # Skip records that don't have a Name field (Airtable omits empty fields)
         self.record_id_to_name[table_name] = {
-            record["id"]: record["fields"]["Name"] for record in records
+            record["id"]: record["fields"]["Name"]
+            for record in records
+            if "Name" in record.get("fields", {})
         }
-        return self.record_id_to_name[table_name][record_id]
+        return self.record_id_to_name[table_name].get(record_id)
 
     async def get_row_from_record_id(self, table_name: str, record_id: str) -> dict:
         url = self.table_url(table_name, record_id)
@@ -80,8 +88,8 @@ async def get_row_from_record_id(self, table_name: str, record_id: str) -> dict:
                 return {}
 
             return res_json["fields"]
-        except Exception as ex:
-            logger.exception(f"Couldn't get row from record id {record_id} in {table_name}", ex)
+        except Exception:
+            logger.exception(f"Couldn't get row from record id {record_id} in {table_name}")
             return {}
 
     async def get_all_records(self, table_name, field=None):
@@ -106,7 +114,12 @@ async def get_all_records(self, table_name, field=None):
             )
 
         if field:
-            return [record["fields"][field] for record in res_json["records"]]
+            # Skip records that don't have the field (Airtable omits empty fields)
+            return [
+                record["fields"][field]
+                for record in res_json["records"]
+                if field in record.get("fields", {})
+            ]
         else:
             return res_json["records"]
 
@@ -141,8 +154,8 @@ async def find_mentors_with_matching_skillsets(self, skillsets):
                     for skillset in skillsets
                 ):
                     partial_match.append(mentor["fields"])
-        except Exception as e:
-            logger.exception("Exception while trying to find filter mentors by skillset", e)
+        except Exception:
+            logger.exception("Exception while trying to filter mentors by skillset")
             return []
 
         if len(complete_match) < 5:
@@ -165,8 +178,8 @@ async def find_records(self, table_name: str, field: str, value: str) -> list:
                 return []
 
             return response["records"]
-        except Exception as ex:
-            logger.exception(f"Exception when attempting to get {field} from {table_name}.", ex)
+        except Exception:
+            logger.exception(f"Exception when attempting to get {field} from {table_name}")
             return []
 
     async def update_request(self, request_record, mentor_id):

pybot/plugins/airtable/api.py

@@ -0,0 +1 @@
+# Integration tests for pybot