Allow variable creation with different target environments (#6168)

* fix value cell click target * improve existing keys and env check, utils * comment out client side special char validation * clean up * lint * check for dups earlier, lint fix * lint * update regex * reset after submission * fix reset bug in edit dialog * update copy * duplicate fix in add dialog * lint * move check existing to submit instead of onblur * feedback
rilldata · Dec 3, 2024 · 96c119e · 96c119e · github-actions · Dec 3, 2024
1 parent e15778d
commit 96c119e
Show file tree

Hide file tree

Showing 9 changed files with 179 additions and 82 deletions.
diff --git a/web-admin/src/features/projects/environment-variables/ActionsCell.svelte b/web-admin/src/features/projects/environment-variables/ActionsCell.svelte
@@ -5,12 +5,13 @@
   import { Trash2Icon, Pencil } from "lucide-svelte";
   import EditDialog from "./EditDialog.svelte";
   import DeleteDialog from "./DeleteDialog.svelte";
+  import type { VariableNames } from "./types";
 
   export let id: string;
   export let environment: string;
   export let name: string;
   export let value: string;
-  export let variableNames: string[] = [];
+  export let variableNames: VariableNames = [];
 
   let isDropdownOpen = false;
   let isEditDialogOpen = false;

diff --git a/web-admin/src/features/projects/environment-variables/ActivityCell.svelte b/web-admin/src/features/projects/environment-variables/ActivityCell.svelte
@@ -20,7 +20,7 @@
 </script>
 
 <Tooltip distance={8} location="top" alignment="start">
-  <div class="text-xs text-gray-500 cursor-pointer">
+  <div class="text-xs text-gray-500 cursor-pointer w-fit">
     {formattedText}
   </div>
   <TooltipContent slot="tooltip-content">

diff --git a/web-admin/src/features/projects/environment-variables/AddDialog.svelte b/web-admin/src/features/projects/environment-variables/AddDialog.svelte
@@ -22,13 +22,14 @@
   import { defaults, superForm } from "sveltekit-superforms";
   import { yup } from "sveltekit-superforms/adapters";
   import { object, string, array } from "yup";
-  import { EnvironmentType } from "./types";
+  import { type VariableNames } from "./types";
   import Input from "@rilldata/web-common/components/forms/Input.svelte";
   import IconButton from "@rilldata/web-common/components/button/IconButton.svelte";
   import { Trash2Icon, UploadIcon } from "lucide-svelte";
+  import { getCurrentEnvironment, isDuplicateKey } from "./utils";
 
   export let open = false;
-  export let variableNames: string[] = [];
+  export let variableNames: VariableNames = [];
 
   let inputErrors: { [key: number]: boolean } = {};
   let isKeyAlreadyExists = false;
@@ -61,8 +62,9 @@
           key: string()
             .optional()
             .matches(
-              /^[a-zA-Z0-9_]+$/,
-              "Key must only contain letters, numbers, and underscores.",
+              /^[a-zA-Z_][a-zA-Z0-9_.]*$/,
+              // See: https://github.com/rilldata/rill/pull/6121/files#diff-04140a6ac071a4bac716371f8b66a56c89c9d52cfbf2b05ea1e14ee8d4e301e7R12
+              "Key must start with a letter or underscore and can only contain letters, digits, underscores, and dots",
             ),
           value: string().optional(),
         }),
@@ -80,41 +82,32 @@
         if (!form.valid) return;
         const values = form.data;
 
-        // Omit draft variables that do not have a key
+        // Check for duplicates before proceeding
+        const duplicates = checkForExistingKeys();
+        if (duplicates > 0) {
+          // Early return without resetting the form
+          return;
+        }
+
+        // Only filter and process if there are no duplicates
         const filteredVariables = values.variables.filter(
           ({ key }) => key !== "",
         );
 
-        // Flatten the variables to match the schema
         const flatVariables = Object.fromEntries(
           filteredVariables.map(({ key, value }) => [key, value]),
         );
 
         try {
           await handleUpdateProjectVariables(flatVariables);
           open = false;
+          handleReset();
         } catch (error) {
           console.error(error);
         }
       },
     });
 
-  function getCurrentEnvironment() {
-    if (isDevelopment && isProduction) {
-      return undefined;
-    }
-
-    if (isDevelopment) {
-      return EnvironmentType.DEVELOPMENT;
-    }
-
-    if (isProduction) {
-      return EnvironmentType.PRODUCTION;
-    }
-
-    return undefined;
-  }
-
   async function handleUpdateProjectVariables(
     flatVariables: AdminServiceUpdateProjectVariablesBodyVariables,
   ) {
@@ -123,7 +116,7 @@
         organization,
         project,
         data: {
-          environment: getCurrentEnvironment(),
+          environment: getCurrentEnvironment(isDevelopment, isProduction),
           variables: flatVariables,
         },
       });
