Treat non-200 HTTP responses as errors (#2243)

* Tag non-200 HTTP responses as errors

Co-authored-by: RobertJoonas <robertjoonas16@gmail.com>

* Send get_invoices/1 errors to Sentry

Co-authored-by: RobertJoonas <robertjoonas16@gmail.com>

* Keep Google API module matching non-200 responses

Co-authored-by: RobertJoonas <robertjoonas16@gmail.com>

* Make sure HTTPClient.Error.t() doesn't appear in the UI

* Unify get_invoices/1 signature

Co-authored-by: RobertJoonas <robertjoonas16@gmail.com>

Co-authored-by: RobertJoonas <robertjoonas16@gmail.com>

lib/plausible/billing/paddle_api.ex

@@ -89,6 +89,10 @@ defmodule Plausible.Billing.PaddleApi do
     end
   end
 
+  @spec get_invoices(Plausible.Billing.Subscription.t()) ::
+          {:ok, list()}
+          | {:error, :request_failed}
+          | {:error, :no_subscription}
   def get_invoices(nil), do: {:error, :no_subscription}
 
   def get_invoices(subscription) do
@@ -103,28 +107,25 @@ defmodule Plausible.Billing.PaddleApi do
       to: Timex.shift(Timex.today(), days: 1) |> Timex.format!("{YYYY}-{0M}-{0D}")
     }
 
-    case HTTPClient.post(invoices_url(), @headers, params) do
+    with {:ok, response} <- HTTPClient.post(invoices_url(), @headers, params),
-      {:ok, response} ->
+         {:ok, body} <- Jason.decode(response.body),
-        body = Jason.decode!(response.body)
+         true <- Map.get(body, "success"),
+         [_ | _] = response <- Map.get(body, "response") do
+      Enum.sort(response, fn %{"payout_date" => d1}, %{"payout_date" => d2} ->
+        Date.compare(Date.from_iso8601!(d1), Date.from_iso8601!(d2)) == :gt
+      end)
+      |> Enum.take(12)
+      |> then(&{:ok, &1})
+    else
+      error ->
+        Sentry.capture_message("Failed to retrieve invoices from Paddle",
+          extra: %{extra: error}
+        )
 
-        if body["success"] && body["response"] != [] do
-          body["response"] |> last_12_invoices()
-        else
-          {:error, :request_failed}
-        end
-
-      {:error, _reason} ->
         {:error, :request_failed}
     end
   end
 
-  defp last_12_invoices(invoice_list) do
-    Enum.sort(invoice_list, fn %{"payout_date" => d1}, %{"payout_date" => d2} ->
-      Date.compare(Date.from_iso8601!(d1), Date.from_iso8601!(d2)) == :gt
-    end)
-    |> Enum.take(12)
-  end
-
   def checkout_domain() do
     case Application.get_env(:plausible, :environment) do
       "dev" -> "https://sandbox-checkout.paddle.com"

lib/plausible/google/http.ex

@@ -138,7 +138,7 @@ defmodule Plausible.Google.HTTP do
       {:ok, %Finch.Response{body: body, status: 200}} ->
         {:ok, Jason.decode!(body)}
 
-      {:ok, %Finch.Response{body: body}} ->
+      {:error, %{reason: %Finch.Response{body: body}}} ->
         Sentry.capture_message("Error fetching Google view ID", extra: Jason.decode!(body))
         {:error, body}
 
@@ -174,16 +174,16 @@ defmodule Plausible.Google.HTTP do
       {:ok, %Finch.Response{body: body, status: 200}} ->
         {:ok, Jason.decode!(body)}
 
-      {:ok, %Finch.Response{body: body, status: 401}} ->
+      {:error, %{reason: %Finch.Response{body: body, status: 401}}} ->
         Sentry.capture_message("Error fetching Google queries", extra: Jason.decode!(body))
         {:error, :invalid_credentials}
 
-      {:ok, %Finch.Response{body: body, status: 403}} ->
+      {:error, %{reason: %Finch.Response{body: body, status: 403}}} ->
         body = Jason.decode!(body)
         Sentry.capture_message("Error fetching Google queries", extra: body)
         {:error, get_in(body, ["error", "message"])}
 
-      {:ok, %Finch.Response{body: body}} ->
+      {:error, %{reason: %Finch.Response{body: body}}} ->
         Sentry.capture_message("Error fetching Google queries", extra: Jason.decode!(body))
         {:error, :unknown}
 
@@ -212,7 +212,7 @@ defmodule Plausible.Google.HTTP do
       {:ok, %Finch.Response{body: body, status: 200}} ->
         {:ok, Jason.decode!(body)}
 
-      {:ok, %Finch.Response{body: body, status: _non_http_200}} ->
+      {:error, %{reason: %Finch.Response{body: body, status: _non_http_200}}} ->
         body
         |> Jason.decode!()
         |> Map.get("error")
@@ -261,7 +261,7 @@ defmodule Plausible.Google.HTTP do
 
         {:ok, date}
 
-      {:ok, %Finch.Response{body: body}} ->
+      {:error, %{reason: %Finch.Response{body: body}}} ->
         Sentry.capture_message("Error fetching Google view ID", extra: Jason.decode!(body))
         {:error, body}
 

lib/plausible/http_client.ex

@@ -9,10 +9,24 @@ defmodule Plausible.HTTPClient do
   URL encoding is invoked.
   """
 
+  defmodule Non200Error do
+    defstruct reason: nil
+
+    @type t :: %__MODULE__{reason: Finch.Response.t()}
+
+    @spec new(Finch.Response.t()) :: t()
+    def new(%Finch.Response{status: status} = response)
+        when is_integer(status) and (status < 200 or status >= 300) do
+      %__MODULE__{reason: response}
+    end
+  end
+
   @type url() :: Finch.Request.url()
   @type headers() :: Finch.Request.headers()
   @type params() :: Finch.Request.body() | map()
-  @type response() :: {:ok, Finch.Response.t()} | {:error, Mint.Types.error() | Finch.Error.t()}
+  @type response() ::
+          {:ok, Finch.Response.t()}
+          | {:error, Mint.Types.error() | Finch.Error.t() | Non200Error.t()}
 
   @doc """
   Make a POST request
@@ -35,7 +49,8 @@ defmodule Plausible.HTTPClient do
 
     method
     |> build_request(url, headers, params)
-    |> do_request
+    |> do_request()
+    |> tag_error()
   end
 
   defp build_request(method, url, headers, params) do
@@ -69,4 +84,17 @@ defmodule Plausible.HTTPClient do
         {Jason.encode!(params), [{"content-type", "application/json"} | headers]}
     end
   end
+
+  defp tag_error({:ok, %Finch.Response{status: status}} = ok)
+       when is_integer(status) and status >= 200 and status < 300 do
+    ok
+  end
+
+  defp tag_error({:ok, %Finch.Response{status: _} = response}) do
+    {:error, Non200Error.new(response)}
+  end
+
+  defp tag_error({:error, _} = error) do
+    error
+  end
 end

lib/plausible_web/templates/auth/user_settings.html.eex

@@ -136,7 +136,7 @@
       </p>
     </div>
 
-  <% invoice_list -> %>
+    <% {:ok, invoice_list} when is_list(invoice_list) -> %>
     <div class="max-w-2xl px-8 pt-6 pb-8 mx-auto mt-16 bg-white border-t-2 border-orange-200 rounded rounded-t-none shadow-md dark:bg-gray-800">
       <h2 class="text-xl font-black dark:text-gray-100">Invoices</h2>
       <div class="my-4 border-b border-gray-300 dark:border-gray-500"></div>

lib/plausible_web/templates/site/settings_search_console.html.eex

@@ -41,7 +41,7 @@
           <%= if error == "invalid_grant" do %>
             <p class="text-red-700 font-medium mt-3"><a href="https://plausible.io/docs/google-search-console-integration#i-get-the-invalid-grant-error">Invalid Grant error returned from Google. <span class="text-indigo-500">See here on how to fix it.</a></p>
           <% else %>
-            <p class="text-red-700 font-medium mt-3"><%= error %></p>
+            <p class="text-red-700 font-medium mt-3">Something went wrong</p>
           <% end %>
       <% end %>
     <% else %>

test/plausible/http_client_test.exs

@@ -102,6 +102,26 @@ defmodule Plausible.HTTPClientTest do
              HTTPClient.post(bypass_url(bypass, path: "/any"), headers_no_content_type, params)
   end
 
+  test "non-200 responses are tagged as errors", %{bypass: bypass} do
+    Bypass.expect_once(bypass, "GET", "/get", fn conn ->
+      Conn.resp(conn, 300, "oops")
+    end)
+
+    assert {:error,
+            %HTTPClient.Non200Error{
+              reason: %Finch.Response{status: 300, body: "oops"}
+            }} = HTTPClient.get(bypass_url(bypass, path: "/get"))
+
+    Bypass.expect_once(bypass, "GET", "/get", fn conn ->
+      Conn.resp(conn, 400, "oops")
+    end)
+
+    assert {:error,
+            %HTTPClient.Non200Error{
+              reason: %Finch.Response{status: 400, body: "oops"}
+            }} = HTTPClient.get(bypass_url(bypass, path: "/get"))
+  end
+
   defp bypass_url(bypass, opts) do
     port = bypass.port
     path = Keyword.get(opts, :path, "/")

test/support/paddle_api_mock.ex

@@ -34,20 +34,21 @@ defmodule Plausible.PaddleApi.Mock do
         {:error, :request_failed}
 
       _ ->
-        [
+        {:ok,
-          %{
+         [
-            "amount" => 11.11,
+           %{
-            "currency" => "EUR",
+             "amount" => 11.11,
-            "payout_date" => "2020-12-24",
+             "currency" => "EUR",
-            "receipt_url" => "https://some-receipt-url.com"
+             "payout_date" => "2020-12-24",
-          },
+             "receipt_url" => "https://some-receipt-url.com"
-          %{
+           },
-            "amount" => 22,
+           %{
-            "currency" => "USD",
+             "amount" => 22,
-            "payout_date" => "2020-11-24",
+             "currency" => "USD",
-            "receipt_url" => "https://other-receipt-url.com"
+             "payout_date" => "2020-11-24",
-          }
+             "receipt_url" => "https://other-receipt-url.com"
-        ]
+           }
+         ]}
     end
   end
 end