Enhancement: Variables correctifs (#1303)

* Fix button ordering on variable modals * Cap length of value to 255 chars * Add value length checks * Remove duplicate length check * Make sure updates are emitted from containing components * Update VariableTable widths and truncation * Fix some reactivity and reset issues * Clean up classes * Add reset method * Remove in-component limit validator, update existing one
PrefectHQ · Apr 3, 2023 · 46c4893 · 46c4893
1 parent 46703c0
commit 46c4893
Show file tree

Hide file tree

Showing 5 changed files with 100 additions and 37 deletions.
diff --git a/src/components/VariableCreateModal.vue b/src/components/VariableCreateModal.vue
@@ -21,11 +21,6 @@
         {{ localization.info.create }}
       </p-button>
     </template>
-    <template #cancel>
-      <p-button inset @click="internalValue = false">
-        {{ localization.info.close }}
-      </p-button>
-    </template>
   </p-modal>
 </template>
 
@@ -37,7 +32,9 @@
   import { useWorkspaceApi } from '@/compositions'
   import { localization } from '@/localization'
   import { Variable, VariableCreate } from '@/models'
-  import { isHandle, isRequired, isString } from '@/utilities'
+  import { isHandle, isLessThanOrEqual, isRequired, isString } from '@/utilities'
+
+  const MAX_CHARS = 255
 
   const props = defineProps<{
     showModal: boolean,
@@ -59,7 +56,7 @@
 
   const api = useWorkspaceApi()
 
-  const isUnique: ValidationRule<string | undefined> = async (value, label, { signal, source, previousValue }) => {
+  const nameIsUnique: ValidationRule<string | undefined> = async (value, label, { signal, source, previousValue }) => {
     if (value === previousValue) {
       return
     }
@@ -91,13 +88,27 @@
   const tags = ref<string[]>([])
 
   const rules: Record<string, ValidationRule<string | undefined>[]> = {
-    name: [isRequired(localization.info.name), isHandle(localization.info.name), isUnique],
-    value: [isRequired(localization.info.value)],
+    name: [
+      isRequired(localization.info.name),
+      isLessThanOrEqual(MAX_CHARS)(localization.info.value),
+      isHandle(localization.info.name),
+      nameIsUnique,
+    ],
+    value: [
+      isRequired(localization.info.value),
+      isLessThanOrEqual(MAX_CHARS)(localization.info.value),
+    ],
   }
 
   const { error: nameErrorMessage, state: nameState } = useValidation(name, localization.info.name, rules.name)
   const { error: valueErrorMessage, state: valueState } = useValidation(value, localization.info.value, rules.value)
 
+  const reset = (): void => {
+    name.value = undefined
+    value.value = undefined
+    tags.value = []
+  }
+
   const submit = async (): Promise<void> => {
     const valid = await validate()
 
@@ -114,6 +125,7 @@
         showToast(localization.success.createVariable, 'success')
         emit('create', variable)
         internalValue.value = false
+        reset()
       } catch (error) {
         console.error(error)
         showToast(localization.error.createVariable, 'error')

diff --git a/src/components/VariableEditModal.vue b/src/components/VariableEditModal.vue
@@ -1,5 +1,5 @@
 <template>
-  <p-modal v-model:showModal="internalValue" :title="localization.info.newVariable">
+  <p-modal v-model:showModal="internalValue" :title="localization.info.editVariable(name)">
     <p-form @submit="submit">
       <p-content>
         <p-label :label="localization.info.name" :state="nameState" :message="nameErrorMessage">
@@ -21,11 +21,6 @@
         {{ localization.info.save }}
       </p-button>
     </template>
-    <template #cancel>
-      <p-button inset @click="internalValue = false">
-        {{ localization.info.close }}
-      </p-button>
-    </template>
   </p-modal>
 </template>
 
@@ -37,7 +32,9 @@
   import { useWorkspaceApi } from '@/compositions'
   import { localization } from '@/localization'
   import { Variable, VariableEdit } from '@/models'
-  import { isHandle, isRequired, isString } from '@/utilities'
+  import { isHandle, isRequired, isString, isLessThanOrEqual } from '@/utilities'
+
+  const MAX_CHARS = 255
 
   const props = defineProps<{
     variable: Variable,
@@ -60,7 +57,7 @@
 
   const api = useWorkspaceApi()
 
-  const isUnique: ValidationRule<string | undefined> = async (value, label, { signal, source, previousValue }) => {
+  const nameIsUnique: ValidationRule<string | undefined> = async (value, label, { signal, source, previousValue }) => {
     if (value === previousValue) {
       return
     }
@@ -97,8 +94,16 @@
   const tags = ref<string[]>(props.variable.tags)
 
   const rules: Record<string, ValidationRule<string | undefined>[]> = {
-    name: [isRequired(localization.info.name), isHandle(localization.info.name), isUnique],
-    value: [isRequired(localization.info.value)],
+    name: [
+      isRequired(localization.info.name),
+      isLessThanOrEqual(MAX_CHARS)(localization.info.name),
+      isHandle(localization.info.name),
+      nameIsUnique,
+    ],
+    value: [
+      isRequired(localization.info.value),
+      isLessThanOrEqual(MAX_CHARS)(localization.info.value),
+    ],
   }
 
   const { error: nameErrorMessage, state: nameState } = useValidation(name, localization.info.name, rules.name)

diff --git a/src/components/VariablesTable.vue b/src/components/VariablesTable.vue
@@ -17,18 +17,21 @@
         </div>
       </template>
 
-      <p-table :data="variables" :columns="columns">
+      <p-table :data="variables" :columns="columns" :column-classes="columnClass">
         <template #selection-heading>
-          <p-checkbox v-if="variables.length" v-model="model" @update:model-value="selectAllVariables" />
-          <div v-else />
+          <div class="variables-table__selection">
+            <p-checkbox v-if="variables.length" v-model="model" @update:model-value="selectAllVariables" />
+          </div>
         </template>
 
         <template #selection="{ row }">
           <p-checkbox v-model="selectedVariables" :value="row.id" />
         </template>
 
         <template #name="{ row }">
-          <span>{{ row.name }}</span>
+          <div class="variables-table__name">
+            {{ row.name }}
+          </div>
         </template>
 
         <template #updated="{ row }">
@@ -44,8 +47,8 @@
         </template>
 
         <template #action="{ row }">
-          <div class="variables-table__action">
-            <VariableMenu :variable="row" size="xs" @delete="refreshSubscriptions" />
+          <div :key="row.id" class="variables-table__action">
+            <VariableMenu :variable="row" size="xs" @delete="refreshSubscriptions" @update="handleUpdate" />
           </div>
         </template>
 
@@ -71,13 +74,13 @@
 </template>
 
 <script lang="ts" setup>
-  import { PTable, PEmptyResults, CheckboxModel } from '@prefecthq/prefect-design'
+  import { PTable, PEmptyResults, CheckboxModel, TableColumn, ClassValue } from '@prefecthq/prefect-design'
   import { useDebouncedRef, useSubscription } from '@prefecthq/vue-compositions'
   import { computed, ref } from 'vue'
   import { VariablesDeleteButton, VariableMenu, ResultsCount, SearchInput, SelectedCount } from '@/components'
   import { useCan, useVariablesFilter, useWorkspaceApi } from '@/compositions'
   import { localization } from '@/localization'
-  import { VariablesFilter } from '@/models/Filters'
+  import { VariablesFilter, Variable } from '@/models'
   import { variableSortOptions } from '@/types'
   import { formatDateTimeNumeric } from '@/utilities/dates'
 
@@ -118,12 +121,11 @@
     {
       property: 'name',
       label: 'Name',
-      width: '64px',
+      width: '124px',
     },
     {
       property: 'value',
       label: 'Value',
-      width: '124px',
     },
     {
       property: 'updated',
@@ -133,14 +135,20 @@
     {
       property: 'tags',
       label: 'Tags',
-      width: '124px',
+      width: '248px',
     },
     {
       label: 'Action',
       width: '42px',
     },
   ]
 
+  function columnClass(column: TableColumn): ClassValue {
+    return {
+      'variables-table__value-td': column.label === 'Value',
+    }
+  }
+
   const selectedVariables = ref<string[]>([])
   const selectAllVariables = (allVariablesSelected: CheckboxModel): string[] => {
     if (allVariablesSelected) {
@@ -175,13 +183,18 @@
 
   const emit = defineEmits<{
     (event: 'delete'): void,
+    (event: 'update', value: Variable): void,
   }>()
 
   const deleteVariables = (): void => {
     selectedVariables.value = []
     refreshSubscriptions()
     emit('delete')
   }
+
+  const handleUpdate = (variable: Variable): void => {
+    emit('update', variable)
+  }
 </script>
 
 <style>
@@ -213,5 +226,21 @@
 
 .variables-table__action { @apply
   text-right
+  max-w-[42px]
+}
+
+.variables-table__selection { @apply
+  max-w-[20px]
+}
+
+.variables-table__value-td,
+.variables-table__name { @apply
+  min-w-0
+  max-w-0
+  truncate
+}
+
+.variables-table__name { @apply
+  max-w-[124px]
 }
 </style>
diff --git a/src/localization/locale/en.ts b/src/localization/locale/en.ts
@@ -45,6 +45,10 @@ export const en = {
     resumeFlowRun: 'Failed to resume flow run',
     retryRun: 'Failed to retry flow run',
     scheduleFlowRun: 'Failed to schedule flow run',
+    arrayValueTooLong: (property: string, max: number) => `${property} must have fewer than ${max} items`,
+    stringValueTooLong: (property: string, max: number) => `${property} must be less than or equal to ${max} characters`,
+    numberValueTooLarge: (property: string, max: number) => `${property} must be less than or equal to ${max}`,
+    valueTooLarge: (property: string, max: number) => `${property} must be less than or equal to ${max}`,
     submitNotification: 'Failed to submit notification',
     updateBlock: 'Failed to update block',
     updateNotification: 'Failed to update notification',

diff --git a/src/utilities/validation.ts b/src/utilities/validation.ts
@@ -1,4 +1,5 @@
 import { isDateAfter, isDateAfterOrEqual, isDateBefore, isDateBeforeOrEqual } from '@prefecthq/prefect-design'
+import { localization } from '@/localization'
 import { isEmptyArray } from '@/utilities/arrays'
 import { isDate, isInvalidDate, formatDate, formatDateTimeNumeric } from '@/utilities/dates'
 import { isEmptyString, isValidEmailAddress } from '@/utilities/strings'
@@ -110,19 +111,31 @@ export const isLessThanOrEqual = (max: number): ValidationMethodFactory => prope
     return true
   }
 
-  if (Array.isArray(value) && value.length <= max) {
-    return true
+  if (Array.isArray(value)) {
+    if (value.length <= max) {
+      return true
+    }
+
+    return localization.error.arrayValueTooLong(property, max)
   }
 
-  if (typeof value === 'string' && value.length <= max) {
-    return true
+  if (typeof value === 'string') {
+    if (value.length <= max) {
+      return true
+    }
+
+    return localization.error.stringValueTooLong(property, max)
   }
 
-  if (typeof value === 'number' && value <= max) {
-    return true
+  if (typeof value === 'number') {
+    if (value <= max) {
+      return true
+    }
+
+    return localization.error.numberValueTooLarge(property, max)
   }
 
-  return `${property} must be less than or equal to ${max}`
+  return localization.error.valueTooLarge(property, max)
 }
 
 export const isGreaterThan = (min: number): ValidationMethodFactory => property => (value: unknown) => {