@@ -172,17 +165,29 @@
   }
 
   function checkForExistingKeys() {
-    const existingKeys = $form.variables.map((variable) => variable.key);
     inputErrors = {};
     isKeyAlreadyExists = false;
 
-    existingKeys.forEach((key, index) => {
-      // Case sensitive
-      if (variableNames.some((existingKey) => existingKey === key)) {
-        inputErrors[index] = true;
+    const existingKeys = $form.variables.map((variable) => {
+      return {
+        environment: getCurrentEnvironment(isDevelopment, isProduction),
+        name: variable.key,
+      };
+    });
+
+    let duplicateCount = 0;
+    existingKeys.forEach((key, idx) => {
+      const variableEnvironment = key.environment;
+      const variableKey = key.name;
+
+      if (isDuplicateKey(variableEnvironment, variableKey, variableNames)) {
+        inputErrors[idx] = true;
         isKeyAlreadyExists = true;
+        duplicateCount++;
       }
     });
+
+    return duplicateCount;
   }
 
   function handleFileUpload(event) {
@@ -356,7 +361,9 @@
             {#if isKeyAlreadyExists}
               <div class="mt-1">
                 <p class="text-xs text-red-600 font-normal">
-                  These keys already exist for this project.
+                  {Object.keys(inputErrors).length > 1
+                    ? "These keys already exist for your target environment(s)"
+                    : "This key already exists for your target environment(s)"}
                 </p>
               </div>
             {/if}

diff --git a/web-admin/src/features/projects/environment-variables/EditDialog.svelte b/web-admin/src/features/projects/environment-variables/EditDialog.svelte
@@ -21,16 +21,20 @@
   import { defaults, superForm } from "sveltekit-superforms";
   import { yup } from "sveltekit-superforms/adapters";
   import { object, string } from "yup";
-  import { EnvironmentType } from "./types";
+  import { EnvironmentType, type VariableNames } from "./types";
   import Input from "@rilldata/web-common/components/forms/Input.svelte";
-  import { onMount } from "svelte";
+  import {
+    getCurrentEnvironment,
+    getEnvironmentType,
+    isDuplicateKey,
+  } from "./utils";
 
   export let open = false;
   export let id: string;
   export let environment: string;
   export let name: string;
   export let value: string;
-  export let variableNames: string[] = [];
+  export let variableNames: VariableNames = [];
 
   let initialEnvironment: {
     isDevelopment: boolean;
@@ -45,7 +49,7 @@
   $: project = $page.params.project;
 
   $: isEnvironmentSelected = isDevelopment || isProduction;
-  $: hasChanges =
+  $: hasNewChanges =
     $form.key !== initialValues.key ||
     $form.value !== initialValues.value ||
     initialEnvironment?.isDevelopment !== isDevelopment ||
@@ -67,8 +71,9 @@
       key: string()
         .optional()
         .matches(
-          /^[a-zA-Z0-9_]+$/,
-          "Key must only contain letters, numbers, and underscores",
+          /^[a-zA-Z_][a-zA-Z0-9_.]*$/,
+          // See: https://github.com/rilldata/rill/pull/6121/files#diff-04140a6ac071a4bac716371f8b66a56c89c9d52cfbf2b05ea1e14ee8d4e301e7R12
+          "Key must start with a letter or underscore and can only contain letters, digits, underscores, and dots",
         ),
       value: string().optional(),
     }),
@@ -92,38 +97,27 @@
       if (!form.valid) return;
       const values = form.data;
 
+      checkForExistingKeys();
+      if (isKeyAlreadyExists) return;
+
       const flatVariable = {
         [values.key]: values.value,
       };
 
       try {
         await handleUpdateProjectVariables(flatVariable);
         open = false;
+        handleReset();
       } catch (error) {
         console.error(error);
       }
     },
   });
 
-  function processEnvironment() {
-    if (isDevelopment && isProduction) {
-      return undefined;
-    }
-
-    if (isDevelopment) {
-      return EnvironmentType.DEVELOPMENT;
-    }
-
-    if (isProduction) {
-      return EnvironmentType.PRODUCTION;
-    }
-
-    return undefined;
-  }
-
   async function handleUpdateProjectVariables(
     flatVariable: AdminServiceUpdateProjectVariablesBodyVariables,
   ) {
+    // Check if the key has changed, if so, check for existing keys
     if ($form.key !== initialValues.key) {
       checkForExistingKeys();
     }
@@ -146,7 +140,7 @@
           organization,
           project,
           data: {
-            environment: processEnvironment(),
+            environment: getCurrentEnvironment(isDevelopment, isProduction),
             variables: flatVariable,
           },
         });
