From f45f49169fd4188a6c91cf2fc8b3ac5a8d33b266 Mon Sep 17 00:00:00 2001 From: James Chapman Date: Wed, 10 Dec 2025 15:23:09 +0000 Subject: [PATCH 01/15] [WIP] Submitting new properties during upgrade --- api_app/_version.py | 2 +- api_app/db/repositories/resources.py | 23 +- ui/app/package-lock.json | 4 +- ui/app/package.json | 2 +- .../shared/ConfirmUpgradeResource.tsx | 229 +++++++++++++++++- 5 files changed, 246 insertions(+), 14 deletions(-) diff --git a/api_app/_version.py b/api_app/_version.py index 4e32d0c791..a5e1b0f66d 100644 --- a/api_app/_version.py +++ b/api_app/_version.py @@ -1 +1 @@ -__version__ = "0.25.4" +__version__ = "0.25.5" diff --git a/api_app/db/repositories/resources.py b/api_app/db/repositories/resources.py index efa8af841e..3bdfc307fa 100644 --- a/api_app/db/repositories/resources.py +++ b/api_app/db/repositories/resources.py @@ -126,7 +126,7 @@ async def patch_resource(self, resource: Resource, resource_patch: ResourcePatch resource.templateVersion = resource_patch.templateVersion if resource_patch.properties is not None and len(resource_patch.properties) > 0: - self.validate_patch(resource_patch, resource_template_repo, resource_template, resource_action) + await self.validate_patch(resource_patch, resource_template_repo, resource_template, resource_action) # if we're here then we're valid - update the props + persist resource.properties.update(resource_patch.properties) @@ -183,16 +183,31 @@ async def validate_template_version_patch(self, resource: Resource, resource_pat except EntityDoesNotExist: raise TargetTemplateVersionDoesNotExist(f"Template '{resource_template.name}' not found for resource type '{resource_template.resourceType}' with target template version '{resource_patch.templateVersion}'") - def validate_patch(self, resource_patch: ResourcePatch, resource_template_repo: ResourceTemplateRepository, resource_template: ResourceTemplate, resource_action: str): + async def validate_patch(self, resource_patch: ResourcePatch, resource_template_repo: ResourceTemplateRepository, resource_template: ResourceTemplate, resource_action: str): # get the enriched (combined) template enriched_template = resource_template_repo.enrich_template(resource_template, is_update=True) - # validate the PATCH data against a cut down version of the full template. + # get the schema for the target version if upgrade is happening + if resource_patch.templateVersion is not None: + # fetch the template for the target version + target_template = await resource_template_repo.get_template_by_name_and_version(resource_template.name, resource_patch.templateVersion, resource_template.resourceType) + enriched_template = resource_template_repo.enrich_template(target_template, is_update=True) + + # # validate the PATCH data against the target schema. update_template = copy.deepcopy(enriched_template) update_template["required"] = [] update_template["properties"] = {} for prop_name, prop in enriched_template["properties"].items(): - if (resource_action == RESOURCE_ACTION_INSTALL or prop.get("updateable", False) is True): + # Allow property if installing, explicitly updateable, or when upgrading and the new property is being added in this patch + if ( + resource_action == RESOURCE_ACTION_INSTALL + or prop.get("updateable", False) is True + or ( + resource_patch.templateVersion is not None + and resource_patch.properties is not None + and prop_name in resource_patch.properties + ) + ): update_template["properties"][prop_name] = prop self._validate_resource_parameters(resource_patch.dict(), update_template) diff --git a/ui/app/package-lock.json b/ui/app/package-lock.json index b0fbb60cf3..8c4aafd216 100644 --- a/ui/app/package-lock.json +++ b/ui/app/package-lock.json @@ -1,12 +1,12 @@ { "name": "tre-ui", - "version": "0.8.19", + "version": "0.8.20", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "tre-ui", - "version": "0.8.19", + "version": "0.8.20", "dependencies": { "@azure/msal-browser": "^2.35.0", "@azure/msal-react": "^1.5.12", diff --git a/ui/app/package.json b/ui/app/package.json index 345c0297ef..3400a0c5f3 100644 --- a/ui/app/package.json +++ b/ui/app/package.json @@ -1,6 +1,6 @@ { "name": "tre-ui", - "version": "0.8.19", + "version": "0.8.20", "private": true, "type": "module", "dependencies": { diff --git a/ui/app/src/components/shared/ConfirmUpgradeResource.tsx b/ui/app/src/components/shared/ConfirmUpgradeResource.tsx index 8967b08e01..3742928ad4 100644 --- a/ui/app/src/components/shared/ConfirmUpgradeResource.tsx +++ b/ui/app/src/components/shared/ConfirmUpgradeResource.tsx @@ -8,8 +8,9 @@ import { MessageBar, MessageBarType, Icon, + Stack, } from "@fluentui/react"; -import React, { useContext, useState } from "react"; +import React, { useContext, useState, useEffect } from "react"; import { AvailableUpgrade, Resource } from "../../models/resource"; import { HttpMethod, @@ -23,24 +24,102 @@ import { LoadingState } from "../../models/loadingState"; import { ExceptionLayout } from "./ExceptionLayout"; import { useAppDispatch } from "../../hooks/customReduxHooks"; import { addUpdateOperation } from "../shared/notifications/operationsSlice"; +import Form from "@rjsf/fluent-ui"; +import validator from "@rjsf/validator-ajv8"; interface ConfirmUpgradeProps { resource: Resource; onDismiss: () => void; } +// Utility to get all property keys from template schema's properties object recursively, flattening nested if needed +const getAllPropertyKeys = (properties: any, prefix = ""): string[] => { + if (!properties) return []; + let keys: string[] = []; + for (const [key, value] of Object.entries(properties)) { + if (value && typeof value === "object" && 'properties' in value) { + // recur for nested properties + keys = keys.concat(getAllPropertyKeys(value["properties"], prefix + key + ".")); + } else { + keys.push(prefix + key); + } + } + return keys; +}; + +// Utility to build a reduced schema with only given keys and their nested schema (depth 1), including required +const buildReducedSchema = (fullSchema: any, keys: string[]): any => { + if (!fullSchema || !fullSchema.properties) return null; + const reducedProperties: any = {}; + const required: string[] = []; + + keys.forEach((key) => { + // Only allow top-level property keys (no nested with dots) for simplicity here + const topKey = key.split('.')[0]; + if (fullSchema.properties[topKey]) { + if (!reducedProperties[topKey]) { + reducedProperties[topKey] = fullSchema.properties[topKey]; + if (fullSchema.required && fullSchema.required.includes(topKey)) { + required.push(topKey); + } + } + } + }); + + return { + type: "object", + properties: reducedProperties, + required: required.length > 0 ? required : undefined, + }; +}; + +// Utility to collect direct property keys referenced inside an 'if' schema (simple/general) +const collectIfKeys = (ifSchema: any): string[] => { + const keys: string[] = []; + if (!ifSchema) return keys; + // handle simple 'properties' usage + if (ifSchema.properties) { + keys.push(...Object.keys(ifSchema.properties)); + } + // handle consts inside properties: { properties: { foo: { const: ... } } } + // other complex constructs are ignored for simplicity + return keys; +}; + +// Extract conditional blocks where the 'if' references any of the new keys. Include their then/else blocks. +const extractConditionalBlocks = (schema: any, newKeys: string[]) => { + const conditionalEntries: any[] = []; + if (!schema) return { allOf: [] }; + const allOf = schema.allOf || []; + allOf.forEach((entry: any) => { + if (entry && entry.if) { + const ifKeys = collectIfKeys(entry.if); + // include entry if any ifKey matches a new key (top-level match) + if (ifKeys.some((k) => newKeys.some((nk) => nk.split('.')[0] === k))) { + conditionalEntries.push(entry); + } + } + }); + return { allOf: conditionalEntries }; +}; + export const ConfirmUpgradeResource: React.FunctionComponent< ConfirmUpgradeProps > = (props: ConfirmUpgradeProps) => { const apiCall = useAuthApiCall(); const [selectedVersion, setSelectedVersion] = useState(""); - const [apiError, setApiError] = useState({} as APIError); + const [apiError, setApiError] = useState(null); const [requestLoadingState, setRequestLoadingState] = useState( LoadingState.Ok, ); const workspaceCtx = useContext(WorkspaceContext); const dispatch = useAppDispatch(); + const [newPropertiesToFill, setNewPropertiesToFill] = useState([]); + const [newPropertyValues, setNewPropertyValues] = useState({}); + const [loadingSchema, setLoadingSchema] = useState(false); + const [newTemplateSchema, setNewTemplateSchema] = useState(null); + const upgradeProps = { type: DialogType.normal, title: `Upgrade Template Version?`, @@ -60,10 +139,94 @@ export const ConfirmUpgradeResource: React.FunctionComponent< props.resource.resourceType === ResourceType.WorkspaceService || props.resource.resourceType === ResourceType.UserResource; + // Fetch new template schema and identify new properties missing in current resource + useEffect(() => { + if (!selectedVersion) { + setNewPropertiesToFill([]); + setNewPropertyValues({}); + setNewTemplateSchema(null); + return; + } + + const fetchNewTemplateSchema = async () => { + setLoadingSchema(true); + setApiError(null); + try { + let fetchUrl = ""; + if (props.resource.resourceType === ResourceType.Workspace) { + fetchUrl = `/workspace-templates/${props.resource.templateName}?version=${selectedVersion}`; + } else { + const templateType = props.resource.resourceType.toLowerCase(); + fetchUrl = `/templates/${templateType}/${selectedVersion}`; + } + + const res = await apiCall( + fetchUrl, + HttpMethod.Get, + wsAuth ? workspaceCtx.workspaceApplicationIdURI : undefined, + undefined, + ResultType.JSON, + ); + + // Use full fetched schema from API + setNewTemplateSchema(res); + + const newSchemaProps = res?.properties || {}; + const currentProps = props.resource.properties || {}; + + const newKeys = getAllPropertyKeys(newSchemaProps); + const currentKeys = getAllPropertyKeys(currentProps); + + const newPropKeys = newKeys.filter((k) => !currentKeys.includes(k)); + + setNewPropertiesToFill(newPropKeys); + + // prefill newPropertyValues with schema defaults or empty string + setNewPropertyValues( + newPropKeys.reduce((acc, key) => { + // Get top-level portion of the key + const topKey = key.split('.')[0]; + const defaultValue = res?.properties?.[topKey]?.default; + acc[key] = defaultValue !== undefined ? defaultValue : ''; + return acc; + }, {} as any), + ); + } catch (err: any) { + if (!err.userMessage) { + err.userMessage = "Failed to fetch new template schema"; + } + setApiError(err); + } finally { + setLoadingSchema(false); + } + }; + + fetchNewTemplateSchema(); + }, [selectedVersion]); + const upgradeCall = async () => { setRequestLoadingState(LoadingState.Loading); try { - let body = { templateVersion: selectedVersion }; + let body: any = { templateVersion: selectedVersion }; + + // Patch only the new properties inside the 'properties' field + if (!body.properties) { + body.properties = {}; + } + console.log("New property values to set:", newPropertyValues); + Object.keys(newPropertyValues).forEach((key) => { + const val = newPropertyValues[key]; + body.properties[key] = val; + }); + console.log("Final upgrade body:", body); + + // Include other optional properties if applicable + // Object.keys(newPropertyValues).forEach((key) => { + // if (newPropertiesToFill.includes(key)) { + // body.properties[key] = newPropertyValues[key]; + // } + // }); + let op = await apiCall( props.resource.resourcePath, HttpMethod.Patch, @@ -77,12 +240,38 @@ export const ConfirmUpgradeResource: React.FunctionComponent< dispatch(addUpdateOperation(op.operation)); props.onDismiss(); } catch (err: any) { - err.userMessage = "Failed to upgrade resource"; + if (!err.userMessage) { + err.userMessage = "Failed to upgrade resource"; + } setApiError(err); setRequestLoadingState(LoadingState.Error); } }; + // Use buildReducedSchema to include only new properties + const reducedSchemaProperties = newTemplateSchema + ? buildReducedSchema(newTemplateSchema, newPropertiesToFill) + : null; + + // Extract any conditional blocks from full schema, filtered by new properties + const conditionalBlocks = newTemplateSchema ? extractConditionalBlocks(newTemplateSchema, newPropertiesToFill) : {}; + + // Compose final schema combining reduced properties with conditional blocks + const finalSchema = reducedSchemaProperties + ? { ...reducedSchemaProperties, ...conditionalBlocks } + : null; + + // UI schema override: hide the form's submit button because we use external Upgrade button + // start with existing UI order and classNames from full schema uiSchema + const baseUiSchema = newTemplateSchema?.uiSchema || {}; + + // Compose final uiSchema merging baseUiSchema with our overrides + const uiSchema = { + ...baseUiSchema, + "ui:submitButtonOptions": { norender: true }, + // overview: { "ui:widget": "textarea" }, + }; + const onRenderOption = (option: any): JSX.Element => { return (
@@ -129,6 +318,28 @@ export const ConfirmUpgradeResource: React.FunctionComponent< Upgrading the template version is irreversible. + + {loadingSchema && } + + {!loadingSchema && newPropertiesToFill.length > 0 && ( + + + You must specify values for new properties: + + + {finalSchema && ( +
setNewPropertyValues(e.formData)} + /> + )} + + )} + 0 && + Object.values(newPropertyValues).some( + (v) => v === "" || v === undefined, + )) + } text="Upgrade" onClick={() => upgradeCall()} /> @@ -156,7 +373,7 @@ export const ConfirmUpgradeResource: React.FunctionComponent< /> )} {requestLoadingState === LoadingState.Error && ( - + )} From 52de907c92f3147503dd1c9e81fc210b22612a18 Mon Sep 17 00:00:00 2001 From: "google-labs-jules[bot]" <161369871+google-labs-jules[bot]@users.noreply.github.com> Date: Wed, 10 Dec 2025 15:39:21 +0000 Subject: [PATCH 02/15] fix(ui): Handle conditional properties in upgrade form This commit aligns the resource upgrade process with the update process by correctly handling conditional properties in the JSON schema. - The schema generation logic in `ConfirmUpgradeResource.tsx` is updated to include conditional blocks (`if`/`then`/`else`) when the condition is based on an existing property. - New read-only properties are now submitted during the upgrade process. --- .../shared/ConfirmUpgradeResource.tsx | 30 ++++++++++--------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/ui/app/src/components/shared/ConfirmUpgradeResource.tsx b/ui/app/src/components/shared/ConfirmUpgradeResource.tsx index 3742928ad4..213a49a4c7 100644 --- a/ui/app/src/components/shared/ConfirmUpgradeResource.tsx +++ b/ui/app/src/components/shared/ConfirmUpgradeResource.tsx @@ -73,29 +73,31 @@ const buildReducedSchema = (fullSchema: any, keys: string[]): any => { }; }; -// Utility to collect direct property keys referenced inside an 'if' schema (simple/general) -const collectIfKeys = (ifSchema: any): string[] => { +// Utility to collect direct property keys referenced inside conditional schemas +const collectConditionalKeys = (entry: any): string[] => { const keys: string[] = []; - if (!ifSchema) return keys; - // handle simple 'properties' usage - if (ifSchema.properties) { - keys.push(...Object.keys(ifSchema.properties)); - } - // handle consts inside properties: { properties: { foo: { const: ... } } } - // other complex constructs are ignored for simplicity - return keys; + if (!entry) return keys; + const collect = (schemaPart: any) => { + if (schemaPart && schemaPart.properties) { + keys.push(...Object.keys(schemaPart.properties)); + } + }; + collect(entry.if); + collect(entry.then); + collect(entry.else); + return [...new Set(keys)]; }; -// Extract conditional blocks where the 'if' references any of the new keys. Include their then/else blocks. +// Extract conditional blocks that reference any of the new keys. const extractConditionalBlocks = (schema: any, newKeys: string[]) => { const conditionalEntries: any[] = []; if (!schema) return { allOf: [] }; const allOf = schema.allOf || []; allOf.forEach((entry: any) => { if (entry && entry.if) { - const ifKeys = collectIfKeys(entry.if); - // include entry if any ifKey matches a new key (top-level match) - if (ifKeys.some((k) => newKeys.some((nk) => nk.split('.')[0] === k))) { + const conditionalKeys = collectConditionalKeys(entry); + // include entry if any conditionalKey matches a new key (top-level match) + if (conditionalKeys.some((k) => newKeys.some((nk) => nk.split('.')[0] === k))) { conditionalEntries.push(entry); } } From 25ded790502613ff2d65c095d7a5b7a3d5aa5d0b Mon Sep 17 00:00:00 2001 From: "google-labs-jules[bot]" <161369871+google-labs-jules[bot]@users.noreply.github.com> Date: Wed, 10 Dec 2025 15:52:24 +0000 Subject: [PATCH 03/15] fix(ui): Handle conditional properties in upgrade form This commit aligns the resource upgrade process with the update process by correctly handling conditional properties in the JSON schema. - The schema generation logic in `ConfirmUpgradeResource.tsx` is updated to include conditional blocks (`if`/`then`/`else`) when the condition is based on an existing property. - New read-only properties are now submitted during the upgrade process. - The `liveOmit` prop is added to the form to prevent the submission of unevaluated properties from conditionally hidden fields. --- ui/app/src/components/shared/ConfirmUpgradeResource.tsx | 1 + 1 file changed, 1 insertion(+) diff --git a/ui/app/src/components/shared/ConfirmUpgradeResource.tsx b/ui/app/src/components/shared/ConfirmUpgradeResource.tsx index 213a49a4c7..78abe7e8bc 100644 --- a/ui/app/src/components/shared/ConfirmUpgradeResource.tsx +++ b/ui/app/src/components/shared/ConfirmUpgradeResource.tsx @@ -331,6 +331,7 @@ export const ConfirmUpgradeResource: React.FunctionComponent< {finalSchema && ( Date: Thu, 11 Dec 2025 16:03:41 +0000 Subject: [PATCH 04/15] support all types of template --- .../shared/ConfirmUpgradeResource.tsx | 68 ++++++++++++------- 1 file changed, 43 insertions(+), 25 deletions(-) diff --git a/ui/app/src/components/shared/ConfirmUpgradeResource.tsx b/ui/app/src/components/shared/ConfirmUpgradeResource.tsx index 78abe7e8bc..a2a94c4067 100644 --- a/ui/app/src/components/shared/ConfirmUpgradeResource.tsx +++ b/ui/app/src/components/shared/ConfirmUpgradeResource.tsx @@ -12,6 +12,8 @@ import { } from "@fluentui/react"; import React, { useContext, useState, useEffect } from "react"; import { AvailableUpgrade, Resource } from "../../models/resource"; +import { ApiEndpoint } from "../../models/apiEndpoints"; +import { WorkspaceService } from "../../models/workspaceService"; import { HttpMethod, ResultType, @@ -88,7 +90,7 @@ const collectConditionalKeys = (entry: any): string[] => { return [...new Set(keys)]; }; -// Extract conditional blocks that reference any of the new keys. +// Extract conditional blocks that reference any of the new properties. const extractConditionalBlocks = (schema: any, newKeys: string[]) => { const conditionalEntries: any[] = []; if (!schema) return { allOf: [] }; @@ -150,17 +152,50 @@ export const ConfirmUpgradeResource: React.FunctionComponent< return; } + // Construct API paths for templates of specified resourceType + let templateListPath; + // Usually, the GET path would be `${templateGetPath}/${selectedTemplate}`, but there's an exception for user resources + let templateGetPath; + + let workspaceApplicationIdURI = undefined; + switch (props.resource.resourceType) { + case ResourceType.Workspace: + templateListPath = ApiEndpoint.WorkspaceTemplates; + templateGetPath = templateListPath; + break; + case ResourceType.WorkspaceService: + templateListPath = ApiEndpoint.WorkspaceServiceTemplates; + templateGetPath = templateListPath; + break; + case ResourceType.SharedService: + templateListPath = ApiEndpoint.SharedServiceTemplates; + templateGetPath = templateListPath; + break; + case ResourceType.UserResource: + if (props.resource.properties.parentWorkspaceService) { + // If we are upgrading a user resource, parent resource must have a workspaceId + const workspaceId = (props.resource.properties.parentWorkspaceService as WorkspaceService) + .workspaceId; + templateListPath = `${ApiEndpoint.Workspaces}/${workspaceId}/${ApiEndpoint.WorkspaceServiceTemplates}/${props.resource.properties.parentWorkspaceService.templateName}/${ApiEndpoint.UserResourceTemplates}`; + templateGetPath = `${ApiEndpoint.WorkspaceServiceTemplates}/${props.resource.properties.parentWorkspaceService.templateName}/${ApiEndpoint.UserResourceTemplates}`; + workspaceApplicationIdURI = props.resource.properties.parentWorkspaceService.workspaceApplicationIdURI; + break; + } else { + throw Error( + "Parent workspace service must be passed as prop when creating user resource.", + ); + } + default: + throw Error("Unsupported resource type."); + } + const fetchNewTemplateSchema = async () => { setLoadingSchema(true); setApiError(null); try { let fetchUrl = ""; - if (props.resource.resourceType === ResourceType.Workspace) { - fetchUrl = `/workspace-templates/${props.resource.templateName}?version=${selectedVersion}`; - } else { - const templateType = props.resource.resourceType.toLowerCase(); - fetchUrl = `/templates/${templateType}/${selectedVersion}`; - } + + fetchUrl = `${templateGetPath}/${props.resource.templateName}?version=${selectedVersion}`; const res = await apiCall( fetchUrl, @@ -211,23 +246,7 @@ export const ConfirmUpgradeResource: React.FunctionComponent< try { let body: any = { templateVersion: selectedVersion }; - // Patch only the new properties inside the 'properties' field - if (!body.properties) { - body.properties = {}; - } - console.log("New property values to set:", newPropertyValues); - Object.keys(newPropertyValues).forEach((key) => { - const val = newPropertyValues[key]; - body.properties[key] = val; - }); - console.log("Final upgrade body:", body); - - // Include other optional properties if applicable - // Object.keys(newPropertyValues).forEach((key) => { - // if (newPropertiesToFill.includes(key)) { - // body.properties[key] = newPropertyValues[key]; - // } - // }); + body.properties = newPropertyValues; let op = await apiCall( props.resource.resourcePath, @@ -271,7 +290,6 @@ export const ConfirmUpgradeResource: React.FunctionComponent< const uiSchema = { ...baseUiSchema, "ui:submitButtonOptions": { norender: true }, - // overview: { "ui:widget": "textarea" }, }; const onRenderOption = (option: any): JSX.Element => { From 1797f336a60fe5bbb1e53bcf93c328dae4cb639c Mon Sep 17 00:00:00 2001 From: James Chapman Date: Thu, 11 Dec 2025 17:14:45 +0000 Subject: [PATCH 05/15] show properties to be removed --- .../shared/ConfirmUpgradeResource.tsx | 34 ++++++++++++++----- 1 file changed, 26 insertions(+), 8 deletions(-) diff --git a/ui/app/src/components/shared/ConfirmUpgradeResource.tsx b/ui/app/src/components/shared/ConfirmUpgradeResource.tsx index a2a94c4067..0a3bd3fb22 100644 --- a/ui/app/src/components/shared/ConfirmUpgradeResource.tsx +++ b/ui/app/src/components/shared/ConfirmUpgradeResource.tsx @@ -123,6 +123,7 @@ export const ConfirmUpgradeResource: React.FunctionComponent< const [newPropertyValues, setNewPropertyValues] = useState({}); const [loadingSchema, setLoadingSchema] = useState(false); const [newTemplateSchema, setNewTemplateSchema] = useState(null); + const [removedProperties, setRemovedProperties] = useState([]); const upgradeProps = { type: DialogType.normal, @@ -149,6 +150,7 @@ export const ConfirmUpgradeResource: React.FunctionComponent< setNewPropertiesToFill([]); setNewPropertyValues({}); setNewTemplateSchema(null); + setRemovedProperties([]); return; } @@ -197,7 +199,7 @@ export const ConfirmUpgradeResource: React.FunctionComponent< fetchUrl = `${templateGetPath}/${props.resource.templateName}?version=${selectedVersion}`; - const res = await apiCall( + const newTemplate = await apiCall( fetchUrl, HttpMethod.Get, wsAuth ? workspaceCtx.workspaceApplicationIdURI : undefined, @@ -205,25 +207,37 @@ export const ConfirmUpgradeResource: React.FunctionComponent< ResultType.JSON, ); + const currentTemplate = await apiCall( + `${templateGetPath}/${props.resource.templateName}?version=${props.resource.templateVersion}`, + HttpMethod.Get, + wsAuth ? workspaceCtx.workspaceApplicationIdURI : undefined, + undefined, + ResultType.JSON, + ); + // Use full fetched schema from API - setNewTemplateSchema(res); + setNewTemplateSchema(newTemplate); - const newSchemaProps = res?.properties || {}; - const currentProps = props.resource.properties || {}; + const newSchemaProps = newTemplate?.properties || {}; + const currentProps = currentTemplate?.properties || {}; const newKeys = getAllPropertyKeys(newSchemaProps); + console.log("New template property keys:", newKeys); const currentKeys = getAllPropertyKeys(currentProps); - + console.log("Current resource property keys:", currentKeys); const newPropKeys = newKeys.filter((k) => !currentKeys.includes(k)); - + console.log("Identified new property keys to fill:", newPropKeys); + const removedPropsArray = currentKeys.filter((k) => !newKeys.includes(k)); + console.log("Identified removed property keys:", removedPropsArray); setNewPropertiesToFill(newPropKeys); + setRemovedProperties(removedPropsArray); // prefill newPropertyValues with schema defaults or empty string setNewPropertyValues( newPropKeys.reduce((acc, key) => { // Get top-level portion of the key const topKey = key.split('.')[0]; - const defaultValue = res?.properties?.[topKey]?.default; + const defaultValue = newTemplate?.properties?.[topKey]?.default; acc[key] = defaultValue !== undefined ? defaultValue : ''; return acc; }, {} as any), @@ -340,7 +354,11 @@ export const ConfirmUpgradeResource: React.FunctionComponent< {loadingSchema && } - + {!loadingSchema && removedProperties.length > 0 && ( + + Warning: The following properties are no longer present and will be removed: {removedProperties.join(', ')} + + )} {!loadingSchema && newPropertiesToFill.length > 0 && ( From 01f3245c221034dbc242a71bb46a1e231356388f Mon Sep 17 00:00:00 2001 From: James Chapman Date: Thu, 11 Dec 2025 17:15:12 +0000 Subject: [PATCH 06/15] remove logging --- ui/app/src/components/shared/ConfirmUpgradeResource.tsx | 4 ---- 1 file changed, 4 deletions(-) diff --git a/ui/app/src/components/shared/ConfirmUpgradeResource.tsx b/ui/app/src/components/shared/ConfirmUpgradeResource.tsx index 0a3bd3fb22..b274f9486e 100644 --- a/ui/app/src/components/shared/ConfirmUpgradeResource.tsx +++ b/ui/app/src/components/shared/ConfirmUpgradeResource.tsx @@ -222,13 +222,9 @@ export const ConfirmUpgradeResource: React.FunctionComponent< const currentProps = currentTemplate?.properties || {}; const newKeys = getAllPropertyKeys(newSchemaProps); - console.log("New template property keys:", newKeys); const currentKeys = getAllPropertyKeys(currentProps); - console.log("Current resource property keys:", currentKeys); const newPropKeys = newKeys.filter((k) => !currentKeys.includes(k)); - console.log("Identified new property keys to fill:", newPropKeys); const removedPropsArray = currentKeys.filter((k) => !newKeys.includes(k)); - console.log("Identified removed property keys:", removedPropsArray); setNewPropertiesToFill(newPropKeys); setRemovedProperties(removedPropsArray); From 4b66ea543c353da528fc986d8350d2f37220191a Mon Sep 17 00:00:00 2001 From: James Chapman Date: Fri, 12 Dec 2025 10:46:27 +0000 Subject: [PATCH 07/15] update warning message --- ui/app/src/components/shared/ConfirmUpgradeResource.tsx | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/ui/app/src/components/shared/ConfirmUpgradeResource.tsx b/ui/app/src/components/shared/ConfirmUpgradeResource.tsx index b274f9486e..263938c660 100644 --- a/ui/app/src/components/shared/ConfirmUpgradeResource.tsx +++ b/ui/app/src/components/shared/ConfirmUpgradeResource.tsx @@ -159,7 +159,7 @@ export const ConfirmUpgradeResource: React.FunctionComponent< // Usually, the GET path would be `${templateGetPath}/${selectedTemplate}`, but there's an exception for user resources let templateGetPath; - let workspaceApplicationIdURI = undefined; + // let workspaceApplicationIdURI = undefined; switch (props.resource.resourceType) { case ResourceType.Workspace: templateListPath = ApiEndpoint.WorkspaceTemplates; @@ -180,7 +180,7 @@ export const ConfirmUpgradeResource: React.FunctionComponent< .workspaceId; templateListPath = `${ApiEndpoint.Workspaces}/${workspaceId}/${ApiEndpoint.WorkspaceServiceTemplates}/${props.resource.properties.parentWorkspaceService.templateName}/${ApiEndpoint.UserResourceTemplates}`; templateGetPath = `${ApiEndpoint.WorkspaceServiceTemplates}/${props.resource.properties.parentWorkspaceService.templateName}/${ApiEndpoint.UserResourceTemplates}`; - workspaceApplicationIdURI = props.resource.properties.parentWorkspaceService.workspaceApplicationIdURI; + // workspaceApplicationIdURI = props.resource.properties.parentWorkspaceService.workspaceApplicationIdURI; break; } else { throw Error( @@ -352,7 +352,7 @@ export const ConfirmUpgradeResource: React.FunctionComponent< {loadingSchema && } {!loadingSchema && removedProperties.length > 0 && ( - Warning: The following properties are no longer present and will be removed: {removedProperties.join(', ')} + Warning: The following properties are no longer present in the template and will be removed: {removedProperties.join(', ')} )} {!loadingSchema && newPropertiesToFill.length > 0 && ( From f6c044eb5bd76af0af588bfe4436149c3955ce6e Mon Sep 17 00:00:00 2001 From: James Chapman Date: Fri, 12 Dec 2025 16:55:59 +0000 Subject: [PATCH 08/15] add capability to remove properties that no longer exist in the template --- api_app/db/repositories/resources.py | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/api_app/db/repositories/resources.py b/api_app/db/repositories/resources.py index 3bdfc307fa..938fc47b56 100644 --- a/api_app/db/repositories/resources.py +++ b/api_app/db/repositories/resources.py @@ -111,6 +111,16 @@ async def validate_input_against_template(self, template_name: str, resource_inp return parse_obj_as(ResourceTemplate, template) + def _get_all_property_keys_from_template(self, resource_template: ResourceTemplate) -> set: + properties = set(resource_template.properties.keys()) + if resource_template.allOf: + for condition in resource_template.allOf: + if 'then' in condition and 'properties' in condition['then']: + properties.update(condition['then']['properties'].keys()) + if 'else' in condition and 'properties' in condition['else']: + properties.update(condition['else']['properties'].keys()) + return properties + async def patch_resource(self, resource: Resource, resource_patch: ResourcePatch, resource_template: ResourceTemplate, etag: str, resource_template_repo: ResourceTemplateRepository, resource_history_repo: ResourceHistoryRepository, user: User, resource_action: str, force_version_update: bool = False) -> Tuple[Resource, ResourceTemplate]: await resource_history_repo.create_resource_history_item(resource) # now update the resource props @@ -123,6 +133,16 @@ async def patch_resource(self, resource: Resource, resource_patch: ResourcePatch if resource_patch.templateVersion is not None: await self.validate_template_version_patch(resource, resource_patch, resource_template_repo, resource_template, force_version_update) + new_template = await resource_template_repo.get_template_by_name_and_version(resource.templateName, resource_patch.templateVersion, resource.resourceType) + + old_properties = self._get_all_property_keys_from_template(resource_template) + new_properties = self._get_all_property_keys_from_template(new_template) + + properties_to_remove = old_properties - new_properties + for prop in properties_to_remove: + if prop in resource.properties: + del resource.properties[prop] + resource.templateVersion = resource_patch.templateVersion if resource_patch.properties is not None and len(resource_patch.properties) > 0: @@ -193,7 +213,7 @@ async def validate_patch(self, resource_patch: ResourcePatch, resource_template_ target_template = await resource_template_repo.get_template_by_name_and_version(resource_template.name, resource_patch.templateVersion, resource_template.resourceType) enriched_template = resource_template_repo.enrich_template(target_template, is_update=True) - # # validate the PATCH data against the target schema. + # validate the PATCH data against the target schema. update_template = copy.deepcopy(enriched_template) update_template["required"] = [] update_template["properties"] = {} From d06037d330b5dce0b190c9c532839404cd9ed638 Mon Sep 17 00:00:00 2001 From: James Chapman Date: Mon, 15 Dec 2025 17:05:59 +0000 Subject: [PATCH 09/15] add tests and resolve issue causing failing test --- api_app/db/repositories/resources.py | 20 +- .../test_resource_repository.py | 178 +++++++++++++++++- 2 files changed, 184 insertions(+), 14 deletions(-) diff --git a/api_app/db/repositories/resources.py b/api_app/db/repositories/resources.py index 938fc47b56..bbf3f30eba 100644 --- a/api_app/db/repositories/resources.py +++ b/api_app/db/repositories/resources.py @@ -113,12 +113,12 @@ async def validate_input_against_template(self, template_name: str, resource_inp def _get_all_property_keys_from_template(self, resource_template: ResourceTemplate) -> set: properties = set(resource_template.properties.keys()) - if resource_template.allOf: + if "allOf" in resource_template: for condition in resource_template.allOf: - if 'then' in condition and 'properties' in condition['then']: - properties.update(condition['then']['properties'].keys()) - if 'else' in condition and 'properties' in condition['else']: - properties.update(condition['else']['properties'].keys()) + if "then" in condition and "properties" in condition["then"]: + properties.update(condition["then"]["properties"].keys()) + if "else" in condition and "properties" in condition["else"]: + properties.update(condition["else"]["properties"].keys()) return properties async def patch_resource(self, resource: Resource, resource_patch: ResourcePatch, resource_template: ResourceTemplate, etag: str, resource_template_repo: ResourceTemplateRepository, resource_history_repo: ResourceHistoryRepository, user: User, resource_action: str, force_version_update: bool = False) -> Tuple[Resource, ResourceTemplate]: @@ -207,6 +207,9 @@ async def validate_patch(self, resource_patch: ResourcePatch, resource_template_ # get the enriched (combined) template enriched_template = resource_template_repo.enrich_template(resource_template, is_update=True) + # get the old template properties for comparison during upgrades + old_template_properties = set(enriched_template["properties"].keys()) + # get the schema for the target version if upgrade is happening if resource_patch.templateVersion is not None: # fetch the template for the target version @@ -218,14 +221,19 @@ async def validate_patch(self, resource_patch: ResourcePatch, resource_template_ update_template["required"] = [] update_template["properties"] = {} for prop_name, prop in enriched_template["properties"].items(): - # Allow property if installing, explicitly updateable, or when upgrading and the new property is being added in this patch + # Allow property if: + # 1. Installing (all properties allowed) + # 2. Property is explicitly updateable + # 3. Upgrading and the property is NEW (not in old template) and being added in this patch if ( resource_action == RESOURCE_ACTION_INSTALL or prop.get("updateable", False) is True or ( resource_patch.templateVersion is not None + and resource_patch.templateVersion != resource_template.version and resource_patch.properties is not None and prop_name in resource_patch.properties + and prop_name not in old_template_properties ) ): update_template["properties"][prop_name] = prop diff --git a/api_app/tests_ma/test_db/test_repositories/test_resource_repository.py b/api_app/tests_ma/test_db/test_repositories/test_resource_repository.py index 60b8569c53..e734b36f33 100644 --- a/api_app/tests_ma/test_db/test_repositories/test_resource_repository.py +++ b/api_app/tests_ma/test_db/test_repositories/test_resource_repository.py @@ -4,6 +4,7 @@ import pytest import pytest_asyncio from mock import patch, MagicMock +from pydantic import parse_obj_as from jsonschema.exceptions import ValidationError from resources import strings @@ -140,6 +141,58 @@ def sample_nested_template() -> ResourceTemplate: ).dict(exclude_none=True) +def sample_resource_template_with_new_property(version: str = "0.2.0") -> dict: + """ + Returns a template similar to sample_resource_template but with an additional + 'new_property' that is not updateable. Useful for testing template upgrades. + """ + return ResourceTemplate( + id="123", + name="tre-user-resource", + description="description", + version=version, + resourceType=ResourceType.UserResource, + current=True, + required=['os_image', 'title'], + properties={ + 'title': { + 'type': 'string', + 'title': 'Title of the resource' + }, + 'os_image': { + 'type': 'string', + 'title': 'Windows image', + 'description': 'Select Windows image to use for VM', + 'enum': [ + 'Windows 10', + 'Server 2019 Data Science VM' + ], + 'updateable': False + }, + 'vm_size': { + 'type': 'string', + 'title': 'VM Size', + 'description': 'Select Windows image to use for VM', + 'enum': [ + 'small', + 'large' + ], + 'updateable': True + }, + 'new_property': { + 'type': 'string', + 'title': 'New non-updateable property', + 'enum': [ + 'value1', + 'value2' + ], + 'updateable': False + } + }, + actions=[] + ).dict(exclude_none=True) + + @pytest.mark.asyncio @patch("db.repositories.resources.ResourceRepository._get_enriched_template") @patch("db.repositories.resources.ResourceRepository._validate_resource_parameters", return_value=None) @@ -370,10 +423,11 @@ async def test_patch_resource_preserves_property_history(_, __, ___, resource_re resource_repo.update_item_with_etag.assert_called_with(expected_resource, etag) +@pytest.mark.asyncio @patch('db.repositories.resources.ResourceTemplateRepository.enrich_template') -def test_validate_patch_with_good_fields_passes(template_repo, resource_repo): +async def test_validate_patch_with_good_fields_passes(template_repo, resource_repo): """ - Make sure that patch is NOT valid when non-updateable fields are included + Make sure that patch is valid when updateable fields are included """ template_repo.enrich_template = MagicMock(return_value=sample_resource_template()) @@ -381,11 +435,12 @@ def test_validate_patch_with_good_fields_passes(template_repo, resource_repo): # check it's valid when updating a single updateable prop patch = ResourcePatch(isEnabled=True, properties={'vm_size': 'large'}) - resource_repo.validate_patch(patch, template_repo, template, strings.RESOURCE_ACTION_UPDATE) + await resource_repo.validate_patch(patch, template_repo, template, strings.RESOURCE_ACTION_UPDATE) +@pytest.mark.asyncio @patch('db.repositories.resources.ResourceTemplateRepository.enrich_template') -def test_validate_patch_with_bad_fields_fails(template_repo, resource_repo): +async def test_validate_patch_with_bad_fields_fails(template_repo, resource_repo): """ Make sure that patch is NOT valid when non-updateable fields are included """ @@ -396,14 +451,121 @@ def test_validate_patch_with_bad_fields_fails(template_repo, resource_repo): # check it's invalid when sending an unexpected field patch = ResourcePatch(isEnabled=True, properties={'vm_size': 'large', 'unexpected_field': 'surprise!'}) with pytest.raises(ValidationError): - resource_repo.validate_patch(patch, template_repo, template, strings.RESOURCE_ACTION_INSTALL) + await resource_repo.validate_patch(patch, template_repo, template, strings.RESOURCE_ACTION_UPDATE) - # check it's invalid when sending a bad value + # check it's invalid when sending a bad value (new install) patch = ResourcePatch(isEnabled=True, properties={'vm_size': 'huge'}) with pytest.raises(ValidationError): - resource_repo.validate_patch(patch, template_repo, template, strings.RESOURCE_ACTION_INSTALL) + await resource_repo.validate_patch(patch, template_repo, template, strings.RESOURCE_ACTION_INSTALL) + + # check it's invalid when sending a bad value (update) + patch = ResourcePatch(isEnabled=True, properties={'vm_size': 'huge'}) + with pytest.raises(ValidationError): + await resource_repo.validate_patch(patch, template_repo, template, strings.RESOURCE_ACTION_UPDATE) # check it's invalid when trying to update a non-updateable field patch = ResourcePatch(isEnabled=True, properties={'vm_size': 'large', 'os_image': 'linux'}) with pytest.raises(ValidationError): - resource_repo.validate_patch(patch, template_repo, template, strings.RESOURCE_ACTION_INSTALL) + await resource_repo.validate_patch(patch, template_repo, template, strings.RESOURCE_ACTION_UPDATE) + + +@pytest.mark.asyncio +@patch('db.repositories.resources.ResourceTemplateRepository.get_template_by_name_and_version') +@patch('db.repositories.resources.ResourceTemplateRepository.enrich_template') +async def test_validate_patch_allows_new_non_updateable_property_during_upgrade(enrich_template_mock, get_template_mock, resource_repo): + """ + Test that during a template upgrade, new properties (not in old version) can be specified + even if they are marked as updateable: false in the new template version + """ + # Old template has os_image and vm_size + old_template = sample_resource_template() + old_template['version'] = '0.1.0' + + # New template adds a new property 'new_property' that is not updateable + new_template = sample_resource_template_with_new_property(version='0.2.0') + + # Mock the template repository + template_repo = MagicMock() + template_repo.get_template_by_name_and_version = AsyncMock(return_value=parse_obj_as(ResourceTemplate, new_template)) + template_repo.enrich_template = MagicMock(side_effect=[old_template, new_template]) + + # Patch includes the new property during upgrade - this should be ALLOWED + patch = ResourcePatch(templateVersion='0.2.0', properties={'new_property': 'value1'}) + + # This should NOT raise a ValidationError + await resource_repo.validate_patch(patch, template_repo, parse_obj_as(ResourceTemplate, old_template), strings.RESOURCE_ACTION_UPDATE) + + +@pytest.mark.asyncio +@patch('db.repositories.resources.ResourceTemplateRepository.get_template_by_name_and_version') +@patch('db.repositories.resources.ResourceTemplateRepository.enrich_template') +async def test_validate_patch_rejects_existing_non_updateable_property_during_upgrade(enrich_template_mock, get_template_mock, resource_repo): + """ + Test that during a template upgrade, existing non-updateable properties still cannot be modified + """ + # Old template has os_image (non-updateable) and vm_size (updateable) + old_template = sample_resource_template() + + # New template is the same but version 0.2.0 + new_template = copy.deepcopy(old_template) + + # Mock the template repository + template_repo = MagicMock() + template_repo.get_template_by_name_and_version = AsyncMock(return_value=parse_obj_as(ResourceTemplate, new_template)) + template_repo.enrich_template = MagicMock(side_effect=[old_template, new_template]) + + # Try to update existing non-updateable property during upgrade - this should FAIL + patch = ResourcePatch(templateVersion='0.2.0', properties={'os_image': 'Windows 10'}) + + with pytest.raises(ValidationError): + await resource_repo.validate_patch(patch, template_repo, parse_obj_as(ResourceTemplate, old_template), strings.RESOURCE_ACTION_UPDATE) + + +@pytest.mark.asyncio +@patch('db.repositories.resources.ResourceTemplateRepository.get_template_by_name_and_version') +@patch('db.repositories.resources.ResourceTemplateRepository.enrich_template') +async def test_validate_patch_allows_updateable_property_during_upgrade(enrich_template_mock, get_template_mock, resource_repo): + """ + Test that during a template upgrade, updateable properties can still be modified + """ + # Old template 0.1.0 + old_template = sample_resource_template() + + # New template 0.2.0 + new_template = copy.deepcopy(old_template) + + # Mock the template repository + template_repo = MagicMock() + template_repo.get_template_by_name_and_version = AsyncMock(return_value=parse_obj_as(ResourceTemplate, new_template)) + template_repo.enrich_template = MagicMock(side_effect=[old_template, new_template]) + + # Update existing updateable property during upgrade - this should work + patch = ResourcePatch(templateVersion='0.2.0', properties={'vm_size': 'large'}) + + # This should NOT raise a ValidationError + await resource_repo.validate_patch(patch, template_repo, parse_obj_as(ResourceTemplate, old_template), strings.RESOURCE_ACTION_UPDATE) + + +@pytest.mark.asyncio +@patch('db.repositories.resources.ResourceTemplateRepository.get_template_by_name_and_version') +@patch('db.repositories.resources.ResourceTemplateRepository.enrich_template') +async def test_validate_patch_allows_mix_of_new_and_updateable_properties_during_upgrade(enrich_template_mock, get_template_mock, resource_repo): + """ + Test that during upgrade, you can specify both new non-updateable properties and existing updateable properties + """ + # Old template + old_template = sample_resource_template() + + # New template adds new_property + new_template = sample_resource_template_with_new_property(version='0.2.0') + + # Mock the template repository + template_repo = MagicMock() + template_repo.get_template_by_name_and_version = AsyncMock(return_value=parse_obj_as(ResourceTemplate, new_template)) + template_repo.enrich_template = MagicMock(side_effect=[old_template, new_template]) + + # Patch with both new non-updateable property and existing updateable property + patch = ResourcePatch(templateVersion='0.2.0', properties={'new_property': 'value1', 'vm_size': 'large'}) + + # This should NOT raise a ValidationError + await resource_repo.validate_patch(patch, template_repo, parse_obj_as(ResourceTemplate, old_template), strings.RESOURCE_ACTION_UPDATE) From 7ad0a7af56080e3817b86b491a3d19da11c91145 Mon Sep 17 00:00:00 2001 From: James Chapman Date: Tue, 16 Dec 2025 09:48:33 +0000 Subject: [PATCH 10/15] update changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index edcbaa8586..376a3194e0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ ENHANCEMENTS: * API: Replace HTTP_422_UNPROCESSABLE_ENTITY response with HTTP_422_UNPROCESSABLE_CONTENT as per RFC 9110 ([#4742](https://github.com/microsoft/AzureTRE/issues/4742)) * Change Group.ReadWrite.All permission to Group.Create for AUTO_WORKSPACE_GROUP_CREATION ([#4772](https://github.com/microsoft/AzureTRE/issues/4772)) * Make workspace shared storage quota updateable ([#4314](https://github.com/microsoft/AzureTRE/issues/4314)) +* Allow new template properties to be specified during template upgrades. Remove Template properties that no longer exist. BUG FIXES: * Fix circular dependancy in base workspace. ([#4756](https://github.com/microsoft/AzureTRE/pull/4756)) From a27267727cb36878a5a832ad0766a8440a9292b4 Mon Sep 17 00:00:00 2001 From: James Chapman Date: Tue, 16 Dec 2025 11:06:23 +0000 Subject: [PATCH 11/15] cache currrent template --- .../shared/ConfirmUpgradeResource.tsx | 33 ++++++++++++++----- 1 file changed, 25 insertions(+), 8 deletions(-) diff --git a/ui/app/src/components/shared/ConfirmUpgradeResource.tsx b/ui/app/src/components/shared/ConfirmUpgradeResource.tsx index 263938c660..2e09f81e97 100644 --- a/ui/app/src/components/shared/ConfirmUpgradeResource.tsx +++ b/ui/app/src/components/shared/ConfirmUpgradeResource.tsx @@ -10,7 +10,7 @@ import { Icon, Stack, } from "@fluentui/react"; -import React, { useContext, useState, useEffect } from "react"; +import React, { useContext, useState, useEffect, useRef } from "react"; import { AvailableUpgrade, Resource } from "../../models/resource"; import { ApiEndpoint } from "../../models/apiEndpoints"; import { WorkspaceService } from "../../models/workspaceService"; @@ -125,6 +125,14 @@ export const ConfirmUpgradeResource: React.FunctionComponent< const [newTemplateSchema, setNewTemplateSchema] = useState(null); const [removedProperties, setRemovedProperties] = useState([]); + // Cache for current template to avoid refetching the same template repeatedly while selecting versions + const currentTemplateRef = useRef(null); + + // Invalidate cache if the resource's template name or current template version changes + useEffect(() => { + currentTemplateRef.current = null; + }, [props.resource.templateName, props.resource.templateVersion]); + const upgradeProps = { type: DialogType.normal, title: `Upgrade Template Version?`, @@ -207,13 +215,20 @@ export const ConfirmUpgradeResource: React.FunctionComponent< ResultType.JSON, ); - const currentTemplate = await apiCall( - `${templateGetPath}/${props.resource.templateName}?version=${props.resource.templateVersion}`, - HttpMethod.Get, - wsAuth ? workspaceCtx.workspaceApplicationIdURI : undefined, - undefined, - ResultType.JSON, - ); + // Reuse cached current template if available to avoid redundant network calls + let currentTemplate; + if (currentTemplateRef.current) { + currentTemplate = currentTemplateRef.current; + } else { + currentTemplate = await apiCall( + `${templateGetPath}/${props.resource.templateName}?version=${props.resource.templateVersion}`, + HttpMethod.Get, + wsAuth ? workspaceCtx.workspaceApplicationIdURI : undefined, + undefined, + ResultType.JSON, + ); + currentTemplateRef.current = currentTemplate; + } // Use full fetched schema from API setNewTemplateSchema(newTemplate); @@ -223,8 +238,10 @@ export const ConfirmUpgradeResource: React.FunctionComponent< const newKeys = getAllPropertyKeys(newSchemaProps); const currentKeys = getAllPropertyKeys(currentProps); + const newPropKeys = newKeys.filter((k) => !currentKeys.includes(k)); const removedPropsArray = currentKeys.filter((k) => !newKeys.includes(k)); + setNewPropertiesToFill(newPropKeys); setRemovedProperties(removedPropsArray); From b02794e84eb0b20857c64ee14dd4798a24098c8c Mon Sep 17 00:00:00 2001 From: James Chapman Date: Fri, 19 Dec 2025 17:45:24 +0000 Subject: [PATCH 12/15] Add test --- .../shared/ConfirmUpgradeResource.test.tsx | 704 ++++++++++++++++++ 1 file changed, 704 insertions(+) create mode 100644 ui/app/src/components/shared/ConfirmUpgradeResource.test.tsx diff --git a/ui/app/src/components/shared/ConfirmUpgradeResource.test.tsx b/ui/app/src/components/shared/ConfirmUpgradeResource.test.tsx new file mode 100644 index 0000000000..ac034b69e5 --- /dev/null +++ b/ui/app/src/components/shared/ConfirmUpgradeResource.test.tsx @@ -0,0 +1,704 @@ +import React from "react"; +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { render, screen, fireEvent, waitFor, createPartialFluentUIMock } from "../../test-utils"; +import { ConfirmUpgradeResource } from "./ConfirmUpgradeResource"; +import { Resource, AvailableUpgrade } from "../../models/resource"; +import { ResourceType } from "../../models/resourceType"; +import { WorkspaceContext } from "../../contexts/WorkspaceContext"; +import { CostResource } from "../../models/costs"; + +// Mock dependencies +const mockApiCall = vi.fn(); +const mockDispatch = vi.fn(); + +// Mock template schemas +const mockCurrentTemplateSchema = { + properties: { + display_name: { type: "string" }, + resource_key: { type: "string" }, + existing_property: { type: "string" }, + }, + required: ["display_name"], +}; + +const mockNewTemplateSchema = { + properties: { + display_name: { type: "string" }, + resource_key: { type: "string" }, + new_property: { type: "string", default: "default_value" }, + }, + required: ["display_name"], + uiSchema: {}, +}; + +vi.mock("../../hooks/useAuthApiCall", () => ({ + useAuthApiCall: () => mockApiCall, + HttpMethod: { Patch: "PATCH", Get: "GET" }, + ResultType: { JSON: "JSON" }, +})); + +vi.mock("../../hooks/customReduxHooks", () => ({ + useAppDispatch: () => mockDispatch, +})); + +vi.mock("../shared/notifications/operationsSlice", () => ({ + addUpdateOperation: vi.fn(), + default: (state: any = { items: [] }) => state +})); + +// Mock FluentUI components using centralized mocks +vi.mock("@fluentui/react", async () => { + const actual = await vi.importActual("@fluentui/react"); + return { + ...actual, + ...createPartialFluentUIMock([ + 'Dialog', + 'DialogFooter', + 'DialogType', + 'PrimaryButton', + 'DefaultButton', + 'Dropdown', + 'Spinner', + 'MessageBar', + 'MessageBarType', + 'Icon' + ]), + }; +}); + +vi.mock("./ExceptionLayout", () => ({ + ExceptionLayout: ({ e }: any) => ( +
{e.userMessage}
+ ), +})); + +const mockAvailableUpgrades: AvailableUpgrade[] = [ + { version: "1.1.0", forceUpdateRequired: false }, + { version: "1.2.0", forceUpdateRequired: false }, + { version: "2.0.0", forceUpdateRequired: true }, +]; + +const mockResource: Resource = { + id: "test-resource-id", + resourceType: ResourceType.WorkspaceService, + templateName: "test-template", + templateVersion: "1.0.0", + resourcePath: "/workspaces/test-workspace/workspace-services/test-resource-id", + resourceVersion: 1, + isEnabled: true, + properties: { + display_name: "Test Resource", + }, + _etag: "test-etag", + updatedWhen: Date.now(), + deploymentStatus: "deployed", + availableUpgrades: mockAvailableUpgrades, + history: [], + user: { + id: "test-user-id", + name: "Test User", + email: "test@example.com", + roleAssignments: [], + roles: ["workspace_researcher"], + }, +}; + +const mockWorkspaceContext = { + costs: [] as CostResource[], + workspace: { + id: "test-workspace-id", + isEnabled: true, + resourcePath: "/workspaces/test-workspace-id", + resourceVersion: 1, + resourceType: ResourceType.Workspace, + templateName: "base", + templateVersion: "1.0.0", + availableUpgrades: [], + deploymentStatus: "deployed", + updatedWhen: Date.now(), + history: [], + _etag: "test-etag", + properties: { + display_name: "Test Workspace", + }, + user: { + id: "test-user-id", + name: "Test User", + email: "test@example.com", + roleAssignments: [], + roles: ["workspace_owner"], + }, + workspaceURL: "https://workspace.example.com", + }, + workspaceApplicationIdURI: "test-app-id-uri", + roles: ["workspace_owner"], + setCosts: vi.fn(), + setRoles: vi.fn(), + setWorkspace: vi.fn(), +}; + +const renderWithWorkspaceContext = (component: React.ReactElement) => { + return render( + + {component} + + ); +}; + +describe("ConfirmUpgradeResource Component", () => { + const mockOnDismiss = vi.fn(); + + beforeEach(() => { + vi.clearAllMocks(); + // Mock API call to return templates for GET requests + mockApiCall.mockImplementation((url, method) => { + if (method === "GET" && url.includes("?version=")) { + if (url.includes("version=1.0.0")) { + return Promise.resolve(mockCurrentTemplateSchema); + } else { + return Promise.resolve(mockNewTemplateSchema); + } + } + return Promise.resolve({ operation: { id: "operation-id", status: "running" } }); + }); + }); + + it("renders upgrade dialog with correct title and content", () => { + renderWithWorkspaceContext( + + ); + + expect(screen.getByTestId("dialog-title")).toHaveTextContent( + "Upgrade Template Version?" + ); + expect(screen.getByTestId("dialog-subtext")).toHaveTextContent( + "Are you sure you want upgrade the template version of Test Resource from version 1.0.0?" + ); + }); + + it("shows warning message about irreversible upgrade", () => { + renderWithWorkspaceContext( + + ); + + expect(screen.getByTestId("message-bar")).toBeInTheDocument(); + expect(screen.getByText("Upgrading the template version is irreversible.")).toBeInTheDocument(); + }); + + it("renders dropdown with available upgrade versions", () => { + renderWithWorkspaceContext( + + ); + + const dropdown = screen.getByTestId("dropdown"); + expect(dropdown).toBeInTheDocument(); + + // Check that non-major upgrades are included (force update required = false) + expect(screen.getByText("1.1.0")).toBeInTheDocument(); + expect(screen.getByText("1.2.0")).toBeInTheDocument(); + + // Major upgrade (force update required = true) should not be included in regular dropdown + expect(screen.queryByText("2.0.0")).not.toBeInTheDocument(); + }); + + it("disables upgrade button when no version is selected", () => { + renderWithWorkspaceContext( + + ); + + const upgradeButton = screen.getByTestId("primary-button"); + expect(upgradeButton).toBeDisabled(); + }); + + it("enables upgrade button when version is selected", async () => { + renderWithWorkspaceContext( + + ); + + const dropdown = screen.getByTestId("dropdown"); + fireEvent.change(dropdown, { target: { value: "1.1.0" } }); + + // Wait for schema to load and button to become enabled + await waitFor(() => { + const upgradeButton = screen.getByTestId("primary-button"); + expect(upgradeButton).not.toBeDisabled(); + }); + }); + + it("calls API with selected version on upgrade", async () => { + const mockOperation = { id: "operation-id", status: "running" }; + mockApiCall.mockImplementation((url, method) => { + if (method === "PATCH") { + return Promise.resolve({ operation: mockOperation }); + } + // Handle GET requests for schemas + if (method === "GET" && url.includes("?version=")) { + if (url.includes("version=1.0.0")) { + return Promise.resolve(mockCurrentTemplateSchema); + } else { + return Promise.resolve(mockNewTemplateSchema); + } + } + return Promise.resolve({ operation: mockOperation }); + }); + + renderWithWorkspaceContext( + + ); + + // Select a version + const dropdown = screen.getByTestId("dropdown"); + fireEvent.change(dropdown, { target: { value: "1.1.0" } }); + + // Wait for schema to load + await waitFor(() => { + expect(screen.queryByText("Loading new template schema...")).not.toBeInTheDocument(); + }); + + // Click upgrade + const upgradeButton = screen.getByTestId("primary-button"); + fireEvent.click(upgradeButton); + + await waitFor(() => { + expect(mockApiCall).toHaveBeenCalledWith( + mockResource.resourcePath, + "PATCH", + mockWorkspaceContext.workspaceApplicationIdURI, + expect.objectContaining({ + templateVersion: "1.1.0", + properties: expect.any(Object), + }), + "JSON", + undefined, + undefined, + mockResource._etag + ); + }); + + expect(mockDispatch).toHaveBeenCalled(); + expect(mockOnDismiss).toHaveBeenCalled(); + }); + + it("shows loading spinner during API call", async () => { + mockApiCall.mockImplementation((url, method) => { + if (method === "PATCH") { + return new Promise((resolve) => + setTimeout( + () => { + resolve({ operation: { id: "operation-id", status: "running" } }); + }, + 100 + ) + ); + } + // Handle GET requests for schemas + if (method === "GET" && url.includes("?version=")) { + if (url.includes("version=1.0.0")) { + return Promise.resolve(mockCurrentTemplateSchema); + } else { + return Promise.resolve(mockNewTemplateSchema); + } + } + return Promise.resolve({ + operation: { id: "operation-id", status: "running" }, + }); + }); + + renderWithWorkspaceContext( + + ); + + // Select a version + const dropdown = screen.getByTestId("dropdown"); + fireEvent.change(dropdown, { target: { value: "1.1.0" } }); + + // Wait for schema to load + await waitFor(() => { + expect( + screen.queryByText("Loading new template schema...") + ).not.toBeInTheDocument(); + }); + + // Click upgrade and check for loading spinner + const upgradeButton = screen.getByTestId("primary-button"); + fireEvent.click(upgradeButton); + + expect(screen.getByTestId("spinner")).toBeInTheDocument(); + expect(screen.getByText("Sending request...")).toBeInTheDocument(); + }); + + it("displays error when API call fails", async () => { + mockApiCall.mockImplementation((url, method) => { + if (method === "PATCH") { + return Promise.reject(new Error("Network error")); + } + // Handle GET requests for schemas + if (method === "GET" && url.includes("?version=")) { + if (url.includes("version=1.0.0")) { + return Promise.resolve(mockCurrentTemplateSchema); + } else { + return Promise.resolve(mockNewTemplateSchema); + } + } + return Promise.reject(new Error("Network error")); + }); + + renderWithWorkspaceContext( + + ); + + // Select a version + const dropdown = screen.getByTestId("dropdown"); + fireEvent.change(dropdown, { target: { value: "1.1.0" } }); + + // Wait for schema to load + await waitFor(() => { + expect( + screen.queryByText("Loading new template schema...") + ).not.toBeInTheDocument(); + }); + + // Click upgrade + const upgradeButton = screen.getByTestId("primary-button"); + fireEvent.click(upgradeButton); + + await waitFor(() => { + expect(screen.getByTestId("exception-layout")).toBeInTheDocument(); + expect(screen.getByText("Failed to upgrade resource")).toBeInTheDocument(); + }); + }); + + it("uses workspace auth for workspace service resources", async () => { + const mockOperation = { id: "operation-id", status: "running" }; + mockApiCall.mockImplementation((url, method) => { + if (method === "PATCH") { + return Promise.resolve({ operation: mockOperation }); + } + // Handle GET requests for schemas + if (method === "GET" && url.includes("?version=")) { + if (url.includes("version=1.0.0")) { + return Promise.resolve(mockCurrentTemplateSchema); + } else { + return Promise.resolve(mockNewTemplateSchema); + } + } + return Promise.resolve({ operation: mockOperation }); + }); + + renderWithWorkspaceContext( + + ); + + // Select a version + const dropdown = screen.getByTestId("dropdown"); + fireEvent.change(dropdown, { target: { value: "1.1.0" } }); + + // Wait for schema to load + await waitFor(() => { + expect( + screen.queryByText("Loading new template schema...") + ).not.toBeInTheDocument(); + }); + + // Click upgrade + const upgradeButton = screen.getByTestId("primary-button"); + fireEvent.click(upgradeButton); + + await waitFor(() => { + expect(mockApiCall).toHaveBeenCalledWith( + expect.any(String), + "PATCH", + mockWorkspaceContext.workspaceApplicationIdURI, // should use workspace auth + expect.any(Object), + "JSON", + undefined, + undefined, + expect.any(String) + ); + }); + }); + + it("does not use workspace auth for shared service resources", async () => { + const sharedServiceResource = { + ...mockResource, + resourceType: ResourceType.SharedService, + }; + const mockOperation = { id: "operation-id", status: "running" }; + mockApiCall.mockImplementation((url, method) => { + if (method === "PATCH") { + return Promise.resolve({ operation: mockOperation }); + } + // Handle GET requests for schemas + if (method === "GET" && url.includes("?version=")) { + if (url.includes("version=1.0.0")) { + return Promise.resolve(mockCurrentTemplateSchema); + } else { + return Promise.resolve(mockNewTemplateSchema); + } + } + return Promise.resolve({ operation: mockOperation }); + }); + + renderWithWorkspaceContext( + + ); + + // Select a version + const dropdown = screen.getByTestId("dropdown"); + fireEvent.change(dropdown, { target: { value: "1.1.0" } }); + + // Wait for schema to load + await waitFor(() => { + expect( + screen.queryByText("Loading new template schema...") + ).not.toBeInTheDocument(); + }); + + // Click upgrade + const upgradeButton = screen.getByTestId("primary-button"); + fireEvent.click(upgradeButton); + + await waitFor(() => { + expect(mockApiCall).toHaveBeenCalledWith( + expect.any(String), + "PATCH", + undefined, // should not use workspace auth + expect.any(Object), + "JSON", + undefined, + undefined, + expect.any(String) + ); + }); + }); + + it("filters out major upgrades from dropdown options", () => { + const resourceWithMajorUpgrade = { + ...mockResource, + availableUpgrades: [ + { version: "1.1.0", forceUpdateRequired: false }, + { version: "2.0.0", forceUpdateRequired: true }, + { version: "1.2.0", forceUpdateRequired: false }, + ], + }; + + renderWithWorkspaceContext( + + ); + + // Minor updates should be available + expect(screen.getByText("1.1.0")).toBeInTheDocument(); + expect(screen.getByText("1.2.0")).toBeInTheDocument(); + + // Major update should not be available in dropdown + expect(screen.queryByText("2.0.0")).not.toBeInTheDocument(); + }); + + it("displays form when new properties need to be added", async () => { + renderWithWorkspaceContext( + + ); + + // Select a version that has new properties + const dropdown = screen.getByTestId("dropdown"); + fireEvent.change(dropdown, { target: { value: "1.1.0" } }); + + // Wait for schema to load + await waitFor(() => { + expect( + screen.queryByText("Loading new template schema...") + ).not.toBeInTheDocument(); + }); + + // Should show info message about new properties + expect(screen.getByText("You must specify values for new properties:")).toBeInTheDocument(); + + // The form input for new_property should be rendered + expect(screen.getByDisplayValue("default_value")).toBeInTheDocument(); + }); + + it("displays warning about removed properties", async () => { + renderWithWorkspaceContext( + + ); + + // Select a version + const dropdown = screen.getByTestId("dropdown"); + fireEvent.change(dropdown, { target: { value: "1.1.0" } }); + + // Wait for schema to load + await waitFor(() => { + expect( + screen.queryByText("Loading new template schema...") + ).not.toBeInTheDocument(); + }); + + // Should show warning about removed properties + expect(screen.getByText(/Warning: The following properties are no longer present/)).toBeInTheDocument(); + expect(screen.getByText(/existing_property/)).toBeInTheDocument(); + }); + + it("disables upgrade button when required new properties are cleared", async () => { + renderWithWorkspaceContext( + + ); + + // Select a version + const dropdown = screen.getByTestId("dropdown"); + fireEvent.change(dropdown, { target: { value: "1.1.0" } }); + + // Wait for schema to load + await waitFor(() => { + expect( + screen.queryByText("Loading new template schema...") + ).not.toBeInTheDocument(); + }); + + // Find the input field and clear it + const inputField = screen.getByDisplayValue("default_value"); + fireEvent.change(inputField, { target: { value: "" } }); + + // Button should now be disabled because the required property is empty + await waitFor(() => { + const upgradeButton = screen.getByTestId("primary-button"); + expect(upgradeButton).toBeDisabled(); + }); + }); + + it("enables upgrade button when all new properties are filled in", async () => { + renderWithWorkspaceContext( + + ); + + // Select a version + const dropdown = screen.getByTestId("dropdown"); + fireEvent.change(dropdown, { target: { value: "1.1.0" } }); + + // Wait for schema to load + await waitFor(() => { + expect( + screen.queryByText("Loading new template schema...") + ).not.toBeInTheDocument(); + }); + + // Find the new_property input field and fill it + const inputField = screen.getByDisplayValue("default_value"); + fireEvent.change(inputField, { target: { value: "filled_value" } }); + + // Button should now be enabled + await waitFor(() => { + const upgradeButton = screen.getByTestId("primary-button"); + expect(upgradeButton).not.toBeDisabled(); + }); + }); + + it("includes new property values in upgrade API call", async () => { + const mockOperation = { id: "operation-id", status: "running" }; + mockApiCall.mockImplementation((url, method) => { + if (method === "PATCH") { + return Promise.resolve({ operation: mockOperation }); + } + // Handle GET requests for schemas + if (method === "GET" && url.includes("?version=")) { + if (url.includes("version=1.0.0")) { + return Promise.resolve(mockCurrentTemplateSchema); + } else { + return Promise.resolve(mockNewTemplateSchema); + } + } + return Promise.resolve({ operation: mockOperation }); + }); + + renderWithWorkspaceContext( + + ); + + // Select a version + const dropdown = screen.getByTestId("dropdown"); + fireEvent.change(dropdown, { target: { value: "1.1.0" } }); + + // Wait for schema to load + await waitFor(() => { + expect( + screen.queryByText("Loading new template schema...") + ).not.toBeInTheDocument(); + }); + + // Fill in the new property + const inputField = screen.getByDisplayValue("default_value"); + fireEvent.change(inputField, { target: { value: "custom_value" } }); + + // Click upgrade + await waitFor(() => { + const upgradeButton = screen.getByTestId("primary-button"); + fireEvent.click(upgradeButton); + }); + + // Verify the API call includes the new property value + await waitFor(() => { + expect(mockApiCall).toHaveBeenCalledWith( + mockResource.resourcePath, + "PATCH", + mockWorkspaceContext.workspaceApplicationIdURI, + expect.objectContaining({ + templateVersion: "1.1.0", + properties: expect.objectContaining({ + new_property: "custom_value", + }), + }), + "JSON", + undefined, + undefined, + mockResource._etag + ); + }); + }); +}); From e99a78f5f24721076f2bab3754ee6e68b226e961 Mon Sep 17 00:00:00 2001 From: James Chapman Date: Mon, 22 Dec 2025 17:32:18 +0000 Subject: [PATCH 13/15] fix ws template auth --- ui/app/src/components/shared/ConfirmUpgradeResource.tsx | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/ui/app/src/components/shared/ConfirmUpgradeResource.tsx b/ui/app/src/components/shared/ConfirmUpgradeResource.tsx index 2e09f81e97..b2dbf2b4eb 100644 --- a/ui/app/src/components/shared/ConfirmUpgradeResource.tsx +++ b/ui/app/src/components/shared/ConfirmUpgradeResource.tsx @@ -188,7 +188,6 @@ export const ConfirmUpgradeResource: React.FunctionComponent< .workspaceId; templateListPath = `${ApiEndpoint.Workspaces}/${workspaceId}/${ApiEndpoint.WorkspaceServiceTemplates}/${props.resource.properties.parentWorkspaceService.templateName}/${ApiEndpoint.UserResourceTemplates}`; templateGetPath = `${ApiEndpoint.WorkspaceServiceTemplates}/${props.resource.properties.parentWorkspaceService.templateName}/${ApiEndpoint.UserResourceTemplates}`; - // workspaceApplicationIdURI = props.resource.properties.parentWorkspaceService.workspaceApplicationIdURI; break; } else { throw Error( @@ -210,7 +209,7 @@ export const ConfirmUpgradeResource: React.FunctionComponent< const newTemplate = await apiCall( fetchUrl, HttpMethod.Get, - wsAuth ? workspaceCtx.workspaceApplicationIdURI : undefined, + props.resource.resourceType === ResourceType.UserResource ? workspaceCtx.workspaceApplicationIdURI : undefined, undefined, ResultType.JSON, ); @@ -223,7 +222,7 @@ export const ConfirmUpgradeResource: React.FunctionComponent< currentTemplate = await apiCall( `${templateGetPath}/${props.resource.templateName}?version=${props.resource.templateVersion}`, HttpMethod.Get, - wsAuth ? workspaceCtx.workspaceApplicationIdURI : undefined, + props.resource.resourceType === ResourceType.UserResource ? workspaceCtx.workspaceApplicationIdURI : undefined, undefined, ResultType.JSON, ); From 1d0b51b7427a47fb480a97433cc4766e83e6ad67 Mon Sep 17 00:00:00 2001 From: James Chapman Date: Wed, 24 Dec 2025 14:46:07 +0000 Subject: [PATCH 14/15] ui version bump --- ui/app/package-lock.json | 4 ++-- ui/app/package.json | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/ui/app/package-lock.json b/ui/app/package-lock.json index 8c4aafd216..56d958e735 100644 --- a/ui/app/package-lock.json +++ b/ui/app/package-lock.json @@ -1,12 +1,12 @@ { "name": "tre-ui", - "version": "0.8.20", + "version": "0.8.21", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "tre-ui", - "version": "0.8.20", + "version": "0.8.21", "dependencies": { "@azure/msal-browser": "^2.35.0", "@azure/msal-react": "^1.5.12", diff --git a/ui/app/package.json b/ui/app/package.json index 3400a0c5f3..2ed0b4bef2 100644 --- a/ui/app/package.json +++ b/ui/app/package.json @@ -1,6 +1,6 @@ { "name": "tre-ui", - "version": "0.8.20", + "version": "0.8.21", "private": true, "type": "module", "dependencies": { From 2ec1049c511e80759c6bd6ad196b529ce0e18f64 Mon Sep 17 00:00:00 2001 From: James Chapman Date: Mon, 5 Jan 2026 11:40:14 +0000 Subject: [PATCH 15/15] bump version --- api_app/_version.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api_app/_version.py b/api_app/_version.py index 5461be1190..4f3cae6929 100644 --- a/api_app/_version.py +++ b/api_app/_version.py @@ -1 +1 @@ -__version__ = "0.25.7" +__version__ = "0.25.8"