From b78ddaba27d0380252a4fa85a7d2af599c139031 Mon Sep 17 00:00:00 2001
From: Ben Whitmore <ben.whitmore0@gmail.com>
Date: Thu, 18 Mar 2021 18:49:05 +1300
Subject: [PATCH] fix(form-builder): fix deletions and page crashes

Most of #42 is fixed. There is still a page crash on deleting a form (workaround is to refresh).
---
 ...tProperties.jsx => ComponentProperties.js} | 128 +++++++++------
 .../src/components/FormBuilder.js             |  57 ++++---
 .../src/components/FormBuilderLayout.js       | 154 ++++++++++++++++++
 .../src/components/FormBuilderLayout.jsx      | 118 --------------
 .../src/components/FormBuilderPage.js         |  54 +++---
 .../{FormProperties.jsx => FormProperties.js} |  53 ++++--
 .../builderComponents/OptionsField.js         |  94 ++++++-----
 server/formbuilder/src/resolvers.js           |  88 +++++-----
 8 files changed, 438 insertions(+), 308 deletions(-)
 rename app/components/component-formbuilder/src/components/{ComponentProperties.jsx => ComponentProperties.js} (55%)
 create mode 100644 app/components/component-formbuilder/src/components/FormBuilderLayout.js
 delete mode 100644 app/components/component-formbuilder/src/components/FormBuilderLayout.jsx
 rename app/components/component-formbuilder/src/components/{FormProperties.jsx => FormProperties.js} (76%)

diff --git a/app/components/component-formbuilder/src/components/ComponentProperties.jsx b/app/components/component-formbuilder/src/components/ComponentProperties.js
similarity index 55%
rename from app/components/component-formbuilder/src/components/ComponentProperties.jsx
rename to app/components/component-formbuilder/src/components/ComponentProperties.js
index 33397de0ee..fa9c3224f3 100644
--- a/app/components/component-formbuilder/src/components/ComponentProperties.jsx
+++ b/app/components/component-formbuilder/src/components/ComponentProperties.js
@@ -1,4 +1,5 @@
 import React, { useState } from 'react'
+import PropTypes from 'prop-types'
 import { map, omitBy } from 'lodash'
 import { ValidatedFieldFormik, Menu, Button } from '@pubsweet/ui'
 import { Formik } from 'formik'
