From 7e9c7e0b952c11dc34299c8dbc702ad616919c7d Mon Sep 17 00:00:00 2001 From: Luca Zaninotto Date: Tue, 10 Dec 2024 12:46:03 +0100 Subject: [PATCH] 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 --- .../changes/relate_target_groups.ex | 33 ++--------- .../update_channel/update_channel.ex | 17 ++++++ .../target_groups_are_available.ex | 55 +++++++++++++++++++ .../target_groups_are_unrelated.ex | 53 ++++++++++++++++++ .../mutation/create_update_channel_test.exs | 15 +++-- .../mutation/update_update_channel_test.exs | 18 ++++-- 6 files changed, 154 insertions(+), 37 deletions(-) create mode 100644 backend/lib/edgehog/update_campaigns/update_channel/validations/target_groups_are_available.ex create mode 100644 backend/lib/edgehog/update_campaigns/update_channel/validations/target_groups_are_unrelated.ex 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 index bb3f11129..6fd9b467e 100644 --- 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 index f6e765eff..8bbc8e1bb 100644 --- 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/backend/lib/edgehog/update_campaigns/update_channel/validations/target_groups_are_available.ex b/backend/lib/edgehog/update_campaigns/update_channel/validations/target_groups_are_available.ex new file mode 100644 index 000000000..cdda35aeb --- /dev/null +++ b/backend/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, charlists: :as_list)}" + )} + end +end diff --git a/backend/lib/edgehog/update_campaigns/update_channel/validations/target_groups_are_unrelated.ex b/backend/lib/edgehog/update_campaigns/update_channel/validations/target_groups_are_unrelated.ex new file mode 100644 index 000000000..7e0b49c50 --- /dev/null +++ b/backend/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, charlists: :as_list)}" + )} + 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 index 154d9e34c..86893f780 100644 --- 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 index b4b1e3a47..23d0ea5d8 100644 --- 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 == "[#{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 == "[#{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