@@ -155,7 +149,10 @@
       // If the key remains the same, update the environment or value
       if (initialValues.key === $form.key) {
         // If environment has changed, remove the old key and add the new key
-        if (initialValues.environment !== processEnvironment()) {
+        if (
+          initialValues.environment !==
+          getCurrentEnvironment(isDevelopment, isProduction)
+        ) {
           await $updateProjectVariables.mutateAsync({
             organization,
             project,
@@ -170,7 +167,7 @@
           organization,
           project,
           data: {
-            environment: processEnvironment(),
+            environment: getCurrentEnvironment(isDevelopment, isProduction),
             variables: flatVariable,
           },
         });
@@ -204,21 +201,30 @@
 
   function handleReset() {
     reset();
+    isDevelopment = false;
+    isProduction = false;
+    inputErrors = {};
     isKeyAlreadyExists = false;
   }
 
   function checkForExistingKeys() {
-    const existingKeys = [$form.key];
     inputErrors = {};
     isKeyAlreadyExists = false;
 
-    existingKeys.forEach((key, index) => {
-      // Case sensitive
-      if (variableNames.some((existingKey) => existingKey === key)) {
-        inputErrors[index] = true;
-        isKeyAlreadyExists = true;
-      }
-    });
+    const existingKey = {
+      environment: getEnvironmentType(
+        getCurrentEnvironment(isDevelopment, isProduction),
+      ),
+      name: $form.key,
+    };
+
+    const variableEnvironment = existingKey.environment;
+    const variableKey = existingKey.name;
+
+    if (isDuplicateKey(variableEnvironment, variableKey, variableNames)) {
+      inputErrors[0] = true;
+      isKeyAlreadyExists = true;
+    }
   }
 
   function setInitialCheckboxState() {
@@ -236,13 +242,17 @@
     }
   }
 
-  onMount(() => {
+  function handleDialogOpen() {
     setInitialCheckboxState();
     initialEnvironment = {
       isDevelopment,
       isProduction,
     };
-  });
+  }
+
+  $: if (open) {
+    handleDialogOpen();
+  }
 </script>
 
 <Dialog
@@ -298,11 +308,6 @@
                   : ""}
                 placeholder="Key"
                 on:input={(e) => handleKeyChange(e)}
-                onBlur={() => {
-                  if ($form.key !== initialValues.key) {
-                    checkForExistingKeys();
-                  }
-                }}
               />
               <Input
                 bind:value={$form.value}
@@ -322,7 +327,7 @@
             {#if isKeyAlreadyExists}
               <div class="mt-1">
                 <p class="text-xs text-red-600 font-normal">
-                  This key already exists for this project.
+                  This key already exists for your target environment(s)
                 </p>
               </div>
             {/if}
@@ -343,7 +348,7 @@
         type="primary"
         form={$formId}
         disabled={$submitting ||
-          !hasChanges ||
+          !hasNewChanges ||
           !isEnvironmentSelected ||
           hasExistingKeys ||
           $allErrors.length > 0}

diff --git a/web-admin/src/features/projects/environment-variables/EnvironmentVariablesTable.svelte b/web-admin/src/features/projects/environment-variables/EnvironmentVariablesTable.svelte
@@ -8,10 +8,11 @@
   import KeyCell from "./KeyCell.svelte";
   import ValueCell from "./ValueCell.svelte";
   import ActionsCell from "./ActionsCell.svelte";
+  import type { VariableNames } from "./types";
 
   export let data: V1ProjectVariable[];
   export let emptyText: string = "No environment variables";
-  export let variableNames: string[] = [];
+  export let variableNames: VariableNames = [];
 
   const columns: ColumnDef<V1ProjectVariable, any>[] = [
     {

diff --git a/web-admin/src/features/projects/environment-variables/KeyCell.svelte b/web-admin/src/features/projects/environment-variables/KeyCell.svelte
@@ -5,8 +5,8 @@
   export let name: string;
 
   // NOTE: if environment is empty, the variable is shared for all environments
-  function getEnvironmentLabel(environment: string) {
-    if (environment === "") {
+  function getEnvironmentType(environment: string) {
+    if (environment === EnvironmentType.UNDEFINED) {
       return "Development, Production";
     }
     if (environment === EnvironmentType.DEVELOPMENT) {
@@ -18,7 +18,7 @@
     return "";
   }
 
-  $: environmentLabel = getEnvironmentLabel(environment);
+  $: environmentLabel = getEnvironmentType(environment);
 </script>
 
 <div class="flex flex-col">