@@ -18,22 +19,21 @@ const MenuComponents = input => (
 )
 
 const ComponentProperties = ({
-  properties,
-  setComponent,
+  onSubmit,
   selectedComponent,
-  handleSubmit,
+  setComponentType,
   setFieldValue,
 }) => (
   <Page>
-    <form onSubmit={handleSubmit}>
+    <form onSubmit={onSubmit}>
       <Heading>Component Properties</Heading>
       <Section>
-        <Legend space>Choose Component</Legend>
+        <Legend space>Component Type</Legend>
         <ValidatedFieldFormik
           component={MenuComponents}
           name="component"
           onChange={value => {
-            setComponent(value)
+            setComponentType(value)
             setFieldValue('component', value)
           }}
         />
@@ -46,16 +46,9 @@ const ComponentProperties = ({
               component={elements[value.component].default}
               key={`${selectedComponent}-${key}`}
               name={key}
-              onChange={event => {
-                let value = {}
-                if (event.target) {
-                  // eslint-disable-next-line prefer-destructuring
-                  value = event.target.value
-                } else {
-                  value = event
-                }
-                setFieldValue(key, value)
-              }}
+              onChange={val =>
+                setFieldValue(key, val.target ? val.target.value : val)
+              }
               {...value.props}
             />
           </Section>
@@ -67,15 +60,39 @@ const ComponentProperties = ({
   </Page>
 )
 
-const UpdateForm = ({ handleSubmit, properties, setFieldValue }) => (
+ComponentProperties.propTypes = {
+  onSubmit: PropTypes.func.isRequired,
+  selectedComponent: PropTypes.string,
+  setComponentType: PropTypes.func.isRequired,
+  setFieldValue: PropTypes.func.isRequired,
+}
+
+ComponentProperties.defaultProps = {
+  selectedComponent: null,
+}
+
+const UpdateForm = ({ onSubmit, properties, setFieldValue }) => (
   <FormProperties
-    handleSubmit={handleSubmit}
     mode="update"
+    onSubmit={onSubmit}
     properties={properties}
     setFieldValue={setFieldValue}
   />
 )
 
+UpdateForm.propTypes = {
+  onSubmit: PropTypes.func.isRequired,
+  properties: PropTypes.shape({
+    id: PropTypes.string.isRequired,
+    name: PropTypes.string.isRequired,
+    description: PropTypes.string.isRequired,
+    haspopup: PropTypes.string.isRequired,
+    popuptitle: PropTypes.string,
+    popupdescription: PropTypes.string,
+  }).isRequired,
+  setFieldValue: PropTypes.func.isRequired,
+}
+
 const prepareForSubmit = values => {
   const cleanedValues = omitBy(values, value => value === '')
   return JSON.stringify(cleanedValues)
@@ -109,34 +126,43 @@ const prepareForSubmit = values => {
 //   )(ComponentForm),
 // )
 
-const ComponentForm = ({ updateForm, updateFormElement, ...props }) => {
-  const [selectedComponent, setComponent] = useState(
-    props.properties.properties.component,
-  )
-  return props.properties.type === 'form' ? (
-    <Formik
-      initialValues={props.properties.properties}
-      onSubmit={values =>
-        updateForm({
-          variables: { formId: values.id, form: prepareForSubmit(values) },
-        })
-      }
-    >
-      {formikProps => (
-        <UpdateForm
-          handleSubmit={formikProps.handleSubmit}
-          properties={props.properties}
-          setFieldValue={formikProps.setFieldValue}
-        />
-      )}
-    </Formik>
-  ) : (
+const ComponentForm = ({
+  fieldOrForm,
+  isField,
+  formId,
+  updateForm,
+  updateFormElement,
+}) => {
+  const [componentType, setComponentType] = useState(fieldOrForm.component)
+
+  if (!isField)
+    return (
+      <Formik
+        initialValues={fieldOrForm}
+        onSubmit={values =>
+          updateForm({
+            variables: { formId: values.id, form: prepareForSubmit(values) },
+          })
+        }
+      >
+        {formikProps => (
+          <UpdateForm
+            onSubmit={formikProps.handleSubmit}
+            properties={fieldOrForm}
+            setFieldValue={formikProps.setFieldValue}
+          />
+        )}
+      </Formik>
+    )
+
+  return (
     <Formik
-      initialValues={props.properties.properties}
+      initialValues={fieldOrForm}
+      key={fieldOrForm.id}
       onSubmit={values =>
         updateFormElement({
           variables: {
-            formId: props.properties.formId,
+            formId,
             element: prepareForSubmit(values),
           },
         })
@@ -144,10 +170,9 @@ const ComponentForm = ({ updateForm, updateFormElement, ...props }) => {
     >
       {formikProps => (
         <ComponentProperties
-          displayName="ComponentSubmit"
-          handleSubmit={formikProps.handleSubmit}
-          selectedComponent={selectedComponent}
-          setComponent={setComponent}
+          onSubmit={formikProps.handleSubmit}
+          selectedComponent={componentType}
+          setComponentType={setComponentType}
           setFieldValue={formikProps.setFieldValue}
         />
       )}
@@ -155,4 +180,15 @@ const ComponentForm = ({ updateForm, updateFormElement, ...props }) => {
   )
 }
 
+ComponentForm.propTypes = {
+  fieldOrForm: PropTypes.shape({
+    id: PropTypes.string.isRequired,
+    component: PropTypes.string,
+  }).isRequired,
+  isField: PropTypes.bool.isRequired,
+  formId: PropTypes.string.isRequired,
+  updateForm: PropTypes.func.isRequired,
+  updateFormElement: PropTypes.func.isRequired,
+}
+
 export default ComponentForm
diff --git a/app/components/component-formbuilder/src/components/FormBuilder.js b/app/components/component-formbuilder/src/components/FormBuilder.js
index 6af3039da5..fa43e49271 100644
--- a/app/components/component-formbuilder/src/components/FormBuilder.js
+++ b/app/components/component-formbuilder/src/components/FormBuilder.js
@@ -1,4 +1,4 @@
-import React, { useState } from 'react'
+import React from 'react'
 import PropTypes from 'prop-types'
 import styled, { withTheme } from 'styled-components'
 import { unescape } from 'lodash'
@@ -61,7 +61,7 @@ const createMarkup = encodedHtml => ({
 
 const BuilderElement = ({
   elements,
-  setProperties,
+  setActiveFieldId,
   deleteFormElement,
   formId,
 }) => {
@@ -71,26 +71,18 @@ const BuilderElement = ({
 
   return orderedElements.map(element => (
     <Element key={`element-${element.id}`}>
-      <Action
-        onClick={() =>
-          setProperties({
-            type: 'element',
-            formId,
-            properties: element,
-          })
-        }
-      >
+      <Action onClick={() => setActiveFieldId(element.id)}>
         <ElementTitle dangerouslySetInnerHTML={createMarkup(element.title)} /> (
         {element.component})
       </Action>
       <Action
-        onClick={() =>
+        onClick={() => {
           deleteFormElement({
             variables: { formId, elementId: element.id },
           })
-        }
+        }}
       >
-        x
+        🗙
       </Action>
     </Element>
   ))
@@ -101,11 +93,11 @@ BuilderElement.propTypes = {
     PropTypes.shape({
       id: PropTypes.string.isRequired,
       title: PropTypes.string.isRequired,
-      component: PropTypes.string.isRequired,
+      component: PropTypes.string,
       order: PropTypes.string,
     }).isRequired,
   ).isRequired,
-  setProperties: PropTypes.func.isRequired,
+  setActiveFieldId: PropTypes.func.isRequired,
   deleteFormElement: PropTypes.func.isRequired,
   formId: PropTypes.string.isRequired,
 }
@@ -134,24 +126,30 @@ AddButtonElement.propTypes = {
   addElement: PropTypes.func.isRequired,
 }
 
-const FormBuilder = ({ form, setProperties, deleteFormElement }) => {
-  const [elements, setElements] = useState(form.children || [])
-
-  const addElement = element => {
-    setElements([...elements, element])
-  }
-
+const FormBuilder = ({
+  form,
+  setActiveFieldId,
+  addFormElement,
+  deleteFormElement,
+}) => {
   return (
     <Page>
-      <AddButtonElement addElement={addElement} />
-      {elements && elements.length > 0 && (
+      {form.children && form.children.length > 0 && (
         <BuilderElement
           deleteFormElement={deleteFormElement}
-          elements={elements}
+          elements={form.children}
           formId={form.id}
-          setProperties={setProperties}
+          setActiveFieldId={setActiveFieldId}
         />
       )}
+      <AddButtonElement
+        addElement={newElement => {
+          addFormElement({
+            variables: { element: JSON.stringify(newElement), formId: form.id },
+          })
+          setActiveFieldId(newElement.id)
+        }}
+      />
     </Page>
   )
 }
@@ -163,12 +161,13 @@ FormBuilder.propTypes = {
       PropTypes.shape({
         id: PropTypes.string.isRequired,
         title: PropTypes.string.isRequired,
-        component: PropTypes.string.isRequired,
+        component: PropTypes.string,
         order: PropTypes.string,
       }).isRequired,
     ).isRequired,
   }).isRequired,
-  setProperties: PropTypes.func.isRequired,
+  setActiveFieldId: PropTypes.func.isRequired,
+  addFormElement: PropTypes.func.isRequired,
   deleteFormElement: PropTypes.func.isRequired,
 }
 
diff --git a/app/components/component-formbuilder/src/components/FormBuilderLayout.js b/app/components/component-formbuilder/src/components/FormBuilderLayout.js
new file mode 100644
index 0000000000..b31deffc6a
--- /dev/null
+++ b/app/components/component-formbuilder/src/components/FormBuilderLayout.js
@@ -0,0 +1,154 @@
+import React from 'react'
+import PropTypes from 'prop-types'
+import { forEach } from 'lodash'
+import styled from 'styled-components'
+import { Tabs, Action } from '@pubsweet/ui'
+import { Columns, Details, Form } from './style'
+import ComponentProperties from './ComponentProperties'
+import FormBuilder from './FormBuilder'
+import {
+  Container,
+  HeadingWithAction,
+  Heading,
+  SectionContent,
+  SectionRow,
+} from '../../../shared'
+
+const DeleteIcon = styled(Action)`
+  line-height: 1.15;
+  margin-left: 10px;
+`
+
+const FormBuilderLayout = ({
+  forms,
+  activeFormId,
+  activeFieldId,
+  deleteForm,
+  deleteFormElement,
+  updateForm,
+  createForm,
+  updateFormElement,
+  setActiveFieldId,
+  setActiveFormId,
+}) => {
+  const sections = []
+  forEach(forms, form => {
+    sections.push({
+      content: (
+        <SectionContent>
+          <SectionRow>
+            <FormBuilder
+              addFormElement={updateFormElement}
+              deleteFormElement={deleteFormElement}
+              form={form}
+              setActiveFieldId={setActiveFieldId}
+            />
+          </SectionRow>
+        </SectionContent>
+      ),
+      key: `${form.id}`,
+      label: [
+        form.name,
+        <DeleteIcon
+          key="delete-form"
+          onClick={e => {
+            e.preventDefault()
+            deleteForm({
+              variables: { formId: form.id },
+            })
+            setActiveFormId(forms.find(f => f.id !== form.id)?.id ?? 'new')
+          }}
+        >
+          🗙
+        </DeleteIcon>,
+      ],
+    })
+  })
+
+  sections.push({
+    content: <SectionContent />,
+    key: 'new',
+    label: '✚ Add Form',
+  })
+
+  const activeForm = forms.find(f => f.id === activeFormId) ?? {
+    children: [],
+    id: '',
+    name: '',
+    description: '',
+    haspopup: 'false',
+  }
+
+  const activeField = activeForm.children.find(
+    elem => elem.id === activeFieldId,
+  )
+
+  const fieldOrForm = activeField ?? activeForm
+
+  return (
+    <Container>
+      <HeadingWithAction>
+        <Heading>Form Builder</Heading>
+      </HeadingWithAction>
+      <Columns>
+        <Form>
+          <Tabs
+            activeKey={`${activeFormId}`}
+            onChange={tab => {
+              setActiveFormId(tab)
+              setActiveFieldId(null)
+            }}
+            sections={sections}
+          />
+        </Form>
+        <Details>
+          <SectionContent>
+            <SectionRow>
+              <ComponentProperties
+                fieldOrForm={fieldOrForm}
+                formId={activeForm.id}
+                isField={!!activeField}
+                key={
+                  fieldOrForm.id /* `${fieldOrForm.id}${
+                  fieldOrForm.component ? `-${fieldOrForm.component}` : ''
+                }` */
+                }
+                updateForm={updateForm}
+                updateFormElement={updateFormElement}
+              />
+            </SectionRow>
+          </SectionContent>
+        </Details>
+      </Columns>
+    </Container>
+  )
+}
+
+FormBuilderLayout.propTypes = {
+  forms: PropTypes.arrayOf(
+    PropTypes.shape({
+      id: PropTypes.string.isRequired,
+      children: PropTypes.arrayOf(
+        PropTypes.shape({
+          id: PropTypes.string.isRequired,
+          component: PropTypes.string,
+        }).isRequired,
+      ).isRequired,
+    }).isRequired,
+  ).isRequired,
+  activeFormId: PropTypes.string.isRequired,
+  activeFieldId: PropTypes.string,
+  deleteForm: PropTypes.func.isRequired,
+  deleteFormElement: PropTypes.func.isRequired,
+  updateForm: PropTypes.func.isRequired,
+  createForm: PropTypes.func.isRequired,
+  updateFormElement: PropTypes.func.isRequired,
+  setActiveFieldId: PropTypes.func.isRequired,
+  setActiveFormId: PropTypes.func.isRequired,
+}
+
+FormBuilderLayout.defaultProps = {
+  activeFieldId: null,
+}
+
+export default FormBuilderLayout
diff --git a/app/components/component-formbuilder/src/components/FormBuilderLayout.jsx b/app/components/component-formbuilder/src/components/FormBuilderLayout.jsx
deleted file mode 100644
index 0e2e5a4e81..0000000000
--- a/app/components/component-formbuilder/src/components/FormBuilderLayout.jsx
+++ /dev/null
@@ -1,118 +0,0 @@
-import React from 'react'
-import { forEach } from 'lodash'
-import styled from 'styled-components'
-import { Tabs, Action } from '@pubsweet/ui'
-import { Columns, Details, Form } from './style'
-import ComponentProperties from './ComponentProperties'
-import FormBuilder from './FormBuilder'
-import FormProperties from './FormProperties'
-import {
-  Container,
-  HeadingWithAction,
-  Heading,
-  SectionContent,
-  SectionRow,
-} from '../../../shared'
-
-const DeleteIcon = styled(Action)`
-  margin-left: 10px;
-  line-height: 1.15;
-`
-
-const FormBuilderLayout = ({
-  getForms,
-  properties,
-  deleteForm,
-  deleteFormElement,
-  updateForm,
-  createForm,
-  updateFormElement,
-  setProperties,
-  setActiveTab,
-  activeTab,
-}) => {
-  const Sections = []
-  forEach(getForms, (form, key) => {
-    Sections.push({
-      content: (
-        <SectionContent>
-          <SectionRow>
-            <FormBuilder
-              deleteFormElement={deleteFormElement}
-              form={form}
-              key={`form-builder-${key}`}
-              setProperties={setProperties}
-            />
-          </SectionRow>
-        </SectionContent>
-      ),
-      key: `${key}`,
-      label: [
-        form.name,
-        <DeleteIcon
-          key={`delete-form-${key}`}
-          onClick={e => {
-            e.preventDefault()
-            deleteForm(form.id)
-          }}
-        >
-          x
-        </DeleteIcon>,
-      ],
-    })
-  })
-
-  Sections.push({
-    content: (
-      <SectionContent>
-        <SectionRow>
-          <FormProperties
-            key="form-builder-new"
-            mode="create"
-            onSubmitFn={createForm}
-            properties={{}}
-          />
-        </SectionRow>
-      </SectionContent>
-    ),
-    key: 'new',
-    label: '+ Add Form',
-  })
-  return (
-    <Container>
-      <HeadingWithAction>
-        <Heading>Form Builder</Heading>
-      </HeadingWithAction>
-      <Columns>
-        <Form>
-          <Tabs
-            activeKey={`${activeTab}`}
-            onChange={tab => {
-              setProperties({
-                type: 'form',
-                properties: getForms[tab],
-              })
-              setActiveTab(tab)
-            }}
-            sections={Sections}
-          />
-        </Form>
-        <Details>
-          <SectionContent>
-            <SectionRow>
-              <ComponentProperties
-                key={`${properties.type}-${(properties.properties || {}).id}`}
-                properties={properties}
-                setActiveTab={setActiveTab}
-                updateForm={updateForm}
-                updateFormElement={updateFormElement}
-              />
-            </SectionRow>
-          </SectionContent>
-        </Details>
-      </Columns>
-    </Container>
-  )
-}
-
-export default FormBuilderLayout
diff --git a/app/components/component-formbuilder/src/components/FormBuilderPage.js b/app/components/component-formbuilder/src/components/FormBuilderPage.js
index 1cbb566c74..435d69d247 100644
--- a/app/components/component-formbuilder/src/components/FormBuilderPage.js
+++ b/app/components/component-formbuilder/src/components/FormBuilderPage.js
@@ -35,53 +35,61 @@ const deleteFormMutation = gql`
 
 const query = gql`
   query {
-    currentUser {
-      id
-      username
-      admin
-    }
-
     getForms
   }
 `
 
-const FormBuilderPage = props => {
+const FormBuilderPage = () => {
   const { loading, data, error } = useQuery(query)
 
-  const [deleteForm] = useMutation(deleteFormMutation)
-  const [deleteFormElement] = useMutation(deleteFormElementMutation)
+  // TODO Structure forms for graphql and retrieve IDs from these mutations to allow Apollo Cache to do its magic, rather than forcing refetch.
+  const [deleteForm] = useMutation(deleteFormMutation, {
+    refetchQueries: [{ query }],
+  })
+
+  const [deleteFormElement] = useMutation(deleteFormElementMutation, {
+    refetchQueries: [{ query }],
+  })
+
+  const [updateForm] = useMutation(updateFormMutation, {
+    refetchQueries: [{ query }],
+  })
+
+  const [updateFormElement] = useMutation(updateFormElementMutation, {
+    refetchQueries: [{ query }],
+  })
 
-  const [updateForm] = useMutation(updateFormMutation)
-  const [updateFormElement] = useMutation(updateFormElementMutation)
-  const [createForm] = useMutation(createFormMutation)
+  const [createForm] = useMutation(createFormMutation, {
+    refetchQueries: [{ query }],
+  })
 
-  const [properties, setProperties] = useState({ type: 'form', properties: {} })
-  const [activeTab, setActiveTab] = useState()
+  // const [properties, setProperties] = useState({ type: 'form', properties: {} })
+  const [activeFormId, setActiveFormId] = useState()
+  const [activeFieldId, setActiveFieldId] = useState()
 
   useEffect(() => {
     if (!loading && data) {
       if (data.getForms.length) {
-        setActiveTab(0)
-        setProperties({ type: 'form', properties: data.getForms[0] })
+        setActiveFormId(data.getForms[0].id)
       } else {
-        setActiveTab('new')
+        setActiveFormId('new')
       }
     }
   }, [loading, data])
 
-  if (loading || activeTab === undefined) return <Spinner />
+  if (loading || !activeFormId) return <Spinner />
   if (error) return JSON.stringify(error)
 
   return (
     <FormBuilderLayout
-      activeTab={activeTab}
+      activeFieldId={activeFieldId}
+      activeFormId={activeFormId}
       createForm={createForm}
       deleteForm={deleteForm}
       deleteFormElement={deleteFormElement}
-      getForms={data.getForms}
-      properties={properties}
-      setActiveTab={setActiveTab}
-      setProperties={setProperties}
+      forms={data.getForms}
+      setActiveFieldId={setActiveFieldId}
+      setActiveFormId={setActiveFormId}
       updateForm={updateForm}
       updateFormElement={updateFormElement}
     />
diff --git a/app/components/component-formbuilder/src/components/FormProperties.jsx b/app/components/component-formbuilder/src/components/FormProperties.js
similarity index 76%
rename from app/components/component-formbuilder/src/components/FormProperties.jsx
rename to app/components/component-formbuilder/src/components/FormProperties.js
index 84a2b1fdab..3de97578fc 100644
--- a/app/components/component-formbuilder/src/components/FormProperties.jsx
+++ b/app/components/component-formbuilder/src/components/FormProperties.js
@@ -1,4 +1,5 @@
 import React, { useState } from 'react'
+import PropTypes from 'prop-types'
 import { isEmpty } from 'lodash'
 import styled from 'styled-components'
 import { Button, TextField, ValidatedFieldFormik } from '@pubsweet/ui'
@@ -22,14 +23,13 @@ export const Section = styled.div`
 
 const FormProperties = ({
   handleSubmit,
-  properties,
   mode,
-  setPopup,
   popup,
-  values,
+  properties,
   setFieldValue,
+  setPopup,
 }) =>
-  isEmpty(properties.properties) && mode !== 'create' ? (
+  isEmpty(properties) && mode !== 'create' ? ( // TODO Move this check to the wrapper so we don't have to pass in the entire properties just for this
     <Page>
       <span>&nbsp;</span>
     </Page>
@@ -38,7 +38,7 @@ const FormProperties = ({
       <form onSubmit={handleSubmit}>
         <Heading>{mode === 'create' ? 'Create Form' : 'Update Form'}</Heading>
         <Section id="form.id" key="form.id">
-          <Legend>ID Form</Legend>
+          <Legend>Form ID</Legend>
           <ValidatedFieldFormik component={idText} name="id" />
         </Section>
         <Section id="form.name" key="form.name">
@@ -100,13 +100,29 @@ const FormProperties = ({
     </Page>
   )
 
+FormProperties.propTypes = {
+  handleSubmit: PropTypes.func.isRequired,
+  mode: PropTypes.string.isRequired,
+  popup: PropTypes.string.isRequired,
+  properties: PropTypes.shape({
+    id: PropTypes.string.isRequired,
+    name: PropTypes.string.isRequired,
+    description: PropTypes.string.isRequired,
+    haspopup: PropTypes.string.isRequired,
+    popuptitle: PropTypes.string,
+    popupdescription: PropTypes.string,
+  }).isRequired,
+  setFieldValue: PropTypes.func.isRequired,
+  setPopup: PropTypes.func.isRequired,
+}
+
 const FormPropertiesWrapper = ({
-  properties,
+  onSubmit,
   mode,
-  handleSubmit,
-  ...props
+  properties,
+  setFieldValue,
 }) => {
-  const [popup, setPopup] = useState((properties.properties || {}).haspopup)
+  const [popup, setPopup] = useState((properties || {}).haspopup)
   return (
     // <Formik
     //   initialValues={pick(properties.properties, [
@@ -123,19 +139,32 @@ const FormPropertiesWrapper = ({
     // >
     //   {props => (
     <FormProperties
-      handleSubmit={handleSubmit}
+      handleSubmit={onSubmit}
       mode={mode}
       popup={popup}
       properties={properties}
-      setFieldValue={props.setFieldValue}
+      setFieldValue={setFieldValue}
       setPopup={setPopup}
-      values={props.values}
     />
     // )}
     // </Formik>
   )
 }
 
+FormPropertiesWrapper.propTypes = {
+  onSubmit: PropTypes.func.isRequired,
+  mode: PropTypes.string.isRequired,
+  properties: PropTypes.shape({
+    id: PropTypes.string.isRequired,
+    name: PropTypes.string.isRequired,
+    description: PropTypes.string.isRequired,
+    haspopup: PropTypes.string.isRequired,
+    popuptitle: PropTypes.string,
+    popupdescription: PropTypes.string,
+  }).isRequired,
+  setFieldValue: PropTypes.func.isRequired,
+}
+
 export default FormPropertiesWrapper
 // export default compose(
 // withProps(({ properties }) => {
diff --git a/app/components/component-formbuilder/src/components/builderComponents/OptionsField.js b/app/components/component-formbuilder/src/components/builderComponents/OptionsField.js
index 1a3e4d58ba..4994fd2a02 100644
--- a/app/components/component-formbuilder/src/components/builderComponents/OptionsField.js
+++ b/app/components/component-formbuilder/src/components/builderComponents/OptionsField.js
@@ -28,48 +28,62 @@ const valueInput = input => (
   <TextField label="Value Option" placeholder="Enter value…" {...input} />
 )
 
-const renderOptions = ({ form: { values }, push, remove }) => (
-  <ul>
-    <UnbulletedList>
-      <li>
-        <Button onClick={() => push()} plain type="button">
-          Add another option
-        </Button>
-      </li>
-      {(values.options || []).map((option, index) => (
-        <li key={option.value}>
-          <Spacing>
-            <Option>
-              Option:&nbsp;
-              {values.options.length > 1 && (
-                <Button onClick={() => remove(index)} type="button">
-                  Remove
-                </Button>
-              )}
-            </Option>
-            <div>
-              <Inline>
-                <ValidatedFieldFormik
-                  component={keyInput}
-                  name={`options.${index}.label`}
-                  required
-                />
-              </Inline>
+const renderOptions = ({ form: { values }, push, remove }) => {
+  const hasNewOption = values.options.some(
+    opt => opt === undefined || !opt.label || !opt.value,
+  )
 
-              <Inline>
-                <ValidatedFieldFormik
-                  component={valueInput}
-                  name={`options.${index}.value`}
-                  required
-                />
-              </Inline>
-            </div>
-          </Spacing>
+  return (
+    <ul>
+      <UnbulletedList>
+        {(values.options || []).map((option, index) => (
+          // index as key is better than using label or value, which would cause rerender and lose focus during editing.
+          // TODO: store an internal ID for each option, and use that as key; OR provide a way of editing an option that doesn't update the option list until the user clicks away (or a modal editor)
+          // eslint-disable-next-line react/no-array-index-key
+          <li key={index}>
+            <Spacing>
+              <Option>
+                Option:&nbsp;
+                {values.options.length > 1 && (
+                  <Button onClick={() => remove(index)} type="button">
+                    Remove
+                  </Button>
+                )}
+              </Option>
+              <div>
+                <Inline>
+                  <ValidatedFieldFormik
+                    component={keyInput}
+                    name={`options.${index}.label`}
+                    required
+                  />
+                </Inline>
+
+                <Inline>
+                  <ValidatedFieldFormik
+                    component={valueInput}
+                    name={`options.${index}.value`}
+                    required
+                  />
+                </Inline>
+              </div>
+            </Spacing>
+          </li>
+        ))}
+        <li>
+          <Button
+            disabled={hasNewOption}
+            onClick={() => push()}
+            plain
+            type="button"
+          >
+            Add another option
+          </Button>
         </li>
-      ))}
-    </UnbulletedList>
-  </ul>
-)
+      </UnbulletedList>
+    </ul>
+  )
+}
 
 const OptionsField = () => (
   <FieldArray component={renderOptions} name="options" />
diff --git a/server/formbuilder/src/resolvers.js b/server/formbuilder/src/resolvers.js
index 9ecdfa6c2a..5fb84808e2 100644
--- a/server/formbuilder/src/resolvers.js
+++ b/server/formbuilder/src/resolvers.js
@@ -3,11 +3,11 @@ const fs = require('fs')
 const path = require('path')
 const { readFiles, mkdirp } = require('./util')
 
-const writeJson = (path, object) =>
-  fs.writeFileSync(path, JSON.stringify(object, null, 2))
+const writeJson = (filePath, object) =>
+  fs.writeFileSync(filePath, JSON.stringify(object, null, 2))
 
-const mergeFiles = path =>
-  readFiles(path).then(files => {
+const mergeFiles = folderPath =>
+  readFiles(folderPath).then(files => {
     const forms = []
     files.forEach((item, index) => {
       const content = JSON.parse(item.content)
@@ -24,10 +24,11 @@ const resolvers = {
         const folderPath = `${config.get(
           'pubsweet-component-xpub-formbuilder.path',
         )}/`
-        const path = `${folderPath}${formId}.json`
 
-        if (fs.existsSync(path)) {
-          fs.unlinkSync(path)
+        const filePath = `${folderPath}${formId}.json`
+
+        if (fs.existsSync(filePath)) {
+          fs.unlinkSync(filePath)
         }
 
         const forms = await mergeFiles(folderPath)
@@ -43,32 +44,34 @@ const resolvers = {
           'pubsweet-component-xpub-formbuilder.path',
         )}/`
 
-        const path = `${folderPath}/${formId}.json`
-        const forms = JSON.parse(fs.readFileSync(path, 'utf8'))
+        const filePath = `${folderPath}/${formId}.json`
+        const form = JSON.parse(fs.readFileSync(filePath, 'utf8'))
 
-        if (forms.children) {
-          const children = forms.children.filter(el => el.id !== elementId)
-          forms.children = children
-          writeJson(path, forms)
+        if (form.children) {
+          const children = form.children.filter(el => el.id !== elementId)
+          form.children = children
+          writeJson(filePath, form)
         }
 
-        const form = await mergeFiles(folderPath)
-        return form
+        const forms = await mergeFiles(folderPath)
+        return forms
       } catch (err) {
         throw new Error(err)
       }
     },
     async createForm(_, { form }, ctx) {
       try {
-        form = JSON.parse(form)
+        const formObj = JSON.parse(form)
+
         const folderPath = `${config.get(
           'pubsweet-component-xpub-formbuilder.path',
         )}/`
-        const path = `${folderPath}/${form.id}.json`
 
-        if (!fs.existsSync(path)) {
+        const filePath = `${folderPath}/${formObj.id}.json`
+
+        if (!fs.existsSync(filePath)) {
           mkdirp(folderPath)
-          writeJson(path, form)
+          writeJson(filePath, formObj)
         }
 
         const forms = await mergeFiles(folderPath)
@@ -80,24 +83,26 @@ const resolvers = {
     },
     async updateForm(_, { form, formId }, ctx) {
       // DONE
-      form = JSON.parse(form)
+      let formObj = JSON.parse(form)
+
       try {
         const folderPath = `${config.get(
           'pubsweet-component-xpub-formbuilder.path',
         )}/`
-        let path = `${folderPath}${formId}.json`
-
-        if (fs.existsSync(path)) {
-          let forms = JSON.parse(fs.readFileSync(path, 'utf8'))
-          forms = Object.assign(forms, form)
-          form = forms
-          if (formId !== form.id) {
-            fs.unlinkSync(path)
-            path = `${folderPath}${form.id}.json`
+
+        let filePath = `${folderPath}${formId}.json`
+
+        if (fs.existsSync(filePath)) {
+          const oldForm = JSON.parse(fs.readFileSync(filePath, 'utf8'))
+          formObj = { ...oldForm, ...formObj }
+
+          if (formId !== formObj.id) {
+            fs.unlinkSync(filePath)
+            filePath = `${folderPath}${formObj.id}.json`
           }
         }
 
-        writeJson(path, form)
+        writeJson(filePath, formObj)
 
         const forms = await mergeFiles(folderPath)
 
@@ -108,24 +113,26 @@ const resolvers = {
     },
     async updateFormElement(_, { element, formId }, ctx) {
       // DONE
-      element = JSON.parse(element)
+      const elementObj = JSON.parse(element)
 
       const folderPath = `${config.get(
         'pubsweet-component-xpub-formbuilder.path',
       )}/`
-      const path = `${folderPath}${formId}.json`
-      const form = JSON.parse(fs.readFileSync(path, 'utf8'))
+
+      const filePath = `${folderPath}${formId}.json`
+      const form = JSON.parse(fs.readFileSync(filePath, 'utf8'))
+
       if (!form.children) {
-        form.children = [element]
-      } else if (form.children.some(e => e.id === element.id)) {
+        form.children = [elementObj]
+      } else if (form.children.some(e => e.id === elementObj.id)) {
         form.children = form.children.map(value =>
-          value.id === element.id ? element : value,
+          value.id === elementObj.id ? elementObj : value,
         )
       } else {
-        form.children.push(element)
+        form.children.push(elementObj)
       }
 
-      writeJson(path, form)
+      writeJson(filePath, form)
       return mergeFiles(folderPath)
     },
   },
@@ -155,8 +162,9 @@ const resolvers = {
         const folderPath = `${config.get(
           'pubsweet-component-xpub-formbuilder.path',
         )}/`
-        const path = `${folderPath}${formId}.json`
-        const forms = JSON.parse(fs.readFileSync(path, 'utf8'))
+
+        const filePath = `${folderPath}${formId}.json`
+        const forms = JSON.parse(fs.readFileSync(filePath, 'utf8'))
 
         return forms
       } catch (err) {
-- 
GitLab