Tidy up verification code a little (#4156)

lib/plausible/verification/check.ex

@@ -1,13 +1,13 @@
 defmodule Plausible.Verification.Check do
   @moduledoc """
   Behaviour to be implemented by specific site verification checks.
-  `friendly_name()` doesn't necessarily reflect the actual check description,
+  `report_progress_as()` doesn't necessarily reflect the actual check description,
   it serves as a user-facing message grouping mechanism, to prevent frequent message flashing when checks rotate often.
   Each check operates on `state()` and is expected to return it, optionally modified, by all means.
   `perform_safe/1` is used to guarantee no exceptions are thrown by faulty implementations, not to interrupt LiveView.
   """
   @type state() :: Plausible.Verification.State.t()
-  @callback friendly_name() :: String.t()
+  @callback report_progress_as() :: String.t()
   @callback perform(state()) :: state()
 
   defmacro __using__(_) do
@@ -30,7 +30,7 @@ defmodule Plausible.Verification.Check do
             "Error running check #{inspect(__MODULE__)} on #{state.url}: #{inspect(e)}"
           )
 
-          put_diagnostics(state, service_error: true)
+          put_diagnostics(state, service_error: e)
       end
     end
   end

lib/plausible/verification/checks/csp.ex

@@ -6,7 +6,7 @@ defmodule Plausible.Verification.Checks.CSP do
   use Plausible.Verification.Check
 
   @impl true
-  def friendly_name, do: "We're visiting your site to ensure that everything is working"
+  def report_progress_as, do: "We're visiting your site to ensure that everything is working"
 
   @impl true
   def perform(%State{assigns: %{headers: headers}} = state) do

lib/plausible/verification/checks/fetch_body.ex

@@ -7,7 +7,7 @@ defmodule Plausible.Verification.Checks.FetchBody do
   use Plausible.Verification.Check
 
   @impl true
-  def friendly_name, do: "We're visiting your site to ensure that everything is working"
+  def report_progress_as, do: "We're visiting your site to ensure that everything is working"
 
   @impl true
   def perform(%State{url: "https://" <> _ = url} = state) do

lib/plausible/verification/checks/installation.ex

@@ -19,7 +19,7 @@ defmodule Plausible.Verification.Checks.Installation do
   use Plausible.Verification.Check
 
   @impl true
-  def friendly_name, do: "We're verifying that your visitors are being counted correctly"
+  def report_progress_as, do: "We're verifying that your visitors are being counted correctly"
 
   @impl true
   def perform(%State{url: url} = state) do

lib/plausible/verification/checks/scan_body.ex

@@ -5,7 +5,7 @@ defmodule Plausible.Verification.Checks.ScanBody do
   use Plausible.Verification.Check
 
   @impl true
-  def friendly_name, do: "We're visiting your site to ensure that everything is working"
+  def report_progress_as, do: "We're visiting your site to ensure that everything is working"
 
   @impl true
   def perform(%State{assigns: %{raw_body: body}} = state) when is_binary(body) do

lib/plausible/verification/checks/snippet.ex

@@ -7,7 +7,7 @@ defmodule Plausible.Verification.Checks.Snippet do
   use Plausible.Verification.Check
 
   @impl true
-  def friendly_name, do: "We're looking for the Plausible snippet on your site"
+  def report_progress_as, do: "We're looking for the Plausible snippet on your site"
 
   @impl true
   def perform(%State{assigns: %{document: document}} = state) do

lib/plausible/verification/checks/snippet_cache_bust.ex

@@ -9,7 +9,7 @@ defmodule Plausible.Verification.Checks.SnippetCacheBust do
   use Plausible.Verification.Check
 
   @impl true
-  def friendly_name, do: "We're looking for the Plausible snippet on your site"
+  def report_progress_as, do: "We're looking for the Plausible snippet on your site"
 
   @impl true
   def perform(

lib/plausible/verification/diagnostics.ex

@@ -320,7 +320,7 @@ defmodule Plausible.Verification.Diagnostics do
     %Result{
       ok?: false,
       errors: [error.message],
-      recommendations: [{error.recommendation, error.url}]
+      recommendations: [%{text: error.recommendation, url: error.url}]
     }
   end
 

lib/plausible_web/live/components/verification.ex

@@ -31,7 +31,7 @@ defmodule PlausibleWeb.Live.Components.Verification do
         :if={not @finished? or (not @modal? and @success?)}
         class="mx-auto flex h-12 w-12 items-center justify-center rounded-full bg-green-100 dark:bg-gray-700"
       >
-        <div class="block pulsating-circle" }></div>
+        <div class="block pulsating-circle"></div>
       </div>
 
       <div
