From 2ce4fe3cc869358364667dce98379b60f4d6e417 Mon Sep 17 00:00:00 2001
From: shibao <shibao@bubbletea.dev>
Date: Wed, 23 Nov 2022 21:15:52 -0500
Subject: [PATCH] improve accounts context

---
 lib/memex/accounts.ex                         | 74 ++++++++++---------
 lib/memex/accounts/user.ex                    | 45 ++++++-----
 .../user_registration_controller.ex           |  3 +-
 priv/gettext/default.pot                      |  2 +-
 priv/gettext/errors.pot                       | 16 ++--
 priv/gettext/prompts.pot                      |  2 +-
 test/memex/accounts_test.exs                  |  5 +-
 test/support/fixtures.ex                      |  7 +-
 8 files changed, 79 insertions(+), 75 deletions(-)

diff --git a/lib/memex/accounts.ex b/lib/memex/accounts.ex
index a51eb80..6ddbc49 100644
--- a/lib/memex/accounts.ex
+++ b/lib/memex/accounts.ex
@@ -23,7 +23,7 @@ defmodule Memex.Accounts do
       nil
 
   """
-  @spec get_user_by_email(String.t()) :: User.t() | nil
+  @spec get_user_by_email(email :: String.t()) :: User.t() | nil
   def get_user_by_email(email) when is_binary(email), do: Repo.get_by(User, email: email)
 
   @doc """
@@ -38,7 +38,7 @@ defmodule Memex.Accounts do
       nil
 
   """
-  @spec get_user_by_email_and_password(String.t(), String.t()) ::
+  @spec get_user_by_email_and_password(email :: String.t(), password :: String.t()) ::
           User.t() | nil
   def get_user_by_email_and_password(email, password)
       when is_binary(email) and is_binary(password) do
@@ -86,7 +86,7 @@ defmodule Memex.Accounts do
       [%User{}]
 
   """
-  @spec list_users_by_role(:admin | :user) :: [User.t()]
+  @spec list_users_by_role(User.role()) :: [User.t()]
   def list_users_by_role(role) do
     role = role |> to_string()
     Repo.all(from u in User, where: u.role == ^role, order_by: u.email)
@@ -106,15 +106,21 @@ defmodule Memex.Accounts do
       {:error, %Changeset{}}
 
   """
-  @spec register_user(map()) :: {:ok, User.t()} | {:error, Changeset.t(User.new_user())}
+  @spec register_user(attrs :: map()) :: {:ok, User.t()} | {:error, User.changeset()}
   def register_user(attrs) do
-    # if no registered users, make first user an admin
-    role =
-      if Repo.one!(from u in User, select: count(u.id), distinct: true) == 0,
-        do: "admin",
-        else: "user"
+    Multi.new()
+    |> Multi.one(:users_count, from(u in User, select: count(u.id), distinct: true))
+    |> Multi.insert(:add_user, fn %{users_count: count} ->
+      # if no registered users, make first user an admin
+      role = if count == 0, do: "admin", else: "user"
 
-    %User{} |> User.registration_changeset(attrs |> Map.put("role", role)) |> Repo.insert()
+      User.registration_changeset(attrs) |> User.role_changeset(role)
+    end)
+    |> Repo.transaction()
+    |> case do
+      {:ok, %{add_user: user}} -> {:ok, user}
+      {:error, :add_user, changeset, _changes_so_far} -> {:error, changeset}
+    end
   end
 
   @doc """
@@ -126,12 +132,10 @@ defmodule Memex.Accounts do
       %Changeset{data: %User{}}
 
   """
-  @spec change_user_registration(User.t() | User.new_user()) ::
-          Changeset.t(User.t() | User.new_user())
-  @spec change_user_registration(User.t() | User.new_user(), map()) ::
-          Changeset.t(User.t() | User.new_user())
-  def change_user_registration(user, attrs \\ %{}),
-    do: User.registration_changeset(user, attrs, hash_password: false)
+  @spec change_user_registration() :: User.changeset()
+  @spec change_user_registration(attrs :: map()) :: User.changeset()
+  def change_user_registration(attrs \\ %{}),
+    do: User.registration_changeset(attrs, hash_password: false)
 
   ## Settings
 
@@ -144,7 +148,7 @@ defmodule Memex.Accounts do
       %Changeset{data: %User{}}
 
   """
