Skip to content

Commit 54628ed

Browse files
joshkjoshk
authored
Deployment Group validations improvements + live socket form recovery (#2380)
The core of this PR is focused on Deployment Group settings form recovery, with the following highlights also addressed: - Remove the use of `whitelist`ing params from Deployment Group controllers (thats what changesets are for) - Fix an issue where firmware or archives from another Org could be assigned to a Deployment - An overhaul of the Deployment Group validations, including addressing an issue where some fields could be set to negative values - Use `assoc_constraint` to make sure firmware, archives, and the associated product and org exist - Remove a duplicated device count `prepare_changes`
1 parent 2658068 commit 54628ed

File tree

28 files changed

+486
-374
lines changed

28 files changed

+486
-374
lines changed

‎lib/nerves_hub/archives.ex‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,17 @@ defmodule NervesHub.Archives do
5656
end
5757
end
5858

59+
def get_by_product_and_id(%Product{id: product_id}, id) do
60+
Archive
61+
|> where([a], a.id == ^id)
62+
|> where([a], a.product_id == ^product_id)
63+
|> Repo.one()
64+
|> case do
65+
nil -> {:error, :not_found}
66+
firmware -> {:ok, firmware}
67+
end
68+
end
69+
5970
@spec get_by_product_and_uuid!(Product.t(), String.t()) :: Archive.t()
6071
def get_by_product_and_uuid!(product, uuid) do
6172
Archive

‎lib/nerves_hub/firmwares.ex‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -144,7 +144,7 @@ defmodule NervesHub.Firmwares do
144144
|> Enum.reverse()
145145
end
146146

147-
@spec get_firmware(Org.t(), integer()) ::
147+
@spec get_firmware(Org.t() | Product.t(), integer()) ::
148148
{:ok, Firmware.t()}
149149
| {:error, :not_found}
150150
def get_firmware(_, nil) do
@@ -163,6 +163,17 @@ defmodule NervesHub.Firmwares do
163163
end
164164
end
165165

166+
def get_firmware(%Product{id: product_id}, id) do
167+
Firmware
168+
|> where([f], f.id == ^id)
169+
|> where([f], f.product_id == ^product_id)
170+
|> Repo.one()
171+
|> case do
172+
nil -> {:error, :not_found}
173+
firmware -> {:ok, firmware}
174+
end
175+
end
176+
166177
def get_firmware!(firmware_id), do: Repo.get!(Firmware, firmware_id)
167178

168179
def get_firmware_for_device(%Device{firmware_metadata: nil}), do: []

‎lib/nerves_hub/managed_deployments.ex‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -330,11 +330,12 @@ defmodule NervesHub.ManagedDeployments do
330330
Ecto.Changeset.change(%DeploymentGroup{})
331331
end
332332

333-
@spec create_deployment_group(map()) :: {:ok, DeploymentGroup.t()} | {:error, Changeset.t()}
334-
def create_deployment_group(params) do
335-
changeset = DeploymentGroup.create_changeset(%DeploymentGroup{}, params)
336-
337-
case Repo.insert(changeset) do
333+
@spec create_deployment_group(map(), Product.t()) ::
334+
{:ok, DeploymentGroup.t()} | {:error, Changeset.t()}
335+
def create_deployment_group(params, %Product{} = product) do
336+
DeploymentGroup.create_changeset(params, product)
337+
|> Repo.insert()
338+
|> case do
338339
{:ok, deployment_group} ->
339340
deployment_created_event(deployment_group)
340341

‎lib/nerves_hub/managed_deployments/deployment_group.ex‎

Lines changed: 101 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -5,10 +5,12 @@ defmodule NervesHub.ManagedDeployments.DeploymentGroup do
55
import Ecto.Query
66

77
alias NervesHub.Accounts.Org
8+
alias NervesHub.Archives
89
alias NervesHub.Archives.Archive
910
alias NervesHub.Devices.Device
1011
alias NervesHub.Devices.InflightUpdate
1112
alias NervesHub.Devices.UpdateStat
13+
alias NervesHub.Firmwares
1214
alias NervesHub.Firmwares.Firmware
1315
alias NervesHub.ManagedDeployments.DeploymentRelease
1416
alias NervesHub.Products.Product
@@ -17,30 +19,14 @@ defmodule NervesHub.ManagedDeployments.DeploymentGroup do
1719

1820
@type t :: %__MODULE__{}
1921

20-
@required_fields [
21-
:org_id,
22-
:firmware_id,
23-
:name,
24-
:is_active,
25-
:product_id,
22+
@numeric_fields_with_defaults [
2623
:concurrent_updates,
27-
:inflight_update_expiration_minutes
28-
]
29-
30-
@optional_fields [
31-
:archive_id,
3224
:device_failure_threshold,
3325
:device_failure_rate_seconds,
3426
:device_failure_rate_amount,
3527
:failure_threshold,
36-
:healthy,
37-
:penalty_timeout_minutes,
38-
:connecting_code,
39-
:total_updating_devices,
40-
:current_updated_devices,
41-
:queue_management,
42-
:delta_updatable,
43-
:status
28+
:inflight_update_expiration_minutes,
29+
:penalty_timeout_minutes
4430
]
4531

4632
@derive {Phoenix.Param, key: :name}
@@ -64,7 +50,7 @@ defmodule NervesHub.ManagedDeployments.DeploymentGroup do
6450
field(:device_failure_rate_seconds, :integer, default: 180)
6551
field(:device_failure_rate_amount, :integer, default: 5)
6652
field(:failure_threshold, :integer, default: 50)
67-
field(:is_active, :boolean)
53+
field(:is_active, :boolean, default: false)
6854
field(:name, :string)
6955
field(:healthy, :boolean, default: true)
7056
field(:penalty_timeout_minutes, :integer, default: 1440)
@@ -90,49 +76,125 @@ defmodule NervesHub.ManagedDeployments.DeploymentGroup do
9076
timestamps()
9177
end
9278

93-
@spec create_changeset(DeploymentGroup.t(), map()) :: Ecto.Changeset.t()
94-
def create_changeset(%DeploymentGroup{} = deployment, params) do
95-
base_changeset(deployment, params)
79+
@spec create_changeset(map(), Product.t()) :: Ecto.Changeset.t()
80+
def create_changeset(params, product) do
81+
%DeploymentGroup{}
82+
|> cast(params, [:name, :delta_updatable, :firmware_id])
83+
|> cast_and_validate_firmware(product)
84+
|> validate_required([:name, :delta_updatable])
85+
|> unique_constraint(:name, name: :deployments_product_id_name_index)
86+
|> cast_embed(:conditions, required: true, with: &conditions_changeset/2)
9687
end
9788

9889
@spec update_changeset(DeploymentGroup.t(), map()) :: Ecto.Changeset.t()
9990
def update_changeset(%DeploymentGroup{} = deployment, params) do
100-
base_changeset(deployment, params)
101-
|> prepare_changes(fn changeset ->
91+
deployment
92+
|> cast(params, [
93+
:name,
94+
:is_active,
95+
:delta_updatable,
96+
:connecting_code,
97+
:queue_management,
98+
:firmware_id,
99+
:archive_id
100+
])
101+
|> cast_and_validate_numeric_fields(params)
102+
|> cast_embed(:conditions, required: true, with: &conditions_changeset/2)
103+
|> cast_and_validate_firmware()
104+
|> cast_and_validate_archive()
105+
|> validate_required([:name, :delta_updatable, :is_active, :queue_management])
106+
|> unique_constraint(:name, name: :deployments_product_id_name_index)
107+
|> prepare_current_updated_devices()
108+
|> prepare_device_count()
109+
|> prepare_status()
110+
end
111+
112+
defp cast_and_validate_numeric_fields(changeset, params) do
113+
changeset
114+
|> cast(params, @numeric_fields_with_defaults, empty_values: [nil])
115+
|> validate_number(:concurrent_updates, greater_than: 0)
116+
|> validate_number(:device_failure_threshold, greater_than: 0)
117+
|> validate_number(:device_failure_rate_seconds, greater_than_or_equal_to: 60)
118+
|> validate_number(:device_failure_rate_amount, greater_than: 0)
119+
|> validate_number(:failure_threshold, greater_than: 0)
120+
|> validate_number(:inflight_update_expiration_minutes, greater_than_or_equal_to: 30)
121+
|> validate_number(:penalty_timeout_minutes, greater_than_or_equal_to: 60)
122+
end
123+
124+
defp cast_and_validate_firmware(changeset, product \\ nil) do
125+
product = product || %Product{id: changeset.data.product_id}
126+
127+
if firmware_id = changeset.changes[:firmware_id] do
128+
case Firmwares.get_firmware(product, firmware_id) do
129+
{:ok, firmware} ->
130+
changeset
131+
|> put_change(:org_id, firmware.org_id)
132+
|> put_change(:product_id, firmware.product_id)
133+
|> put_change(:firmware_id, firmware.id)
134+
|> assoc_constraint(:org)
135+
|> assoc_constraint(:product)
136+
|> assoc_constraint(:firmware)
137+
138+
{:error, _} ->
139+
add_error(changeset, :firmware_id, "does not exist")
140+
end
141+
else
142+
changeset
143+
|> validate_required([:firmware_id])
144+
end
145+
end
146+
147+
defp cast_and_validate_archive(changeset) do
148+
if archive_id = changeset.changes[:archive_id] do
149+
%Product{id: changeset.data.product_id}
150+
|> Archives.get_by_product_and_id(archive_id)
151+
|> case do
152+
{:ok, archive} ->
153+
changeset
154+
|> put_change(:archive_id, archive.id)
155+
|> assoc_constraint(:archive)
156+
157+
{:error, _} ->
158+
add_error(changeset, :archive_id, "invalid archive")
159+
end
160+
else
161+
changeset
162+
end
163+
end
164+
165+
defp prepare_current_updated_devices(changeset) do
166+
prepare_changes(changeset, fn changeset ->
102167
if changeset.changes[:firmware_id] do
103168
put_change(changeset, :current_updated_devices, 0)
104169
else
105170
changeset
106171
end
107172
end)
108-
|> prepare_changes(fn changeset ->
109-
device_count =
110-
Device
111-
|> select([d], count(d))
112-
|> where([d], d.deployment_id == ^deployment.id)
113-
|> changeset.repo.one()
173+
end
114174

115-
put_change(changeset, :device_count, device_count)
116-
end)
117-
|> prepare_changes(fn changeset ->
175+
defp prepare_device_count(changeset) do
176+
prepare_changes(changeset, fn changeset ->
118177
device_count =
119178
Device
120179
|> select([d], count(d))
121-
|> where([d], d.deployment_id == ^deployment.id)
180+
|> where([d], d.deployment_id == ^changeset.data.id)
122181
|> changeset.repo.one()
123182

124183
put_change(changeset, :device_count, device_count)
125184
end)
126-
|> prepare_changes(fn changeset ->
185+
end
186+
187+
defp prepare_status(changeset) do
188+
prepare_changes(changeset, fn changeset ->
127189
case changeset do
128190
%{changes: %{delta_updatable: true}} = changeset ->
129-
Ecto.Changeset.put_change(changeset, :status, :preparing)
191+
put_change(changeset, :status, :preparing)
130192

131193
%{changes: %{delta_updatable: false}} = changeset ->
132-
Ecto.Changeset.put_change(changeset, :status, :ready)
194+
put_change(changeset, :status, :ready)
133195

134196
%{changes: %{is_active: true}} = changeset ->
135-
Ecto.Changeset.put_change(changeset, :status, :preparing)
197+
put_change(changeset, :status, :preparing)
136198

137199
changeset ->
138200
changeset
@@ -146,14 +208,6 @@ defmodule NervesHub.ManagedDeployments.DeploymentGroup do
146208
|> validate_required([:status])
147209
end
148210

149-
defp base_changeset(%DeploymentGroup{} = deployment, params) do
150-
deployment
151-
|> cast(params, @required_fields ++ @optional_fields)
152-
|> validate_required(@required_fields)
153-
|> unique_constraint(:name, name: :deployments_product_id_name_index)
154-
|> cast_embed(:conditions, required: true, with: &conditions_changeset/2)
155-
end
156-
157211
def conditions_changeset(conditions, attrs) do
158212
conditions
159213
|> cast(attrs, [:tags, :version], empty_values: [""])

‎lib/nerves_hub_web/components/deployment_group_page/settings.ex‎

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -29,15 +29,15 @@ defmodule NervesHubWeb.Components.DeploymentGroupPage.Settings do
2929
def render(assigns) do
3030
~H"""
3131
<div class="flex flex-col items-start justify-between gap-4 p-6">
32-
<.form for={@form} class="w-full flex flex-col gap-4" phx-submit="update-deployment-group" phx-target={@myself}>
32+
<.form id="deployment-form" for={@form} class="w-full flex flex-col gap-4" phx-change="validate-deployment-group" phx-submit="update-deployment-group" phx-target={@myself}>
3333
<div class="w-2/3 flex flex-col bg-zinc-900 border border-zinc-700 rounded">
3434
<div class="flex justify-between items-center h-14 px-4 border-b border-zinc-700">
3535
<div class="text-base text-neutral-50 font-medium">General settings</div>
3636
</div>
3737
3838
<div class="flex p-6 gap-6">
3939
<div class="w-1/2 flex flex-col gap-6">
40-
<.input field={@form[:name]} label="Name" placeholder="Production" />
40+
<.input field={@form[:name]} label="Name" placeholder="Production" phx-debounce="blur" />
4141
<.input field={@form[:delta_updatable]} type="checkbox" label="Delta updates">
4242
<:rich_hint>
4343
When enabled, the deployment group will only send delta updates.
@@ -222,7 +222,7 @@ defmodule NervesHubWeb.Components.DeploymentGroupPage.Settings do
222222
223223
<div class="flex flex-col p-6 gap-6">
224224
<div class="w-2/3 flex flex-col gap-6">
225-
<.input field={@form[:connecting_code]} type="textarea" rows={8} label="Run this code when the device first connects to the console.">
225+
<.input field={@form[:connecting_code]} type="textarea" rows={8} label="Run this code when the device first connects to the console." phx-debounce="2000">
226226
<:rich_hint>
227227
<p>
228228
Make sure this is valid Elixir and will not crash the device.
@@ -263,6 +263,16 @@ defmodule NervesHubWeb.Components.DeploymentGroupPage.Settings do
263263
end
264264

265265
@impl Phoenix.LiveComponent
266+
def handle_event("validate-deployment-group", %{"deployment_group" => params}, socket) do
267+
changeset =
268+
socket.assigns.deployment_group
269+
|> DeploymentGroup.update_changeset(params)
270+
271+
socket
272+
|> assign(:form, to_form(changeset, action: :validate))
273+
|> noreply()
274+
end
275+
266276
def handle_event("update-deployment-group", %{"deployment_group" => params}, socket) do
267277
%{
268278
org_user: org_user,
@@ -275,9 +285,6 @@ defmodule NervesHubWeb.Components.DeploymentGroupPage.Settings do
275285

276286
authorized!(:"deployment_group:update", org_user)
277287

278-
firmware_changed? = params["firmware_id"] != to_string(deployment_group.firmware_id)
279-
%{firmware: %{id: old_firmware_id}} = deployment_group
280-
281288
case ManagedDeployments.update_deployment_group(deployment_group, params) do
282289
{:ok, updated} ->
283290
# Use original deployment so changes will get
@@ -288,13 +295,6 @@ defmodule NervesHubWeb.Components.DeploymentGroupPage.Settings do
288295
"User #{user.name} updated deployment group #{updated.name}"
289296
)
290297

291-
# no need to subscribe to new firmware here, we do that in the summary component
292-
if firmware_changed? do
293-
:ok = Firmwares.unsubscribe_firmware_delta_target(old_firmware_id)
294-
end
295-
296-
# TODO: if we move away from slugs with deployment names we won't need
297-
# to use `push_navigate` here.
298298
socket
299299
|> put_flash(:info, "Deployment Group updated")
300300
|> push_navigate(to: ~p"/org/#{org}/#{product}/deployment_groups/#{updated}")

‎lib/nerves_hub_web/controllers/api/deployment_group_controller.ex‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -32,10 +32,7 @@ defmodule NervesHubWeb.API.DeploymentGroupController do
3232
uuid ->
3333
with {:ok, firmware} <- Firmwares.get_firmware_by_product_and_uuid(product, uuid),
3434
params = Map.put(params, "firmware_id", firmware.id),
35-
params = Map.put(params, "org_id", org.id),
36-
params = Map.put(params, "product_id", product.id),
37-
params = whitelist(params, @whitelist_fields),
38-
{:ok, deployment_group} <- ManagedDeployments.create_deployment_group(params) do
35+
{:ok, deployment_group} <- ManagedDeployments.create_deployment_group(params, product) do
3936
DeploymentGroupTemplates.audit_deployment_created(user, deployment_group)
4037

4138
conn

0 commit comments

Comments
 (0)