refactor controller for better control and o11y

Author: zcesur

lib/algora_web/controllers/webhooks/github_controller.ex

@@ -13,24 +13,23 @@ defmodule AlgoraWeb.Webhooks.GithubController do
   # TODO: auto-retry failed deliveries with exponential backoff
 
   def new(conn, params) do
-    case Webhook.new(conn) do
-      {:ok, %Webhook{delivery: _delivery, event: event, installation_id: _installation_id}} ->
-        author = get_author(event, params)
-        body = get_body(event, params)
-        process_commands(body, author, params)
-
-        conn |> put_status(:accepted) |> json(%{status: "ok"})
-
+    with {:ok, %Webhook{event: event}} <- Webhook.new(conn),
+         {:ok, _} <- process_commands(event, params) do
+      conn |> put_status(:accepted) |> json(%{status: "ok"})
+    else
       {:error, :missing_header} ->
         conn |> put_status(:bad_request) |> json(%{error: "Missing header"})
 
       {:error, :signature_mismatch} ->
         conn |> put_status(:unauthorized) |> json(%{error: "Signature mismatch"})
+
+      {:error, reason} ->
+        Logger.error("Error processing webhook: #{inspect(reason)}")
+        conn |> put_status(:internal_server_error) |> json(%{error: "Internal server error"})
     end
   rescue
     e ->
       Logger.error("Unexpected error: #{inspect(e)}")
-
       conn |> put_status(:internal_server_error) |> json(%{error: "Internal server error"})
   end
 
@@ -137,19 +136,33 @@ defmodule AlgoraWeb.Webhooks.GithubController do
     end
   end
 
-  defp execute_command({command, _} = args, _author, _params),
-    do: Logger.info("Unhandled command: #{command} #{inspect(args)}")
+  defp execute_command(_command, _author, _params) do
+    {:error, :unhandled_command}
+  end
+
+  def process_commands(event, params) do
+    author = get_author(event, params)
+    body = get_body(event, params)
 
-  def process_commands(body, author, params) when is_binary(body) do
     case Github.Command.parse(body) do
-      {:ok, commands} -> Enum.map(commands, &execute_command(&1, author, params))
-      # TODO: handle errors
-      {:error, error} -> Logger.error("Error parsing commands: #{inspect(error)}")
+      {:ok, commands} ->
+        Enum.reduce_while(commands, {:ok, []}, fn command, {:ok, results} ->
+          case execute_command(command, author, params) do
+            {:ok, result} ->
+              {:cont, {:ok, [result | results]}}
+
+            error ->
+              Logger.error("Command execution failed for #{inspect(command)}: #{inspect(error)}")
+              {:halt, error}
+          end
+        end)
+
+      {:error, error} ->
+        Logger.error("Error parsing commands: #{inspect(error)}")
+        {:error, error}
     end
   end
 
-  def process_commands(_body, _author, _params), do: nil
-
   defp get_author("issues", params), do: params["issue"]["user"]
   defp get_author("issue_comment", params), do: params["comment"]["user"]
   defp get_author("pull_request", params), do: params["pull_request"]["user"]

test/algora_web/controllers/webhooks/github_controller_test.exs

@@ -47,57 +47,66 @@ defmodule AlgoraWeb.Webhooks.GithubControllerTest do
 
     @tag user: @unauthorized_user
     test "handles bounty command with unauthorized user", %{user: user} do
-      assert process_bounty_command("/bounty $100", user)[:ok] == nil
-      assert process_bounty_command("/bounty $100", user)[:error] == :unauthorized
+      assert {:error, :unauthorized} = process_bounty_command("/bounty $100", user)
     end
 
     test "handles bounty command without amount" do
-      assert process_bounty_command("/bounty")[:ok] == nil
-      assert process_bounty_command("/bounty")[:error] == nil
+      assert {:ok, []} = process_bounty_command("/bounty")
     end
 
     test "handles valid bounty command with $ prefix" do