-  @spec change_user_email(User.t(), map()) :: Changeset.t(User.t())
+  @spec change_user_email(User.t(), attrs :: map()) :: User.changeset()
   def change_user_email(user, attrs \\ %{}), do: User.email_changeset(user, attrs)
 
   @doc """
@@ -156,7 +160,7 @@ defmodule Memex.Accounts do
       %Changeset{data: %User{}}
 
   """
-  @spec change_user_role(User.t(), atom()) :: Changeset.t(User.t())
+  @spec change_user_role(User.t(), User.role()) :: User.changeset()
   def change_user_role(user, role), do: User.role_changeset(user, role)
 
   @doc """
@@ -172,8 +176,8 @@ defmodule Memex.Accounts do
       {:error, %Changeset{}}
 
   """
-  @spec apply_user_email(User.t(), String.t(), map()) ::
-          {:ok, User.t()} | {:error, Changeset.t(User.t())}
+  @spec apply_user_email(User.t(), password :: String.t(), attrs :: map()) ::
+          {:ok, User.t()} | {:error, User.changeset()}
   def apply_user_email(user, password, attrs) do
     user
     |> User.email_changeset(attrs)
@@ -187,7 +191,7 @@ defmodule Memex.Accounts do
   If the token matches, the user email is updated and the token is deleted.
   The confirmed_at date is also updated to the current time.
   """
-  @spec update_user_email(User.t(), String.t()) :: :ok | :error
+  @spec update_user_email(User.t(), token :: String.t()) :: :ok | :error
   def update_user_email(user, token) do
     context = "change:#{user.email}"
 
@@ -200,7 +204,7 @@ defmodule Memex.Accounts do
     end
   end
 
-  @spec user_email_multi(User.t(), String.t(), String.t()) :: Multi.t()
+  @spec user_email_multi(User.t(), email :: String.t(), context :: String.t()) :: Multi.t()
   defp user_email_multi(user, email, context) do
     changeset = user |> User.email_changeset(%{email: email}) |> User.confirm_changeset()
 
@@ -218,7 +222,8 @@ defmodule Memex.Accounts do
       {:ok, %{to: ..., body: ...}}
 
   """
-  @spec deliver_update_email_instructions(User.t(), String.t(), function) :: Job.t()
+  @spec deliver_update_email_instructions(User.t(), current_email :: String.t(), function) ::
+          Job.t()
   def deliver_update_email_instructions(user, current_email, update_email_url_fun)
       when is_function(update_email_url_fun, 1) do
     {encoded_token, user_token} = UserToken.build_email_token(user, "change:#{current_email}")
@@ -235,7 +240,7 @@ defmodule Memex.Accounts do
       %Changeset{data: %User{}}
 
   """
-  @spec change_user_password(User.t(), map()) :: Changeset.t(User.t())
+  @spec change_user_password(User.t(), attrs :: map()) :: User.changeset()
   def change_user_password(user, attrs \\ %{}),
     do: User.password_changeset(user, attrs, hash_password: false)
 
@@ -251,8 +256,8 @@ defmodule Memex.Accounts do
       {:error, %Changeset{}}
 
   """
-  @spec update_user_password(User.t(), String.t(), map()) ::
-          {:ok, User.t()} | {:error, Changeset.t(User.t())}
+  @spec update_user_password(User.t(), password :: String.t(), attrs :: map()) ::
+          {:ok, User.t()} | {:error, User.changeset()}
   def update_user_password(user, password, attrs) do
     changeset =
       user
@@ -278,7 +283,7 @@ defmodule Memex.Accounts do
       %Changeset{data: %User{}}
 
   """
-  @spec change_user_locale(User.t()) :: Changeset.t(User.t())
+  @spec change_user_locale(User.t()) :: User.changeset()
   def change_user_locale(%{locale: locale} = user), do: User.locale_changeset(user, locale)
 
   @doc """
@@ -294,7 +299,7 @@ defmodule Memex.Accounts do
 
   """
   @spec update_user_locale(User.t(), locale :: String.t()) ::
-          {:ok, User.t()} | {:error, Changeset.t(User.t())}
+          {:ok, User.t()} | {:error, User.changeset()}
   def update_user_locale(user, locale),
     do: user |> User.locale_changeset(locale) |> Repo.update()
 
@@ -310,7 +315,7 @@ defmodule Memex.Accounts do
       %User{}
 
   """
-  @spec delete_user!(User.t(), User.t()) :: User.t()
+  @spec delete_user!(user_to_delete :: User.t(), User.t()) :: User.t()
   def delete_user!(user, %User{role: :admin}), do: user |> Repo.delete!()
   def delete_user!(%User{id: user_id} = user, %User{id: user_id}), do: user |> Repo.delete!()
 
@@ -329,7 +334,7 @@ defmodule Memex.Accounts do
   @doc """
   Gets the user with the given signed token.
   """
-  @spec get_user_by_session_token(String.t()) :: User.t()
+  @spec get_user_by_session_token(token :: String.t()) :: User.t()
   def get_user_by_session_token(token) do
     {:ok, query} = UserToken.verify_session_token_query(token)
     Repo.one(query)
@@ -338,7 +343,7 @@ defmodule Memex.Accounts do
   @doc """
   Deletes the signed token with the given context.
   """
-  @spec delete_session_token(String.t()) :: :ok
+  @spec delete_session_token(token :: String.t()) :: :ok
   def delete_session_token(token) do
     Repo.delete_all(UserToken.token_and_context_query(token, "session"))
     :ok
@@ -394,7 +399,7 @@ defmodule Memex.Accounts do
   If the token matches, the user account is marked as confirmed
   and the token is deleted.
   """
-  @spec confirm_user(String.t()) :: {:ok, User.t()} | atom()
+  @spec confirm_user(token :: String.t()) :: {:ok, User.t()} | atom()
   def confirm_user(token) do
     with {:ok, query} <- UserToken.verify_email_token_query(token, "confirm"),
          %User{} = user <- Repo.one(query),
@@ -443,7 +448,7 @@ defmodule Memex.Accounts do
       nil
 
   """
-  @spec get_user_by_reset_password_token(String.t()) :: User.t() | nil
+  @spec get_user_by_reset_password_token(token :: String.t()) :: User.t() | nil
   def get_user_by_reset_password_token(token) do
     with {:ok, query} <- UserToken.verify_email_token_query(token, "reset_password"),
          %User{} = user <- Repo.one(query) do
@@ -465,7 +470,8 @@ defmodule Memex.Accounts do
       {:error, %Changeset{}}
 
   """
-  @spec reset_user_password(User.t(), map()) :: {:ok, User.t()} | {:error, Changeset.t(User.t())}
+  @spec reset_user_password(User.t(), attrs :: map()) ::
+          {:ok, User.t()} | {:error, User.changeset()}
   def reset_user_password(user, attrs) do
     Multi.new()
     |> Multi.update(:user, User.password_changeset(user, attrs))
diff --git a/lib/memex/accounts/user.ex b/lib/memex/accounts/user.ex
index 7fa0ebd..411a5a3 100644
--- a/lib/memex/accounts/user.ex
+++ b/lib/memex/accounts/user.ex
@@ -7,7 +7,7 @@ defmodule Memex.Accounts.User do
   import Ecto.Changeset
   import MemexWeb.Gettext
   alias Ecto.{Changeset, UUID}
-  alias Memex.{Accounts.User, Invites.Invite}
+  alias Memex.Invites.Invite
 
   @derive {Inspect, except: [:password]}
   @primary_key {:id, :binary_id, autogenerate: true}
@@ -25,20 +25,22 @@ defmodule Memex.Accounts.User do
     timestamps()
   end
 
-  @type t :: %User{
+  @type t :: %__MODULE__{
           id: id(),
           email: String.t(),
           password: String.t(),
           hashed_password: String.t(),
           confirmed_at: NaiveDateTime.t(),
-          role: atom(),
+          role: role(),
           invites: [Invite.t()],
           locale: String.t() | nil,
           inserted_at: NaiveDateTime.t(),
           updated_at: NaiveDateTime.t()
         }
-  @type new_user :: %User{}
+  @type new_user :: %__MODULE__{}
   @type id :: UUID.t()
+  @type changeset :: Changeset.t(t() | new_user())
+  @type role :: :user | :admin | String.t()
 
   @doc """
   A user changeset for registration.
