From 835445d748751625a8e8d9f8c4d3bf929ce367f1 Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Tue, 24 Mar 2026 15:57:09 +0100 Subject: [PATCH 1/2] Fixes removing (hiding) user file sources / object stores Allows metadata-only updates to skip variable validation when no new variable values are provided. Prevents hide (remove) operations from failing for instances backed by templates with required variables, while preserving the previously stored values. Adds regression coverage for both supported instance types to ensure hidden updates remain safe and predictable. --- lib/galaxy/managers/_config_templates.py | 5 ++-- .../app/managers/test_user_file_sources.py | 20 ++++++++++++++++ .../app/managers/test_user_object_stores.py | 24 ++++++++++++++++--- 3 files changed, 44 insertions(+), 5 deletions(-) diff --git a/lib/galaxy/managers/_config_templates.py b/lib/galaxy/managers/_config_templates.py index 5b5dda62af0..56c5b354e6c 100644 --- a/lib/galaxy/managers/_config_templates.py +++ b/lib/galaxy/managers/_config_templates.py @@ -296,8 +296,9 @@ def update_template_instance( payload: UpdateInstancePayload, template: Template, ): - validate_specified_datatypes_variables(payload.variables or {}, template) - validate_no_extra_variables_defined(payload.variables or {}, template) + if payload.variables is not None: + validate_specified_datatypes_variables(payload.variables, template) + validate_no_extra_variables_defined(payload.variables, template) if payload.name is not None: template_instance.name = payload.name if payload.description is not None: diff --git a/test/unit/app/managers/test_user_file_sources.py b/test/unit/app/managers/test_user_file_sources.py index f817ab5ec39..e48f8c746c2 100644 --- a/test/unit/app/managers/test_user_file_sources.py +++ b/test/unit/app/managers/test_user_file_sources.py @@ -480,6 +480,26 @@ class TestFileSourcesTestCase(BaseTestCase): assert user_file_source_showed.variables assert user_file_source_showed.variables["var1"] == "newval" + def test_hide_without_variables_update_on_required_variable_template(self, tmp_path): + self._init_managers(tmp_path, config_dict=simple_variable_template(tmp_path)) + create_payload = CreateInstancePayload( + name=SIMPLE_FILE_SOURCE_NAME, + description=SIMPLE_FILE_SOURCE_DESCRIPTION, + template_id="simple_variable", + template_version=0, + variables={"var1": "requiredval"}, + secrets={}, + ) + user_file_source = self._create_instance(create_payload) + + hide = UpdateInstancePayload(hidden=True) + self._modify(user_file_source, hide) + + user_file_source_showed = self.manager.show(self.trans, user_file_source.uuid) + assert user_file_source_showed.hidden + assert user_file_source_showed.variables + assert user_file_source_showed.variables["var1"] == "requiredval" + def test_hide(self, tmp_path): user_file_source = self._init_and_create_simple(tmp_path) diff --git a/test/unit/app/managers/test_user_object_stores.py b/test/unit/app/managers/test_user_object_stores.py index c4b8bfa70b8..3c3959bba0a 100644 --- a/test/unit/app/managers/test_user_object_stores.py +++ b/test/unit/app/managers/test_user_object_stores.py @@ -1,6 +1,4 @@ -from typing import ( - Optional, -) +from typing import Optional from yaml import safe_load @@ -150,6 +148,26 @@ class TestUserObjectStoreTestCase(BaseTestCase): assert user_object_store_showed.variables assert user_object_store_showed.variables["var1"] == "newval" + def test_hide_without_variables_update_on_required_variable_template(self, tmp_path): + self._init_managers(tmp_path, config_dict=simple_variable_template(tmp_path)) + create_payload = CreateInstancePayload( + name=SIMPLE_FILE_SOURCE_NAME, + description=SIMPLE_FILE_SOURCE_DESCRIPTION, + template_id="simple_variable", + template_version=0, + variables={"var1": "requiredval"}, + secrets={}, + ) + user_object_store = self._create_instance(create_payload) + + hide = UpdateInstancePayload(hidden=True) + self._modify(user_object_store, hide) + + user_object_store_showed = self.manager.show(self.trans, user_object_store.uuid) + assert user_object_store_showed.hidden + assert user_object_store_showed.variables + assert user_object_store_showed.variables["var1"] == "requiredval" + def test_update_errors_on_extra_variables(self, tmp_path): self._init_managers(tmp_path, config_dict=simple_variable_template(tmp_path)) create_payload = CreateInstancePayload( From b85f8a07af2d340dc479ae6f598788591153d1f0 Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Tue, 24 Mar 2026 15:57:11 +0100 Subject: [PATCH 2/2] Improves remove error feedback Shows a user-facing error instead of silently do nothing when instance removal fails. Adds focused coverage for successful and failed removal flows across both supported instance types. --- .../Instances/InstanceDropdown.test.ts | 97 +++++++++++++++++ .../Instances/InstanceDropdown.vue | 6 +- .../Instances/InstanceDropdown.test.ts | 103 ++++++++++++++++++ .../Instances/InstanceDropdown.vue | 6 +- 4 files changed, 208 insertions(+), 4 deletions(-) create mode 100644 client/src/components/FileSources/Instances/InstanceDropdown.test.ts create mode 100644 client/src/components/ObjectStore/Instances/InstanceDropdown.test.ts diff --git a/client/src/components/FileSources/Instances/InstanceDropdown.test.ts b/client/src/components/FileSources/Instances/InstanceDropdown.test.ts new file mode 100644 index 00000000000..91e0f6caa32 --- /dev/null +++ b/client/src/components/FileSources/Instances/InstanceDropdown.test.ts @@ -0,0 +1,97 @@ +import { getLocalVue } from "@tests/vitest/helpers"; +import { shallowMount } from "@vue/test-utils"; +import flushPromises from "flush-promises"; +import { beforeEach, describe, expect, it, vi } from "vitest"; + +import { useServerMock } from "@/api/client/__mocks__"; +import type { UserFileSourceModel } from "@/api/fileSources"; + +import FileSourceInstanceDropdown from "./InstanceDropdown.vue"; +import ConfigurationInstanceDropdown from "@/components/ConfigTemplates/InstanceDropdown.vue"; + +const { toastErrorMock } = vi.hoisted(() => ({ + toastErrorMock: vi.fn(), +})); + +vi.mock("@/composables/toast", () => ({ + Toast: { + error: toastErrorMock, + }, +})); + +vi.mock("@/stores/fileSourceTemplatesStore", () => ({ + useFileSourceTemplatesStore: () => ({ + canUpgrade: () => false, + }), +})); + +const { server, http } = useServerMock(); + +const FILE_SOURCE: UserFileSourceModel = { + uuid: "test-file-source-id", + name: "Test File Source", + description: null, + template_id: "simple_variable", + template_version: 0, + active: true, + hidden: false, + purged: false, + secrets: [], + type: "posix", + uri_root: "gxfiles://test-file-source-id", + variables: null, +}; + +describe("File Source Instance Dropdown", () => { + const localVue = getLocalVue(true); + + beforeEach(() => { + server.resetHandlers(); + toastErrorMock.mockClear(); + }); + + it("emits entryRemoved when remove succeeds", async () => { + let requestBody: Record | undefined; + server.use( + http.put("/api/file_source_instances/{uuid}", async ({ request, response }) => { + requestBody = (await request.json()) as Record; + return response(200).json(FILE_SOURCE); + }), + ); + + const wrapper = shallowMount(FileSourceInstanceDropdown as object, { + localVue, + propsData: { + fileSource: FILE_SOURCE, + }, + }); + + wrapper.findComponent(ConfigurationInstanceDropdown).vm.$emit("remove"); + await flushPromises(); + + expect(requestBody).toEqual({ hidden: true }); + expect(wrapper.emitted("entryRemoved")).toBeTruthy(); + expect(toastErrorMock).not.toHaveBeenCalled(); + }); + + it("shows toast and does not emit entryRemoved when remove fails", async () => { + server.use( + http.put("/api/file_source_instances/{uuid}", ({ response }) => { + return response("4XX").json({ err_code: 400, err_msg: "Unable to remove" }, { status: 400 }); + }), + ); + + const wrapper = shallowMount(FileSourceInstanceDropdown as object, { + localVue, + propsData: { + fileSource: FILE_SOURCE, + }, + }); + + wrapper.findComponent(ConfigurationInstanceDropdown).vm.$emit("remove"); + await flushPromises(); + + expect(toastErrorMock).toHaveBeenCalledWith("Unable to remove", "Failed to remove instance"); + expect(wrapper.emitted("entryRemoved")).toBeFalsy(); + }); +}); diff --git a/client/src/components/FileSources/Instances/InstanceDropdown.vue b/client/src/components/FileSources/Instances/InstanceDropdown.vue index 6a5135569eb..181e14a2f3f 100644 --- a/client/src/components/FileSources/Instances/InstanceDropdown.vue +++ b/client/src/components/FileSources/Instances/InstanceDropdown.vue @@ -3,8 +3,9 @@ import { computed } from "vue"; import { GalaxyApi } from "@/api"; import type { UserFileSourceModel } from "@/api/fileSources"; +import { Toast } from "@/composables/toast"; import { useFileSourceTemplatesStore } from "@/stores/fileSourceTemplatesStore"; -import { rethrowSimple } from "@/utils/simple-error"; +import { errorMessageAsString } from "@/utils/simple-error"; import InstanceDropdown from "@/components/ConfigTemplates/InstanceDropdown.vue"; @@ -30,7 +31,8 @@ async function onRemove() { }); if (error) { - rethrowSimple(error); + Toast.error(errorMessageAsString(error, "Failed to remove instance."), "Failed to remove instance"); + return; } emit("entryRemoved"); diff --git a/client/src/components/ObjectStore/Instances/InstanceDropdown.test.ts b/client/src/components/ObjectStore/Instances/InstanceDropdown.test.ts new file mode 100644 index 00000000000..960eb791c01 --- /dev/null +++ b/client/src/components/ObjectStore/Instances/InstanceDropdown.test.ts @@ -0,0 +1,103 @@ +import { getLocalVue } from "@tests/vitest/helpers"; +import { shallowMount } from "@vue/test-utils"; +import flushPromises from "flush-promises"; +import { beforeEach, describe, expect, it, vi } from "vitest"; + +import { useServerMock } from "@/api/client/__mocks__"; + +import type { UserConcreteObjectStore } from "./types"; + +import ObjectStoreInstanceDropdown from "./InstanceDropdown.vue"; +import ConfigurationInstanceDropdown from "@/components/ConfigTemplates/InstanceDropdown.vue"; + +const { toastErrorMock } = vi.hoisted(() => ({ + toastErrorMock: vi.fn(), +})); + +vi.mock("@/composables/toast", () => ({ + Toast: { + error: toastErrorMock, + }, +})); + +vi.mock("@/stores/objectStoreTemplatesStore", () => ({ + useObjectStoreTemplatesStore: () => ({ + canUpgrade: () => false, + }), +})); + +const { server, http } = useServerMock(); + +const OBJECT_STORE: UserConcreteObjectStore = { + uuid: "test-object-store-id", + name: "Test Object Store", + description: null, + template_id: "simple_variable", + template_version: 0, + active: true, + hidden: false, + purged: false, + private: false, + object_store_id: null, + device: null, + object_expires_after_days: null, + badges: [], + quota: { enabled: false }, + secrets: [], + type: "disk", + variables: null, +}; + +describe("Object Store Instance Dropdown", () => { + const localVue = getLocalVue(true); + + beforeEach(() => { + server.resetHandlers(); + toastErrorMock.mockClear(); + }); + + it("emits entryRemoved when remove succeeds", async () => { + let requestBody: Record | undefined; + server.use( + http.put("/api/object_store_instances/{uuid}", async ({ request, response }) => { + requestBody = (await request.json()) as Record; + return response(200).json(OBJECT_STORE); + }), + ); + + const wrapper = shallowMount(ObjectStoreInstanceDropdown as object, { + localVue, + propsData: { + objectStore: OBJECT_STORE, + }, + }); + + wrapper.findComponent(ConfigurationInstanceDropdown).vm.$emit("remove"); + await flushPromises(); + + expect(requestBody).toEqual({ hidden: true }); + expect(wrapper.emitted("entryRemoved")).toBeTruthy(); + expect(toastErrorMock).not.toHaveBeenCalled(); + }); + + it("shows toast and does not emit entryRemoved when remove fails", async () => { + server.use( + http.put("/api/object_store_instances/{uuid}", ({ response }) => { + return response("4XX").json({ err_code: 400, err_msg: "Unable to remove" }, { status: 400 }); + }), + ); + + const wrapper = shallowMount(ObjectStoreInstanceDropdown as object, { + localVue, + propsData: { + objectStore: OBJECT_STORE, + }, + }); + + wrapper.findComponent(ConfigurationInstanceDropdown).vm.$emit("remove"); + await flushPromises(); + + expect(toastErrorMock).toHaveBeenCalledWith("Unable to remove", "Failed to remove instance"); + expect(wrapper.emitted("entryRemoved")).toBeFalsy(); + }); +}); diff --git a/client/src/components/ObjectStore/Instances/InstanceDropdown.vue b/client/src/components/ObjectStore/Instances/InstanceDropdown.vue index aef05a0d50b..4fab7668500 100644 --- a/client/src/components/ObjectStore/Instances/InstanceDropdown.vue +++ b/client/src/components/ObjectStore/Instances/InstanceDropdown.vue @@ -2,8 +2,9 @@ import { computed } from "vue"; import { GalaxyApi } from "@/api"; +import { Toast } from "@/composables/toast"; import { useObjectStoreTemplatesStore } from "@/stores/objectStoreTemplatesStore"; -import { rethrowSimple } from "@/utils/simple-error"; +import { errorMessageAsString } from "@/utils/simple-error"; import type { UserConcreteObjectStore } from "./types"; @@ -29,7 +30,8 @@ async function onRemove() { }); if (error) { - rethrowSimple(error); + Toast.error(errorMessageAsString(error, "Failed to remove instance."), "Failed to remove instance"); + return; } emit("entryRemoved");