refactor controller for better control and o11y

Author: zcesur

lib/algora/bounties/bounties.ex

@@ -56,8 +56,8 @@ defmodule Algora.Bounties do
       {:error, %{errors: [ticket_id: {_, [constraint: :unique, constraint_name: _]}]}} ->
         {:error, :already_exists}
 
-      {:error, _changeset} ->
-        {:error, :internal_server_error}
+      {:error, _changeset} = error ->
+        error
     end
   end
 
@@ -147,8 +147,8 @@ defmodule Algora.Bounties do
       {:error, %{errors: [ticket_id: {_, [constraint: :unique, constraint_name: _]}]}} ->
         {:error, :already_exists}
 
-      {:error, _changeset} ->
-        {:error, :internal_server_error}
+      {:error, _changeset} = error ->
+        error
     end
   end
 

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, reason} = error ->
+        Logger.error("Error parsing commands: #{inspect(reason)}")
+        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"]

lib/algora_web/live/org/bounty_hook.ex

@@ -27,9 +27,6 @@ defmodule AlgoraWeb.Org.BountyHook do
       {:error, :already_exists} ->
         {:halt, put_flash(socket, :warning, "You have already created a bounty for this ticket")}
 
-      {:error, :internal_server_error} ->
-        {:halt, put_flash(socket, :error, "Something went wrong")}
-
       {:error, _reason} ->
         changeset = add_error(socket.assigns.new_bounty_form.changeset, :github_issue_url, "Invalid URL")
         {:halt, assign(socket, :new_bounty_form, to_form(changeset))}

lib/algora_web/live/org/create_bounty_live.ex

@@ -468,9 +468,6 @@ defmodule AlgoraWeb.Org.CreateBountyLive do
         {:error, :already_exists} ->
           {:noreply, put_flash(socket, :warning, "You have already created a bounty for this ticket")}
 
-        {:error, :internal_server_error} ->
-          {:noreply, put_flash(socket, :error, "Something went wrong")}
-
         {:error, _reason} ->
           changeset = add_error(socket.assigns.new_bounty_form.changeset, :github_issue_url, "Invalid URL")
           {:noreply, assign(socket, :new_bounty_form, to_form(changeset))}

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