Improve User Creation Form (#1111)

* Attempt to validate when input loses focus * Attempt to render errors one after another * user new inputs for better harmonised form components * Identify error message with data-tag attribute * Shift error message position * Update CHANGELOG.md * Use gettext to translate errors for frontend * Prevent form from jumping on and off when error appears, validate only on blur * Remove unused Accounts.change_user_details function * Update errors.po * Remove unused function from migration
OpenFn · Sep 21, 2023 · fb2011a · fb2011a
1 parent b10383e
commit fb2011a
Show file tree

Hide file tree

Showing 67 changed files with 559 additions and 635 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -14,6 +14,9 @@ and this project adheres to
 
 ### Fixed
 
+- Creating a new user without a password fails and there is no user feedback
+  [#731](https://github.com/OpenFn/Lightning/issues/731)
+
 ## [v0.9.2] - 2023-09-20
 
 ### Added
@@ -37,8 +40,6 @@ and this project adheres to
 - Modified audit trail to handle lots of different kind of audit events
   [#271](https://github.com/OpenFn/Lightning/issues/271)/[#44](https://github.com/OpenFn/Lightning/issues/44)
 
-### Fixed
-
 - Fix randomly unresponsive job panel after job deletion
   [#1113](https://github.com/OpenFn/Lightning/issues/1113)
 

diff --git a/lib/lightning/accounts.ex b/lib/lightning/accounts.ex
@@ -87,6 +87,12 @@ defmodule Lightning.Accounts do
     {:ok, %{users_deleted: users_to_delete}}
   end
 
+  def create_user(attrs) do
+    %User{}
+    |> User.changeset(attrs)
+    |> Repo.insert()
+  end
+
   @doc """
   Returns the list of users.
 
@@ -373,10 +379,6 @@ defmodule Lightning.Accounts do
     User.user_registration_changeset(attrs, hash_password: false)
   end
 
-  def change_user_details(%User{} = user, attrs \\ %{}) do
-    User.details_changeset(user, attrs)
-  end
-
   def update_user_details(%User{} = user, attrs \\ %{}) do
     User.details_changeset(user, attrs)
     |> Repo.update()
@@ -517,10 +519,10 @@ defmodule Lightning.Accounts do
     |> Ecto.Changeset.validate_change(:email, fn :email, email ->
       cond do
         user.email == email ->
-          [email: "Please change your email"]
+          [email: "has not changed"]
 
         Lightning.Repo.exists?(User |> where(email: ^email)) ->
-          [email: "Email already exists"]
+          [email: "has already been taken"]
 
         true ->
           []
@@ -531,7 +533,7 @@ defmodule Lightning.Accounts do
       if Bcrypt.verify_pass(password, user.hashed_password) do
         []
       else
-        [current_password: "Password does not match"]
+        [current_password: "does not match password"]
       end
     end)
   end
@@ -565,6 +567,10 @@ defmodule Lightning.Accounts do
     User.password_changeset(user, attrs, hash_password: false)
   end
 
+  def change_user(user, attrs) do
+    User.changeset(user, attrs)
+  end
+
   @doc """
   Updates the user password.
 

diff --git a/lib/lightning/accounts/user.ex b/lib/lightning/accounts/user.ex
@@ -43,6 +43,19 @@ defmodule Lightning.Accounts.User do
     timestamps()
   end
 
+  def changeset(user, attrs) do
+    user
+    |> cast(attrs, [
+      :first_name,
+      :last_name,
+      :email,
+      :password
+    ])
+    |> validate_name()
+    |> validate_email()
+    |> validate_password([])
+  end
+
   @common_registration_attrs %{
     first_name: :string,
     last_name: :string,
@@ -127,7 +140,7 @@ defmodule Lightning.Accounts.User do
 
   defp validate_email(changeset) do
     changeset
-    |> validate_required([:email, :first_name])
+    |> validate_required(:email, message: "can't be blank")
     |> validate_format(:email, ~r/^[^\s]+@[^\s]+$/,
       message: "must have the @ sign and no spaces"
     )
@@ -143,14 +156,14 @@ defmodule Lightning.Accounts.User do
 
   defp validate_password(changeset, opts) do
     changeset
-    |> validate_required([:password])
+    |> validate_required(:password, message: "can't be blank")
     |> validate_length(:password, min: 8, max: 72)
     |> maybe_hash_password(opts)
   end
 
   defp validate_name(changeset) do
     changeset
-    |> validate_required([:first_name, :last_name])
+    |> validate_required([:first_name, :last_name], message: "can't be blank")
   end
 
   defp validate_role(changeset) do

diff --git a/lib/lightning/projects/project_credential.ex b/lib/lightning/projects/project_credential.ex
@@ -34,7 +34,7 @@ defmodule Lightning.Projects.ProjectCredential do
     |> cast(attrs, [:credential_id, :project_id])
     |> validate_required([:project_id])
     |> unique_constraint([:project_id, :credential_id],
-      message: "Credential already added to this project."
+      message: "credential already added to this project."
     )
   end
 end
diff --git a/lib/lightning/projects/project_user.ex b/lib/lightning/projects/project_user.ex
@@ -53,7 +53,7 @@ defmodule Lightning.Projects.ProjectUser do
     |> cast(attrs, [:user_id, :project_id, :role, :digest, :failure_alert])
     |> validate_required([:user_id])
     |> unique_constraint([:project_id, :user_id],
-      message: "User already a member of this project."
+      message: "user already a member of this project."
     )
   end
 end
diff --git a/lib/lightning/workflows/workflow.ex b/lib/lightning/workflows/workflow.ex
@@ -57,7 +57,7 @@ defmodule Lightning.Workflows.Workflow do
     |> assoc_constraint(:project)
     |> validate_required([:name])
     |> unique_constraint([:name, :project_id],
-      message: "A workflow with this name already exists in this project."
+      message: "a workflow with this name already exists in this project."
     )
   end
 

diff --git a/lib/lightning_web.ex b/lib/lightning_web.ex
@@ -40,6 +40,7 @@ defmodule LightningWeb do
       import Plug.Conn
       import LightningWeb.Gettext
       import LightningWeb.UserAuth, only: [fetch_current_user: 2]
+      import LightningWeb.Components.NewInputs
       alias LightningWeb.Router.Helpers, as: Routes
 
       unquote(verified_routes())
@@ -77,6 +78,7 @@ defmodule LightningWeb do
   def live_component do
     quote do
       use Phoenix.LiveComponent
+      import LightningWeb.Components.NewInputs
 
       unquote(html_helpers())
     end
@@ -85,6 +87,7 @@ defmodule LightningWeb do
   def component do
     quote do
       use Phoenix.Component
+      import LightningWeb.Components.NewInputs
 
       unquote(html_helpers())
     end
@@ -100,10 +103,9 @@ defmodule LightningWeb do
   defp html_helpers do
     quote do
       # Use all HTML functionality (forms, tags, etc)
-      use Phoenix.HTML
+      import Phoenix.HTML
 
       # Import LiveView and .heex helpers (live_render, live_patch, <.form>, etc)
-      use Phoenix.Component
       import LightningWeb.LiveHelpers
       import LightningWeb.CoreComponents
       alias LightningWeb.LayoutComponents
@@ -141,6 +143,8 @@ defmodule LightningWeb do
       import Phoenix.Controller,
         only: [get_csrf_token: 0, view_module: 1, view_template: 1]
 
+      import LightningWeb.Components.NewInputs
+
       # Include general helpers for rendering HTML
       unquote(html_helpers())
     end

diff --git a/lib/lightning_web/components/core_components.ex b/lib/lightning_web/components/core_components.ex
@@ -5,48 +5,27 @@ defmodule LightningWeb.CoreComponents do
 
   # TODO: Remove `Phoenix.HTML` and `error_tag` once we are in
   # a better position to conform the more recent Phoenix conventions.
-  use Phoenix.HTML
+  # use Phoenix.HTML
 
   alias Phoenix.LiveView.JS
 
+  import LightningWeb.Components.NewInputs
+
   @doc """
   Generates tag for inlined form input errors.
   """
-  def error_tag(form, field), do: error_tag(form, field, [])
-
-  def error_tag(form, field, attrs) when is_list(attrs) do
-    Enum.map(Keyword.get_values(form.errors, field), fn error ->
-      content_tag(
-        :span,
-        translate_error(error),
-        Keyword.merge(
-          [phx_feedback_for: Phoenix.HTML.Form.input_name(form, field)],
-          attrs
-        )
-      )
-    end)
-  end
 
-  def translate_error({msg, opts}) do
-    # You can make use of gettext to translate error messages by
-    # uncommenting and adjusting the following code:
+  attr :field, Phoenix.HTML.FormField,
+    doc:
+      "a form field struct retrieved from the form, for example: @form[:email]"
 
-    # if count = opts[:count] do
-    #   Gettext.dngettext(LightningWeb.Gettext, "errors", msg, msg, count, opts)
-    # else
-    #   Gettext.dgettext(LightningWeb.Gettext, "errors", msg, opts)
-    # end
+  def old_error(%{field: field} = assigns) do
+    assigns =
+      assigns |> assign(:errors, Enum.map(field.errors, &translate_error(&1)))
 
-    Enum.reduce(opts, msg, fn {key, value}, acc ->
-      String.replace(acc, "%{#{key}}", fn _ -> to_string(value) end)
-    end)
-  end
-
-  @doc """
-  Translates the errors for a field from a keyword list of errors.
-  """
-  def translate_errors(errors, field) when is_list(errors) do
-    for {^field, {msg, opts}} <- errors, do: translate_error({msg, opts})
+    ~H"""
+    <.error :for={msg <- @errors}><%= msg %></.error>
+    """
   end
 
   def show_modal(js \\ %JS{}, id) when is_binary(id) do

diff --git a/lib/lightning_web/components/layouts/_user_menu.html.heex b/lib/lightning_web/components/layouts/_user_menu.html.heex
@@ -1,11 +1,21 @@
 <ul>
   <%= if @current_user do %>
     <li><%= @current_user.email %></li>
-    <li><%= link("Settings", to: Routes.profile_edit_path(@conn, :edit)) %></li>
+    <li>
+      <.link href={Routes.profile_edit_path(@conn, :edit)}>
+        Settings
+      </.link>
+    </li>
   <% else %>
     <li>
-      <%= link("Register", to: Routes.user_registration_path(@conn, :new)) %>
+      <.link href={Routes.user_registration_path(@conn, :new)}>
+        Register
+      </.link>
+    </li>
+    <li>
+      <.link href={Routes.user_session_path(@conn, :new)}>
+        Log in
+      </.link>
     </li>
-    <li><%= link("Log in", to: Routes.user_session_path(@conn, :new)) %></li>
   <% end %>
 </ul>
diff --git a/lib/lightning_web/components/new_inputs.ex b/lib/lightning_web/components/new_inputs.ex
@@ -14,32 +14,51 @@ defmodule LightningWeb.Components.NewInputs do
       <.button>Send!</.button>
       <.button phx-click="go" class="ml-2">Send!</.button>
   """
+  attr :id, :string, default: "no-id"
   attr :type, :string, default: nil
   attr :class, :string, default: nil
   attr :rest, :global, include: ~w(disabled form name value)
+  attr :tooltip, :any, default: nil
 
   slot :inner_block, required: true
 
   def button(assigns) do
+    assigns = tooltip_when_disabled(assigns)
+
     ~H"""
-    <button
-      type={@type}
-      class={[
-        "inline-flex justify-center py-2 px-4 border border-transparent
+    <span {@span_attrs}>
+      <button
+        type={@type}
+        class={[
+          "inline-flex justify-center py-2 px-4 border border-transparent
       shadow-sm text-sm font-medium rounded-md text-white focus:outline-none
       focus:ring-2 focus:ring-offset-2 focus:ring-primary-500",
-        "bg-primary-600 hover:bg-primary-700",
-        "disabled:bg-primary-300",
-        "phx-submit-loading:opacity-75 ",
-        @class
-      ]}
-      {@rest}
-    >
-      <%= render_slot(@inner_block) %>
-    </button>
+          "bg-primary-600 hover:bg-primary-700",
+          "disabled:bg-primary-300",
+          "phx-submit-loading:opacity-75 ",
+          @class
+        ]}
+        {@rest}
+      >
+        <%= render_slot(@inner_block) %>
+      </button>
+    </span>
     """
   end
 
+  defp tooltip_when_disabled(assigns) do
+    with true <- Map.get(assigns.rest, :disabled, false),
+         tooltip when not is_nil(tooltip) <- Map.get(assigns, :tooltip) do
+      assign(assigns, :span_attrs, %{
+        "id" => assigns.id,
+        "phx-hook" => "Tooltip",
+        "aria-label" => tooltip
+      })
+    else
+      _ -> assign(assigns, :span_attrs, %{})
+    end
+  end
+
   @doc """
   Renders an input with label and error messages.
 
@@ -194,6 +213,7 @@ defmodule LightningWeb.Components.NewInputs do
         name={@name}
         id={@id}
         value={Phoenix.HTML.Form.normalize_value(@type, @value)}
+        phx-debounce="blur"
         class={[
           "mt-2 block w-full rounded-lg text-slate-900 focus:ring-0 sm:text-sm sm:leading-6",
           "phx-no-feedback:border-slate-300 phx-no-feedback:focus:border-slate-400",
@@ -202,15 +222,17 @@ defmodule LightningWeb.Components.NewInputs do
         ]}
         {@rest}
       />
-      <.error :for={msg <- @errors}><%= msg %></.error>
+      <div class="error-space h-6">
+        <.error :for={msg <- @errors}><%= msg %></.error>
+      </div>
     </div>
     """
   end
 
   @doc """
   Renders a label.
   """
-  attr :for, :string, default: nil
+  attr :for, :any, default: nil
   slot :inner_block, required: true
 
   def label(assigns) do
@@ -228,7 +250,10 @@ defmodule LightningWeb.Components.NewInputs do
 
   def error(assigns) do
     ~H"""
-    <p class="inline-flex items-center gap-x-1.5 text-xs text-danger-600 phx-no-feedback:hidden">
+    <p
+      data-tag="error_message"
+      class="mt-3 inline-flex items-center gap-x-1.5 text-xs text-danger-600 phx-no-feedback:hidden"
+    >
       <.icon name="hero-exclamation-circle" class="h-4 w-4" />
       <%= render_slot(@inner_block) %>
     </p>
@@ -282,4 +307,11 @@ defmodule LightningWeb.Components.NewInputs do
       Gettext.dgettext(LightningWeb.Gettext, "errors", msg, opts)
     end
   end
+
+  @doc """
+  Translates the errors for a field from a keyword list of errors.
+  """
+  def translate_errors(errors, field) when is_list(errors) do
+    for {^field, {msg, opts}} <- errors, do: translate_error({msg, opts})
+  end
 end