@@ -57,12 +59,11 @@ defmodule Memex.Accounts.User do
       validations on a LiveView form), this option can be set to `false`.
       Defaults to `true`.
   """
-  @spec registration_changeset(t() | new_user(), attrs :: map()) :: Changeset.t(t() | new_user())
-  @spec registration_changeset(t() | new_user(), attrs :: map(), opts :: keyword()) ::
-          Changeset.t(t() | new_user())
-  def registration_changeset(user, attrs, opts \\ []) do
-    user
-    |> cast(attrs, [:email, :password, :role, :locale])
+  @spec registration_changeset(attrs :: map()) :: changeset()
+  @spec registration_changeset(attrs :: map(), opts :: keyword()) :: changeset()
+  def registration_changeset(attrs, opts \\ []) do
+    %__MODULE__{}
+    |> cast(attrs, [:email, :password, :locale])
     |> validate_email()
     |> validate_password(opts)
   end
@@ -71,12 +72,12 @@ defmodule Memex.Accounts.User do
   A user changeset for role.
 
   """
-  @spec role_changeset(t(), role :: atom()) :: Changeset.t(t())
+  @spec role_changeset(t() | new_user() | changeset(), role()) :: changeset()
   def role_changeset(user, role) do
     user |> cast(%{"role" => role}, [:role])
   end
 
-  @spec validate_email(Changeset.t(t() | new_user())) :: Changeset.t(t() | new_user())
+  @spec validate_email(changeset()) :: changeset()
   defp validate_email(changeset) do
     changeset
     |> validate_required([:email])
@@ -88,8 +89,7 @@ defmodule Memex.Accounts.User do
     |> unique_constraint(:email)
   end
 
-  @spec validate_password(Changeset.t(t() | new_user()), opts :: keyword()) ::
-          Changeset.t(t() | new_user())
+  @spec validate_password(changeset(), opts :: keyword()) :: changeset()
   defp validate_password(changeset, opts) do
     changeset
     |> validate_required([:password])
@@ -100,8 +100,7 @@ defmodule Memex.Accounts.User do
     |> maybe_hash_password(opts)
   end
 
-  @spec maybe_hash_password(Changeset.t(t() | new_user()), opts :: keyword()) ::
-          Changeset.t(t() | new_user())
+  @spec maybe_hash_password(changeset(), opts :: keyword()) :: changeset()
   defp maybe_hash_password(changeset, opts) do
     hash_password? = Keyword.get(opts, :hash_password, true)
     password = get_change(changeset, :password)
@@ -120,7 +119,7 @@ defmodule Memex.Accounts.User do
 
   It requires the email to change otherwise an error is added.
   """
-  @spec email_changeset(t(), attrs :: map()) :: Changeset.t(t())
+  @spec email_changeset(t(), attrs :: map()) :: changeset()
   def email_changeset(user, attrs) do
     user
     |> cast(attrs, [:email])
@@ -143,8 +142,8 @@ defmodule Memex.Accounts.User do
       validations on a LiveView form), this option can be set to `false`.
       Defaults to `true`.
   """
-  @spec password_changeset(t(), attrs :: map()) :: Changeset.t(t())
-  @spec password_changeset(t(), attrs :: map(), opts :: keyword()) :: Changeset.t(t())
+  @spec password_changeset(t(), attrs :: map()) :: changeset()
+  @spec password_changeset(t(), attrs :: map(), opts :: keyword()) :: changeset()
   def password_changeset(user, attrs, opts \\ []) do
     user
     |> cast(attrs, [:password])
