review feedback

Author: artcz

intbot/core/bot/channel_router.py

@@ -76,9 +76,6 @@ def github_router(wh: Webhook) -> DiscordChannel:
     elif project.id == GithubProjects.em_project:
         return Channels.em_channel
 
-    else:
-        ...
-
     # Then we have our open source repositories, like website, this bot, and
     # some others, that we also might want to route to different channels
     if repository == GithubRepositories.website_repo:

intbot/core/integrations/github.py

@@ -120,11 +120,15 @@ def get_repository(self):  # pragma: no cover
 class GithubProjectV2Item(GithubWebhook):
     # NOTE: This might be something for pydantic schemas in the future
 
+    @property
     def sender(self):
         sender = self.get_sender()
 
         return f"[@{sender.login}]({sender.html_url})"
 
+    def get_sender(self) -> GithubSender:
+        return GithubSender.model_validate(self.content["sender"])
+
     def github_object(self) -> GithubDraftIssue | GithubIssue:
         content = self.extra["content"]
         typename = content.pop("__typename")
@@ -143,41 +147,40 @@ def get_repository(self):
         # Not relevant at the moment
         return ...
 
-    def get_sender(self) -> GithubSender:
-        return GithubSender.model_validate(self.content["sender"])
-
     def changes(self) -> dict:
-        if "changes" in self.content:
-            fv = self.content["changes"]["field_value"]
-            field_name = fv["field_name"]
-            field_type = fv["field_type"]
-
-            if field_type == "date":
-                changed_from = (
-                    fv["from"].split("T")[0] if fv["from"] is not None else "None"
-                )
-                changed_to = fv["to"].split("T")[0] if fv["to"] is not None else "None"
-
-            elif field_type == "single_select":
-                changed_from = fv["from"]["name"] if fv["from"] is not None else "None"
-                changed_to = fv["to"]["name"] if fv["to"] is not None else "None"
-
-            else:
-                changed_from = "None"
-                changed_to = "None"
-
-            return {
-                "field": field_name,
-                "from": changed_from,
-                "to": changed_to,
-            }
 
-        return {}
+        # Early return! \o/
+        if "changes" not in self.content:
+            # Fallback because some webhooks just don't have changes.
+            return {}
+
+        fv = self.content["changes"]["field_value"]
+        field_name = fv["field_name"]
+        field_type = fv["field_type"]
+
+        if field_type == "date":
+            changed_from = (
+                fv["from"].split("T")[0] if fv["from"] is not None else "None"
+            )
+            changed_to = fv["to"].split("T")[0] if fv["to"] is not None else "None"
+
+        elif field_type == "single_select":
+            changed_from = fv["from"]["name"] if fv["from"] is not None else "None"
+            changed_to = fv["to"]["name"] if fv["to"] is not None else "None"
+
+        else:
+            changed_from = "None"
+            changed_to = "None"
+
+        return {
+            "field": field_name,
+            "from": changed_from,
+            "to": changed_to,
+        }
 
     def as_discord_message(self) -> str:
         message = "{sender} {action} {details}".format
 
-        sender = self.sender()
         changes = self.changes()
 
         if changes:
@@ -190,7 +193,7 @@ def as_discord_message(self) -> str:
 
         return message(
             **{
-                "sender": sender,
+                "sender": self.sender,
                 "action": self.action,
                 "details": details,
             }
@@ -214,6 +217,11 @@ def prep_github_webhook(wh: Webhook):
     raise ValueError(f"Event `{event}` not supported")
 
 
+class GithubAPIError(Exception):
+    """Custom exception for GithubAPI Errors"""
+    pass
+
+
 # Should we have a separate GithubClient that encapsulates this?
 # Or at least a function that runs the request.
 def fetch_github_project_item(item_id: str) -> dict[str, Any]:
@@ -227,7 +235,7 @@ def fetch_github_project_item(item_id: str) -> dict[str, Any]:
     if response.status_code == 200:
         return response.json()["data"]["node"]
     else:
-        raise Exception(f"GitHub API error: {response.status_code} - {response.text}")
+        raise GithubAPIError(f"GitHub API error: {response.status_code} - {response.text}")
 
 
 def parse_github_webhook(wh: Webhook):

intbot/tests/test_integrations/test_github.py

@@ -3,6 +3,7 @@
 from core.integrations.github import (
     GITHUB_API_URL,
     GithubProjectV2Item,
+    GithubAPIError,
     GithubSender,
     parse_github_webhook,
     prep_github_webhook,
@@ -284,11 +285,10 @@ def test_sender_formats_sender_correctly(self, gh_data):
         )
         gwh = parse_github_webhook(wh)
 
-        sender = gwh.sender()
 
-        assert isinstance(sender, str)
+        assert isinstance(gwh.sender, str)
         assert (
-            sender == "[@github-project-automation[bot]]("
+            gwh.sender == "[@github-project-automation[bot]]("
             "https://github.com/apps/github-project-automation"
             ")"
         )
@@ -354,7 +354,7 @@ def test_prep_github_webhook_reraises_exception_in_case_of_API_error():
 
     respx.post(GITHUB_API_URL).mock(return_value=Response(500, json={"lol": "failed"}))
 
-    with pytest.raises(Exception, match='GitHub API error: 500 - {"lol":"failed"}'):
+    with pytest.raises(GithubAPIError, match='GitHub API error: 500 - {"lol":"failed"}'):
         wh = prep_github_webhook(wh)