Commit 448b580c authored by Mikko Ahlroth's avatar Mikko Ahlroth

Refactor preferences page

* Added tests
* Use changesets properly
* Keep URL when submitting
* Use proper tags in HTML
* Style error help texts nicely
parent 17f40861
......@@ -14,7 +14,7 @@ import terms_page from './terms_page';
const ROUTES = [
[/^\/users\/[^/]+\/?$/, profile_page],
[/^\/my\/machines\/?$/, machine_page],
[/^\/my\/preferences\/?$/, preferences_page],
[/^\/my\/preferences\/?/, preferences_page],
[/^\/my\/consent\/?$/, terms_page],
[/^\/?$/, index_page]
];
......
......@@ -40,10 +40,9 @@ defmodule CodeStats.User do
|> validate_required([:username, :password, :terms_version])
|> put_change(:private_profile, false)
|> validate_length(:username, min: 1, max: 64)
|> validate_length(:email, min: 1, max: 255)
|> validate_length(:password, min: 6, max: 255)
|> validate_format(:username, ~r/^[^\/#%?&=+]+$/)
|> validate_latest_terms()
|> password_validations()
|> common_validations()
|> unique_constraint(:username)
|> unique_constraint(:lower_username)
......@@ -66,6 +65,7 @@ defmodule CodeStats.User do
data
|> cast(params, [:password])
|> validate_required([:password])
|> password_validations()
|> update_change(:password, &hash_password/1)
end
......@@ -315,7 +315,13 @@ defmodule CodeStats.User do
# Common validations for creating and editing users
defp common_validations(changeset) do
changeset
|> validate_format(:email, ~r/^$|@/)
|> validate_length(:email, min: 3, max: 255)
|> validate_format(:email, ~r/^$|^.+@.+$/)
end
defp password_validations(changeset) do
changeset
|> validate_length(:password, min: 6, max: 255)
end
# Validate that the accepted terms version is the latest one, user cannot accept older
......
......@@ -4,8 +4,11 @@ defmodule CodeStatsWeb.PreferencesController do
alias CodeStats.User
alias CodeStatsWeb.AuthUtils
require Logger
plug(:set_title)
@spec edit(Plug.Conn.t(), map()) :: Plug.Conn.t()
def edit(conn, _params) do
changeset = User.updating_changeset(AuthUtils.get_current_user(conn))
......@@ -14,7 +17,16 @@ defmodule CodeStatsWeb.PreferencesController do
|> render("preferences.html", changeset: changeset)
end
def do_edit(conn, %{"user" => user}) do
@doc """
Common action for all editing calls: currently preferences and passwords.
The body is chosen by the given hidden field.
"""
@spec do_edit(Plug.Conn.t(), map()) :: Plug.Conn.t()
def do_edit(conn, params)
# Edit case for editing user's information (w/o password)
def do_edit(conn, %{"user" => %{"type" => "edit"} = user}) do
changeset = User.updating_changeset(AuthUtils.get_current_user(conn), user)
case AuthUtils.update_user(changeset) do
......@@ -27,47 +39,73 @@ defmodule CodeStatsWeb.PreferencesController do
conn
|> common_edit_assigns()
|> put_flash(:error, "Error updating preferences.")
|> render("preferences.html", error_changeset: error_changeset)
|> render("preferences.html", changeset: error_changeset)
end
end
def change_password(conn, %{"old_password" => old_password, "new_password" => new_password}) do
# Edit case for editing user's password
def do_edit(conn, %{
"user" =>
%{
"type" => "password",
"old_password" => old_password,
"password" => _
} = params
}) do
user = AuthUtils.get_current_user(conn)
password_changeset = User.password_changeset(user, params)
if AuthUtils.check_user_password(user, old_password) do
password_changeset = User.password_changeset(user, %{password: new_password})
case AuthUtils.update_user(password_changeset) do
%User{} ->
conn
|> put_flash(:success, "Password changed.")
|> redirect(to: Routes.preferences_path(conn, :edit))
%Ecto.Changeset{} ->
conn
|> put_flash(:error, "Error changing password.")
|> redirect(to: Routes.preferences_path(conn, :edit))
end
else
with {:old_pass, true} <- {:old_pass, AuthUtils.check_user_password(user, old_password)},
{:updated, %User{}} <- {:updated, AuthUtils.update_user(password_changeset)} do
conn
|> put_flash(:error, "Old password was wrong!")
|> put_flash(:success, "Password changed.")
|> redirect(to: Routes.preferences_path(conn, :edit))
else
err ->
error_changeset =
case err do
{:old_pass, false} ->
# We need to add an action to the changeset so that Phoenix will display the error,
# otherwise it will think the changeset was not processed (as it has not been passed
# to any Repo call) and will not show the errors
%{password_changeset | action: :update}
|> Ecto.Changeset.add_error(:old_password, "does not match your current password")
{:updated, cset} ->
cset
end
conn
|> common_edit_assigns()
|> put_flash(:error, "Error changing password.")
|> render("preferences.html", pass_changeset: error_changeset)
end
end
# Edit case for any other data, just return an error.
def do_edit(conn, _params) do
conn
|> common_edit_assigns()
|> put_flash(:error, "Unknown error in preferences.")
|> render("preferences.html")
end
@doc """
Deletes user's account.
Action for deleting user and all their information from system.
"""
@spec delete(Plug.Conn.t(), map) :: Plug.Conn.t()
@spec delete(Plug.Conn.t(), map()) :: Plug.Conn.t()
def delete(conn, params) do
AuthUtils.delete_user_action(conn, params, {&Routes.preferences_path/2, :edit})
end
# Common edit assigns, including empty changesets that will be overridden in specific clauses
defp common_edit_assigns(conn) do
user_data = AuthUtils.get_current_user(conn)
conn
|> assign(:user, user_data)
|> assign(:changeset, User.updating_changeset(AuthUtils.get_current_user(conn)))
|> assign(:pass_changeset, User.password_changeset(AuthUtils.get_current_user(conn)))
end
defp set_title(conn, _opts) do
......
......@@ -81,9 +81,7 @@ defmodule CodeStatsWeb.Router do
get("/preferences", PreferencesController, :edit)
put("/preferences", PreferencesController, :do_edit)
post("/password", PreferencesController, :change_password)
post("/sound_of_inevitability", PreferencesController, :delete)
delete("/preferences", PreferencesController, :delete)
get("/machines", MachineController, :list)
post("/machines", MachineController, :add)
......
......@@ -8,18 +8,18 @@
<h2>User details</h2>
<%= form_for(@changeset, Routes.preferences_path(@conn, :do_edit), fn f -> %>
<label for="user_email" class="<%= if f.errors[:email], do: "has-error" %>">Email address</label>
<%= hidden_input(f, :type, value: "edit", id: "edit-type") %>
<%= label(f, :email, "Email address") %>
<div class="input">
<%= email_input(f, :email) %>
<%= if message = f.errors[:email] do %>
<p class="help-block has-error"><%= message %></p>
<% end %>
<%= email_input(f, :email, minlength: 3, maxlength: 255) %>
<%= error_tag(f, :email) %>
<p class="help-block">
Email is only used for important notifications about the service, not for promotions or spam.
</p>
</div>
<label for="user_private_profile">Private profile</label>
<%= label(f, :private_profile, "Private profile") %>
<div class="input">
<%= checkbox(f, :private_profile) %>
......@@ -35,25 +35,26 @@
<div id="change-password" class="stripe">
<h2>Change password</h2>
<%= form_tag(Routes.preferences_path(@conn, :change_password)) %>
<label for="old_password" class="<%= if assigns[:old_password_error], do: "has-error" %>">Old password (required)</label>
<%= form_for(@pass_changeset, Routes.preferences_path(@conn, :do_edit), fn f -> %>
<%= hidden_input(f, :type, value: "password", id: "password-type") %>
<%= label(f, :old_password, "Current password (required)") %>
<div class="input">
<input type="password" id="old_password" name="old_password" required />
<%= if assigns[:old_password_error] do %>
<p class="help-block has-error"><%= assigns[:old_password_error] %></p>
<% end %>
<%= password_input(f, :old_password, autocomplete: "current-password", required: true) %>
<%= error_tag(f, :old_password) %>
<p class="help-block">
For security, to change your password, you need to also give your current password.
</p>
</div>
<label for="new_password" class="<%= if assigns[:new_password_error], do: "has-error" %>">New password (required)</label>
<div class="input">
<input type="password" id="new_password" name="new_password" class="form-control" required />
<%= if assigns[:new_password_error] do %>
<p class="help-block has-error"><%= assigns[:new_password_error] %></p>
<% end %>
</div>
<%= label(f, :password, "New password (required)") %>
<div class="input">
<%= password_input(f, :password, autocomplete: "new-password", required: true, minlength: 6) %>
<%= error_tag(f, :password) %>
</div>
<div class="submit"><button type="submit">Change password</button></div>
</form>
<% end) %>
</div>
<div id="export-data">
......@@ -87,7 +88,7 @@
the delete button. If you do this, all your data will be removed and cannot be recovered.
</p>
<%= form_tag(Routes.preferences_path(@conn, :delete)) %>
<%= form_tag(Routes.preferences_path(@conn, :do_edit), method: :delete) %>
<label><strong>This cannot be undone!</strong></label>
<input type="text" class="form-control" name="delete_confirmation" maxlength="6" required />
......
......@@ -8,9 +8,10 @@ defmodule CodeStatsWeb.ErrorHelpers do
@doc """
Generates tag for inlined form input errors.
"""
@spec error_tag(Phoenix.HTML.FormData.t(), atom()) :: Phoenix.HTML.Safe.t()
def error_tag(form, field) do
if error = form.errors[field] do
content_tag(:span, translate_error(error), class: "help-block")
content_tag(:p, translate_error(error), class: "help-block has-error")
end
end
......
defmodule CodeStatsWeb.PreferencesControllerTests do
use CodeStatsWeb.ConnCase
alias CodeStats.UserHelpers
alias CodeStats.{User, Repo}
setup context do
{:ok, user} = UserHelpers.create_user("foo@bar", "foo", "abcdef")
Map.put(context, :user, user)
end
test "unauthed user cannot view preferences", %{conn: conn} do
conn = get(conn, "/my/preferences")
assert is_binary(html_response(conn, 403))
end
test "authed user can view preferences", %{conn: conn, user: user} do
conn = conn |> UserHelpers.force_login(user) |> get("/my/preferences")
assert is_binary(html_response(conn, 200))
end
test "email edit", %{conn: conn, user: user} do
conn =
conn
|> UserHelpers.force_login(user)
|> put("/my/preferences", %{user: %{type: "edit", email: "bar@baz"}})
assert redirected_to(conn) == "/my/preferences"
user = from(u in User, where: u.id == ^user.id) |> Repo.one!()
assert user.email == "bar@baz"
end
test "removal of email", %{conn: conn, user: user} do
conn =
conn
|> UserHelpers.force_login(user)
|> put("/my/preferences", %{user: %{type: "edit", email: ""}})
assert redirected_to(conn) == "/my/preferences"
user = from(u in User, where: u.id == ^user.id) |> Repo.one!()
assert is_nil(user.email)
end
test "private profile edit", %{conn: conn, user: user} do
conn =
conn
|> UserHelpers.force_login(user)
|> put("/my/preferences", %{
user: %{type: "edit", email: user.email, private_profile: "true"}
})
assert redirected_to(conn) == "/my/preferences"
user = from(u in User, where: u.id == ^user.id) |> Repo.one!()
assert user.private_profile
end
test "invalid email", %{conn: conn, user: user} do
conn =
conn
|> UserHelpers.force_login(user)
|> put("/my/preferences", %{
user: %{type: "edit", email: "bar"}
})
assert html_response(conn, 200) =~ "Error updating preferences"
end
test "password change", %{conn: conn, user: user} do
conn =
conn
|> UserHelpers.force_login(user)
|> put("/my/preferences", %{
user: %{type: "password", old_password: "abcdef", password: "abcdef"}
})
assert redirected_to(conn) == "/my/preferences"
new_user = from(u in User, where: u.id == ^user.id) |> Repo.one!()
assert new_user.password != user.password
end
test "too short password", %{conn: conn, user: user} do
conn =
conn
|> UserHelpers.force_login(user)
|> put("/my/preferences", %{
user: %{type: "password", old_password: "abcdef", password: "abcde"}
})
assert html_response(conn, 200) =~ "should be at least 6 character(s)"
new_user = from(u in User, where: u.id == ^user.id) |> Repo.one!()
assert new_user.password == user.password
end
test "old password wrong", %{conn: conn, user: user} do
conn =
conn
|> UserHelpers.force_login(user)
|> put("/my/preferences", %{
user: %{type: "password", old_password: "def", password: "abcdef"}
})
assert html_response(conn, 200) =~ "does not match"
new_user = from(u in User, where: u.id == ^user.id) |> Repo.one!()
assert new_user.password == user.password
end
test "invalid edit", %{conn: conn, user: user} do
conn =
conn
|> UserHelpers.force_login(user)
|> put("/my/preferences", %{foo: "bar"})
assert html_response(conn, 200) =~ "Unknown error in preferences"
end
test "delete user", %{conn: conn, user: user} do
conn =
conn
|> UserHelpers.force_login(user)
|> delete("/my/preferences", %{delete_confirmation: "DELETE"})
assert redirected_to(conn) == "/"
# This is dirty, delete is run in background task so wait for that to complete
Process.sleep(1_000)
deleted_user = from(u in User, where: u.id == ^user.id) |> Repo.one()
assert is_nil(deleted_user)
end
test "wrong input for delete", %{conn: conn, user: user} do
conn =
conn
|> UserHelpers.force_login(user)
|> delete("/my/preferences", %{delete_confirmation: "dElEtE"})
assert redirected_to(conn) == "/my/preferences"
end
end
......@@ -15,4 +15,8 @@ defmodule CodeStats.UserHelpers do
})
|> Repo.insert()
end
def force_login(conn, user) do
Plug.Test.init_test_session(conn, %{CodeStatsWeb.AuthUtils.auth_key() => user.id})
end
end
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment