Re-working the error handling for the SQL relationship modal, as well as adding some better defaults for the majority of the options to make the UI a bit easier to use.

packages/builder/src/components/backend/Datasources/CreateEditRelationship.svelte

@@ -10,17 +10,17 @@
   } from "@budibase/bbui"
   import { tables } from "stores/backend"
   import { Helpers } from "@budibase/bbui"
+  import { RelationshipErrorChecker } from "./relationshipErrors"
+  import { onMount } from "svelte"
 
   export let save
   export let datasource
   export let plusTables = []
   export let fromRelationship = {}
   export let toRelationship = {}
+  export let selectedFromTable
   export let close
 
-  const colNotSet = "Please specify a column name"
-  const relationshipAlreadyExists =
-    "A relationship between these tables already exists."
   const relationshipTypes = [
     {
       label: "One to Many",
@@ -42,8 +42,11 @@
   )
 
   let tableOptions
+  let errorChecker = new RelationshipErrorChecker(
+    invalidThroughTable,
+    relationshipExists
+  )
   let errors = {}
-  let hasClickedSave = !!fromRelationship.relationshipType
   let fromPrimary,
     fromForeign,
     fromTable,
@@ -51,12 +54,19 @@
     throughTable,
     fromColumn,
     toColumn
-  let fromId, toId, throughId, throughToKey, throughFromKey
+  let fromId = selectedFromTable?._id,
+    toId,
+    throughId,
+    throughToKey,
+    throughFromKey
   let isManyToMany, isManyToOne, relationshipType
+  let hasValidated = false
 
   $: {
     if (!fromPrimary) {
       fromPrimary = fromRelationship.foreignKey
+    }
+    if (!fromForeign) {
       fromForeign = toRelationship.foreignKey
     }
     if (!fromColumn && !errors.fromColumn) {
@@ -77,7 +87,8 @@
       throughToKey = fromRelationship.throughTo
     }
     if (!relationshipType) {
-      relationshipType = fromRelationship.relationshipType
+      relationshipType =
+        fromRelationship.relationshipType || RelationshipTypes.MANY_TO_ONE
     }
   }
 
@@ -85,7 +96,7 @@
     label: table.name,
     value: table._id,
   }))
-  $: valid = getErrorCount(errors) === 0 || !hasClickedSave
+  $: valid = getErrorCount(errors) === 0 && allRequiredAttributesSet()
 
   $: isManyToMany = relationshipType === RelationshipTypes.MANY_TO_MANY
   $: isManyToOne = relationshipType === RelationshipTypes.MANY_TO_ONE
@@ -95,10 +106,20 @@
 
   $: toRelationship.relationshipType = fromRelationship?.relationshipType
 
-  const getErrorCount = errors =>
-    Object.entries(errors)
+  function getErrorCount(errors) {
+    return Object.entries(errors)
       .filter(entry => !!entry[1])
       .map(entry => entry[0]).length
+  }
+
+  function allRequiredAttributesSet() {
+    const base = fromTable && toTable && fromColumn && toColumn
+    if (isManyToOne) {
+      return base && fromPrimary && fromForeign
+    } else {
+      return base && throughTable && throughFromKey && throughToKey
+    }
+  }
 
   function invalidThroughTable() {
     // need to know the foreign key columns to check error
@@ -118,93 +139,48 @@
   }
 
   function validate() {
-    const isMany = relationshipType === RelationshipTypes.MANY_TO_MANY
-    const tableNotSet = "Please specify a table"
-    const foreignKeyNotSet = "Please pick a foreign key"
+    if (!allRequiredAttributesSet() && !hasValidated) {
+      return
+    }
+    hasValidated = true
+    errorChecker.setType(relationshipType)
     const errObj = {}
-    if (!relationshipType) {
-      errObj.relationshipType = "Please specify a relationship type"
-    }
-    if (!fromTable) {
-      errObj.fromTable = tableNotSet
-    }
-    if (!toTable) {
-      errObj.toTable = tableNotSet
-    }
-    if (isMany && !throughTable) {
-      errObj.throughTable = tableNotSet
-    }
-    if (isMany && !throughFromKey) {
-      errObj.throughFromKey = foreignKeyNotSet
-    }
-    if (isMany && !throughToKey) {
-      errObj.throughToKey = foreignKeyNotSet
-    }
-    if (invalidThroughTable()) {
-      errObj.throughTable =
-        "Ensure non-key columns are nullable or auto-generated"
-    }
-    if (!isMany && !fromForeign) {
-      errObj.fromForeign = foreignKeyNotSet
-    }
-    if (!fromColumn) {
-      errObj.fromColumn = colNotSet
-    }
-    if (!toColumn) {
-      errObj.toColumn = colNotSet
-    }
-    if (!isMany && !fromPrimary) {
-      errObj.fromPrimary = "Please pick the primary key"
-    }
-    if (isMany && relationshipExists()) {
-      errObj.fromTable = relationshipAlreadyExists
-      errObj.toTable = relationshipAlreadyExists
-    }
-
+    errObj.relationshipType = errorChecker.relationshipTypeSet(relationshipType)
+    errObj.fromTable = errorChecker.tableSet(fromTable)
+    errObj.toTable = errorChecker.tableSet(toTable)
+    errObj.throughTable = errorChecker.throughTableSet(throughTable)
+    errObj.throughFromKey = errorChecker.manyForeignKeySet(throughFromKey)
+    errObj.throughToKey = errorChecker.manyForeignKeySet(throughToKey)
+    errObj.throughTable = errorChecker.throughIsNullable()
+    errObj.fromForeign = errorChecker.foreignKeySet(fromForeign)
+    errObj.fromPrimary = errorChecker.foreignKeySet(fromPrimary)
+    errObj.fromTable = errorChecker.doesRelationshipExists()
+    errObj.toTable = errorChecker.doesRelationshipExists()
     // currently don't support relationships back onto the table itself, needs to relate out
-    const tableError = "From/to/through tables must be different"
-    if (fromTable && (fromTable === toTable || fromTable === throughTable)) {
-      errObj.fromTable = tableError
-    }
-    if (toTable && (toTable === fromTable || toTable === throughTable)) {
-      errObj.toTable = tableError
-    }
-    if (
-      throughTable &&
-      (throughTable === fromTable || throughTable === toTable)
-    ) {
-      errObj.throughTable = tableError
-    }
-    const colError = "Column name cannot be an existing column"
-    if (isColumnNameBeingUsed(toTable, fromColumn, originalFromColumnName)) {
-      errObj.fromColumn = colError
-    }
-    if (isColumnNameBeingUsed(fromTable, toColumn, originalToColumnName)) {
-      errObj.toColumn = colError
-    }
-
-    let fromType, toType
-    if (fromPrimary && fromForeign) {
-      fromType = fromTable?.schema[fromPrimary]?.type
-      toType = toTable?.schema[fromForeign]?.type
-    }
-    if (fromType && toType && fromType !== toType) {
-      errObj.fromForeign =
-        "Column type of the foreign key must match the primary key"
-    }
-
+    errObj.fromTable = errorChecker.differentTables(fromId, toId, throughId)
+    errObj.toTable = errorChecker.differentTables(toId, fromId, throughId)
+    errObj.throughTable = errorChecker.differentTables(throughId, fromId, toId)
+    errObj.fromColumn = errorChecker.columnBeingUsed(
+      toTable,
+      fromColumn,
+      originalFromColumnName
+    )
+    errObj.toColumn = errorChecker.columnBeingUsed(
+      fromTable,
+      toColumn,
+      originalToColumnName
+    )
+    errObj.fromForeign = errorChecker.typeMismatch(
+      fromTable,
+      toTable,
+      fromPrimary,
+      fromForeign
+    )
+    console.log(errObj)
     errors = errObj
     return getErrorCount(errors) === 0
   }
 
-  function isColumnNameBeingUsed(table, columnName, originalName) {
-    if (!table || !columnName || columnName === originalName) {
-      return false
-    }
-    const keys = Object.keys(table.schema).map(key => key.toLowerCase())
-    return keys.indexOf(columnName.toLowerCase()) !== -1
-  }
-
   function buildRelationships() {
     const id = Helpers.uuid()
     //Map temporary variables
@@ -320,7 +296,6 @@
   }
 
   async function saveRelationship() {
-    hasClickedSave = true
     if (!validate()) {
       return false
     }
@@ -342,6 +317,20 @@
     await tables.fetch()
     close()
   }