-      assert process_bounty_command("/bounty $100")[:ok].amount == ~M[100]usd
+      assert {:ok, [bounty]} = process_bounty_command("/bounty $100")
+      assert bounty.amount == ~M[100]usd
     end
 
     test "handles invalid bounty command with $ suffix" do
-      assert process_bounty_command("/bounty 100$")[:ok].amount == ~M[100]usd
+      assert {:ok, [bounty]} = process_bounty_command("/bounty 100$")
+      assert bounty.amount == ~M[100]usd
     end
 
     test "handles bounty command without $ symbol" do
-      assert process_bounty_command("/bounty 100")[:ok].amount == ~M[100]usd
+      assert {:ok, [bounty]} = process_bounty_command("/bounty 100")
+      assert bounty.amount == ~M[100]usd
     end
 
     test "handles bounty command with decimal amount" do
-      assert process_bounty_command("/bounty 100.50")[:ok].amount == ~M[100.50]usd
+      assert {:ok, [bounty]} = process_bounty_command("/bounty 100.50")
+      assert bounty.amount == ~M[100.50]usd
     end
 
     test "handles bounty command with partial decimal amount" do
-      assert process_bounty_command("/bounty 100.5")[:ok].amount == ~M[100.5]usd
+      assert {:ok, [bounty]} = process_bounty_command("/bounty 100.5")
+      assert bounty.amount == ~M[100.5]usd
     end
 
     test "handles bounty command with decimal amount and $ prefix" do
-      assert process_bounty_command("/bounty $100.50")[:ok].amount == ~M[100.50]usd
+      assert {:ok, [bounty]} = process_bounty_command("/bounty $100.50")
+      assert bounty.amount == ~M[100.50]usd
     end
 
     test "handles bounty command with partial decimal amount and $ prefix" do
-      assert process_bounty_command("/bounty $100.5")[:ok].amount == ~M[100.5]usd
+      assert {:ok, [bounty]} = process_bounty_command("/bounty $100.5")
+      assert bounty.amount == ~M[100.5]usd
     end
 
     test "handles bounty command with decimal amount and $ suffix" do
-      assert process_bounty_command("/bounty 100.50$")[:ok].amount == ~M[100.50]usd
+      assert {:ok, [bounty]} = process_bounty_command("/bounty 100.50$")
+      assert bounty.amount == ~M[100.50]usd
     end
 
     test "handles bounty command with partial decimal amount and $ suffix" do
-      assert process_bounty_command("/bounty 100.5$")[:ok].amount == ~M[100.5]usd
+      assert {:ok, [bounty]} = process_bounty_command("/bounty 100.5$")
+      assert bounty.amount == ~M[100.5]usd
     end
 
     test "handles bounty command with comma separator" do
-      assert process_bounty_command("/bounty 1,000")[:ok].amount == ~M[1000]usd
+      assert {:ok, [bounty]} = process_bounty_command("/bounty 1,000")
+      assert bounty.amount == ~M[1000]usd
     end
 
     test "handles bounty command with comma separator and decimal amount" do
-      assert process_bounty_command("/bounty 1,000.50")[:ok].amount == ~M[1000.50]usd
+      assert {:ok, [bounty]} = process_bounty_command("/bounty 1,000.50")
+      assert bounty.amount == ~M[1000.50]usd
     end
   end
 
@@ -184,14 +193,17 @@ defmodule AlgoraWeb.Webhooks.GithubControllerTest do
   end
 
   # Helper function to process bounty commands
-  defp process_bounty_command(body, author \\ @admin_user) do
-    full_body = """
+  defp process_bounty_command(command, author \\ @admin_user) do
+    body = """
     Lorem
-    ipsum #{body} dolor
+    ipsum #{command} dolor
     sit
     amet
     """
 
-    GithubController.process_commands(full_body, %{"login" => author}, @params)
+    GithubController.process_commands(
+      "issue_comment",
+      Map.put(@params, "comment", %{"user" => %{"login" => author}, "body" => body})
+    )
   end
 end