chore(update_channel): better errors

When creating and updating an update channel, target group ids are validated to be available, meaning: - they are present in the DB - they are not related to any other update channel closes #529 Signed-off-by: Luca Zaninotto <[email protected]>
edgehog-device-manager · Dec 10, 2024 · 158b288 · 158b288
1 parent dd0722e
commit 158b288
Show file tree

Hide file tree

Showing 6 changed files with 154 additions and 37 deletions.
diff --git a/backend/lib/edgehog/update_campaigns/update_channel/changes/relate_target_groups.ex b/backend/lib/edgehog/update_campaigns/update_channel/changes/relate_target_groups.ex
@@ -22,36 +22,13 @@ defmodule Edgehog.UpdateCampaigns.UpdateChannel.Changes.RelateTargetGroups do
   @moduledoc false
   use Ash.Resource.Change
 
-  alias Ash.Error.Changes.InvalidArgument
-  alias Edgehog.Groups.DeviceGroup
-
   @impl Ash.Resource.Change
-  def change(changeset, _opts, context) do
-    %{tenant: tenant} = context
-
+  def change(changeset, _opts, _context) do
     {:ok, target_group_ids} = Ash.Changeset.fetch_argument(changeset, :target_group_ids)
 
-    Ash.Changeset.after_action(changeset, fn _changeset, update_channel ->
-      relate_target_groups(tenant, update_channel, target_group_ids)
-    end)
-  end
-
-  defp relate_target_groups(tenant, update_channel, target_group_ids) do
-    %Ash.BulkResult{status: status, records: records} =
-      DeviceGroup
-      |> Ash.Query.filter(id in ^target_group_ids)
-      |> Ash.Query.filter(is_nil(update_channel_id))
-      |> Ash.Query.set_tenant(tenant)
-      |> Ash.bulk_update(:update_update_channel, %{update_channel_id: update_channel.id}, return_records?: true)
-
-    if status == :success and length(records) == length(target_group_ids) do
-      {:ok, update_channel}
-    else
-      {:error,
-       InvalidArgument.exception(
-         field: :target_group_ids,
-         message: "some target groups were not found or are already associated with an update channel"
-       )}
-    end
+    Ash.Changeset.manage_relationship(changeset, :target_groups, target_group_ids,
+      on_lookup: :relate,
+      on_no_match: :error
+    )
   end
 end
diff --git a/backend/lib/edgehog/update_campaigns/update_channel/update_channel.ex b/backend/lib/edgehog/update_campaigns/update_channel/update_channel.ex
@@ -28,6 +28,7 @@ defmodule Edgehog.UpdateCampaigns.UpdateChannel do
 
   alias Edgehog.UpdateCampaigns.UpdateChannel.Calculations
   alias Edgehog.UpdateCampaigns.UpdateChannel.Changes