+
+  function changed(fn) {
+    if (fn && typeof fn === "function") {
+      fn()
+    }
+    validate()
+  }
+
+  onMount(() => {
+    if (selectedFromTable) {
+      fromColumn = selectedFromTable.name
+      fromPrimary = selectedFromTable?.primary[0] || null
+    }
+  })
 </script>
 
 <ModalContent
@@ -355,34 +344,32 @@
     options={relationshipTypes}
     bind:value={relationshipType}
     bind:error={errors.relationshipType}
-    on:change={() => (errors.relationshipType = null)}
+    on:change={changed}
   />
   <div class="headings">
     <Detail>Tables</Detail>
   </div>
-  <Select
-    label="Select from table"
-    options={tableOptions}
-    bind:value={fromId}
-    bind:error={errors.fromTable}
-    on:change={e => {
-      fromColumn = tableOptions.find(opt => opt.value === e.detail)?.label || ""
-      if (errors.fromTable === relationshipAlreadyExists) {
-        errors.toColumn = null
-      }
-      errors.fromTable = null
-      errors.fromColumn = null
-      errors.toTable = null
-      errors.throughTable = null
-    }}
-  />
+  {#if !selectedFromTable}
+    <Select
+      label="Select from table"
+      options={tableOptions}
+      bind:value={fromId}
+      bind:error={errors.fromTable}
+      on:change={e =>
+        changed(() => {
+          const table = plusTables.find(tbl => tbl._id === e.detail)
+          fromColumn = table?.name || ""
+          fromPrimary = table?.primary?.[0]
+        })}
+    />
+  {/if}
   {#if isManyToOne && fromTable}
     <Select
       label={`Primary Key (${fromTable.name})`}
       options={Object.keys(fromTable.schema)}
       bind:value={fromPrimary}
       bind:error={errors.fromPrimary}
-      on:change={() => (errors.fromPrimary = null)}
+      on:change={changed}
     />
   {/if}
   <Select
@@ -390,16 +377,12 @@
     options={tableOptions}
     bind:value={toId}
     bind:error={errors.toTable}
-    on:change={e => {
-      toColumn = tableOptions.find(opt => opt.value === e.detail)?.label || ""
-      if (errors.toTable === relationshipAlreadyExists) {
-        errors.fromColumn = null
-      }
-      errors.toTable = null
-      errors.toColumn = null
-      errors.fromTable = null
-      errors.throughTable = null
-    }}
+    on:change={e =>
+      changed(() => {
+        const table = plusTables.find(tbl => tbl._id === e.detail)
+        toColumn = table.name || ""
+        fromForeign = null
+      })}
   />
   {#if isManyToMany}
     <Select
@@ -407,11 +390,11 @@
       options={tableOptions}
       bind:value={throughId}
       bind:error={errors.throughTable}
-      on:change={() => {
-        errors.fromTable = null
-        errors.toTable = null
-        errors.throughTable = null
-      }}
+      on:change={() =>
+        changed(() => {
+          throughToKey = null
+          throughFromKey = null
+        })}
     />
     {#if fromTable && toTable && throughTable}
       <Select
@@ -419,24 +402,24 @@
         options={Object.keys(throughTable?.schema)}
         bind:value={throughToKey}
         bind:error={errors.throughToKey}
-        on:change={e => {
-          if (throughFromKey === e.detail) {
-            throughFromKey = null
-          }
-          errors.throughToKey = null
-        }}
+        on:change={e =>
+          changed(() => {
+            if (throughFromKey === e.detail) {
+              throughFromKey = null
+            }
+          })}
       />
       <Select
         label={`Foreign Key (${toTable?.name})`}
         options={Object.keys(throughTable?.schema)}
         bind:value={throughFromKey}
         bind:error={errors.throughFromKey}
-        on:change={e => {
-          if (throughToKey === e.detail) {
-            throughToKey = null
-          }
-          errors.throughFromKey = null
-        }}
+        on:change={e =>
+          changed(() => {
+            if (throughToKey === e.detail) {
+              throughToKey = null
+            }
+          })}
       />
     {/if}
   {:else if isManyToOne && toTable}
@@ -445,7 +428,7 @@
       options={Object.keys(toTable?.schema)}
       bind:value={fromForeign}
       bind:error={errors.fromForeign}
-      on:change={() => (errors.fromForeign = null)}
+      on:change={changed}
     />
   {/if}
   <div class="headings">
@@ -459,15 +442,13 @@
     label="From table column"
     bind:value={fromColumn}
     bind:error={errors.fromColumn}
-    on:change={e => {
-      errors.fromColumn = e.detail?.length > 0 ? null : colNotSet
-    }}
+    on:change={changed}
   />
   <Input
     label="To table column"
     bind:value={toColumn}
     bind:error={errors.toColumn}
-    on:change={e => (errors.toColumn = e.detail?.length > 0 ? null : colNotSet)}
+    on:change={changed}
   />
   <div slot="footer">
     {#if originalFromColumnName != null}

packages/builder/src/components/backend/Datasources/relationshipErrors.js

@@ -0,0 +1,86 @@
+import { RelationshipTypes } from "constants/backend"
+
+export const typeMismatch =
+  "Column type of the foreign key must match the primary key"
+export const columnCantExist = "Column name cannot be an existing column"
+export const mustBeDifferentTables = "From/to/through tables must be different"
+export const primaryKeyNotSet = "Please pick the primary key"
+export const throughNotNullable =
+  "Ensure non-key columns are nullable or auto-generated"
+export const noRelationshipType = "Please specify a relationship type"
+export const tableNotSet = "Please specify a table"
+export const foreignKeyNotSet = "Please pick a foreign key"
+export const relationshipAlreadyExists =
+  "A relationship between these tables already exists"
+
+function isColumnNameBeingUsed(table, columnName, originalName) {
+  if (!table || !columnName || columnName === originalName) {
+    return false
+  }
+  const keys = Object.keys(table.schema).map(key => key.toLowerCase())
+  return keys.indexOf(columnName.toLowerCase()) !== -1
+}
+
+export class RelationshipErrorChecker {
+  constructor(invalidThroughTableFn, relationshipExistsFn) {
+    this.invalidThroughTable = invalidThroughTableFn
+    this.relationshipExists = relationshipExistsFn
+  }
+
+  setType(type) {
+    this.type = type
+  }
+
+  isMany() {
+    return this.type === RelationshipTypes.MANY_TO_MANY
+  }
+
+  relationshipTypeSet(type) {
+    return !type ? noRelationshipType : null
+  }
+
+  tableSet(table) {
+    return !table ? tableNotSet : null
+  }
+
+  throughTableSet(table) {
+    return this.isMany() && !table ? tableNotSet : null
+  }
+
+  manyForeignKeySet(key) {
+    return this.isMany() && !key ? foreignKeyNotSet : null
+  }
+
+  foreignKeySet(key) {
+    return !this.isMany() && !key ? foreignKeyNotSet : null
+  }
+
+  throughIsNullable() {
+    return this.invalidThroughTable() ? throughNotNullable : null
+  }
+
+  doesRelationshipExists() {
+    return this.isMany() && this.relationshipExists()
+      ? relationshipAlreadyExists
+      : null
+  }
+
+  differentTables(table1, table2, table3) {
+    // currently don't support relationships back onto the table itself, needs to relate out
+    const error = table1 && (table1 === table2 || (table3 && table1 === table3))
+    return error ? mustBeDifferentTables : null
+  }
+
+  columnBeingUsed(table, column, ogName) {
+    return isColumnNameBeingUsed(table, column, ogName) ? columnCantExist : null
+  }
+
+  typeMismatch(fromTable, toTable, primary, foreign) {
+    let fromType, toType
+    if (primary && foreign) {
+      fromType = fromTable?.schema[primary]?.type
+      toType = toTable?.schema[foreign]?.type
+    }
+    return fromType && toType && fromType !== toType ? typeMismatch : null
+  }
+}