From 07b316c71184b7a01a6d01c876c91673c6ada12d Mon Sep 17 00:00:00 2001 From: minnakt Date: Wed, 13 Sep 2023 15:53:52 -0400 Subject: [PATCH] Change form schema and address feedback --- .../distroSettings/provider_section.ts | 34 +++------- src/components/Header/Navbar.tsx | 4 +- src/constants/fieldMaps.ts | 7 --- src/gql/mocks/getSpruceConfig.ts | 4 +- src/gql/queries/index.ts | 4 +- src/hooks/useSpruceConfig.ts | 4 +- src/pages/commits/index.tsx | 4 +- .../tabs/ProviderTab/ProviderTab.tsx | 15 +++-- .../tabs/ProviderTab/getFormSchema.ts | 26 +++++--- .../tabs/ProviderTab/transformerUtils.ts | 36 +++++------ .../tabs/ProviderTab/transformers.test.ts | 62 ++++++++++++++----- .../tabs/ProviderTab/transformers.ts | 47 ++++++-------- .../distroSettings/tabs/ProviderTab/types.ts | 22 ++----- 13 files changed, 133 insertions(+), 136 deletions(-) diff --git a/cypress/integration/distroSettings/provider_section.ts b/cypress/integration/distroSettings/provider_section.ts index 6748ae1e31..9bb11ac57e 100644 --- a/cypress/integration/distroSettings/provider_section.ts +++ b/cypress/integration/distroSettings/provider_section.ts @@ -9,36 +9,31 @@ describe("provider section", () => { it("successfully updates static provider fields", () => { cy.dataCy("provider-select").contains("Static IP/VM"); - // Correct fields are displayed - cy.dataCy("provider-settings").within(() => { - cy.get("button").should("have.length", 1); - cy.get("textarea").should("have.length", 1); - cy.get("input[type=checkbox]").should("have.length", 1); - cy.get("input[type=text]").should("have.length", 0); - }); + // Correct section is displayed. + cy.dataCy("static-provider-settings").should("exist"); + // Change field values. cy.getInputByLabel("User Data").type("my user data"); cy.getInputByLabel("Merge with existing user data").check({ force: true, }); cy.contains("button", "Add security group").click(); cy.getInputByLabel("Security Group ID").type("group-1234"); - save(); cy.validateToast("success"); + // Revert fields to original values. cy.getInputByLabel("User Data").clear(); cy.getInputByLabel("Merge with existing user data").uncheck({ force: true, }); cy.dataCy("delete-item-button").click(); - save(); cy.validateToast("success"); }); }); - describe.only("docker", () => { + describe("docker", () => { beforeEach(() => { cy.visit("/distro/ubuntu1604-container-test/settings/provider"); }); @@ -46,22 +41,12 @@ describe("provider section", () => { it("successfully updates docker provider fields", () => { cy.dataCy("provider-select").contains("Docker"); - // Correct fields are displayed - cy.dataCy("provider-settings").within(() => { - cy.getInputByLabel("Docker Image URL").should("exist"); - cy.getInputByLabel("Image Build Method").should("exist"); - cy.getInputByLabel("Username for Registries").should("exist"); - cy.getInputByLabel("Password for Registries").should("exist"); - cy.getInputByLabel("Container Pool ID").should("exist"); - cy.getInputByLabel("Pool Mapping Information").should("exist"); - cy.getInputByLabel("User Data").should("exist"); - cy.getInputByLabel("Merge with existing user data").should("exist"); - cy.contains("button", "Add security group").should("exist"); - }); + // Correct section is displayed. + cy.dataCy("docker-provider-settings").should("exist"); // Change field values. cy.selectLGOption("Image Build Method", "Pull"); - cy.selectLGOption("Container Pool ID", /^test-pool$/); + cy.selectLGOption("Container Pool ID", "test-pool-2"); cy.getInputByLabel("Username for Registries").type("username"); cy.getInputByLabel("Password for Registries").type("password"); cy.getInputByLabel("User Data").type("my user data"); @@ -73,14 +58,13 @@ describe("provider section", () => { // Revert fields to original values. cy.selectLGOption("Image Build Method", "Import"); - cy.selectLGOption("Container Pool ID", "ubuntu-test-pool"); + cy.selectLGOption("Container Pool ID", "test-pool-1"); cy.getInputByLabel("Username for Registries").clear(); cy.getInputByLabel("Password for Registries").clear(); cy.getInputByLabel("User Data").clear(); cy.getInputByLabel("Merge with existing user data").uncheck({ force: true, }); - save(); cy.validateToast("success"); }); diff --git a/src/components/Header/Navbar.tsx b/src/components/Header/Navbar.tsx index 1ce994c084..73b16a9235 100644 --- a/src/components/Header/Navbar.tsx +++ b/src/components/Header/Navbar.tsx @@ -13,7 +13,7 @@ import { getCommitsRoute, getUserPatchesRoute, routes } from "constants/routes"; import { size } from "constants/tokens"; import { useAuthStateContext } from "context/auth"; import { UserQuery, SpruceConfigQuery } from "gql/generated/types"; -import { GET_USER, GET_SPRUCE_CONFIG } from "gql/queries"; +import { GET_USER, SPRUCE_CONFIG } from "gql/queries"; import { useLegacyUIURL } from "hooks"; import { AuxiliaryDropdown } from "./AuxiliaryDropdown"; import { UserDropdown } from "./UserDropdown"; @@ -43,7 +43,7 @@ export const Navbar: React.FC = () => { const currProject = projectFromUrl ?? Cookies.get(CURRENT_PROJECT); - const { data: configData } = useQuery(GET_SPRUCE_CONFIG, { + const { data: configData } = useQuery(SPRUCE_CONFIG, { skip: currProject !== undefined, }); diff --git a/src/constants/fieldMaps.ts b/src/constants/fieldMaps.ts index f54c14c92f..1368f86f5b 100644 --- a/src/constants/fieldMaps.ts +++ b/src/constants/fieldMaps.ts @@ -167,13 +167,6 @@ export const timeZones = [ }, ]; -export const awsRegions = [ - { - str: "US-East-1", - value: "us-east-1", - }, -]; - const listOfDateFormatStrings = [ "MM-dd-yyyy", "dd-MM-yyyy", diff --git a/src/gql/mocks/getSpruceConfig.ts b/src/gql/mocks/getSpruceConfig.ts index 5061ee8cc2..fa7b23ad3e 100644 --- a/src/gql/mocks/getSpruceConfig.ts +++ b/src/gql/mocks/getSpruceConfig.ts @@ -2,7 +2,7 @@ import { SpruceConfigQuery, SpruceConfigQueryVariables, } from "gql/generated/types"; -import { GET_SPRUCE_CONFIG } from "gql/queries"; +import { SPRUCE_CONFIG } from "gql/queries"; import { ApolloMock } from "types/gql"; export const getSpruceConfigMock: ApolloMock< @@ -10,7 +10,7 @@ export const getSpruceConfigMock: ApolloMock< SpruceConfigQueryVariables > = { request: { - query: GET_SPRUCE_CONFIG, + query: SPRUCE_CONFIG, variables: {}, }, result: { diff --git a/src/gql/queries/index.ts b/src/gql/queries/index.ts index 45251a628d..dff6ac2ef2 100644 --- a/src/gql/queries/index.ts +++ b/src/gql/queries/index.ts @@ -70,7 +70,7 @@ import PROJECT_HEALTH_VIEW from "./project-health-view.graphql"; import GET_PROJECT_PATCHES from "./project-patches.graphql"; import GET_SPAWN_EXPIRATION_INFO from "./spawn-expiration.graphql"; import GET_SPAWN_TASK from "./spawn-task.graphql"; -import GET_SPRUCE_CONFIG from "./spruce-config.graphql"; +import SPRUCE_CONFIG from "./spruce-config.graphql"; import GET_SUBNET_AVAILABILITY_ZONES from "./subnet-availability-zones.graphql"; import TASK_QUEUE_DISTROS from "./task-queue-distros.graphql"; import USER_DISTRO_SETTINGS_PERMISSIONS from "./user-distro-settings-permissions.graphql"; @@ -131,7 +131,7 @@ export { GET_REPO_SETTINGS, GET_SPAWN_EXPIRATION_INFO, GET_SPAWN_TASK, - GET_SPRUCE_CONFIG, + SPRUCE_CONFIG, GET_SUBNET_AVAILABILITY_ZONES, GET_SYSTEM_LOGS, GET_TASK_ALL_EXECUTIONS, diff --git a/src/hooks/useSpruceConfig.ts b/src/hooks/useSpruceConfig.ts index f83aa912bb..0bf1ede1e3 100644 --- a/src/hooks/useSpruceConfig.ts +++ b/src/hooks/useSpruceConfig.ts @@ -3,11 +3,11 @@ import { SpruceConfigQuery, SpruceConfigQueryVariables, } from "gql/generated/types"; -import { GET_SPRUCE_CONFIG } from "gql/queries"; +import { SPRUCE_CONFIG } from "gql/queries"; export const useSpruceConfig = () => { const { data } = useQuery( - GET_SPRUCE_CONFIG + SPRUCE_CONFIG ); const { spruceConfig } = data || {}; diff --git a/src/pages/commits/index.tsx b/src/pages/commits/index.tsx index 0b56e2d691..0577aa68f5 100644 --- a/src/pages/commits/index.tsx +++ b/src/pages/commits/index.tsx @@ -29,7 +29,7 @@ import { MainlineCommitsQueryVariables, ProjectHealthView, } from "gql/generated/types"; -import { GET_MAINLINE_COMMITS, GET_SPRUCE_CONFIG } from "gql/queries"; +import { GET_MAINLINE_COMMITS, SPRUCE_CONFIG } from "gql/queries"; import { usePageTitle, usePolling, @@ -77,7 +77,7 @@ const Commits = () => { const { data: spruceData } = useQuery< SpruceConfigQuery, SpruceConfigQueryVariables - >(GET_SPRUCE_CONFIG, { + >(SPRUCE_CONFIG, { skip: !!projectIdentifier || !!recentlySelectedProject, }); diff --git a/src/pages/distroSettings/tabs/ProviderTab/ProviderTab.tsx b/src/pages/distroSettings/tabs/ProviderTab/ProviderTab.tsx index 7f898c1fa7..7ed8152e67 100644 --- a/src/pages/distroSettings/tabs/ProviderTab/ProviderTab.tsx +++ b/src/pages/distroSettings/tabs/ProviderTab/ProviderTab.tsx @@ -5,25 +5,28 @@ import { useDistroSettingsContext } from "pages/distroSettings/Context"; import { omitTypename } from "utils/string"; import { BaseTab } from "../BaseTab"; import { getFormSchema } from "./getFormSchema"; -import { TabProps } from "./types"; +import { TabProps, ProviderFormState } from "./types"; export const ProviderTab: React.FC = ({ distroData }) => { const initialFormState = distroData; const { getTab } = useDistroSettingsContext(); - const { formData } = getTab(DistroSettingsTabRoutes.Provider); + // @ts-expect-error - see TabState for details. + const { formData }: { formData: ProviderFormState } = getTab( + DistroSettingsTabRoutes.Provider + ); const { containerPools } = useSpruceConfig(); - const { pools = [] } = containerPools || {}; + const { pools } = containerPools || {}; - const selectedPoolId = formData?.providerSettings?.containerPoolId; - const selectedPool = pools.find((p) => p.id === selectedPoolId) ?? null; + const selectedPoolId = formData?.dockerProviderSettings?.containerPoolId; + const selectedPool = pools?.find((p) => p.id === selectedPoolId) ?? null; const poolMappingInfo = selectedPool ? JSON.stringify(omitTypename(selectedPool), null, 4) : ""; const formSchema = useMemo( - () => getFormSchema({ pools, poolMappingInfo }), + () => getFormSchema({ pools: pools || [], poolMappingInfo }), [pools, poolMappingInfo] ); diff --git a/src/pages/distroSettings/tabs/ProviderTab/getFormSchema.ts b/src/pages/distroSettings/tabs/ProviderTab/getFormSchema.ts index 4ef41ebef2..b1af6247fe 100644 --- a/src/pages/distroSettings/tabs/ProviderTab/getFormSchema.ts +++ b/src/pages/distroSettings/tabs/ProviderTab/getFormSchema.ts @@ -61,14 +61,10 @@ export const getFormSchema = ({ }, }, }, - providerSettings: { + staticProviderSettings: { type: "object" as "object", title: "", - properties: { - userData: staticProviderSettings.userData, - mergeUserData: staticProviderSettings.mergeUserData, - securityGroups: staticProviderSettings.securityGroups, - }, + properties: staticProviderSettings, }, }, }, @@ -81,7 +77,7 @@ export const getFormSchema = ({ }, }, }, - providerSettings: { + dockerProviderSettings: { type: "object" as "object", title: "", properties: { @@ -119,8 +115,20 @@ export const getFormSchema = ({ "ui:data-cy": "provider-select", }, }, - providerSettings: { - "ui:data-cy": "provider-settings", + staticProviderSettings: { + "ui:data-cy": "static-provider-settings", + "ui:ObjectFieldTemplate": CardFieldTemplate, + userData: { + "ui:widget": "textarea", + "ui:elementWrapperCSS": textAreaCSS, + }, + securityGroups: { + "ui:addButtonText": "Add security group", + "ui:orderable": false, + }, + }, + dockerProviderSettings: { + "ui:data-cy": "docker-provider-settings", "ui:ObjectFieldTemplate": CardFieldTemplate, userData: { "ui:widget": "textarea", diff --git a/src/pages/distroSettings/tabs/ProviderTab/transformerUtils.ts b/src/pages/distroSettings/tabs/ProviderTab/transformerUtils.ts index d39edb8473..af09741ce5 100644 --- a/src/pages/distroSettings/tabs/ProviderTab/transformerUtils.ts +++ b/src/pages/distroSettings/tabs/ProviderTab/transformerUtils.ts @@ -1,3 +1,5 @@ +import { BuildType } from "./types"; + type FieldGetter = (providerSettings: Record) => { form: Record; gql: Record; @@ -41,7 +43,7 @@ const getImageUrl = ((providerSettings) => ({ const getBuildType = ((providerSettings) => ({ form: { - buildType: providerSettings.build_type ?? "", + buildType: (providerSettings.build_type ?? "") as BuildType, }, gql: { build_type: providerSettings.buildType, @@ -77,13 +79,11 @@ export const staticProviderSettings = ((providerSettings = {}) => { ...mergeUserData.form, ...securityGroups.form, }, - gql: [ - { - ...userData.gql, - ...mergeUserData.gql, - ...securityGroups.gql, - }, - ], + gql: { + ...userData.gql, + ...mergeUserData.gql, + ...securityGroups.gql, + }, }; }) satisfies FieldGetter; @@ -106,16 +106,14 @@ export const dockerProviderSettings = ((providerSettings = {}) => { ...mergeUserData.form, ...securityGroups.form, }, - gql: [ - { - ...imageUrl.gql, - ...buildType.gql, - ...registryUser.gql, - ...registryPassword.gql, - ...userData.gql, - ...mergeUserData.gql, - ...securityGroups.gql, - }, - ], + gql: { + ...imageUrl.gql, + ...buildType.gql, + ...registryUser.gql, + ...registryPassword.gql, + ...userData.gql, + ...mergeUserData.gql, + ...securityGroups.gql, + }, }; }) satisfies FieldGetter; diff --git a/src/pages/distroSettings/tabs/ProviderTab/transformers.test.ts b/src/pages/distroSettings/tabs/ProviderTab/transformers.test.ts index f035f2b3c3..e61914bebe 100644 --- a/src/pages/distroSettings/tabs/ProviderTab/transformers.test.ts +++ b/src/pages/distroSettings/tabs/ProviderTab/transformers.test.ts @@ -5,13 +5,34 @@ import { BuildType, ProviderFormState } from "./types"; describe("provider tab", () => { describe("static provider", () => { - const staticDistroData = { ...distroData }; + const staticDistroData = { + ...distroData, + provider: Provider.Static, + containerPool: "", + providerSettingsList: [ + { + user_data: "", + merge_user_data: false, + security_group_ids: ["1"], + }, + ], + }; const staticForm: ProviderFormState = { provider: { providerName: Provider.Static, }, - providerSettings: { + staticProviderSettings: { + userData: "", + mergeUserData: false, + securityGroups: ["1"], + }, + dockerProviderSettings: { + imageUrl: "", + buildType: "" as BuildType, + registryUsername: "", + registryPassword: "", + containerPoolId: "", userData: "", mergeUserData: false, securityGroups: ["1"], @@ -40,26 +61,33 @@ describe("provider tab", () => { }); describe("docker provider", () => { - const dockerDistroData = { ...distroData }; - dockerDistroData.provider = Provider.Docker; - dockerDistroData.containerPool = "pool-1"; - dockerDistroData.providerSettingsList = [ - { - image_url: "https://some-url", - build_type: "import", - docker_registry_user: "testuser", - docker_registry_pw: "abc-123", - user_data: "", - merge_user_data: false, - security_group_ids: ["1"], - }, - ]; + const dockerDistroData = { + ...distroData, + provider: Provider.Docker, + containerPool: "pool-1", + providerSettingsList: [ + { + image_url: "https://some-url", + build_type: "import", + docker_registry_user: "testuser", + docker_registry_pw: "abc-123", + user_data: "", + merge_user_data: false, + security_group_ids: ["1"], + }, + ], + }; const dockerForm: ProviderFormState = { provider: { providerName: Provider.Docker, }, - providerSettings: { + staticProviderSettings: { + userData: "", + mergeUserData: false, + securityGroups: ["1"], + }, + dockerProviderSettings: { imageUrl: "https://some-url", buildType: BuildType.Import, registryUsername: "testuser", diff --git a/src/pages/distroSettings/tabs/ProviderTab/transformers.ts b/src/pages/distroSettings/tabs/ProviderTab/transformers.ts index 87ad514b8f..51c219d223 100644 --- a/src/pages/distroSettings/tabs/ProviderTab/transformers.ts +++ b/src/pages/distroSettings/tabs/ProviderTab/transformers.ts @@ -13,29 +13,18 @@ export const gqlToForm = ((data) => { const { containerPool, provider, providerSettingsList } = data; - switch (provider) { - case Provider.Static: - return { - provider: { - providerName: Provider.Static, - }, - providerSettings: { - ...staticProviderSettings(providerSettingsList[0]).form, - }, - }; - case Provider.Docker: - return { - provider: { - providerName: Provider.Docker, - }, - providerSettings: { - ...dockerProviderSettings(providerSettingsList[0]).form, - containerPoolId: containerPool, - }, - }; - default: - throw new Error(`Unknown provider '${provider}'`); - } + return { + provider: { + providerName: provider, + }, + staticProviderSettings: { + ...staticProviderSettings(providerSettingsList[0]).form, + }, + dockerProviderSettings: { + ...dockerProviderSettings(providerSettingsList[0]).form, + containerPoolId: containerPool, + }, + }; }) satisfies GqlToFormFunction; export const formToGql = ((data, distro) => { @@ -49,18 +38,22 @@ export const formToGql = ((data, distro) => { ...distro, provider: Provider.Static, providerSettingsList: [ - ...staticProviderSettings(data.providerSettings).gql, + { + ...staticProviderSettings(data.staticProviderSettings).gql, + }, ], + containerPool: "", }; case Provider.Docker: return { ...distro, provider: Provider.Docker, providerSettingsList: [ - ...dockerProviderSettings(data.providerSettings).gql, + { + ...dockerProviderSettings(data.dockerProviderSettings).gql, + }, ], - // @ts-ignore-error - containerPoolId will exist in DockerFormState. - containerPool: data.providerSettings.containerPoolId, + containerPool: data.dockerProviderSettings.containerPoolId, }; default: return distro; diff --git a/src/pages/distroSettings/tabs/ProviderTab/types.ts b/src/pages/distroSettings/tabs/ProviderTab/types.ts index b5553b916e..42178bcfc7 100644 --- a/src/pages/distroSettings/tabs/ProviderTab/types.ts +++ b/src/pages/distroSettings/tabs/ProviderTab/types.ts @@ -5,22 +5,17 @@ export enum BuildType { Pull = "pull", } -interface StaticProviderFormState { +// TODO: Append type with additional provider options, e.g. type ProviderFormState = StaticProviderFormState | DockerProviderFormState +export type ProviderFormState = { provider: { - providerName: Provider.Static; + providerName: Provider; }; - providerSettings: { + staticProviderSettings: { userData: string; mergeUserData: boolean; securityGroups: string[]; }; -} - -interface DockerProviderFormState { - provider: { - providerName: Provider.Docker; - }; - providerSettings: { + dockerProviderSettings: { imageUrl: string; buildType: BuildType; registryUsername: string; @@ -30,12 +25,7 @@ interface DockerProviderFormState { mergeUserData: boolean; securityGroups: string[]; }; -} - -// TODO: Append type with additional provider options, e.g. type ProviderFormState = StaticProviderFormState | DockerProviderFormState -export type ProviderFormState = - | StaticProviderFormState - | DockerProviderFormState; +}; export type TabProps = { distroData: ProviderFormState;