+  alias Edgehog.UpdateCampaigns.UpdateChannel.Validations
 
   resource do
     description """
@@ -63,6 +64,14 @@ defmodule Edgehog.UpdateCampaigns.UpdateChannel do
       change Changes.RelateTargetGroups do
         where present(:target_group_ids)
       end
+
+      validate Validations.TargetGroupsAreAvailable do
+        where present(:target_group_ids)
+      end
+
+      validate Validations.TargetGroupsAreUnrelated do
+        where present(:target_group_ids)
+      end
     end
 
     update :update do
@@ -89,6 +98,14 @@ defmodule Edgehog.UpdateCampaigns.UpdateChannel do
       change Changes.RelateTargetGroups do
         where present(:target_group_ids)
       end
+
+      validate Validations.TargetGroupsAreAvailable do
+        where present(:target_group_ids)
+      end
+
+      validate Validations.TargetGroupsAreUnrelated do
+        where present(:target_group_ids)
+      end
     end
 
     destroy :destroy do

diff --git a/...nd/lib/edgehog/update_campaigns/update_channel/validations/target_groups_are_available.ex b/...nd/lib/edgehog/update_campaigns/update_channel/validations/target_groups_are_available.ex
@@ -0,0 +1,55 @@
+#
+# This file is part of Edgehog.
+#
+# Copyright 2024 SECO Mind Srl
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#    http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+# SPDX-License-Identifier: Apache-2.0
+#
+
+defmodule Edgehog.UpdateCampaigns.UpdateChannel.Validations.TargetGroupsAreAvailable do
+  @moduledoc false
+
+  use Ash.Resource.Validation
+
+  alias Ash.Error.Changes.InvalidArgument
+  alias Edgehog.Groups.DeviceGroup
+
+  @impl Ash.Resource.Validation
+  def validate(changeset, _opts, context) do
+    %{tenant: tenant} = context
+
+    {:ok, target_group_ids} = Ash.Changeset.fetch_argument(changeset, :target_group_ids)
+
+    {:ok, target_groups} =
+      DeviceGroup
+      |> Ash.Query.set_tenant(tenant)
+      |> Ash.Query.filter(id in ^target_group_ids)
+      |> Ash.read()
+
+    read_groups_ids = Enum.map(target_groups, & &1.id)
+
+    not_found_groups =
+      Enum.reject(target_group_ids, &(&1 in read_groups_ids))
+
+    if Enum.empty?(not_found_groups),
+      do: :ok,
+      else:
+        {:error,
+         InvalidArgument.exception(
+           field: :target_group_ids,
+           message: "some target groups were not found: #{inspect(not_found_groups)}"
+         )}
+  end
+end
diff --git a/...nd/lib/edgehog/update_campaigns/update_channel/validations/target_groups_are_unrelated.ex b/...nd/lib/edgehog/update_campaigns/update_channel/validations/target_groups_are_unrelated.ex
@@ -0,0 +1,53 @@
+#
+# This file is part of Edgehog.
+#
+# Copyright 2024 SECO Mind Srl
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#    http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+# SPDX-License-Identifier: Apache-2.0
+#
+
+defmodule Edgehog.UpdateCampaigns.UpdateChannel.Validations.TargetGroupsAreUnrelated do
+  @moduledoc false
+  use Ash.Resource.Validation
+
+  alias Ash.Error.Changes.InvalidArgument
+  alias Edgehog.Groups.DeviceGroup
+
+  @impl Ash.Resource.Validation
+  def validate(changeset, _opts, context) do
+    %{tenant: tenant} = context
+
+    {:ok, target_group_ids} = Ash.Changeset.fetch_argument(changeset, :target_group_ids)
+
+    {:ok, related_target_groups} =
+      DeviceGroup
+      |> Ash.Query.set_tenant(tenant)
+      |> Ash.Query.filter(id in ^target_group_ids)
+      |> Ash.Query.filter(not is_nil(update_channel_id))
+      |> Ash.read()
+
+    related_target_groups_ids = Enum.map(related_target_groups, & &1.id)
+
+    if Enum.empty?(related_target_groups_ids),
+      do: :ok,
+      else:
+        {:error,
+         InvalidArgument.exception(
+           field: :target_group_ids,
+           message:
+             "some target groups are already associated with an update channel: #{inspect(related_target_groups_ids)}"
+         )}
+  end
+end
diff --git a/backend/test/edgehog_web/schema/mutation/create_update_channel_test.exs b/backend/test/edgehog_web/schema/mutation/create_update_channel_test.exs
@@ -167,7 +167,7 @@ defmodule EdgehogWeb.Schema.Mutation.CreateUpdateChannelTest do
     end
 
     test "fails when trying to use a non-existing target group", %{tenant: tenant} do
-      target_group_id = non_existing_device_group_id(tenant)
+      {target_group_id, original_id} = non_existing_device_group_id(tenant)
 
       error =
         [target_group_ids: [target_group_id], tenant: tenant]
@@ -177,13 +177,16 @@ defmodule EdgehogWeb.Schema.Mutation.CreateUpdateChannelTest do
       assert %{
                path: ["createUpdateChannel"],
                fields: [:target_group_ids],
-               message: "some target groups were not found or are already associated with an update channel",
+               message: "some target groups were not found: " <> missing,
                code: "invalid_argument"
              } = error
+
+      assert missing == "[#{original_id}]"
     end
 
     test "fails when trying to use already assigned target groups", %{tenant: tenant} do
       target_group = device_group_fixture(tenant: tenant)
+
       _ = update_channel_fixture(tenant: tenant, target_group_ids: [target_group.id])
 
       target_group_id = AshGraphql.Resource.encode_relay_id(target_group)
@@ -196,9 +199,13 @@ defmodule EdgehogWeb.Schema.Mutation.CreateUpdateChannelTest do
       assert %{
                path: ["createUpdateChannel"],
                fields: [:target_group_ids],
-               message: "some target groups were not found or are already associated with an update channel",
+               message:
+                 "some target groups are already associated with an update channel: " <>
+                   ret_group_id,
                code: "invalid_argument"
              } = error
+
+      assert ret_group_id == "[#{target_group.id}]"
     end
   end
 
@@ -272,6 +279,6 @@ defmodule EdgehogWeb.Schema.Mutation.CreateUpdateChannelTest do
     id = AshGraphql.Resource.encode_relay_id(fixture)
     :ok = Ash.destroy!(fixture)
 
-    id
+    {id, fixture.id}
   end
 end
diff --git a/backend/test/edgehog_web/schema/mutation/update_update_channel_test.exs b/backend/test/edgehog_web/schema/mutation/update_update_channel_test.exs
@@ -145,7 +145,7 @@ defmodule EdgehogWeb.Schema.Mutation.UpdateUpdateChannelTest do
     end
 
     test "fails when trying to use a non-existing target group id", %{tenant: tenant, id: id} do
-      target_group_id = non_existing_device_group_id(tenant)
+      {target_group_id, original_target_group_id} = non_existing_device_group_id(tenant)
 
       error =
         [id: id, target_group_ids: [target_group_id], tenant: tenant]
@@ -156,13 +156,17 @@ defmodule EdgehogWeb.Schema.Mutation.UpdateUpdateChannelTest do
                path: ["updateUpdateChannel"],
                fields: [:target_group_ids],
                code: "invalid_argument",
-               message: "some target groups were not found or are already associated with an update channel"
+               message: "some target groups were not found: " <> missing
              } = error
+
+      assert missing == "[#{inspect(original_target_group_id)}]"
     end
 
     test "fails when trying to use already assigned target groups", %{tenant: tenant, id: id} do
       target_group = device_group_fixture(tenant: tenant)
-      _ = update_channel_fixture(tenant: tenant, target_group_ids: [target_group.id])
+
+      _ =
+        update_channel_fixture(tenant: tenant, target_group_ids: [target_group.id])
 
       target_group_id = AshGraphql.Resource.encode_relay_id(target_group)
 
@@ -175,8 +179,12 @@ defmodule EdgehogWeb.Schema.Mutation.UpdateUpdateChannelTest do
                path: ["updateUpdateChannel"],
                fields: [:target_group_ids],
                code: "invalid_argument",
-               message: "some target groups were not found or are already associated with an update channel"
+               message:
+                 "some target groups are already associated with an update channel: " <>
+                   ret_update_channel_id
              } = error
+
+      assert ret_update_channel_id == "[#{inspect(target_group.id)}]"
     end
   end
 
@@ -245,6 +253,6 @@ defmodule EdgehogWeb.Schema.Mutation.UpdateUpdateChannelTest do
     id = AshGraphql.Resource.encode_relay_id(fixture)
     :ok = Ash.destroy!(fixture)
 
-    id
+    {id, fixture.id}
   end
 end