Recoverable forms for Support Scripts and Device settings (#2379)

Now, if your live socket connection dies, you won't lose your form
progress.

After making some recommended changes to `core_components.ex`, I also
uncovered an issue with Deployment Group conditions, which was the
reason for the larger set of changes related to switching the conditions
from a map to an embedded schema.

While it would have been nice to separate these changes, you will notice
the two CI failures until this was addressed.

I have a bigger change in the works for improving Deployment Group
params, but first I want to get this merged in.

Author: joshk

lib/nerves_hub/devices.ex

@@ -857,7 +857,7 @@ defmodule NervesHub.Devices do
   """
   def matches_deployment_group?(
         %Device{tags: tags, firmware_metadata: %FirmwareMetadata{version: version}},
-        %DeploymentGroup{conditions: %{"version" => requirement, "tags" => dep_tags}}
+        %DeploymentGroup{conditions: %{version: requirement, tags: dep_tags}}
       ) do
     if version_match?(version, requirement) and tags_match?(tags, dep_tags) do
       true

lib/nerves_hub/managed_deployments.ex

@@ -368,9 +368,9 @@ defmodule NervesHub.ManagedDeployments do
 
   # Check that a device version matches for a deployment's conditions
   # A deployment not having a version condition returns true
-  defp version_match?(_device, %{conditions: %{"version" => ""}}), do: true
+  defp version_match?(_device, %{conditions: %{version: ""}}), do: true
 
-  defp version_match?(device, %{conditions: %{"version" => version}}) when not is_nil(version) do
+  defp version_match?(device, %{conditions: %{version: version}}) when not is_nil(version) do
     Version.match?(device.firmware_metadata.version, version)
   end
 
@@ -437,11 +437,11 @@ defmodule NervesHub.ManagedDeployments do
       |> Repo.one()
 
     bad_version =
-      if deployment_group.conditions["version"] == "" do
+      if deployment_group.conditions.version == "" do
         false
       else
         try do
-          !Version.match?(device_version, deployment_group.conditions["version"])
+          !Version.match?(device_version, deployment_group.conditions.version)
         rescue
           _ ->
             true
@@ -642,7 +642,7 @@ defmodule NervesHub.ManagedDeployments do
   end
 
   # no tags, but version
-  defp do_matched_devices(%DeploymentGroup{conditions: %{"tags" => [], "version" => version}}, query, work_type)
+  defp do_matched_devices(%DeploymentGroup{conditions: %{tags: [], version: version}}, query, work_type)
        when version != "" do
     case work_type do
       :count ->
@@ -661,7 +661,7 @@ defmodule NervesHub.ManagedDeployments do
   end
 
   # tags but no version
-  defp do_matched_devices(%DeploymentGroup{conditions: %{"tags" => tags, "version" => ""}}, query, work_type) do
+  defp do_matched_devices(%DeploymentGroup{conditions: %{tags: tags, version: ""}}, query, work_type) do
     query = where(query, [d], fragment("?::text[] && tags::text[]", ^tags))
 
     case work_type do
@@ -676,7 +676,7 @@ defmodule NervesHub.ManagedDeployments do
   end
 
   # version and tags
-  defp do_matched_devices(%DeploymentGroup{conditions: %{"tags" => tags, "version" => version}}, query, work_type) do
+  defp do_matched_devices(%DeploymentGroup{conditions: %{tags: tags, version: version}}, query, work_type) do
     query = where(query, [d], fragment("?::text[] && tags::text[]", ^tags))
 
     case work_type do

lib/nerves_hub/managed_deployments/deployment_group.ex

@@ -21,7 +21,6 @@ defmodule NervesHub.ManagedDeployments.DeploymentGroup do
     :org_id,
     :firmware_id,
     :name,
-    :conditions,
     :is_active,
     :product_id,
     :concurrent_updates,
@@ -56,7 +55,11 @@ defmodule NervesHub.ManagedDeployments.DeploymentGroup do
     has_many(:deployment_releases, DeploymentRelease)
     has_many(:update_stats, UpdateStat, on_delete: :nilify_all, foreign_key: :deployment_id)
 
-    field(:conditions, :map)
+    embeds_one :conditions, __MODULE__.Conditions, primary_key: false, on_replace: :update do
+      field(:version, :string, default: "")
+      field(:tags, NervesHub.Types.Tag, default: [])
+    end
+
     field(:device_failure_threshold, :integer, default: 3)
     field(:device_failure_rate_seconds, :integer, default: 180)
     field(:device_failure_rate_amount, :integer, default: 5)
@@ -148,47 +151,39 @@ defmodule NervesHub.ManagedDeployments.DeploymentGroup do
     |> cast(params, @required_fields ++ @optional_fields)
     |> validate_required(@required_fields)
     |> unique_constraint(:name, name: :deployments_product_id_name_index)
-    |> validate_conditions()
+    |> cast_embed(:conditions, required: true, with: &conditions_changeset/2)
   end
 
-  defp validate_conditions(changeset) do
-    validate_change(changeset, :conditions, fn
-      :conditions, conditions when conditions == %{} ->
-        [conditions: "can't be blank"]
-
-      :conditions, %{"version" => nil} ->
-        [version: "can't be blank"]
-
-      :conditions, conditions ->
-        types = %{tags: {:array, :string}, version: :string}
-        # merge the new conditions with the existing ones so that we can
-        # update tags and version independently
-        conditions = Map.merge(changeset.data.conditions || %{}, conditions)
-
-        changeset =
-          {%{}, types}
-          # allow "" as valid value for `version`
-          |> cast(conditions, Map.keys(types), empty_values: [nil])
-          |> validate_required([:tags])
-          |> validate_change(
-            :version,
-            fn
-              :version, "" ->
-                []
-
-              :version, nil ->
-                [version: "can't be nil"]
-
-              :version, version ->
-                if Version.parse_requirement(version) == :error do
-                  [version: "must be valid Elixir version requirement string"]
-                else
-                  []
-                end
-            end
-          )
-
-        changeset.errors
+  def conditions_changeset(conditions, attrs) do
+    conditions
+    |> cast(attrs, [:tags, :version], empty_values: [""])
+    |> then(fn changeset ->
+      if Map.has_key?(changeset.changes, :version) && is_nil(changeset.changes.version) do
+        put_change(changeset, :version, "")
+      else
+        changeset
+      end
     end)
+    |> then(fn changeset ->
+      if Map.has_key?(changeset.changes, :tags) && is_nil(changeset.changes.tags) do
+        put_change(changeset, :tags, [])
+      else
+        changeset
+      end
+    end)
+    |> validate_change(
+      :version,
+      fn
+        :version, "" ->
+          []
+
+        :version, version ->
+          if Version.parse_requirement(version) == :error do
+            [version: "must be valid Elixir version requirement string"]
+          else
+            []
+          end
+      end
+    )
   end
 end

lib/nerves_hub/scripts.ex

@@ -48,8 +48,7 @@ defmodule NervesHub.Scripts do
 
   @spec create(Product.t(), User.t(), map()) :: {:ok, Script.t()} | {:error, Changeset.t()}
   def create(product, user, params) do
-    %Script{}
-    |> Script.create_changeset(product, user, params)
+    Script.create_changeset(product, user, params)
     |> Repo.insert()
     |> case do
       {:ok, script} ->

lib/nerves_hub/scripts/script.ex

@@ -23,22 +23,25 @@ defmodule NervesHub.Scripts.Script do
     timestamps()
   end
 
-  def create_changeset(%__MODULE__{} = struct, product, created_by, params) do
+  def validate_changeset(struct \\ %__MODULE__{}, params) do
     struct
     |> cast(params, @required ++ @optional)
+    |> validate_required(@required)
+    |> validate_length(:name, lte: 255)
+  end
+
+  def create_changeset(product, created_by, params) do
+    validate_changeset(params)
     |> put_assoc(:product, product)
+    |> foreign_key_constraint(:product_id)
     |> put_assoc(:created_by, created_by)
-    |> validate_required(@required ++ [:created_by])
-    |> validate_length(:name, lte: 255)
     |> foreign_key_constraint(:created_by_id)
   end
 
   def update_changeset(%__MODULE__{} = struct, edited_by, params \\ %{}) do
     struct
-    |> cast(params, @required ++ @optional)
+    |> validate_changeset(params)
     |> put_change(:last_updated_by_id, edited_by.id)
-    |> validate_required(@required)
-    |> validate_length(:name, lte: 255)
     |> foreign_key_constraint(:last_updated_by_id)
   end
 end

lib/nerves_hub_web/components/core_components.ex

@@ -347,9 +347,11 @@ defmodule NervesHubWeb.CoreComponents do
   slot(:rich_hint)
 
   def input(%{field: %Phoenix.HTML.FormField{} = field} = assigns) do
+    errors = if Phoenix.Component.used_input?(field), do: field.errors, else: []
+
     assigns
     |> assign(field: nil, id: assigns.id || field.id)
-    |> assign(:errors, Enum.map(field.errors, &translate_error/1))
+    |> assign(:errors, Enum.map(errors, &translate_error/1))
     |> assign_new(:name, fn -> if assigns.multiple, do: field.name <> "[]", else: field.name end)
     |> assign_new(:value, fn -> field.value end)
     |> input()

lib/nerves_hub_web/components/deployment_group_page/settings.ex

@@ -92,12 +92,15 @@ defmodule NervesHubWeb.Components.DeploymentGroupPage.Settings do
                 The matching is undertaken when a device connects to the platform.
               </p>
             </div>
-            <div class="w-1/2">
-              <.input field={@form[:tags]} value={tags_to_string(@form[:conditions])} label="Tag(s) distributed to" placeholder="eg. batch-123" />
-            </div>
-            <div class="w-1/2">
-              <.input field={@form[:version]} value={@form[:conditions].value["version"]} label="Version requirement" placeholder="eg. 1.2.3" />
-            </div>
+            <.inputs_for :let={conditions} field={@form[:conditions]}>
+              <div class="w-1/2">
+                <.input field={conditions[:tags]} label="Tag(s) distributed to" placeholder="eg. batch-123" />
+              </div>
+
+              <div class="w-1/2">
+                <.input field={conditions[:version]} label="Version requirement" placeholder="eg. 1.2.3" />
+              </div>
+            </.inputs_for>
           </div>
         </div>
 
@@ -272,7 +275,6 @@ defmodule NervesHubWeb.Components.DeploymentGroupPage.Settings do
 
     authorized!(:"deployment_group:update", org_user)
 
-    params = inject_conditions_map(params)
     firmware_changed? = params["firmware_id"] != to_string(deployment_group.firmware_id)
     %{firmware: %{id: old_firmware_id}} = deployment_group
 
@@ -324,28 +326,6 @@ defmodule NervesHubWeb.Components.DeploymentGroupPage.Settings do
     |> noreply()
   end
 
-  defp inject_conditions_map(%{"version" => version, "tags" => tags} = params) do
-    params
-    |> Map.put("conditions", %{
-      "version" => version,
-      "tags" =>
-        tags
-        |> tags_as_list()
-        |> MapSet.new()
-        |> MapSet.to_list()
-    })
-  end
-
-  defp inject_conditions_map(params), do: params
-
-  defp tags_as_list(""), do: []
-
-  defp tags_as_list(tags) do
-    tags
-    |> String.split(",")
-    |> Enum.map(&String.trim/1)
-  end
-
   def firmware_dropdown_options(firmwares) do
     firmwares
     |> Enum.sort_by(
@@ -400,13 +380,4 @@ defmodule NervesHubWeb.Components.DeploymentGroupPage.Settings do
   defp firmware_display_name(%Firmware{} = f) do
     "#{f.version} - #{f.platform} - #{f.architecture} (#{String.slice(f.uuid, 0..7)})"
   end
-
-  @doc """
-  Convert tags from a list to a comma-separated list (in a string)
-  """
-  def tags_to_string(%Phoenix.HTML.FormField{} = field) do
-    field.value
-    |> Map.get("tags", [])
-    |> Enum.join(", ")
-  end
 end

lib/nerves_hub_web/components/deployment_group_page/summary.ex

@@ -303,14 +303,14 @@ defmodule NervesHubWeb.Components.DeploymentGroupPage.Summary do
             </div>
             <div class="flex gap-4 items-center">
               <span class="text-sm text-nerves-gray-500 w-36">Tag selection:</span>
-              <span :if={Enum.empty?(@deployment_group.conditions["tags"])} class="text-sm text-nerves-gray-500">No tags configured</span>
-              <span :if={Enum.any?(@deployment_group.conditions["tags"])} class="flex gap-1">
-                <span :for={tag <- @deployment_group.conditions["tags"]} class="text-sm text-zinc-300 px-2 py-1 border border-zinc-800 bg-zinc-800 rounded">{tag}</span>
+              <span :if={Enum.empty?(@deployment_group.conditions.tags || [])} class="text-sm text-nerves-gray-500">No tags configured</span>
+              <span :if={Enum.any?(@deployment_group.conditions.tags || [])} class="flex gap-1">
+                <span :for={tag <- @deployment_group.conditions.tags} class="text-sm text-zinc-300 px-2 py-1 border border-zinc-800 bg-zinc-800 rounded">{tag}</span>
               </span>
             </div>
             <div class="flex gap-4 items-center pb-2">
               <span class="text-sm text-nerves-gray-500 w-36">Version requirement:</span>
-              <code class="text-sm text-zinc-300">{@deployment_group.conditions["version"]}</code>
+              <code class="text-sm text-zinc-300">{@deployment_group.conditions.version}</code>
             </div>
             <div
               :if={@deployment_group.device_count > 0 || @unmatched_device_count > 0 || @matched_devices_outside_deployment_group_count > 0}

lib/nerves_hub_web/components/device_page/settings_tab.ex

@@ -6,12 +6,16 @@ defmodule NervesHubWeb.Components.DevicePage.SettingsTab do
 
   alias NervesHub.Certificate
   alias NervesHub.Devices
+  alias NervesHub.Devices.Device
   alias NervesHub.Extensions
 
   alias NervesHub.Repo
 
   def tab_params(_params, _uri, socket) do
+    changeset = Ecto.Changeset.change(socket.assigns.device)
+
     socket
+    |> assign(:settings_form, to_form(changeset))
     |> allow_upload(:certificate,
       accept: :any,
       auto_upload: true,
@@ -24,17 +28,11 @@ defmodule NervesHubWeb.Components.DevicePage.SettingsTab do
   def render(assigns) do
     device = Repo.preload(assigns.device, :device_certificates, force: true)
 
-    changeset = Ecto.Changeset.change(assigns.device)
-
-    assigns =
-      assigns
-      |> Map.put(:device, device)
-      |> Map.put(:settings_form, to_form(changeset))
-      |> Map.put(:available_extensions, extensions())
+    assigns = Map.put(assigns, :device, device)
 
     ~H"""
     <div class="flex flex-col items-start justify-between gap-4 p-6">
-      <.form for={@settings_form} class="w-full" phx-submit="update-device-settings">
+      <.form id="settings-form" for={@settings_form} class="w-full" phx-change="validate-device-settings" phx-submit="update-device-settings">
         <div class="flex flex-col w-full bg-zinc-900 border border-zinc-700 rounded">
           <div class="flex justify-between items-center h-14 px-4 border-b border-zinc-700">
             <div class="text-base text-neutral-50 font-medium">General settings</div>
@@ -68,13 +66,13 @@ defmodule NervesHubWeb.Components.DevicePage.SettingsTab do
                 </p>
               </div> --%>
 
-              <.input field={@settings_form[:description]} label="Description" placeholder="eg. sensor hub at customer X" />
+              <.input field={@settings_form[:description]} label="Description" placeholder="eg. sensor hub at customer X" phx-debounce="blur" />
 
-              <.input field={@settings_form[:tags]} value={Utils.tags_to_string(@settings_form[:tags])} label="Tags" placeholder="eg. batch-123" />
+              <.input field={@settings_form[:tags]} value={Utils.tags_to_string(@settings_form[:tags])} label="Tags" placeholder="eg. batch-123" phx-debounce="blur" />
             </div>
 
             <div class="w-1/2 flex flex-col gap-2">
-              <.input field={@settings_form[:connecting_code]} label="First connect code" type="textarea" rows="10" />
+              <.input field={@settings_form[:connecting_code]} label="First connect code" type="textarea" rows="10" phx-debounce="2000" />
               <div class="text-xs tracking-wide text-zinc-400">Make sure this is valid Elixir and will not crash the device</div>
             </div>
           </div>
@@ -86,7 +84,7 @@ defmodule NervesHubWeb.Components.DevicePage.SettingsTab do
           <div class="text-base text-neutral-50 font-medium">Extensions</div>
         </div>
         <div class="py-2 px-4 flex flex-col gap-1">
-          <div :for={{key, description} <- @available_extensions} class="flex items-center gap-6 h-16 p-2">
+          <div :for={{key, description} <- available_extensions()} class="flex items-center gap-6 h-16 p-2">
             <div class="flex items-center h-8 py-1 px-2 bg-zinc-800 border border-zinc-700 rounded-full">
               <input
                 id={"extension-#{key}"}
@@ -281,6 +279,14 @@ defmodule NervesHubWeb.Components.DevicePage.SettingsTab do
     """
   end
 
+  def hooked_event("validate-device-settings", %{"device" => device_params}, socket) do
+    changeset = Device.changeset(socket.assigns.device, device_params)
+
+    socket
+    |> assign(:settings_form, to_form(changeset, action: :validate))
+    |> halt()
+  end
+
   def hooked_event("update-device-settings", %{"device" => device_params}, socket) do
     authorized!(:"device:update", socket.assigns.org_user)
 
@@ -457,7 +463,7 @@ defmodule NervesHubWeb.Components.DevicePage.SettingsTab do
     end
   end
 
-  defp extensions() do
+  defp available_extensions() do
     for extension <- Extensions.list(),
         into: %{},
         do: {extension, Extensions.module(extension).description()}