@@ -67,10 +67,12 @@ defmodule PlausibleWeb.Live.Components.Verification do
         </p>
         <p :if={not @finished?} class="mt-2 animate-pulse" id="progress"><%= @message %></p>
 
-        <.recommendations
-          :if={@finished? and not @success? and @interpretation}
-          interpretation={@interpretation}
-        />
+        <p :if={@finished? and not @success? and @interpretation} class="mt-2" id="recommendation">
+          <span><%= List.first(@interpretation.recommendations).text %>.&nbsp;</span>
+          <.styled_link href={List.first(@interpretation.recommendations).url} new_tab={true}>
+            Learn more
+          </.styled_link>
+        </p>
       </div>
 
       <div :if={@finished?} class="mt-auto">
@@ -115,19 +117,4 @@ defmodule PlausibleWeb.Live.Components.Verification do
     </div>
     """
   end
-
-  def recommendations(assigns) do
-    ~H"""
-    <p class="mt-2" id="recommendations">
-      <span :for={recommendation <- @interpretation.recommendations} class="recommendation">
-        <span :if={is_binary(recommendation)}><%= recommendation %></span>
-        <span :if={is_tuple(recommendation)}><%= elem(recommendation, 0) %>.&nbsp;</span>
-        <.styled_link :if={is_tuple(recommendation)} href={elem(recommendation, 1)} new_tab={true}>
-          Learn more
-        </.styled_link>
-        <br />
-      </span>
-    </p>
-    """
-  end
 end

lib/plausible_web/live/verification.ex

@@ -108,7 +108,7 @@ defmodule PlausibleWeb.Live.Verification do
 
   def handle_info({:verification_check_start, {check, _state}}, socket) do
     update_component(socket,
-      message: check.friendly_name()
+      message: check.report_progress_as()
     )
 
     {:noreply, socket}

test/plausible/site/verification/checks_test.exs

@@ -9,17 +9,17 @@ defmodule Plausible.Verification.ChecksTest do
 
   @errors Plausible.Verification.Errors.all()
 
-  @normal_body """
-  <html>
-  <head>
-  <script defer data-domain="example.com" src="http://localhost:8000/js/script.js"></script>
-  </head>
-  <body>Hello</body>
-  </html>
-  """
+  describe "successful verification" do
+    @normal_body """
+    <html>
+    <head>
+    <script defer data-domain="example.com" src="http://localhost:8000/js/script.js"></script>
+    </head>
+    <body>Hello</body>
+    </html>
+    """
 
-  describe "running checks" do
-    test "success" do
+    test "definite success" do
       stub_fetch_body(200, @normal_body)
       stub_installation()
 
@@ -28,32 +28,6 @@ defmodule Plausible.Verification.ChecksTest do
       |> assert_ok()
     end
 
-    test "service error - 400" do
-      stub_fetch_body(200, @normal_body)
-      stub_installation(400, %{})
-
-      run_checks()
-      |> Checks.interpret_diagnostics()
-      |> assert_error(@errors.temporary)
-    end
-
-    @tag :slow
-    test "can't fetch body but headless reports ok" do
-      stub_fetch_body(500, "")
-      stub_installation()
-
-      {_, log} =
-        with_log(fn ->
-          run_checks()
-          |> Checks.interpret_diagnostics()
-          |> assert_error(@errors.unreachable, url: "https://example.com")
-        end)
-
-      assert log =~ "3 attempts left"
-      assert log =~ "2 attempts left"
-      assert log =~ "1 attempt left"
-    end
-
     test "fetching will follow 2 redirects" do
       ref = :counters.new(1, [:atomics])
       test = self()
@@ -84,6 +58,123 @@ defmodule Plausible.Verification.ChecksTest do
       refute_receive _
     end
 
+    test "allowed via content-security-policy" do
+      stub_fetch_body(fn conn ->
+        conn
+        |> put_resp_header(
+          "content-security-policy",
+          Enum.random([
+            "default-src 'self'; script-src plausible.io; connect-src #{PlausibleWeb.Endpoint.host()}",
+            "default-src 'self' *.#{PlausibleWeb.Endpoint.host()}"
+          ])
+        )
+        |> put_resp_content_type("text/html")
+        |> send_resp(200, @normal_body)
+      end)
+
+      stub_installation()
+
+      run_checks()
+      |> Checks.interpret_diagnostics()
+      |> assert_ok()
+    end
+
+    @proxied_script_body """
+    <html>
+    <head>
+    <script defer data-domain="example.com" src="https://proxy.example.com/js/script.js"></script>
+    </head>
+    <body>Hello</body>
+    </html>
+    """
+
+    test "proxied setup working OK" do
+      stub_fetch_body(200, @proxied_script_body)
+      stub_installation()
+
+      run_checks()
+      |> Checks.interpret_diagnostics()
+      |> assert_ok()
+    end
+
+    @body_no_snippet """
+    <html> <head> </head> <body> Hello </body> </html>
+    """
+
+    test "non-standard integration where the snippet cannot be found but it works ok in headless" do
+      stub_fetch_body(200, @body_no_snippet)
+      stub_installation(200, plausible_installed(true, 202))
+
+      run_checks()
+      |> Checks.interpret_diagnostics()
+      |> assert_ok()
+    end
+
+    @different_data_domain_body """
+    <html>
+    <head>
+    <script defer data-domain="www.example.com" src="http://localhost:8000/js/script.js"></script>
+    </head>
+    <body>Hello</body>
+    </html>
+    """
+
+    test "data-domain mismatch on redirect chain" do
+      ref = :counters.new(1, [:atomics])
+      test = self()
+
+      Req.Test.stub(Plausible.Verification.Checks.FetchBody, fn conn ->
+        if :counters.get(ref, 1) == 0 do
+          :counters.add(ref, 1, 1)
+          send(test, :redirect_sent)
+
+          conn
+          |> put_resp_header("location", "https://www.example.com")
+          |> send_resp(302, "redirecting to https://www.example.com")
+        else
+          conn
+          |> put_resp_header("content-type", "text/html")
+          |> send_resp(200, @different_data_domain_body)
+        end
+      end)
+
+      stub_installation()
+
+      run_checks()
+      |> Checks.interpret_diagnostics()
+      |> assert_ok()
+
+      assert_receive :redirect_sent
+    end
+  end
+
+  describe "errors" do
+    test "service error - 400" do
+      stub_fetch_body(200, @normal_body)
+      stub_installation(400, %{})
+
+      run_checks()
+      |> Checks.interpret_diagnostics()
+      |> assert_error(@errors.temporary)
+    end
+
+    @tag :slow
+    test "can't fetch body but headless reports ok" do
+      stub_fetch_body(500, "")
+      stub_installation()
+
+      {_, log} =
+        with_log(fn ->
+          run_checks()
+          |> Checks.interpret_diagnostics()
+          |> assert_error(@errors.unreachable, url: "https://example.com")
+        end)
+
+      assert log =~ "3 attempts left"
+      assert log =~ "2 attempts left"
+      assert log =~ "1 attempt left"
+    end
+
     test "fetching will give up at 5th redirect" do
       test = self()
 
@@ -163,16 +254,6 @@ defmodule Plausible.Verification.ChecksTest do
       |> assert_error(@errors.multiple_snippets)
     end
 
-    @body_no_snippet """
-    <html>
-    <head>
-    </head>
-    <body>
-    Hello
-    </body>
-    </html>
-    """
-
     test "detecting snippet after busting cache" do
       stub_fetch_body(fn conn ->
         conn = fetch_query_params(conn)
@@ -324,7 +405,7 @@ defmodule Plausible.Verification.ChecksTest do
         use Plausible.Verification.Check
 
         @impl true
-        def friendly_name, do: "Faulty check"
+        def report_progress_as, do: "Faulty check"
 
         @impl true
         def perform(_), do: raise("boom")
@@ -348,7 +429,7 @@ defmodule Plausible.Verification.ChecksTest do
         use Plausible.Verification.Check
 
         @impl true
-        def friendly_name, do: "Faulty check"
+        def report_progress_as, do: "Faulty check"
 
         @impl true
         def perform(_), do: :erlang.throw(:boom)
@@ -397,27 +478,6 @@ defmodule Plausible.Verification.ChecksTest do
       |> assert_error(@errors.no_snippet)
     end
 
-    test "allowed via content-security-policy" do
-      stub_fetch_body(fn conn ->
-        conn
-        |> put_resp_header(
-          "content-security-policy",
-          Enum.random([
-            "default-src 'self'; script-src plausible.io; connect-src #{PlausibleWeb.Endpoint.host()}",
-            "default-src 'self' *.#{PlausibleWeb.Endpoint.host()}"
-          ])
-        )
-        |> put_resp_content_type("text/html")
-        |> send_resp(200, @normal_body)
-      end)
-
-      stub_installation()
-
-      run_checks()
-      |> Checks.interpret_diagnostics()
-      |> assert_ok()
-    end
-
     test "running checks sends progress messages" do
       stub_fetch_body(200, @normal_body)
       stub_installation()
@@ -474,24 +534,6 @@ defmodule Plausible.Verification.ChecksTest do
       |> assert_error(@errors.unreachable, url: "https://example.com")
     end
 
-    @proxied_script_body """
-    <html>
-    <head>
-    <script defer data-domain="example.com" src="https://proxy.example.com/js/script.js"></script>
-    </head>
-    <body>Hello</body>
-    </html>
-    """
-
-    test "proxied setup working OK" do
-      stub_fetch_body(200, @proxied_script_body)
-      stub_installation()
-
-      run_checks()
-      |> Checks.interpret_diagnostics()
-      |> assert_ok()
-    end
-
     test "proxied setup, function defined but callback won't fire" do
       stub_fetch_body(200, @proxied_script_body)
       stub_installation(200, plausible_installed(true, 0))
@@ -628,15 +670,6 @@ defmodule Plausible.Verification.ChecksTest do
       |> assert_error(@errors.old_script_wp_plugin)
     end
 
-    test "non-standard integration where the snippet cannot be found but it works ok in headless" do
-      stub_fetch_body(200, @body_no_snippet)
-      stub_installation(200, plausible_installed(true, 202))
-
-      run_checks()
-      |> Checks.interpret_diagnostics()
-      |> assert_ok()
-    end
-
     test "fails due to callback status being something unlikely like 500" do
       stub_fetch_body(200, @normal_body)
       stub_installation(200, plausible_installed(true, 500))
@@ -646,15 +679,6 @@ defmodule Plausible.Verification.ChecksTest do
       |> assert_error(@errors.unknown)
     end
 
-    @different_data_domain_body """
-    <html>
-    <head>
-    <script defer data-domain="www.example.com" src="http://localhost:8000/js/script.js"></script>
-    </head>
-    <body>Hello</body>
-    </html>
-    """
-
     test "data-domain mismatch" do
       stub_fetch_body(200, @different_data_domain_body)
       stub_installation()
@@ -664,34 +688,6 @@ defmodule Plausible.Verification.ChecksTest do
       |> assert_error(@errors.different_data_domain, domain: "example.com")
     end
 
-    test "data-domain mismatch on redirect chain" do
-      ref = :counters.new(1, [:atomics])
-      test = self()
-
-      Req.Test.stub(Plausible.Verification.Checks.FetchBody, fn conn ->
-        if :counters.get(ref, 1) == 0 do
-          :counters.add(ref, 1, 1)
-          send(test, :redirect_sent)
-
-          conn
-          |> put_resp_header("location", "https://www.example.com")
-          |> send_resp(302, "redirecting to https://www.example.com")
-        else
-          conn
-          |> put_resp_header("content-type", "text/html")
-          |> send_resp(200, @different_data_domain_body)
-        end
-      end)
-
-      stub_installation()
-
-      run_checks()
-      |> Checks.interpret_diagnostics()
-      |> assert_ok()
-
-      assert_receive :redirect_sent
-    end
-
     @many_snippets_with_domain_mismatch """
     <html>
     <head>
@@ -758,7 +754,7 @@ defmodule Plausible.Verification.ChecksTest do
            ]
 
     assert interpretation.recommendations == [
-             {error.recommendation, error.url}
+             %{text: error.recommendation, url: error.url}
            ]
   end
 

test/plausible_web/live/components/verification_test.exs

@@ -9,7 +9,7 @@ defmodule PlausibleWeb.Live.Components.VerificationTest do
   @pulsating_circle ~s|div#progress-indicator div.pulsating-circle|
   @check_circle ~s|div#progress-indicator #check-circle|
   @error_circle ~s|div#progress-indicator #error-circle|
-  @recommendations ~s|#recommendations .recommendation|
+  @recommendations ~s|#recommendation|
 
   test "renders initial state" do
     html = render_component(@component, domain: "example.com")