@@ -155,7 +154,7 @@ defmodule Memex.Accounts.User do
   @doc """
   Confirms the account by setting `confirmed_at`.
   """
-  @spec confirm_changeset(t() | Changeset.t(t())) :: Changeset.t(t())
+  @spec confirm_changeset(t() | changeset()) :: changeset()
   def confirm_changeset(user_or_changeset) do
     now = NaiveDateTime.utc_now() |> NaiveDateTime.truncate(:second)
     user_or_changeset |> change(confirmed_at: now)
@@ -168,7 +167,7 @@ defmodule Memex.Accounts.User do
   `Bcrypt.no_user_verify/0` to avoid timing attacks.
   """
   @spec valid_password?(t(), String.t()) :: boolean()
-  def valid_password?(%User{hashed_password: hashed_password}, password)
+  def valid_password?(%__MODULE__{hashed_password: hashed_password}, password)
       when is_binary(hashed_password) and byte_size(password) > 0 do
     Bcrypt.verify_pass(password, hashed_password)
   end
@@ -181,7 +180,7 @@ defmodule Memex.Accounts.User do
   @doc """
   Validates the current password otherwise adds an error to the changeset.
   """
-  @spec validate_current_password(Changeset.t(t()), String.t()) :: Changeset.t(t())
+  @spec validate_current_password(changeset(), String.t()) :: changeset()
   def validate_current_password(changeset, password) do
     if valid_password?(changeset.data, password),
       do: changeset,
@@ -191,7 +190,7 @@ defmodule Memex.Accounts.User do
   @doc """
   A changeset for changing the user's locale
   """
-  @spec locale_changeset(t() | Changeset.t(t()), locale :: String.t() | nil) :: Changeset.t(t())
+  @spec locale_changeset(t() | changeset(), locale :: String.t() | nil) :: changeset()
   def locale_changeset(user_or_changeset, locale) do
     user_or_changeset
     |> cast(%{"locale" => locale}, [:locale])
diff --git a/lib/memex_web/controllers/user_registration_controller.ex b/lib/memex_web/controllers/user_registration_controller.ex
index 0b59d89..e6fe16a 100644
--- a/lib/memex_web/controllers/user_registration_controller.ex
+++ b/lib/memex_web/controllers/user_registration_controller.ex
@@ -2,7 +2,6 @@ defmodule MemexWeb.UserRegistrationController do
   use MemexWeb, :controller
   import MemexWeb.Gettext
   alias Memex.{Accounts, Invites}
-  alias Memex.Accounts.User
   alias MemexWeb.HomeLive
 
   def new(conn, %{"invite" => invite_token}) do
@@ -30,7 +29,7 @@ defmodule MemexWeb.UserRegistrationController do
   # renders new user registration page
   defp render_new(conn, invite \\ nil) do
     render(conn, "new.html",
-      changeset: Accounts.change_user_registration(%User{}),
+      changeset: Accounts.change_user_registration(),
       invite: invite,
       page_title: gettext("register")
     )
diff --git a/priv/gettext/default.pot b/priv/gettext/default.pot
index a36bc00..3f11727 100644
--- a/priv/gettext/default.pot
+++ b/priv/gettext/default.pot
@@ -313,7 +313,7 @@ msgstr ""
 msgid "public signups"
 msgstr ""
 
-#: lib/memex_web/controllers/user_registration_controller.ex:35
+#: lib/memex_web/controllers/user_registration_controller.ex:34
 #, elixir-autogen, elixir-format
 msgid "register"
 msgstr ""
diff --git a/priv/gettext/errors.pot b/priv/gettext/errors.pot
index d31e70d..0e5a4bd 100644
--- a/priv/gettext/errors.pot
+++ b/priv/gettext/errors.pot
@@ -52,14 +52,14 @@ msgstr ""
 msgid "Reset password link is invalid or it has expired."
 msgstr ""
 
-#: lib/memex_web/controllers/user_registration_controller.ex:25
-#: lib/memex_web/controllers/user_registration_controller.ex:56
+#: lib/memex_web/controllers/user_registration_controller.ex:24
+#: lib/memex_web/controllers/user_registration_controller.ex:55
 #, elixir-autogen, elixir-format
 msgid "Sorry, public registration is disabled"
 msgstr ""
 
-#: lib/memex_web/controllers/user_registration_controller.ex:15
-#: lib/memex_web/controllers/user_registration_controller.ex:46
+#: lib/memex_web/controllers/user_registration_controller.ex:14
+#: lib/memex_web/controllers/user_registration_controller.ex:45
 #, elixir-autogen, elixir-format
 msgid "Sorry, this invite was not found or expired"
 msgstr ""
@@ -95,22 +95,22 @@ msgstr ""
 msgid "You must confirm your account and log in to access this page."
 msgstr ""
 
-#: lib/memex/accounts/user.ex:130
+#: lib/memex/accounts/user.ex:129
 #, elixir-autogen, elixir-format
 msgid "did not change"
 msgstr ""
 
-#: lib/memex/accounts/user.ex:151
+#: lib/memex/accounts/user.ex:150
 #, elixir-autogen, elixir-format
 msgid "does not match password"
 msgstr ""
 
-#: lib/memex/accounts/user.ex:188
+#: lib/memex/accounts/user.ex:187
 #, elixir-autogen, elixir-format
 msgid "is not valid"
 msgstr ""
 
-#: lib/memex/accounts/user.ex:84
+#: lib/memex/accounts/user.ex:85
 #, elixir-autogen, elixir-format
 msgid "must have the @ sign and no spaces"
 msgstr ""
diff --git a/priv/gettext/prompts.pot b/priv/gettext/prompts.pot
index 0659604..22de7ff 100644
--- a/priv/gettext/prompts.pot
+++ b/priv/gettext/prompts.pot
@@ -95,7 +95,7 @@ msgstr ""
 msgid "Password updated successfully."
 msgstr ""
 
-#: lib/memex_web/controllers/user_registration_controller.ex:74
+#: lib/memex_web/controllers/user_registration_controller.ex:73
 #, elixir-autogen, elixir-format
 msgid "Please check your email to verify your account"
 msgstr ""
diff --git a/test/memex/accounts_test.exs b/test/memex/accounts_test.exs
index 6286899..f7adfcc 100644
--- a/test/memex/accounts_test.exs
+++ b/test/memex/accounts_test.exs
@@ -104,7 +104,7 @@ defmodule Memex.AccountsTest do
 
   describe "change_user_registration/2" do
     test "returns a changeset" do
-      assert %Changeset{} = changeset = Accounts.change_user_registration(%User{})
+      assert %Changeset{} = changeset = Accounts.change_user_registration()
       assert changeset.required == [:password, :email]
     end
 
@@ -112,8 +112,7 @@ defmodule Memex.AccountsTest do
       email = unique_user_email()
       password = valid_user_password()
 
-      changeset =
-        Accounts.change_user_registration(%User{}, %{"email" => email, "password" => password})
+      changeset = Accounts.change_user_registration(%{"email" => email, "password" => password})
 
       assert changeset.valid?
       assert get_change(changeset, :email) == email
diff --git a/test/support/fixtures.ex b/test/support/fixtures.ex
index 267ef73..13b3baa 100644
--- a/test/support/fixtures.ex
+++ b/test/support/fixtures.ex
@@ -3,7 +3,7 @@ defmodule Memex.Fixtures do
   This module defines test helpers for creating entities
   """
 
-  alias Memex.{Accounts, Accounts.User, Email}
+  alias Memex.{Accounts, Accounts.User, Email, Repo}
 
   def unique_user_email, do: "user#{System.unique_integer()}@example.com"
   def valid_user_password, do: "hello world!"
@@ -26,11 +26,12 @@ defmodule Memex.Fixtures do
     attrs
     |> Enum.into(%{
       "email" => unique_user_email(),
-      "password" => valid_user_password(),
-      "role" => "admin"
+      "password" => valid_user_password()
     })
     |> Accounts.register_user()
     |> unwrap_ok_tuple()
+    |> User.role_changeset("admin")
+    |> Repo.update!()
   end
 
   def extract_user_token(fun) do