Improve user experience on confirmation messages

[josevalim]

Those are changes I have applied based on production feedback of the confirmation system. Also, because sometimes e-mail clients may accidentally preview the confirmation link, showing an error message for the cases the user is already logged in can be confusing, so we skip that too.

diff --git a/apps/bytepack_web/lib/bytepack_web/controllers/user_confirmation_controller.ex b/apps/bytepack_web/lib/bytepack_web/controllers/user_confirmation_controller.ex
index 6a89883..ea79aa2 100644
--- a/apps/bytepack_web/lib/bytepack_web/controllers/user_confirmation_controller.ex
+++ b/apps/bytepack_web/lib/bytepack_web/controllers/user_confirmation_controller.ex
@@ -39,9 +39,19 @@ defmodule BytepackWeb.UserConfirmationController do
         |> redirect(to: Routes.user_session_path(conn, :new))

       :error ->
-        conn
-        |> put_flash(:error, "Confirmation token is invalid or it has expired.")
-        |> redirect(to: Routes.user_session_path(conn, :new))
+        # If there is a current user and the account was already confirmed,
+        # then odds are that the confirmation link was already visited, either
+        # by some automation or by the user themselves, so we redirect without
+        # a warning message.
+        case conn.assigns do
+          %{current_user: %{confirmed_at: confirmed_at}} when not is_nil(confirmed_at) ->
+            redirect(conn, to: Routes.user_session_path(conn, :index))
+
+          %{} ->
+            conn
+            |> put_flash(:error, "Account confirmation link is invalid or it has expired.")
+            |> redirect(to: Routes.user_session_path(conn, :new))
+        end
     end
   end
 end
diff --git a/apps/bytepack_web/test/bytepack_web/controllers/user_confirmation_controller_test.exs b/apps/bytepack_web/test/bytepack_web/controllers/user_confirmation_controller_test.exs
index 5a92925..1238712 100644
--- a/apps/bytepack_web/test/bytepack_web/controllers/user_confirmation_controller_test.exs
+++ b/apps/bytepack_web/test/bytepack_web/controllers/user_confirmation_controller_test.exs
@@ -69,15 +69,21 @@ defmodule BytepackWeb.UserConfirmationControllerTest do
       refute get_session(conn, :user_token)
       assert Repo.all(Accounts.UserToken) == []

+      # When not logged in
       conn = get(conn, Routes.user_confirmation_path(conn, :confirm, token))
       assert redirected_to(conn) == Routes.user_session_path(conn, :new)
-      assert get_flash(conn, :error) =~ "Confirmation token is invalid or it has expired"
+      assert get_flash(conn, :error) =~ "Account confirmation link is invalid or it has expired"
+
+      # When logged in
+      conn = build_conn() |> login_user(user) |> get(Routes.user_confirmation_path(conn, :confirm, token))
+      assert redirected_to(conn) == Routes.dashboard_index_path(conn, :index)
+      refute get_flash(conn, :error)
     end

     test "does not confirm email with invalid token", %{conn: conn, user: user} do
       conn = get(conn, Routes.user_confirmation_path(conn, :confirm, "oops"))
       assert redirected_to(conn) == Routes.user_session_path(conn, :new)
-      assert get_flash(conn, :error) =~ "Confirmation token is invalid or it has expired"
+      assert get_flash(conn, :error) =~ "Account confirmation link is invalid or it has expired"
       refute Accounts.get_user!(user.id).confirmed_at
     end
   end

I am submitting them because they are probably worth incorporating here too!

[aaronrenner]

Thanks, @josevalim! It's awesome to get to incorporate feedback from production systems back into the generators.

I'm going to tag it as "Good first issue" for a few days in case someone wants to submit a PR for it. Otherwise I'll be happy to pick it up.

[aaronrenner]

Fixed by #79.